Re: [PATCH] tty races

[Andrew Morton]

Jason Baron <jbaron@xxxxxxxxxx> 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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/