From owner-cvs-src@FreeBSD.ORG  Wed Dec 20 11:14:56 2006
Return-Path: <owner-cvs-src@FreeBSD.ORG>
X-Original-To: cvs-src@FreeBSD.org
Delivered-To: cvs-src@FreeBSD.org
Received: from mx1.FreeBSD.org (mx1.freebsd.org [69.147.83.52])
	by hub.freebsd.org (Postfix) with ESMTP id 5974916A5F9;
	Wed, 20 Dec 2006 11:14:56 +0000 (UTC) (envelope-from mb@imp.ch)
Received: from pop.imp.ch (mx2.imp.ch [157.161.9.17])
	by mx1.FreeBSD.org (Postfix) with ESMTP id 5AB9C43CA5;
	Wed, 20 Dec 2006 11:14:55 +0000 (GMT) (envelope-from mb@imp.ch)
Received: from godot.imp.ch (godot.imp.ch [157.161.4.8])
	by pop.imp.ch (8.13.8/8.13.8/Submit_imp) with ESMTP id kBKBEqGe017658; 
	Wed, 20 Dec 2006 12:14:53 +0100 (CET) (envelope-from mb@imp.ch)
Date: Wed, 20 Dec 2006 12:14:52 +0100 (CET)
From: Martin Blapp <mb@imp.ch>
To: Attilio Rao <attilio@FreeBSD.org>
In-Reply-To: <3bbf2fe10612191850y3a546fcew486575385b1400df@mail.gmail.com>
Message-ID: <20061220120419.M53548@godot.imp.ch>
References: <200612192234.kBJMYYo0055529@repoman.freebsd.org>
	<3bbf2fe10612191850y3a546fcew486575385b1400df@mail.gmail.com>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed
X-Scanned-By: MIMEDefang 2.58 on 157.161.9.65
Cc: cvs-src@FreeBSD.org, Martin Blapp <mbr@FreeBSD.org>, cvs-all@FreeBSD.org,
	src-committers@FreeBSD.org
Subject: Re: cvs commit: src/sys/kern tty.c
X-BeenThere: cvs-src@freebsd.org
X-Mailman-Version: 2.1.5
Precedence: list
List-Id: CVS commit messages for the src tree <cvs-src.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/cvs-src>,
	<mailto:cvs-src-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/cvs-src>
List-Post: <mailto:cvs-src@freebsd.org>
List-Help: <mailto:cvs-src-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/cvs-src>,
	<mailto:cvs-src-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Wed, 20 Dec 2006 11:14:56 -0000


Hi,

> Since proctree_lock is a sx lock which uses 2 condition variables,
> they alredy drop Giant (DROP_GIANT()) before sleeping. Are you secure
> it is the right thing to do here?

Yes I am.

sx_slock() can call cv_wait() which later can call cv_wait_unlock ...
if there is already a lock we need to share ... After a possible
sleep cv_wait_unlock looks like:

cv_wait_unlock(struct cv *cvp, struct mtx *mp)

[...]
         sleepq_lock(cvp);

         cvp->cv_waiters++;
         DROP_GIANT();
         mtx_unlock(mp);

[...]

It can call DROP_GIANT for a short amount of time. This was the race.

I already fixed it (but haven't seen the real cause for the crashes) once in 
STABLE and HEAD, but backed it out again because I (and jhb) were sure that the 
tty driver was protected fully by the GIANT lock after I fixed severall 
possible race places in kern_proc.c. I had no idea that sx_slock()  and sx_slock 
both can drop GIANT for a short time and that we need carefully recheck 
variables.

Martin