From owner-cvs-src@FreeBSD.ORG Tue Aug 2 18:54:08 2005 Return-Path: X-Original-To: cvs-src@FreeBSD.org Delivered-To: cvs-src@FreeBSD.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 2476E16A41F; Tue, 2 Aug 2005 18:54:08 +0000 (GMT) (envelope-from imp@bsdimp.com) Received: from harmony.bsdimp.com (vc4-2-0-87.dsl.netrack.net [199.45.160.85]) by mx1.FreeBSD.org (Postfix) with ESMTP id 7CF6743D4C; Tue, 2 Aug 2005 18:54:07 +0000 (GMT) (envelope-from imp@bsdimp.com) Received: from localhost (localhost.village.org [127.0.0.1] (may be forged)) by harmony.bsdimp.com (8.13.3/8.13.3) with ESMTP id j72Iq3ip029204; Tue, 2 Aug 2005 12:52:03 -0600 (MDT) (envelope-from imp@bsdimp.com) Date: Tue, 02 Aug 2005 12:52:59 -0600 (MDT) Message-Id: <20050802.125259.02265276.imp@bsdimp.com> To: scottl@samsco.org From: "M. Warner Losh" In-Reply-To: <42EFBB3E.4020508@samsco.org> References: <20050802160519.6A91016A431@hub.freebsd.org> <42EFB103.2070307@root.org> <42EFBB3E.4020508@samsco.org> X-Mailer: Mew version 3.3 on Emacs 21.3 / Mule 5.0 (SAKAKI) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-2.0 (harmony.bsdimp.com [127.0.0.1]); Tue, 02 Aug 2005 12:52:04 -0600 (MDT) Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, emax@FreeBSD.org, cvs-all@FreeBSD.org, nate@root.org Subject: Re: cvs commit: src/sys/dev/an if_an.c X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 02 Aug 2005 18:54:08 -0000 In message: <42EFBB3E.4020508@samsco.org> Scott Long writes: : Nate Lawson wrote: : > Maksim Yevmenkin wrote: : > : >> emax 2005-08-02 16:03:51 UTC : >> : >> FreeBSD src repository : >> : >> Modified files: : >> sys/dev/an if_an.c Log: : >> Do not lock an to check gone flag. Only need to hold the lock to set : >> the gone flag. : >> Reviewed by: imp : >> MFC after: 1 day : >> Revision Changes Path : >> 1.69 +1 -2 src/sys/dev/an/if_an.c : >> : >> : >> Index: src/sys/dev/an/if_an.c : >> diff -u src/sys/dev/an/if_an.c:1.68 src/sys/dev/an/if_an.c:1.69 : >> --- src/sys/dev/an/if_an.c:1.68 Wed Jul 27 21:03:35 2005 : >> +++ src/sys/dev/an/if_an.c Tue Aug 2 16:03:51 2005 : >> @@ -826,12 +826,11 @@ : >> struct an_softc *sc = device_get_softc(dev); : >> struct ifnet *ifp = sc->an_ifp; : >> : >> - AN_LOCK(sc); : >> if (sc->an_gone) { : >> - AN_UNLOCK(sc); : >> device_printf(dev,"already unloaded\n"); : >> return(0); : >> } : >> + AN_LOCK(sc); : >> an_stop(sc); : >> sc->an_gone = 1; : >> ifmedia_removeall(&sc->an_ifmedia); : > : > : > This commit is wrong. The race is: : > : > Process 1 Process 2 : > an_detach() : > if (gone) ... : > an_detach() : > if (gone) ... : > LOCK : > an_stop() : > gone = 1 : > UNLOCK : > LOCK : > an_stop() !!! called twice : > gone = 1 : > : > You really need to hold the lock over both the check and set, otherwise : > it's not atomic. : > : > This driver also needs some mtx_asserts in an_reset() and other places : > that must be called with the an lock held. : > : : I agree with Nate. The single act of testing an integer usually doesn't : require a mutex, but making a decision based on that test requires : atomicity. I apologize for not noticing this is prior email. However, nate is wrong here. You can't have two threads in a detach at the same time. Warner