lkml.org 
[lkml]   [2009]   [Jul]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] kdesu broken


    On Wed, 29 Jul 2009, Alan Cox wrote:
    >
    > The rules for hangup and thus hot unplug etc are
    >
    > - The driver ensures that it will not call
    > tty_flip_buffer_push/flush_to_ldisc again for this port until re-opened

    That's just bogus.

    The work is already scheduled to be flushed by a timer. The only thing we
    do is to make it happen _immediately_ (rather than later) when we do that
    tty_flush_to_ldisc().

    IOW, it's not changing what the kernel _does_. It's just moving something
    that will be done to a slightly earlier point.

    If that is wrong as per hangup code, then the bug is in the hangup
    handling, not in the tty_flush_to_ldisc().

    > - The driver calls tty_hangup
    > - tty_hangup ensures that tty_flip_buffer_push cannot occur again
    > (by killing the workqueue)
    > - resources may well then get freed before close()

    They had better not be, since all the data structures touched are inside
    the 'tty_struct' (which we're dereferencing in other ways anyway in that
    whole routine).

    So the only thing that the hangup code needs to do is to make that the
    "tty->buf.work.work" function pointer is a nop. And it does, as far as I
    can tell.

    > The same rules apply for an ldisc change via TIOCSETD
    >
    > Ogawa's patch violates this by calling tty_flush_to_ldisc. If that
    > bridges a hangup then it will call into the ldisc for the dead port and
    > that in turn will call the write method of the driver which will in some
    > cases jump to free memory.

    What you describe is just crazy talk. If Ogawa's fix really causes
    problems, then the hangup code is broken, not Ogawa's trivial patch to
    make sure the work is done when trying to read a tty.

    So regardless, by now we have moved from "trivial bug that bites people in
    real life" to "theoretical bug that looks impossible to trigger".

    That's already a big step forward - along with making the code make more
    sense. Which is always good in itself.

    > The hangup is tty_io:do_tty_hangup which calls tty_ldisc:
    > tty_ldisc_hangup which calls ld->ops->hangup which n_tty doesn't
    > currently have but which it could grow and which may block.

    Well, put this way: the only thing that actually stops the outstanding
    timer (for the delayed work) is the tty_ldisc_halt() call in
    tty_ldisc_hangup(). If that _isn't_ called, then your argument is
    pointless, since the tty_flush_to_ldisc() will be done by a timer later
    (and Ogawa's patch thus clearly introduces nothing new).

    And when it _is_ called, it also clears TTY_LDISC, so now tty_ldisc_ref()
    will return NULL, so then flush_to_ldisc() will be a no-op.

    So as far as I can tell, we already protect against this whole case: if we
    call flush_to_ldisc() after the tty has been HUP'ed, it just won't _do_
    anything (unless the work hasn't been canceled, but in that case the timer
    would have done the same thing, so nothing new is introduced).

    Linus


    \
     
     \ /
      Last update: 2009-10-18 23:28    [W:0.024 / U:63.524 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site