ifaddr refcount problem

[Alan Somers]

On Tue, Jun 24, 2014 at 3:08 AM, Gleb Smirnoff <glebius at 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.
-------------- next part --------------
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);