Re: [PATCH] tty races
From: Andrew Morton
Date: Tue Apr 26 2005 - 01:24:22 EST
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
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/