From owner-cvs-all@FreeBSD.ORG Tue Sep 12 20:03:55 2006 Return-Path: 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 B2B1E16A4D8; Tue, 12 Sep 2006 20:03:55 +0000 (UTC) (envelope-from bde@zeta.org.au) Received: from mailout1.pacific.net.au (mailout1.pacific.net.au [61.8.0.84]) by mx1.FreeBSD.org (Postfix) with ESMTP id 2797B43DC8; Tue, 12 Sep 2006 20:02:19 +0000 (GMT) (envelope-from bde@zeta.org.au) Received: from mailproxy2.pacific.net.au (mailproxy2.pacific.net.au [61.8.2.163]) by mailout1.pacific.net.au (Postfix) with ESMTP id A46FB5A7C82; Wed, 13 Sep 2006 06:02:17 +1000 (EST) Received: from epsplex.bde.org (katana.zip.com.au [61.8.7.246]) by mailproxy2.pacific.net.au (8.13.4/8.13.4/Debian-3sarge3) with ESMTP id k8CK2EU5002139; Wed, 13 Sep 2006 06:02:15 +1000 Date: Wed, 13 Sep 2006 06:02:13 +1000 (EST) From: Bruce Evans X-X-Sender: bde@epsplex.bde.org To: John Baldwin In-Reply-To: <200609111428.01836.jhb@freebsd.org> Message-ID: <20060913051026.D745@epsplex.bde.org> References: <200609101651.k8AGpuqm069774@repoman.freebsd.org> <200609111048.19397.jhb@freebsd.org> <200609111829.58796.max@love2party.net> <200609111428.01836.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: Max Laier , Martin Blapp , cvs-all@FreeBSD.org, src-committers@FreeBSD.org, cvs-src@FreeBSD.org Subject: Re: cvs commit: src/sys/kern tty.c X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Sep 2006 20:03:55 -0000 On Mon, 11 Sep 2006, John Baldwin wrote: > I've told Martin numerous times that t_session is not locked by the proctree > lock and thus by default it is covered by Giant. I think much of the session > stuff still belongs under Giant in fact. I thought that the session stuff was already locked. It has very little to do with ttys. However, apparently, only p_session is covered by session locking, while t_session still needs tty (Giant) locking. It seems unlikely that ttymodem() isn't still under Giant. However, Giant locking for references to t_session and even more important tty things was removed in rev.1.272 of kern_exit.c: % Index: kern_exit.c % =================================================================== % RCS file: /home/ncvs/src/sys/kern/kern_exit.c,v % retrieving revision 1.271 % retrieving revision 1.272 % diff -u -r1.271 -r1.272 % --- kern_exit.c 8 Nov 2005 09:09:26 -0000 1.271 % +++ kern_exit.c 8 Nov 2005 17:11:03 -0000 1.272 % @@ -303,13 +303,13 @@ % vm_map_max(&vm->vm_map)); % } % % - mtx_lock(&Giant); Not long before 1.272, the Giant locking had been pushed down to here. % sx_xlock(&proctree_lock); I don't know exactly what this covers. SESS_LOCK() is not used until later. According to proc.h: p_session: constant until freed (what locks the freeing?) *p_session: mostly locked by SESS_LOCK(), except s_leader also requires the proctree lock % if (SESS_LEADER(p)) { SESS_LEADER() loads s_leader, so why isn't SESS_LOCK() before here? % struct session *sp; % % sp = p->p_session; % if (sp->s_ttyvp) { This seems to need SESS_LOCK() but not proctree_lock. % + locked = VFS_LOCK_GIANT(sp->s_ttyvp->v_mount); % /* % * Controlling process. % * Signal foreground pgrp, s_ttyvp and t_session are referenced just after here. 1.272 is apparently only correct for s_ttyvp (except the session locking was already wrong?). The reference to t_session seems to be only read-only here -- I can't see where it goes away on exit, but think it should -- but nothing good can happen if it changes underneath. Just after this there is a call to ttywait(). ttywait() certainly needs Giant locking. The call is preceded by a comment saying "XXX tp should be locked.". This comment was bogus -- tp was locked by Giant when the comment was written. Now the code is broken instead. % @@ -355,6 +355,7 @@ % * that the session once had a controlling terminal. % * (for logging and informational purposes) % */ % + VFS_UNLOCK_GIANT(locked); % } % SESS_LOCK(p->p_session); % sp->s_leader = NULL; % @@ -363,7 +364,6 @@ % fixjobc(p, p->p_pgrp, 0); % sx_xunlock(&proctree_lock); % (void)acct_process(td); % - mtx_unlock(&Giant); % #ifdef KTRACE % /* % * release trace file Other references to t_session in kern: kern_proc.c: fill_kinfo_proc_only() deferences t_session after checking that it is not NULL. I think the necessary Giant locking is missing here. tty.c: Lots of references to t_session here. None should cause problems directly, since callers are required to provide Giant locking. ttymodem() should only be called from device driver interrupt handlers and these require Giant locking for ordinary i/o too, so the problem is unlikely to be at this level. Bruce