[PATCH] tty races

[PATCH] tty races

[Jason Baron]

There are a couple of tty race conditions, which lead to inconsistent tty 
reference counting and tty layer oopses.

The first is a tty_open vs. tty_close race in drivers/char/tty.io.c. 
Basically, from the time that the tty->count is deemed to be 1 and that we 
are going to free it to the time that TTY_CLOSING bit is set, needs to be 
atomic with respect to the manipulation of tty->count in init_dev(). This 
atomicity was previously guarded by the BKL. However, this is no longer 
true with the addition of a down() call in the middle of the 
release_dev()'s atomic path. So either the down() needs to be moved 
outside the atomic patch or dropped. I would vote for simply dropping it 
as i don't see why it is necessary.

The second race is tty_open vs. tty_open. This race I've seen when the 
virtual console is the tty driver. In con_open(),  vc_allocate() is called 
if the tty->count is 1. However, this check of the tty->count is not 
guarded by the 'tty_sem'. Thus, it is possible for con_open(), to never 
see the tty->count as 1, and thus never call vc_allocate(). This leads to 
a NULL filp->private_data, and an oops.

The test case below reproduces these problems, and the patch fixes it. The 
test case uses /dev/tty9, which is generally restricted to root for 
open(). It may be able to exploit these races using pseudo terminals, 
although i wasn't able to. A previous report of this issue, with an oops 
trace was: http://www.ussg.iu.edu/hypermail/linux/kernel/0503.2/0017.html

thanks,

-Jason


