Re: [PATCH] kdesu broken

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

On Tue, 28 Jul 2009, Linus Torvalds wrote:
> 
> 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).

Btw, if you worry about the fact that this all could happen concurrently 
(ie the hangup is done on one cpu, just as the other cpu is doing that 
flush_to_ldisc() thing), then again, Ogawa's patch doesn't actually change 
anything. The synchronous flush_to_ldisc() (done by Ogawa's patch) could 
equally have been an asynchronous (done by a timer), and the timer may 
already have triggered - so 'tty_ldisc_halt()' doing a cancel on the 
delayed work is too late.

So I don't think there are any new races wrt concurrency there either, 
even though we do not take any locks in the tty_flush_to_ldisc() case. 
Because the timer wouldn't have taken any locks either..

Of course, if "tty_ldisc_halt()" (to remove any pending timer) and 
"tty_ldisc_wait_idle()" (to make sure nothing else is executing right 
then) is not sufficient, then there's a possible problem there if you hit 
the timing just right, but again, that would be true _regardless_ of 
Ogawa's changes as far as I can tell. 

IOW, the whole argument really hinges on the fact that calling 
flush_to_ldisc() manually (without any locking) is really not 
fundamentally any different from the delayed work doing it from a timer. 

And when we _do_ disable the timer, we also make that flush_to_ldisc() 
function be a no-op, so the "tty_ldisc_halt()" effectively stops both the 
timer and (conceptually) the manual call case too. 

So now we have one remaining case, namely the case of the ldisc then being 
re-initialized and TTY_LDISC is set again. But at that point, calling 
flush_to_ldisc() had better be ok again, wouldn't you agree?

		Linus