svn commit: r239334 - head/sys/netinet

[Randall Stewart]

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.

Hmm, it takes two days of pounding to get this by the way. We are using a Shenick with
our proxy that is adding and deleting addresses on a somewhat regular basis while
traffic is flowing ;-0

Something that not a lot of folks do obviously… not sure why I did not
get into DDB, two CPU's faulted at the same time though.. also the HP's that
this thing was running on are not known for being kind on getting into even
DDB ;-(

Be glad when we get them all replaced with iX systems ;-)


R

> -- 
> John Baldwin
> 

------------------------------
Randall Stewart
803-317-4952 (cell)