Date: Tue, 12 Sep 2006 17:52:20 -0500 From: "Christian S.J. Peron" <csjp@FreeBSD.org> To: Bruce Evans <bde@zeta.org.au> Cc: src-committers@FreeBSD.org, John Baldwin <jhb@FreeBSD.org>, cvs-src@FreeBSD.org, cvs-all@FreeBSD.org, Max Laier <max@love2party.net>, Martin Blapp <mbr@FreeBSD.org> Subject: Re: cvs commit: src/sys/kern tty.c Message-ID: <45073A24.6060108@FreeBSD.org> In-Reply-To: <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> <20060913051026.D745@epsplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?45073A24.6060108>