From owner-svn-src-all@FreeBSD.ORG Wed Apr 22 09:33:20 2009 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 8AF68106564A; Wed, 22 Apr 2009 09:33:20 +0000 (UTC) (envelope-from rwatson@FreeBSD.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 65F3E8FC0A; Wed, 22 Apr 2009 09:33:20 +0000 (UTC) (envelope-from rwatson@FreeBSD.org) Received: from fledge.watson.org (fledge.watson.org [65.122.17.41]) by cyrus.watson.org (Postfix) with ESMTPS id 0A54946B5C; Wed, 22 Apr 2009 05:33:20 -0400 (EDT) Date: Wed, 22 Apr 2009 10:33:19 +0100 (BST) From: Robert Watson X-X-Sender: robert@fledge.watson.org To: Bruce Simpson In-Reply-To: <49EEDB9C.8080409@incunabulum.net> Message-ID: References: <200904212243.n3LMhW48027008@svn.freebsd.org> <49EEDB9C.8080409@incunabulum.net> User-Agent: Alpine 2.00 (BSF 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r191367 - head/sys/net 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: Wed, 22 Apr 2009 09:33:21 -0000 On Wed, 22 Apr 2009, Bruce Simpson wrote: >> Start to address a number of races relating to use of ifnet pointers >> after the corresponding interface has been destroyed: >> (1) Add an ifnet refcount, ifp->if_refcount. Initialize it to 1 in >> if_alloc(), and modify if_free_type() to decrement and check the >> refcount. >> (2) Add new if_ref() and if_rele() interfaces to allow kernel code >> walking global interface lists to release IFNET_[RW]LOCK() yet >> keep the ifnet stable. Currently, if_rele() is a no-op wrapper >> around if_free(), but this may change in the future. > > Thanks. I'll try to digest this badly needed work, but might not get around > to updating SSM to use it straight away. As you probably saw from doco, it's > something which SSM bangs right into, inpcbs after all last longer than > ifnets. There are a few parts to our general problem with ifnets going away: (1) Our general ifnet life cycle is poorly defined, so intermediate states may be visible that shouldn't be (for example, if_detach is a bit chaotic, and we've now stretched out the gap between if_detach and if_free using a refcount). We probably need an IFF_DEAD flag to better handle this case, so that once an ifnet is marked as free but still dying due to outstanding references, it can'e bt looked up. Brooks and I have plans to try to hash some of this out at the BSDCan devsummit. (2) Some consumers of ifnets need to be taught to acquire refcounts on the ifnet to prevent it going away during certain operations -- mainly a class of things similar to the one I just fixed relating to user requests that perform operations that you can't hold IFNET_RLOCK() over. (3) Some consumers of ifnets need to be taught to register the ifnet removal event and properly detach themselves. For example, DUMMYNET is a well-known component that likes to hold onto mbufs for a long time, increasing the chances of the "oh, my ifnet is gone" race. Rather than trying to refcount from each mbuf, which would add excessive overhead, I think the answer is to modify these consumers to detect ifnet removal and GC all the mbufs that refer to it. For example, DUMMYNET should probably walk all its queues and m_freem() packets refering to the dying ifnet. I'm not familiar with the workings of SSM, but my feeling is that it probably needs to take the (3) approach rather than (2) -- rather than preventing the ifnet from going away until a socket closes, it should detect that the ifnet is going away and take appropriate remedial action. Possibly it should detect when a similarly configured ifnet re-appears and consider attaching to that, but it will most likely be a different instance of struct ifnet, and there are semantic considerations. Robert N M Watson Computer Laboratory University of Cambridge