cvs commit: src/sys/dev/an if_an.c

[Scott Long]

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