--- linux/drivers/char/tty_io.c.bak
+++ linux/drivers/char/tty_io.c
@@ -1596,14 +1596,9 @@ static void release_dev(struct file * fi
 	 * each iteration we avoid any problems.
 	 */
 	while (1) {
-		/* Guard against races with tty->count changes elsewhere and
-		   opens on /dev/tty */
-		   
-		down(&tty_sem);
 		tty_closing = tty->count <= 1;
 		o_tty_closing = o_tty &&
 			(o_tty->count <= (pty_master ? 1 : 0));
-		up(&tty_sem);
 		do_sleep = 0;
 
 		if (tty_closing) {
@@ -1640,7 +1635,6 @@ static void release_dev(struct file * fi
 	 * block, so it's safe to proceed with closing.
 	 */
 	 
-	down(&tty_sem);
 	if (pty_master) {
 		if (--o_tty->count < 0) {
 			printk(KERN_WARNING "release_dev: bad pty slave count "
@@ -1654,7 +1648,6 @@ static void release_dev(struct file * fi
 		       tty->count, tty_name(tty, buf));
 		tty->count = 0;
 	}
-	up(&tty_sem);
 	
 	/*
 	 * We've decremented tty->count, so we need to remove this file
@@ -1844,9 +1837,10 @@ retry_open:
 	}
 got_driver:
 	retval = init_dev(driver, index, &tty);
-	up(&tty_sem);
-	if (retval)
+	if (retval) {
+		up(&tty_sem);
 		return retval;
+	}
 
 	filp->private_data = tty;
 	file_move(filp, &tty->tty_files);
@@ -1863,6 +1857,7 @@ got_driver:
 		else
 			retval = -ENODEV;
 	}
+	up(&tty_sem);
 	filp->f_flags = saved_flags;
 
 	if (!retval && test_bit(TTY_EXCLUSIVE, &tty->flags) && !capable(CAP_SYS_ADMIN))



#include <sys/types.h>
#include <sys/stat.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>
#include <time.h>
#include <pthread.h>
#include <linux/fb.h>
#include <linux/vt.h>
#include <linux/kd.h>

#define NTHREADS 300

void *thread_function();
int open_fail_num;
int open_success;

int
main(int argc, char *argv[])
{
  	int i, j;
  	pthread_t thread_id[NTHREADS];

  	for(;;) {
      		for(i=0; i < NTHREADS; i++) {
	  		pthread_create(&thread_id[i], NULL, &thread_function, NULL);
		}
      		for(j=0; j < NTHREADS; j++) {
	  		pthread_join(thread_id[j], NULL); 
		}
      		printf("open failures: %i\n", open_fail_num);
      		printf("open success: %i\n", open_success);
    	}
}

void *thread_function()
{
  	int fd;
  	time_t t;
  	int val;
  	int ret;

  	fd = open("/dev/tty9", O_RDWR);

  	val = 0;
  	//call an ioctl
	ret = ioctl(fd, KDGETMODE, &val);
  	if (ret != 0) {
		perror("ioctl error\n");
  	}

  	if (fd < 0) {
      		open_fail_num++;
    	} else {
      		open_success++;
    	}
  	/* just waste some random time */
  	t = (time((time_t *)0) &31L) << 6;
  	while (t-- > 0)
    		(void)time((time_t *)0);
  	close(fd);	
}

Re: [PATCH] tty races

[Andrew Morton]

Jason Baron <jbaron@redhat.com> wrote:
>
> There are a couple of tty race conditions, which lead to inconsistent tty 
>  reference counting and tty layer oopses.
> 
>  The first is a tty_open vs. tty_close race in drivers/char/tty.io.c. 
>  Basically, from the time that the tty->count is deemed to be 1 and that we 
>  are going to free it to the time that TTY_CLOSING bit is set, needs to be 
>  atomic with respect to the manipulation of tty->count in init_dev(). This 
>  atomicity was previously guarded by the BKL. However, this is no longer 
>  true with the addition of a down() call in the middle of the 
>  release_dev()'s atomic path. So either the down() needs to be moved 
>  outside the atomic patch or dropped. I would vote for simply dropping it 
>  as i don't see why it is necessary.

The release_dev() changes looks very fishy to me.  It _removes_ locking. 
If that fixes the testcase then one of two things is happening:

a) we have lock_kernel() coverage and the down()'s sleeping breaks the
   lock_kenrel() coverage or

b) we don't have lock_kernel() coverage, but removing the down() just
   alters the timing and makes the race less probable.

I think it's b).  lock_kernel() coverage in there is very incomplete on the
open() side.

I think it would be better to _increase_ the tty_sem coverage in
release_dev() and to make sure that all callers of init_dev() are using
tty_sem (they are).

One approach would be to require that all callers of release_dev() hold
tty_sem, and make release_dev() drop and reacquire tty_sem in those cases
where release_dev() needs to go to sleep when waiting for other threads of
control to reelase the tty's resources.

>  The second race is tty_open vs. tty_open. This race I've seen when the 
>  virtual console is the tty driver. In con_open(),  vc_allocate() is called 
>  if the tty->count is 1. However, this check of the tty->count is not 
>  guarded by the 'tty_sem'. Thus, it is possible for con_open(), to never 
>  see the tty->count as 1, and thus never call vc_allocate(). This leads to 
>  a NULL filp->private_data, and an oops.
> 
>  The test case below reproduces these problems, and the patch fixes it. The 
>  test case uses /dev/tty9, which is generally restricted to root for 
>  open(). It may be able to exploit these races using pseudo terminals, 
>  although i wasn't able to. A previous report of this issue, with an oops 
>  trace was: http://www.ussg.iu.edu/hypermail/linux/kernel/0503.2/0017.html

So you've extended tty_sem coverage over the tty driver's ->open method.  I
guess that's OK.

Re: [PATCH] tty races

[Jason Baron]

On Mon, 25 Apr 2005, Andrew Morton wrote:

> Jason Baron <jbaron@redhat.com> wrote:
> >
> > There are a couple of tty race conditions, which lead to inconsistent tty 
> >  reference counting and tty layer oopses.
> > 
> >  The first is a tty_open vs. tty_close race in drivers/char/tty.io.c. 
> >  Basically, from the time that the tty->count is deemed to be 1 and that we 
> >  are going to free it to the time that TTY_CLOSING bit is set, needs to be 
> >  atomic with respect to the manipulation of tty->count in init_dev(). This 
> >  atomicity was previously guarded by the BKL. However, this is no longer 
> >  true with the addition of a down() call in the middle of the 
> >  release_dev()'s atomic path. So either the down() needs to be moved 
> >  outside the atomic patch or dropped. I would vote for simply dropping it 
> >  as i don't see why it is necessary.
> 
> The release_dev() changes looks very fishy to me.  It _removes_ locking. 
> If that fixes the testcase then one of two things is happening:
> 
> a) we have lock_kernel() coverage and the down()'s sleeping breaks the
>    lock_kenrel() coverage or
> 
> b) we don't have lock_kernel() coverage, but removing the down() just
>    alters the timing and makes the race less probable.
> 
> I think it's b).  lock_kernel() coverage in there is very incomplete on the
> open() side.
> 

The patch was written for case a. Indeed lock_kernel() may appear 
incomplete on the open side, but it protects paths where we don't sleep. 
So, the 'fast_track' path in 'init_dev', is protected against the 
release_dev path from setting the 'tty_closing' local variable to the 
setting of the TTY_CLOSING flag. Thus, i believe the dropping of the 
down() is correct. 

This was the previous locking model for open vs. close afaict, before the 
down() was introduced in the release_dev path that was supposed to be 
atomic with respect to init_dev().


> I think it would be better to _increase_ the tty_sem coverage in
> release_dev() and to make sure that all callers of init_dev() are using
> tty_sem (they are).
> 
> One approach would be to require that all callers of release_dev() hold
> tty_sem, and make release_dev() drop and reacquire tty_sem in those cases
> where release_dev() needs to go to sleep when waiting for other threads of
> control to reelase the tty's resources.
> 

Indeed, the situation would be improved if it was held around the 
driver->close() routine. This routine does sometimes look at tty->count 
value, see con_close(), where in fact the tty_sem is added to avoid just 
this problem. However, it is incorrect as one can see in release_dev() the 
schedule(), can cause the tty->count to change via tty_open(). However, i 
think this is an extremely rare corner case, b/c con_close() keys off 
tty->count of 1, which implies that this is the last close() and thus the 
schedule for 'write_wait' would seem impossible, although AL Viro has 
said that it is possible in this case. Thus, dropping the tty_sem and 
reacquiring it, probably isn't good, b/c the driver->close() routines can 
free resources based upon tty->count==1. 

The patch was written as the least invasive and low risk way to fix a 
nasty race condition, which has the potential to corrupt data. The oops in 
vt_ioctl has also been seen on system boots with some frequency. The patch 
imo, returns the the tty_open vs. tty_close paths to their original 
locking assumptions which have been well tested.

-Jason

Re: [PATCH] tty races

[Andrew Morton]

Jason Baron <jbaron@redhat.com> wrote:
>
> 
> On Mon, 25 Apr 2005, Andrew Morton wrote:
> 
> > Jason Baron <jbaron@redhat.com> wrote:
> > >
> > > There are a couple of tty race conditions, which lead to inconsistent tty 
> > >  reference counting and tty layer oopses.
> > > 
> > >  The first is a tty_open vs. tty_close race in drivers/char/tty.io.c. 
> > >  Basically, from the time that the tty->count is deemed to be 1 and that we 
> > >  are going to free it to the time that TTY_CLOSING bit is set, needs to be 
> > >  atomic with respect to the manipulation of tty->count in init_dev(). This 
> > >  atomicity was previously guarded by the BKL. However, this is no longer 
> > >  true with the addition of a down() call in the middle of the 
> > >  release_dev()'s atomic path. So either the down() needs to be moved 
> > >  outside the atomic patch or dropped. I would vote for simply dropping it 
> > >  as i don't see why it is necessary.
> > 
> > The release_dev() changes looks very fishy to me.  It _removes_ locking. 
> > If that fixes the testcase then one of two things is happening:
> > 
> > a) we have lock_kernel() coverage and the down()'s sleeping breaks the
> >    lock_kenrel() coverage or
> > 
> > b) we don't have lock_kernel() coverage, but removing the down() just
> >    alters the timing and makes the race less probable.
> > 
> > I think it's b).  lock_kernel() coverage in there is very incomplete on the
> > open() side.
> > 
> 
> The patch was written for case a. Indeed lock_kernel() may appear 
> incomplete on the open side, but it protects paths where we don't sleep. 
> So, the 'fast_track' path in 'init_dev', is protected against the 
> release_dev path from setting the 'tty_closing' local variable to the 
> setting of the TTY_CLOSING flag. Thus, i believe the dropping of the 
> down() is correct. 

