Re: [PATCH] kdesu broken

[Linus Torvalds <torvalds@linux-foundation.org>]

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