[lkml]   [2005]   [Apr]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH] tty races

    On Mon, 25 Apr 2005, Andrew Morton wrote:

    > Jason Baron <> 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/
    > > 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.


    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to
    More majordomo info at
    Please read the FAQ at

     \ /
      Last update: 2005-04-26 15:55    [W:0.022 / U:1.524 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site