I don't see anywhere which takes lock_kernel() on the tty_open() path.

The normal release_dev() path takes lock_kernel(), but two error-path
callers of lock_kernel() also appear to not take lock_kernel().

> This was the previous locking model for open vs. close afaict, before the 
> down() was introduced in the release_dev path that was supposed to be 
> atomic with respect to init_dev().

We want to move away from lock_kernel()-based locking.

> 
> > I think it would be better to _increase_ the tty_sem coverage in
> > release_dev() and to make sure that all callers of init_dev() are using
> > tty_sem (they are).
> > 
> > One approach would be to require that all callers of release_dev() hold
> > tty_sem, and make release_dev() drop and reacquire tty_sem in those cases
> > where release_dev() needs to go to sleep when waiting for other threads of
> > control to reelase the tty's resources.
> > 
> 
> Indeed, the situation would be improved if it was held around the 
> driver->close() routine. This routine does sometimes look at tty->count 
> value, see con_close(), where in fact the tty_sem is added to avoid just 
> this problem. However, it is incorrect as one can see in release_dev() the 
> schedule(), can cause the tty->count to change via tty_open(). However, i 
> think this is an extremely rare corner case, b/c con_close() keys off 
> tty->count of 1, which implies that this is the last close() and thus the 
> schedule for 'write_wait' would seem impossible, although AL Viro has 
> said that it is possible in this case. Thus, dropping the tty_sem and 
> reacquiring it, probably isn't good, b/c the driver->close() routines can 
> free resources based upon tty->count==1. 

