[PATCH]: disassociate tty locking fixups

[PATCH]: disassociate tty locking fixups

[Prarit Bhargava]

Additional tty_mutex locking in do_tty_hangup and disassociate_ctty.  It is
possible that do_tty_hangup sets current->signal->tty = NULL.  If that
happens then disassociate_ctty can corrupt memory.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>

--- linux-2.6.18.x86_64.orig/drivers/char/tty_io.c	2006-10-12 10:05:52.000000000 -0400
+++ linux-2.6.18.x86_64/drivers/char/tty_io.c	2006-10-13 11:19:30.000000000 -0400
@@ -1357,6 +1357,8 @@ static void do_tty_hangup(void *data)
 	  This should get done automatically when the port closes and
 	  tty_release is called */
 	
+	/* it is possible current->signal->tty is modified */
+	mutex_lock(&tty_mutex);
 	read_lock(&tasklist_lock);
 	if (tty->session > 0) {
 		do_each_task_pid(tty->session, PIDTYPE_SID, p) {
@@ -1364,14 +1366,16 @@ static void do_tty_hangup(void *data)
 				p->signal->tty = NULL;
 			if (!p->signal->leader)
 				continue;
+			mutex_unlock(&tty_mutex);
 			group_send_sig_info(SIGHUP, SEND_SIG_PRIV, p);
 			group_send_sig_info(SIGCONT, SEND_SIG_PRIV, p);
+			mutex_lock(&tty_mutex);
 			if (tty->pgrp > 0)
 				p->signal->tty_old_pgrp = tty->pgrp;
 		} while_each_task_pid(tty->session, PIDTYPE_SID, p);
 	}
 	read_unlock(&tasklist_lock);
-
+	mutex_unlock(&tty_mutex);
 	tty->flags = 0;
 	tty->session = 0;
 	tty->pgrp = -1;
@@ -1502,7 +1506,7 @@ void disassociate_ctty(int on_exit)
 			kill_pg(current->signal->tty_old_pgrp, SIGCONT, on_exit);
 		}
 		mutex_unlock(&tty_mutex);
-		unlock_kernel();	
+		unlock_kernel();
 		return;
 	}
 	if (tty_pgrp > 0) {
@@ -1514,8 +1518,12 @@ void disassociate_ctty(int on_exit)
 	/* Must lock changes to tty_old_pgrp */
 	mutex_lock(&tty_mutex);
 	current->signal->tty_old_pgrp = 0;
-	tty->session = 0;
-	tty->pgrp = -1;
+
+	/* It is possible that do_tty_hangup has free'd this tty */
+	if (current->signal->tty) {
+		tty->session = 0;
+		tty->pgrp = -1;
+	}
 
 	/* Now clear signal->tty under the lock */
 	read_lock(&tasklist_lock);

Re: [PATCH]: disassociate tty locking fixups

[Arjan van de Ven]

> @@ -1364,14 +1366,16 @@ static void do_tty_hangup(void *data)
>  				p->signal->tty = NULL;
>  			if (!p->signal->leader)
>  				continue;
> +			mutex_unlock(&tty_mutex);
>  			group_send_sig_info(SIGHUP, SEND_SIG_PRIV, p);
>  			group_send_sig_info(SIGCONT, SEND_SIG_PRIV, p);
> +			mutex_lock(&tty_mutex);
>  			if (tty->pgrp > 0)
>  				p->signal->tty_old_pgrp = tty->pgrp;
>  		} while_each_task_pid(tty->session, PIDTYPE_SID, p);
>  	}
>  	read_unlock(&tasklist_lock);
> -
> +	mutex_unlock(&tty_mutex);

Hi,

what is the lock ordering rules between tasklist_lock and tty_mutex?
In the code above you first take tty_mutex then tasklist_lock, and later
on you drop tty_mutex, with the result that you then have a
tasklist_lock -> tty_mutex order.

This can deadlock if someone gets in the middle with a
mutex_lock(&tty_mutex);
write_lock(&tasklist_lock);
....

in addition, are you sure you don't need to revalidate anything after
retaking the lock?

Greetings,
   Arjan van de Ven

Re: [PATCH]: disassociate tty locking fixups

[Alan Cox]

Ar Sad, 2006-10-14 am 07:22 -0400, ysgrifennodd Prarit Bhargava:
> Additional tty_mutex locking in do_tty_hangup and disassociate_ctty.  It is
> possible that do_tty_hangup sets current->signal->tty = NULL.  If that
> happens then disassociate_ctty can corrupt memory.

Ugly but I don't think the patches are sufficient. Firstly you need to
hold the task lock if you are poking around some other users ->signal,
or that may itself change. (disassociate_ctty seems to have this wrong)

Secondly you appear to have lock ordering issues (you lock tty_mutex in
both orders relative to the task list lock) (you take tty_mutex first,
then the task lock which is correct, but then you drop and retake the
tty_mutex while holding the task lock, which may deadlock)

Can you also explain why the ctty change proposed is neccessary ?

NAK the actual code, provisionally agree with the basic diagnosis of
insufficient locking.

Re: [PATCH]: disassociate tty locking fixups

[Prarit Bhargava]

Alan Cox wrote:

 >Ugly but I don't think the patches are sufficient. Firstly you need to
 >hold the task lock if you are poking around some other users ->signal,
 >or that may itself change. (disassociate_ctty seems to have this wrong)

Ah -- okay.

So the locking order is (for example):

mutex_lock(&tty_mutex);
read_lock(&tasklist_lock);
task_lock(current);

Correct?

 >Secondly you appear to have lock ordering issues (you lock tty_mutex in
 >both orders relative to the task list lock) (you take tty_mutex first,
 >then the task lock which is correct, but then you drop and retake the
 >tty_mutex while holding the task lock, which may deadlock)

Fixed.

 >Can you also explain why the ctty change proposed is neccessary ?

disassociate_ctty can call tty_vhangup which calls do tty_hangup directly.
do_tty_hangup can then set p->signal->tty = NULL, and after returning to
disassociate_ctty, the value of tty will now contain a bad pointer.  (I can
reproduce this behaviour by running the gdb testsuite with slab debug on)

 >NAK the actual code, provisionally agree with the basic diagnosis of
 >insufficient locking.

Arjan wrote:

 >in addition, are you sure you don't need to revalidate anything after
 >retaking the lock?

The only place I need to revalidate data (AFAICT) is in 
disassociate_ctty where
I re-check to see if current->signal->tty is still valid.  Admittedly, I am
looking at a very specific failure path though.

I'll rework the patch and post later.

P.

Re: [PATCH]: disassociate tty locking fixups

[Alan Cox]

Ar Sad, 2006-10-14 am 11:06 -0400, ysgrifennodd Prarit Bhargava:
> So the locking order is (for example):
> 
> mutex_lock(&tty_mutex);
> read_lock(&tasklist_lock);
> task_lock(current);
> 
> Correct?

Yes


Alan