From owner-cvs-src@FreeBSD.ORG Tue Aug 2 18:28:33 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 42DF116A446; Tue, 2 Aug 2005 18:28:33 +0000 (GMT) (envelope-from scottl@samsco.org) Received: from pooker.samsco.org (pooker.samsco.org [168.103.85.57]) by mx1.FreeBSD.org (Postfix) with ESMTP id 80CB643D48; Tue, 2 Aug 2005 18:28:28 +0000 (GMT) (envelope-from scottl@samsco.org) Received: from [192.168.254.11] (junior.samsco.home [192.168.254.11]) (authenticated bits=0) by pooker.samsco.org (8.13.3/8.13.3) with ESMTP id j72IaXE3046423; Tue, 2 Aug 2005 12:36:33 -0600 (MDT) (envelope-from scottl@samsco.org) Message-ID: <42EFBB3E.4020508@samsco.org> Date: Tue, 02 Aug 2005 12:28:14 -0600 From: Scott Long User-Agent: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.7.8) Gecko/20050615 X-Accept-Language: en-us, en MIME-Version: 1.0 To: Nate Lawson References: <20050802160519.6A91016A431@hub.freebsd.org> <42EFB103.2070307@root.org> In-Reply-To: <42EFB103.2070307@root.org> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-2.8 required=3.8 tests=ALL_TRUSTED autolearn=failed version=3.0.2 X-Spam-Checker-Version: SpamAssassin 3.0.2 (2004-11-16) on pooker.samsco.org Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, Maksim Yevmenkin , cvs-all@FreeBSD.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:28:34 -0000 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. Scott