cvs commit: src/sys/kern tty.c

[Bruce Evans]

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