Maybe we can just hold tty_sem across that schedule() in release_dev().

If not, then maybe retest ->count and take avoiding action if it looks like
some other thread is trying to resurrect the tty.  Obviously this is a much
poorer approach.

> The patch was written as the least invasive and low risk way to fix a 
> nasty race condition, which has the potential to corrupt data. The oops in 
> vt_ioctl has also been seen on system boots with some frequency. The patch 
> imo, returns the the tty_open vs. tty_close paths to their original 
> locking assumptions which have been well tested.
> 

I don't think it does, and the original lock_kernel-based locking is
obsolete.

Please, let's do this properly, with real locks.

Re: [PATCH] tty races

[Jason Baron]

On Mon, 2 May 2005, Andrew Morton wrote:

> Jason Baron <jbaron@redhat.com> wrote:
> >
> > 
> > On Mon, 25 Apr 2005, Andrew Morton wrote:
> > 
> > > Jason Baron <jbaron@redhat.com> wrote:
> > > >
> > > > There are a couple of tty race conditions, which lead to inconsistent tty 
> > > >  reference counting and tty layer oopses.
> > > > 
> > > >  The first is a tty_open vs. tty_close race in drivers/char/tty.io.c. 
> > > >  Basically, from the time that the tty->count is deemed to be 1 and that we 
> > > >  are going to free it to the time that TTY_CLOSING bit is set, needs to be 
> > > >  atomic with respect to the manipulation of tty->count in init_dev(). This 
> > > >  atomicity was previously guarded by the BKL. However, this is no longer 
> > > >  true with the addition of a down() call in the middle of the 
> > > >  release_dev()'s atomic path. So either the down() needs to be moved 
> > > >  outside the atomic patch or dropped. I would vote for simply dropping it 
> > > >  as i don't see why it is necessary.
> > > 
> > > The release_dev() changes looks very fishy to me.  It _removes_ locking. 
> > > If that fixes the testcase then one of two things is happening:
> > > 
> > > a) we have lock_kernel() coverage and the down()'s sleeping breaks the
> > >    lock_kenrel() coverage or
> > > 
> > > b) we don't have lock_kernel() coverage, but removing the down() just
> > >    alters the timing and makes the race less probable.
> > > 
> > > I think it's b).  lock_kernel() coverage in there is very incomplete on the
> > > open() side.
> > > 
> > 
> > The patch was written for case a. Indeed lock_kernel() may appear 
> > incomplete on the open side, but it protects paths where we don't sleep. 
> > So, the 'fast_track' path in 'init_dev', is protected against the 
> > release_dev path from setting the 'tty_closing' local variable to the 
> > setting of the TTY_CLOSING flag. Thus, i believe the dropping of the 
> > down() is correct. 
> 
> I don't see anywhere which takes lock_kernel() on the tty_open() path.
> 

fs/char_dev.c:chrdev_open():        

	if (filp->f_op->open) {
                lock_kernel();
                ret = filp->f_op->open(inode,filp);
                unlock_kernel();
        }


> The normal release_dev() path takes lock_kernel(), but two error-path
> callers of lock_kernel() also appear to not take lock_kernel().
> 

these are both on open paths.

> > This was the previous locking model for open vs. close afaict, before the 
> > down() was introduced in the release_dev path that was supposed to be 
> > atomic with respect to init_dev().
> 
> We want to move away from lock_kernel()-based locking.
> 

I completely agree, but unfortunately lock_kernel() is currently used 
extensively throughout the tty layer. 

