From owner-cvs-src@FreeBSD.ORG Tue Sep 12 22:52:24 2006 Return-Path: X-Original-To: cvs-src@FreeBSD.org Delivered-To: cvs-src@FreeBSD.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 69C8516A47E for ; Tue, 12 Sep 2006 22:52:24 +0000 (UTC) (envelope-from csjp@FreeBSD.org) Received: from ems01.seccuris.com (ems01.seccuris.com [204.112.0.35]) by mx1.FreeBSD.org (Postfix) with SMTP id EA3FF43D45 for ; Tue, 12 Sep 2006 22:52:21 +0000 (GMT) (envelope-from csjp@FreeBSD.org) Received: (qmail 5449 invoked by uid 86); 12 Sep 2006 23:26:48 -0000 Received: from unknown (HELO ?127.0.0.1?) (204.112.0.40) by ems01.seccuris.com with SMTP; 12 Sep 2006 23:26:48 -0000 Message-ID: <45073A24.6060108@FreeBSD.org> Date: Tue, 12 Sep 2006 17:52:20 -0500 From: "Christian S.J. Peron" User-Agent: Thunderbird 1.5.0.5 (Macintosh/20060719) MIME-Version: 1.0 To: Bruce Evans References: <200609101651.k8AGpuqm069774@repoman.freebsd.org> <200609111048.19397.jhb@freebsd.org> <200609111829.58796.max@love2party.net> <200609111428.01836.jhb@freebsd.org> <20060913051026.D745@epsplex.bde.org> In-Reply-To: <20060913051026.D745@epsplex.bde.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: src-committers@FreeBSD.org, John Baldwin , cvs-src@FreeBSD.org, cvs-all@FreeBSD.org, Max Laier , Martin Blapp 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Sep 2006 22:52:24 -0000 Rev. 1.272 was my commit, I can't remember the context of the conversations around the Giant push down there, but it's quite possible that I jumped the gun and made a mistake, because I am not sure how much analysis of the TTY locking I did there. Should we roll back that commit and Pickup Giant for the TTY sake? Bruce Evans wrote: > 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 > > -- Christian S.J. Peron csjp@FreeBSD.ORG FreeBSD Committer FreeBSD Security Team