From owner-cvs-all@FreeBSD.ORG  Tue Aug  2 17:45:05 2005
Return-Path: <owner-cvs-all@FreeBSD.ORG>
X-Original-To: cvs-all@FreeBSD.org
Delivered-To: cvs-all@FreeBSD.org
Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125])
	by hub.freebsd.org (Postfix) with ESMTP id 703BF16A421;
	Tue,  2 Aug 2005 17:45:05 +0000 (GMT) (envelope-from nate@root.org)
Received: from www.cryptography.com (li-22.members.linode.com [64.5.53.22])
	by mx1.FreeBSD.org (Postfix) with ESMTP id B402843D5E;
	Tue,  2 Aug 2005 17:45:04 +0000 (GMT) (envelope-from nate@root.org)
Received: from [10.0.0.33] (adsl-67-119-74-222.dsl.sntc01.pacbell.net
	[67.119.74.222])
	by www.cryptography.com (8.12.8/8.12.8) with ESMTP id j72Hioo5006242
	(version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NOT);
	Tue, 2 Aug 2005 10:44:50 -0700
Message-ID: <42EFB103.2070307@root.org>
Date: Tue, 02 Aug 2005 10:44:35 -0700
From: Nate Lawson <nate@root.org>
User-Agent: Mozilla Thunderbird 1.0.6 (Windows/20050716)
X-Accept-Language: en-us, en
MIME-Version: 1.0
To: Maksim Yevmenkin <emax@FreeBSD.org>, src-committers@FreeBSD.org,
	cvs-src@FreeBSD.org, cvs-all@FreeBSD.org
References: <20050802160519.6A91016A431@hub.freebsd.org>
In-Reply-To: <20050802160519.6A91016A431@hub.freebsd.org>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: 
Subject: Re: cvs commit: src/sys/dev/an if_an.c
X-BeenThere: cvs-all@freebsd.org
X-Mailman-Version: 2.1.5
Precedence: list
List-Id: CVS commit messages for the entire tree <cvs-all.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/cvs-all>,
	<mailto:cvs-all-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/cvs-all>
List-Post: <mailto:cvs-all@freebsd.org>
List-Help: <mailto:cvs-all-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/cvs-all>,
	<mailto:cvs-all-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Tue, 02 Aug 2005 17:45:05 -0000

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