Re: [PATCH] kdesu broken

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

On Wed, 29 Jul 2009, Alan Cox wrote:
> > 
> > > - 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).
> 
> You are only looking at pty. That code is used for all the real physical
> tty devices too. With real devices the underlying physical device and its
> structures can get dumped. When you run the n_tty ldisc you call back out
> to the drivers for echo etc.

I actually only meant the "flush_to_ldisc()" part. We'll never get any 
further than that due to the reasons I outlined.

> > 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.
> 
> What happens if the hangup occurs just after you start running the ldisc
> on another CPU ?

But Alan, that was my point: Ogawa's patch in no way changes any existing 
behavior. The case you mention ALREADY HAPPENS, regardless of Ogawa's 
patch.

If a timer goes off at the same time hangup happens, you have the exact 
same situation. You can have one CPU doing hangup processing, and one CPU 
having just triggered the timeout and doing flush_to_ldisc.

> Consider a real tty for a bit
> 
> 	CPU0					CPU1
> n_tty methods
> 	flush_to_ldisc
> 	get ldisc ref
> 						INTERRUPT
> 						tty_hangup
> 						do_tty_hangup

Yes. Consider exactly that. And notice how it happens with or without 
Ogawa's patch. Just instead of "n_tty methods", you have "delayed-work 
timer".

> So as I said before you need to fix flush_to_ldisc and the hangup running
> against one another. At the very least I think you need a
> tty_ldisc_wait_idle(tty); just before
> 
>         if (tty->driver->flags & TTY_DRIVER_RESET_TERMIOS) {
> 
> so that you stall the hangup until n_tty exits the ldisc.

The tty_ldisc_hangup() already does tty_ldisc_wait_idle() after disabling 
the timer (and clearing the TTY_LDISC bit), so that all seems fine 
already. No?

				Linus