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

[M. Warner Losh]

In message: <42EFBB3E.4020508 at samsco.org>
            Scott Long <scottl at samsco.org> 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