On Tue, Jun 24, 2014 at 3:08 AM, Gleb Smirnoff <gleb...@freebsd.org> wrote: > 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.
That looks better. But I think there is one more possibility for a leak. For multicast packets, IFP_TO_IA at line 263 will call ifa_ref(), unless the the interface has no address assigned. How about this patch instead? Also, not tested. > > -- > Totus tuus, Glebius.
Index: sys/netinet/ip_output.c =================================================================== --- sys/netinet/ip_output.c (revision 267801) +++ sys/netinet/ip_output.c (working copy) @@ -136,6 +136,7 @@ struct rtentry *rte; /* cache for ro->ro_rt */ struct in_addr odst; struct m_tag *fwd_tag = NULL; + int have_ia_ref = 0; #ifdef IPSEC int no_route_but_check_spd = 0; #endif @@ -238,6 +239,7 @@ error = ENETUNREACH; goto bad; } + have_ia_ref = 1; ip->ip_dst.s_addr = INADDR_BROADCAST; dst->sin_addr = ip->ip_dst; ifp = ia->ia_ifp; @@ -250,6 +252,7 @@ error = ENETUNREACH; goto bad; } + have_ia_ref = 1; ifp = ia->ia_ifp; ip->ip_ttl = 1; isbroadcast = in_broadcast(dst->sin_addr, ifp); @@ -261,6 +264,7 @@ */ ifp = imo->imo_multicast_ifp; IFP_TO_IA(ifp, ia); + have_ia_ref = 1; isbroadcast = 0; /* fool gcc */ } else { /* @@ -552,8 +556,11 @@ #endif error = netisr_queue(NETISR_IP, m); goto done; - } else + } else { + if (have_ia_ref && ia) + ifa_free(&ia->ifa); goto again; /* Redo the routing table lookup. */ + } } /* See if local, if yes, send it to netisr with IP_FASTFWD_OURS. */ @@ -582,6 +589,8 @@ m->m_flags |= M_SKIP_FIREWALL; m->m_flags &= ~M_IP_NEXTHOP; m_tag_delete(m, fwd_tag); + if (have_ia_ref && ia) + ifa_free(&ia->ifa); goto again; } @@ -694,6 +703,8 @@ done: if (ro == &iproute) RO_RTFREE(ro); + if (have_ia_ref && ia) + ifa_free(&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"