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