Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 17 Aug 2012 08:24:26 -0400
From:      John Baldwin <jhb@freebsd.org>
To:        Randall Stewart <rrs@lakerest.net>
Cc:        svn-src-head@freebsd.org, Randall Stewart <rrs@freebsd.org>, svn-src-all@freebsd.org, src-committers@freebsd.org
Subject:   Re: svn commit: r239334 - head/sys/netinet
Message-ID:  <201208170824.26385.jhb@freebsd.org>
In-Reply-To: <896011BA-5F0C-4371-A9D2-6E96283380E0@lakerest.net>
References:  <201208161755.q7GHtHHZ048693@svn.freebsd.org> <201208161534.42012.jhb@freebsd.org> <896011BA-5F0C-4371-A9D2-6E96283380E0@lakerest.net>

next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday, August 16, 2012 6:33:12 pm Randall Stewart wrote:
> 
> On Aug 16, 2012, at 3:34 PM, John Baldwin wrote:
> 
> > On Thursday, August 16, 2012 1:55:17 pm Randall Stewart wrote:
> >> Author: rrs
> >> Date: Thu Aug 16 17:55:16 2012
> >> New Revision: 239334
> >> URL: http://svn.freebsd.org/changeset/base/239334
> >> 
> >> Log:
> >>  Its never a good idea to double free the same
> >>  address.
> >> 
> >>  MFC after:	1 week (after the other commits ahead of this gets MFC'd)
> >> 
> >> Modified:
> >>  head/sys/netinet/in.c
> >> 
> >> Modified: head/sys/netinet/in.c
> >> 
> > ==============================================================================
> >> --- head/sys/netinet/in.c	Thu Aug 16 17:27:11 2012	(r239333)
> >> +++ head/sys/netinet/in.c	Thu Aug 16 17:55:16 2012	(r239334)
> >> @@ -573,7 +573,7 @@ in_control(struct socket *so, u_long cmd
> >> 	}
> >> 	TAILQ_REMOVE(&ifp->if_addrhead, &ia->ia_ifa, ifa_link);
> >> 	IF_ADDR_WUNLOCK(ifp);
> >> -	ifa_free(&ia->ia_ifa);				/* if_addrhead */
> >> +/*	ifa_free(&ia->ia_ifa);	- Double free?? */	/* if_addrhead */
> > 
> > This isn't a double free.  This is dropping a reference count.  In this case 
> > as the comment suggests, it is removing the reference held by the per-
> > interface if_addrhead list that it was just removed from two lines above.  
> > Later in the function when ifa_free() is invoked:
> > 
> > 	LIST_REMOVE(ia, ia_hash);
> > 	IN_IFADDR_WUNLOCK();
> > 	...
> > 	ifa_free(&ia->ia_ifa);				/* in_ifaddrhead */
> > 
> > It is dropping the reference held by the in_ifaddrhead list which the ifa
> > was removed from by the above LIST_REMOVE().  Are you seeing a panic or
> > refcount underflow or some such?
> > 
> 
> No panic, I wish I were so lucky, I had a lockup/fault at:
> 
> in_gif.c line 410 (this is 9 stable)
> -----------------------
>         IN_IFADDR_RLOCK();
>         TAILQ_FOREACH(ia4, &V_in_ifaddrhead, ia_link) {
>                 if ((ia4->ia_ifa.ifa_ifp->if_flags & IFF_BROADCAST) == 0) <------fault in kernel HERE
>                         continue;
>                 if (ip->ip_src.s_addr == ia4->ia_broadaddr.sin_addr.s_addr) {
>                         IN_IFADDR_RUNLOCK();
>                         return 0;
>                 }
>         }
>         IN_IFADDR_RUNLOCK();
> ------------------------
> 
> I went through and made sure first that every reference using V_in_ifaddrhead
> was properly locking, and they were. The only thing I could find is this. From
> the instructions I could see in the assembly the ia4->ia_ifa.ifa_ifp was NULL. And
> thus caused a deref of a NULL pointer.

I don't think what you found is a bug.  It is clearly dropping two different
references, so I think your fix isn't correct.  I suspect there is an extra
ifa_free() in some other path that you are tripping over.

You can clearly see the two references added when an ifa is added to V_in_ifaddrhead:

			ifa_ref(ifa);			/* if_addrhead */
			IF_ADDR_WLOCK(ifp);
			TAILQ_INSERT_TAIL(&ifp->if_addrhead, ifa, ifa_link);
			IF_ADDR_WUNLOCK(ifp);
			ifa_ref(ifa);			/* in_ifaddrhead */
			IN_IFADDR_WLOCK();
			TAILQ_INSERT_TAIL(&V_in_ifaddrhead, ia, ia_link);
			IN_IFADDR_WUNLOCK();

If you can get a crashdump (not sure it sounds like you are able), I would add
KTR traces of all callers to ifa_free() (logging the reference count value and
pointer) and possibly adding an explicit panic for the reference count in ifa_free
underflowing or overflowing and then look back in history for what ifa_free()
calls were made.

-- 
John Baldwin



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201208170824.26385.jhb>