From owner-svn-src-all@FreeBSD.ORG Fri Aug 17 14:28:11 2012 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 2E22E106566C; Fri, 17 Aug 2012 14:28:11 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigknife-pt.tunnel.tserv9.chi1.ipv6.he.net [IPv6:2001:470:1f10:75::2]) by mx1.freebsd.org (Postfix) with ESMTP id F2F168FC08; Fri, 17 Aug 2012 14:28:10 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 46630B96C; Fri, 17 Aug 2012 10:28:10 -0400 (EDT) From: John Baldwin To: Randall Stewart Date: Fri, 17 Aug 2012 08:24:26 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p17; KDE/4.5.5; amd64; ; ) References: <201208161755.q7GHtHHZ048693@svn.freebsd.org> <201208161534.42012.jhb@freebsd.org> <896011BA-5F0C-4371-A9D2-6E96283380E0@lakerest.net> In-Reply-To: <896011BA-5F0C-4371-A9D2-6E96283380E0@lakerest.net> MIME-Version: 1.0 Content-Type: Text/Plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Message-Id: <201208170824.26385.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Fri, 17 Aug 2012 10:28:10 -0400 (EDT) Cc: svn-src-head@freebsd.org, Randall Stewart , svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r239334 - head/sys/netinet X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 17 Aug 2012 14:28:11 -0000 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