> > 
> > > I think it would be better to _increase_ the tty_sem coverage in
> > > release_dev() and to make sure that all callers of init_dev() are using
> > > tty_sem (they are).
> > > 
> > > One approach would be to require that all callers of release_dev() hold
> > > tty_sem, and make release_dev() drop and reacquire tty_sem in those cases
> > > where release_dev() needs to go to sleep when waiting for other threads of
> > > control to reelase the tty's resources.
> > > 
> > 
> > Indeed, the situation would be improved if it was held around the 
> > driver->close() routine. This routine does sometimes look at tty->count 
> > value, see con_close(), where in fact the tty_sem is added to avoid just 
> > this problem. However, it is incorrect as one can see in release_dev() the 
> > schedule(), can cause the tty->count to change via tty_open(). However, i 
> > think this is an extremely rare corner case, b/c con_close() keys off 
> > tty->count of 1, which implies that this is the last close() and thus the 
> > schedule for 'write_wait' would seem impossible, although AL Viro has 
> > said that it is possible in this case. Thus, dropping the tty_sem and 
> > reacquiring it, probably isn't good, b/c the driver->close() routines can 
> > free resources based upon tty->count==1. 
> 
> Maybe we can just hold tty_sem across that schedule() in release_dev().
> 
> If not, then maybe retest ->count and take avoiding action if it looks like
> some other thread is trying to resurrect the tty.  Obviously this is a much
> poorer approach.
> 
> > The patch was written as the least invasive and low risk way to fix a 
> > nasty race condition, which has the potential to corrupt data. The oops in 
> > vt_ioctl has also been seen on system boots with some frequency. The patch 
> > imo, returns the the tty_open vs. tty_close paths to their original 
> > locking assumptions which have been well tested.
> > 
> 
> I don't think it does, and the original lock_kernel-based locking is
> obsolete.
> 
> Please, let's do this properly, with real locks.
> 

lock_kernel() is used extensively throughout the tty layer. We can 
re-write the locking for the layer, but I'd like to see this bug fix in 
2.6.12, if that isn't done in time.

thanks,

-Jason

Re: [PATCH] tty races

[Andrew Morton]

Jason Baron <jbaron@redhat.com> wrote:
>
> > 
> > I don't see anywhere which takes lock_kernel() on the tty_open() path.
> > 
> 
> fs/char_dev.c:chrdev_open():        
> 
> 	if (filp->f_op->open) {
>                 lock_kernel();
>                 ret = filp->f_op->open(inode,filp);
>                 unlock_kernel();
>         }
> 

hm, we're still doing that.

> > 
> > We want to move away from lock_kernel()-based locking.
> > 
> 
> I completely agree, but unfortunately lock_kernel() is currently used 
> extensively throughout the tty layer. 

Well no - it's being migrated over to use tty_sem.  We shouldn't start
heading in the reverse direction.  Plus your patch reverts part of
http://linux.bkbits.net:8080/linux-2.5/diffs/drivers/char/tty_io.c@1.156?nav=index.html|src/|src/drivers|src/drivers/char|hist/drivers/char/tty_io.c
in ways which might be unsafe.

> lock_kernel() is used extensively throughout the tty layer. We can 
> re-write the locking for the layer, but I'd like to see this bug fix in 
> 2.6.12, if that isn't done in time.

Sorry, but AFAICT all you have done is to advocate for the existing patch
without having attempted to fix this problem with tty_sem.  Please try to
come up with a tty_sem-based fix.

Re: [PATCH] tty races

[Jason Baron]

On Tue, 3 May 2005, Andrew Morton wrote:

> > > We want to move away from lock_kernel()-based locking.
> > > 
> > 
> > I completely agree, but unfortunately lock_kernel() is currently used 
> > extensively throughout the tty layer. 
> 
> Well no - it's being migrated over to use tty_sem.  We shouldn't start
> heading in the reverse direction.  Plus your patch reverts part of
> http://linux.bkbits.net:8080/linux-2.5/diffs/drivers/char/tty_io.c@1.156?nav=index.html|src/|src/drivers|src/drivers/char|hist/drivers/char/tty_io.c
> in ways which might be unsafe.
> 

The patch I proposed does not add any lock_kernel() based locking. The 
only locking it adds is more tty_sem based locking to cover the 
driver->open() method. I agree though that it relies on the BKL for 
correctness.

Indeed, that is precisely that patch which introduced the problems I've 
pointed out.

> > lock_kernel() is used extensively throughout the tty layer. We can 
> > re-write the locking for the layer, but I'd like to see this bug fix in 
> > 2.6.12, if that isn't done in time.
> 
> Sorry, but AFAICT all you have done is to advocate for the existing patch
> without having attempted to fix this problem with tty_sem.  Please try to
> come up with a tty_sem-based fix.
> 

The patch I proposed fixes the open vs. open race using the tty_sem. The 
open vs. close race is closed by removing locking. Less locking seems 
better to me. 

If you're still not happy, I'll wrap the close path in the tty_sem...

thanks,

-Jason

Re: [PATCH] tty races

[Andrew Morton]

Jason Baron <jbaron@redhat.com> wrote:
>
> The patch I proposed fixes the open vs. open race using the tty_sem. The 
>  open vs. close race is closed by removing locking. Less locking seems 
>  better to me. 

But the additional locking is only temporary.  Once we thing the tty_sem
converage is complete, the lock_kernel()s get removed.  

>  If you're still not happy, I'll wrap the close path in the tty_sem...

Would be appreciated, please.