Date: Tue, 02 Aug 2005 12:28:14 -0600 From: Scott Long <scottl@samsco.org> To: Nate Lawson <nate@root.org> Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, Maksim Yevmenkin <emax@FreeBSD.org>, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/an if_an.c Message-ID: <42EFBB3E.4020508@samsco.org> In-Reply-To: <42EFB103.2070307@root.org> References: <20050802160519.6A91016A431@hub.freebsd.org> <42EFB103.2070307@root.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?42EFBB3E.4020508>