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.679 / U:0.664 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site