cvs commit: src/sys/kern tty.c

[Christian S.J. Peron]

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 at FreeBSD.ORG
FreeBSD Committer
FreeBSD Security Team