On Mon, Jun 23, 2014 at 10:44:58AM -0600, Alan Somers wrote:
A> > On Fri, Jun 20, 2014 at 12:15:21PM -0700, Navdeep Parhar wrote:
A> > N> Revision 264905 and 266860 that followed it seem to leak ifaddr
A> > N> references. ifa_ifwithdstaddr and ifa_ifwithnet both install a
A> > N> reference on the ifaddr returned to the caller but ip_output does not
A> > N> release it, eventually leading to a panic when the refcount wraps over
A> > N> to 0 and the ifaddr is freed while it is still on various lists.
A> > N>
A> > N> I'm using this patch for now. Thoughts?
A> > N>
A> > N> Regards,
A> > N> Navdeep
A> > N>
A> > N>
A> > N> diff -r 6dfcecd314af sys/netinet/ip_output.c
A> > N> --- a/sys/netinet/ip_output.c Fri Jun 20 10:33:22 2014 -0700
A> > N> +++ b/sys/netinet/ip_output.c Fri Jun 20 12:07:12 2014 -0700
A> > N> @@ -243,6 +243,7 @@ again:
A> > N> ifp = ia->ia_ifp;
A> > N> ip->ip_ttl = 1;
A> > N> isbroadcast = 1;
A> > N> + ifa_free((void *)ia);
A> > N> } else if (flags & IP_ROUTETOIF) {
A> > N> if ((ia = ifatoia(ifa_ifwithdstaddr(sintosa(dst)))) ==
NULL &&
A> > N> (ia = ifatoia(ifa_ifwithnet(sintosa(dst), 0))) ==
NULL) {
A> > N> @@ -253,6 +254,7 @@ again:
A> > N> ifp = ia->ia_ifp;
A> > N> ip->ip_ttl = 1;
A> > N> isbroadcast = in_broadcast(dst->sin_addr, ifp);
A> > N> + ifa_free((void *)ia);
A> > N> } else if (IN_MULTICAST(ntohl(ip->ip_dst.s_addr)) &&
A> > N> imo != NULL && imo->imo_multicast_ifp != NULL) {
A> > N> /*
A> >
A> > The patch shouldn't use void * casts, but instead specify explicit member:
A> >
A> > ifa_free(&ia->ia_ifa);
A> >
A> > Apart from that it, the patch looks entirely correct and plugging a leak.
A> > Thanks!
A>
A> I still don't see how this patch would work without breaking stuff
A> like the statistics collection at line 673 of ip_output.c. If we call
A> ifa_free immediately after choosing our ifp, then ia won't be
A> available at lines 630 or 673, and ip_output will never record
A> statistics, right?
You are right, thanks.
In case of IP_SENDONES/IP_ROUTETOIF we should hold the reference to ia
throughout the function and free it at the end.
Suggested patch, not tested.
--
Totus tuus, Glebius.
Index: sys/netinet/ip_output.c
===================================================================
--- sys/netinet/ip_output.c (revision 267536)
+++ sys/netinet/ip_output.c (working copy)
@@ -552,8 +552,11 @@ sendit:
#endif
error = netisr_queue(NETISR_IP, m);
goto done;
- } else
+ } else {
+ if (flags & (IP_SENDONES | IP_ROUTETOIF))
+ ifa_free(&ia->ia_ifa);
goto again; /* Redo the routing table lookup. */
+ }
}
/* See if local, if yes, send it to netisr with IP_FASTFWD_OURS. */
@@ -582,6 +585,8 @@ sendit:
m->m_flags |= M_SKIP_FIREWALL;
m->m_flags &= ~M_IP_NEXTHOP;
m_tag_delete(m, fwd_tag);
+ if (flags & (IP_SENDONES | IP_ROUTETOIF))
+ ifa_free(&ia->ia_ifa);
goto again;
}
@@ -694,6 +699,8 @@ passout:
done:
if (ro == &iproute)
RO_RTFREE(ro);
+ if (flags & (IP_SENDONES | IP_ROUTETOIF))
+ ifa_free(&ia->ia_ifa);
return (error);
bad:
m_freem(m);
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
