WARNING at: drivers/char/tty_ldisc.c

WARNING at: drivers/char/tty_ldisc.c

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 942 bytes --]

Hello Linus, Ogawa,
Looks like we have one more bug in tty code:
'shutdown -r now' in 'single' user mode gives following trace:

WARNING at: drivers/char/tty_ldisc.c:209 tty_ldisc_put+0x95/0xa0
---
?printk+0x23/0x36
warn_slowpath_common+0x78/0xa0
?tty_ldisc_put+0x95/0xa0
?tty_ldisc_put+0x95/0xa0
warn_slowpath_null+0x21/0x40
tty_ldisc_put+0x95/0xa0
tty_ldisc_hangup+0xfc/0x1f0
do_tty_hangup+0x131/0x380
disassociate_ctty+0x50/0x210
do_exit+0x6b8/0x700
?put_lock_stats+0x18/0x50
?lock_release_holdtime+0xb2/0x190
?_spin_unlock_irq+0x30/0x70
?trace_hardirqs_on_caller+0x14c/0x1a0
do_group_exit+0x45/0xb0
get_signal_to_deliver+0x226/0x410
do_notify_resume+0xc1+0xa90
?trace_hardirqs_on+0x19/0x40
?_spin_unlock_irqrestore+0x47/0x90
?remove_wait_queue+0x47/0x70
?do_wait+0x19d/0x310
?defult_wake_function+0x0/0x40
?sys_wait4+0xa1/0x100
work_notifysig+0x13/0x19

//There is no trace in syslog, so given one is 'copy-paste' from the paper.

	Sergey

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 315 bytes --]

Re: WARNING at: drivers/char/tty_ldisc.c

[Greg KH]

On Sun, Aug 02, 2009 at 03:01:20PM +0300, Sergey Senozhatsky wrote:
> Hello Linus, Ogawa,
> Looks like we have one more bug in tty code:
> 'shutdown -r now' in 'single' user mode gives following trace:
> 
> WARNING at: drivers/char/tty_ldisc.c:209 tty_ldisc_put+0x95/0xa0

Ugh, someone still has a ldisc reference somehow.  Does this happen for
you on a "normal" boot and shutdown?

thanks,

greg k-h

Re: WARNING at: drivers/char/tty_ldisc.c

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 542 bytes --]

On (08/02/09 09:05), Greg KH wrote:
> On Sun, Aug 02, 2009 at 03:01:20PM +0300, Sergey Senozhatsky wrote:
> > Hello Linus, Ogawa,
> > Looks like we have one more bug in tty code:
> > 'shutdown -r now' in 'single' user mode gives following trace:
> > 
> > WARNING at: drivers/char/tty_ldisc.c:209 tty_ldisc_put+0x95/0xa0
> 
> Ugh, someone still has a ldisc reference somehow.  Does this happen for
> you on a "normal" boot and shutdown?
> 

Hello Greg,
No, I can't see this while shutting down "normal" booted system.

	Sergey

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 315 bytes --]

Re: WARNING at: drivers/char/tty_ldisc.c

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 375 bytes --]

On (08/02/09 09:05), Greg KH wrote:
> On Sun, Aug 02, 2009 at 03:01:20PM +0300, Sergey Senozhatsky wrote:
> > Hello Linus, Ogawa,
> > Looks like we have one more bug in tty code:
> > 'shutdown -r now' in 'single' user mode gives following trace:
> > 
> > WARNING at: drivers/char/tty_ldisc.c:209 tty_ldisc_put+0x95/0xa0
> 

Sorry, I forgot to mention: rc5

	Sergey

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 315 bytes --]

Re: WARNING at: drivers/char/tty_ldisc.c

[Linus Torvalds]

On Sun, 2 Aug 2009, Sergey Senozhatsky wrote:
>
> Looks like we have one more bug in tty code:
> 'shutdown -r now' in 'single' user mode gives following trace:
> 
> WARNING at: drivers/char/tty_ldisc.c:209 tty_ldisc_put+0x95/0xa0
> ---
> warn_slowpath_common+0x78/0xa0
> warn_slowpath_null+0x21/0x40
> tty_ldisc_put+0x95/0xa0
> tty_ldisc_hangup+0xfc/0x1f0
> do_tty_hangup+0x131/0x380
> disassociate_ctty+0x50/0x210
> do_exit+0x6b8/0x700
> do_group_exit+0x45/0xb0
> get_signal_to_deliver+0x226/0x410
> do_notify_resume+0xc1+0xa90
> work_notifysig+0x13/0x19
> 
> //There is no trace in syslog, so given one is 'copy-paste' from the paper.

We have that 'refcount' counter for the ldisc, but we don't actually use 
it for memory management like we should (ie "free the ldisc when count 
goes to zero"), we just decrement it and free the thing.

And it's quite possible that another CPU is doing some tty read thing or 
other that does

	tty_ldisc_try(..) // increments ld->refcount
	...
	tty_ldisc_deref(..) // decrements it.

at the same time. 

The ldisc refcounts are simply done wrong. They are more debugging aids 
(for the case where no races occur), than actual memory management 
refcounts.

Now, when you do refcounts wrong like that, the "fix" for it is to wait 
for the count to go to zero before releasing things (the problem with that 
fix is that it tends to be complicated and prone to deadlocks etc).

And the tty layer really does try to handle this with that 

	tty_ldisc_halt() - clear TTY_LDISC so that no new refs are taken
	tty_ldisc_wait_idle()  - wait for all old refs to go away

which it did just before the call to 'tty_ldisc_reinit()' (which is 
apparently inlined) that will then call 'tty_ldisc_put()', which is what 
complains.

So we've actually just waited for the count to go down to zero, and now 
it's not zero again. Which _should_ be impossible since the TTY_LDISC bit 
is clear, of course.

But there are things that enable TTY_LDISC, like a 'tty_set_ldisc()' done 
on another CPU. Which does try to protect things with 'tty->ldisc_mutex', 
but in the case of pty's in particular, there are _two_ tty's involved, 
and it does things like

	if (o_tty)
		tty_ldisc_enable(o_tty);

to re-enable the "other" tty, without holding the lock for that tty. 

And I don't think we can just take the lock either, because that's an ABBA 
deadlock when you do it trivially.

I dunno. It might not be the above, there might be something else going 
on, I'll have to think about/look at it a bit more. 

		Linus

Re: WARNING at: drivers/char/tty_ldisc.c

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 1667 bytes --]

On (08/02/09 10:16), Linus Torvalds wrote:
> > Looks like we have one more bug in tty code:
> > 'shutdown -r now' in 'single' user mode gives following trace:
> > 
> > WARNING at: drivers/char/tty_ldisc.c:209 tty_ldisc_put+0x95/0xa0
> > ---
> > warn_slowpath_common+0x78/0xa0
> > warn_slowpath_null+0x21/0x40
> > tty_ldisc_put+0x95/0xa0
> > tty_ldisc_hangup+0xfc/0x1f0
> > do_tty_hangup+0x131/0x380
> > disassociate_ctty+0x50/0x210
> > do_exit+0x6b8/0x700
> > do_group_exit+0x45/0xb0
> > get_signal_to_deliver+0x226/0x410
> > do_notify_resume+0xc1+0xa90
> > work_notifysig+0x13/0x19
> > 
> > //There is no trace in syslog, so given one is 'copy-paste' from the paper.
> 
> We have that 'refcount' counter for the ldisc, but we don't actually use 
> it for memory management like we should (ie "free the ldisc when count 
> goes to zero"), we just decrement it and free the thing.
> 
> And it's quite possible that another CPU is doing some tty read thing or 
> other that does
> 
> 	tty_ldisc_try(..) // increments ld->refcount
> 	...
> 	tty_ldisc_deref(..) // decrements it.
> 
> at the same time. 
>

non-SMP system 'fails' as well.

 
> The ldisc refcounts are simply done wrong. They are more debugging aids 
> (for the case where no races occur), than actual memory management 
> refcounts.
> 
tty_ldisc.c:798  tty_ldisc_hangup
	WARN_ON(tty_ldisc_wait_idle(tty) != 0);

gave WARN_ON traces.

So, it seems refcount is wrong before
	tty_ldisc_halt(tty);
	tty_ldisc_wait_idle(tty);

	/* which is */
	if (wait_event_timeout(tty_ldisc_wait, 
		tty->ldisc->refcount == 0, 5 * HZ) == 0)
		return -EBUSY;

	Sergey

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 315 bytes --]

Re: WARNING at: drivers/char/tty_ldisc.c

[Linus Torvalds]

On Sun, 2 Aug 2009, Sergey Senozhatsky wrote:
> 
> non-SMP system 'fails' as well.

Ahh, can you trigger this reliably? Is it 100% of the time when you shut 
down from single user mode? Or just occasionally?

> > The ldisc refcounts are simply done wrong. They are more debugging aids 
> > (for the case where no races occur), than actual memory management 
> > refcounts.
> 
> tty_ldisc.c:798  tty_ldisc_hangup
> 	WARN_ON(tty_ldisc_wait_idle(tty) != 0);
> 
> gave WARN_ON traces.

Yes, good catch. It means that somebody seems to have held on to the 
refcount for more than five seconds.

Which shouldn't happen under any normal situation.

> So, it seems refcount is wrong before
> 	tty_ldisc_halt(tty);
> 	tty_ldisc_wait_idle(tty);

Agreed. Or something is just holding the refcount for too long, possibly 
due to some deadlockish scenario (ie we migth be in "tty_ldisc_flush()", 
and blocked forever on ld->ops->flush_buffer() while holding the ldisc 
refcount. And we hold that whole &tty->ldisc_mutex _while_ waiting, so I 
can easily see things being blocked on each other.

I'd like to drop the ldisc_mutex while sleeping, but we can't. Not every 
caller even holds it. But just for a broken test, can you try the appended 
patch (NOT meant for serious consumption!) to see if it migth be a 
deadlock (broken by the timeout) on that semaphore?

I take it that you can't get a trace with sysrq-T because nothing gets 
logged, and you don't have a serial port console? That would likely 
pinpoint it pretty quickly (you could make the WARN_ON() do a 
"show_state()" instead - no need to actually physically press 'sysrq-t').

			Linus
---
 drivers/char/tty_ldisc.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/char/tty_ldisc.c b/drivers/char/tty_ldisc.c
index acd76b7..eb44c45 100644
--- a/drivers/char/tty_ldisc.c
+++ b/drivers/char/tty_ldisc.c
@@ -795,7 +795,9 @@ void tty_ldisc_hangup(struct tty_struct *tty)
 		if (tty->ldisc) {	/* Not yet closed */
 			/* Switch back to N_TTY */
 			tty_ldisc_halt(tty);
+			mutex_unlock(&tty->ldisc_mutex);	// HACK
 			tty_ldisc_wait_idle(tty);
+			mutex_lock(&tty->ldisc_mutex);		// HACK
 			tty_ldisc_reinit(tty);
 			/* At this point we have a closed ldisc and we want to
 			   reopen it. We could defer this to the next open but

Re: WARNING at: drivers/char/tty_ldisc.c

[OGAWA Hirofumi]

Linus Torvalds <torvalds@linux-foundation.org> writes:

>> So, it seems refcount is wrong before
>> 	tty_ldisc_halt(tty);
>> 	tty_ldisc_wait_idle(tty);
>
> Agreed. Or something is just holding the refcount for too long, possibly 
> due to some deadlockish scenario (ie we migth be in "tty_ldisc_flush()", 
> and blocked forever on ld->ops->flush_buffer() while holding the ldisc 
> refcount. And we hold that whole &tty->ldisc_mutex _while_ waiting, so I 
> can easily see things being blocked on each other.

In this case, what is happening seems to be simple.

2735 tty1	Ss	0:0 init [S]          <- session leader
2736 tty1	S	0:0  \_ bash

Sequence is,

    bash        tty_read()
                    tty_ldisc_ref_wait()               <- take refcount
                    n_tty_read()
                        schedule_timeout()

    init [S]    do_exit()
                    [...]
                        do_tty_hangup()
                            tty_ldisc_hangup()
                                wake_up_interruptible_poll(read_wait)

    bash                /* n_tty_read() can't see the hangup state of tty,
                         * because anybody don't teach it to tty or ldisc */
                        schedule_timeout()             <- wait again

    init [S]                    tty_ldisc_halt()
                                tty_ldisc_wait_idle()  <- wait refcount
                                tty_ldisc_reinit()
                                    WARN_ON()
             
    bash            tty_ldisc_deref()                  <- drop refcount


or like the above. But I don't know what to do in the tty spec. And yes,
probably, I guess another races/deadlocks sequences are possible.

Maybe, it might be sane to return -EIO, because, tty_read() is already
returning -EIO if tty can't grab ldisc. But I'm not sure, and need to
check after some sleep...

And another related point which I'm don't know is why we don't change
console_fops to hung_up_tty_fops in do_tty_hangup() in the below.

Well, even if it changed to hung_up_tty_fops, like comment says, I guess
we would need to check what happen if someone is passing file descriptor
via AF_UNIX though.

do_tty_hangup()
	[....]
	/* This breaks for file handles being sent over AF_UNIX sockets ? */
	list_for_each_entry(filp, &tty->tty_files, f_u.fu_list) {
		[....]
		if (filp->f_op->write != tty_write)
			continue;
		[....]
		filp->f_op = &hung_up_tty_fops;

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: WARNING at: drivers/char/tty_ldisc.c

[Linus Torvalds]

On Mon, 3 Aug 2009, OGAWA Hirofumi wrote:
> 
> In this case, what is happening seems to be simple.
> 
> 2735 tty1	Ss	0:0 init [S]          <- session leader
> 2736 tty1	S	0:0  \_ bash
> 
> Sequence is,
> 
>     bash        tty_read()
>                     tty_ldisc_ref_wait()               <- take refcount
>                     n_tty_read()
>                         schedule_timeout()
> 
>     init [S]    do_exit()
>                     [...]
>                         do_tty_hangup()
>                             tty_ldisc_hangup()
>                                 wake_up_interruptible_poll(read_wait)
> 
>     bash                /* n_tty_read() can't see the hangup state of tty,
>                          * because anybody don't teach it to tty or ldisc */
>                         schedule_timeout()             <- wait again

Hmm. Wouldn't it trigger on tty_hung_up_p(file)?

[ Reading further.. ]

> And another related point which I'm don't know is why we don't change
> console_fops to hung_up_tty_fops in do_tty_hangup() in the below.

Yup, you're right. Because console_fops has 

        .write          = redirected_tty_write,

we won't actually hang up the console due to that test for "write != 
tty_write".

That's just a classic example of some of the crazy hacks we have in the 
tty layers. I do wonder whether it's even necessary any more. Maybe we 
could just hang things up forcefully now and get rid of that console 
handling special case.

But I guess that all does explain why it only happens in single-user mode. 

So exactly what _does_ happen if we get rid of that hack?

		Linus

---
 drivers/char/tty_io.c |   19 +------------------
 1 files changed, 1 insertions(+), 18 deletions(-)

diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index a3afa0c..80540ec 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -496,10 +496,8 @@ static void do_tty_hangup(struct work_struct *work)
 {
 	struct tty_struct *tty =
 		container_of(work, struct tty_struct, hangup_work);
-	struct file *cons_filp = NULL;
 	struct file *filp, *f = NULL;
 	struct task_struct *p;
-	int    closecount = 0, n;
 	unsigned long flags;
 	int refs = 0;
 
@@ -520,11 +518,6 @@ static void do_tty_hangup(struct work_struct *work)
 	file_list_lock();
 	/* This breaks for file handles being sent over AF_UNIX sockets ? */
 	list_for_each_entry(filp, &tty->tty_files, f_u.fu_list) {
-		if (filp->f_op->write == redirected_tty_write)
-			cons_filp = filp;
-		if (filp->f_op->write != tty_write)
-			continue;
-		closecount++;
 		tty_fasync(-1, filp, 0);	/* can't block */
 		filp->f_op = &hung_up_tty_fops;
 	}
@@ -574,17 +567,7 @@ static void do_tty_hangup(struct work_struct *work)
 	while (refs--)
 		tty_kref_put(tty);
 
-	/*
-	 * If one of the devices matches a console pointer, we
-	 * cannot just call hangup() because that will cause
-	 * tty->count and state->count to go out of sync.
-	 * So we just call close() the right number of times.
-	 */
-	if (cons_filp) {
-		if (tty->ops->close)
-			for (n = 0; n < closecount; n++)
-				tty->ops->close(tty, cons_filp);
-	} else if (tty->ops->hangup)
+	if (tty->ops->hangup)
 		(tty->ops->hangup)(tty);
 	/*
 	 * We don't want to have driver/ldisc interactions beyond

Re: WARNING at: drivers/char/tty_ldisc.c

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 2406 bytes --]

On (08/02/09 14:33), Linus Torvalds wrote:
> > And another related point which I'm don't know is why we don't change
> > console_fops to hung_up_tty_fops in do_tty_hangup() in the below.
> 
> Yup, you're right. Because console_fops has 
> 
>         .write          = redirected_tty_write,
> 
> we won't actually hang up the console due to that test for "write != 
> tty_write".
[...]
> So exactly what _does_ happen if we get rid of that hack?
> 
This solution looks like the right one. There is no trace
on shutdown.
I'll compile and test non-SMP kernel (and of course yet more testing for SMP).

	Sergey


>  drivers/char/tty_io.c |   19 +------------------
>  1 files changed, 1 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
> index a3afa0c..80540ec 100644
> --- a/drivers/char/tty_io.c
> +++ b/drivers/char/tty_io.c
> @@ -496,10 +496,8 @@ static void do_tty_hangup(struct work_struct *work)
>  {
>  	struct tty_struct *tty =
>  		container_of(work, struct tty_struct, hangup_work);
> -	struct file *cons_filp = NULL;
>  	struct file *filp, *f = NULL;
>  	struct task_struct *p;
> -	int    closecount = 0, n;
>  	unsigned long flags;
>  	int refs = 0;
>  
> @@ -520,11 +518,6 @@ static void do_tty_hangup(struct work_struct *work)
>  	file_list_lock();
>  	/* This breaks for file handles being sent over AF_UNIX sockets ? */
>  	list_for_each_entry(filp, &tty->tty_files, f_u.fu_list) {
> -		if (filp->f_op->write == redirected_tty_write)
> -			cons_filp = filp;
> -		if (filp->f_op->write != tty_write)
> -			continue;
> -		closecount++;
>  		tty_fasync(-1, filp, 0);	/* can't block */
>  		filp->f_op = &hung_up_tty_fops;
>  	}
> @@ -574,17 +567,7 @@ static void do_tty_hangup(struct work_struct *work)
>  	while (refs--)
>  		tty_kref_put(tty);
>  
> -	/*
> -	 * If one of the devices matches a console pointer, we
> -	 * cannot just call hangup() because that will cause
> -	 * tty->count and state->count to go out of sync.
> -	 * So we just call close() the right number of times.
> -	 */
> -	if (cons_filp) {
> -		if (tty->ops->close)
> -			for (n = 0; n < closecount; n++)
> -				tty->ops->close(tty, cons_filp);
> -	} else if (tty->ops->hangup)
> +	if (tty->ops->hangup)
>  		(tty->ops->hangup)(tty);
>  	/*
>  	 * We don't want to have driver/ldisc interactions beyond
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 315 bytes --]

Re: WARNING at: drivers/char/tty_ldisc.c

[Alan Cox]

> That's just a classic example of some of the crazy hacks we have in the 
> tty layers. I do wonder whether it's even necessary any more. Maybe we 
> could just hang things up forcefully now and get rid of that console 
> handling special case.
> 
> But I guess that all does explain why it only happens in single-user mode. 
> 
> So exactly what _does_ happen if we get rid of that hack?

Serial console breaks if I remember rightly because the hangup takes out
the port and printk can't then use the resources that were attached to
it.

redirected_tty_write should I think count for the redirection to
hung_up_tty_fops, but I'm not sure you want to do the hangup instead of
close.

You could just finish the ldisc refcounting. The last set of patches you
had off me split tty->ldisc from struct tty ready to do exactly that and
I don't think there is anything left that stops it happening now (It was
just not ready in time)

Re: WARNING at: drivers/char/tty_ldisc.c

[Linus Torvalds]

On Sun, 2 Aug 2009, Alan Cox wrote:
> > 
> > So exactly what _does_ happen if we get rid of that hack?
> 
> Serial console breaks if I remember rightly because the hangup takes out
> the port and printk can't then use the resources that were attached to
> it.

Hmm. That sounds like a likely explanation for the hack, but we do 
re-initialize the tty sufficiently that I don't see why the serial console 
would be unable to use it - it's not like we go through the "filp" anyway.

So it must be something about /dev/console itself, not so much the serial 
lines and serial consoles.

I wonder if the hack is strictly necessary, or could perhaps at least be 
moved down a bit. The comment around that area seems to imply there were 
other issues ("will cause tty->count and state->count to go out of sync"), 
and that whole tty->count thing is some seriously old code, and the tty 
layer has changed a lot since 1992.

So I do get the feeling that it may be just old code that simply nobody 
has dared remove - it may have made more sense back when then it does 
now, and has just been carried around.

If it actually were to cause problems with the tty->count thing, I think 
Sergey should have seen it thanks to us still doing that old 
CHECK_TTY_COUNT thing.

> redirected_tty_write should I think count for the redirection to
> hung_up_tty_fops, but I'm not sure you want to do the hangup instead of
> close.

I'll try with a serial console on one of my machines to see if I can see 
anything wrong. It _would_ be nice to get rid of that thing, since it 
clearly causes problems.

> You could just finish the ldisc refcounting. The last set of patches you
> had off me split tty->ldisc from struct tty ready to do exactly that and
> I don't think there is anything left that stops it happening now (It was
> just not ready in time)

I considered it, and it didn't look horrible (the thing really is pretty 
self-contained in tty_ldisc_try() and tty_ldisc_deref()). But the counts 
are off-by-one (ie zero means "one user - the tty itself"), and it really 
isn't the kind of thing I'd like to do surgery on after -rc5 (or even 
after -rc1, for that matter).

But yeah, it _should_ be possible to just get rid of tty_ldisc_wait_idle() 
entirely if we were to just make tty_ldisc_deref() free the ldisc, and 
started the ldisc->refcount from 1. Then "tty_ldisc_put()" should be just 
a regular tty_ldisc_deref() (it would _normally_ be the one that makes it 
go to zero and frees the 'struct ldisc', unless there are outstanding 
references).

So it doesn't look bad. It's just that if it turns out that console 
hackery isn't necessary, I'd much rather get rid of that.

And right now it looks like the console hackery not only isn't necessary, 
but is actively the thing that breaks - the "odd" thing about the 
single-user shutdown is exactly the fact that it opens /dev/console 
instead of a regular tty.

			Linus

Re: WARNING at: drivers/char/tty_ldisc.c

[Linus Torvalds]

On Sun, 2 Aug 2009, Linus Torvalds wrote:
> 
> I'll try with a serial console on one of my machines to see if I can see 
> anything wrong. It _would_ be nice to get rid of that thing, since it 
> clearly causes problems.

Hmm. I don't see anything wrong. Either as a pure serial console with no 
login (console=ttyS0,115400 console=tty0) or with the serial console 
actually being /dev/console and having a login on it etc.

But I have to admit to only using serial consoles for some very 
occasional debugging, so I might have missed any behavioral differences. 
It rebooted cleanly with no messages. But in this case "no messages" can 
obviously be both a good and a bad thing ;)

		Linus

Re: WARNING at: drivers/char/tty_ldisc.c

[Alan Cox]

> I wonder if the hack is strictly necessary, or could perhaps at least be 
> moved down a bit. The comment around that area seems to imply there were 
> other issues ("will cause tty->count and state->count to go out of sync"), 
> and that whole tty->count thing is some seriously old code, and the tty 
> layer has changed a lot since 1992.

They will go out of sync for some hardware. Other stuff will get shut
down. 

> So I do get the feeling that it may be just old code that simply nobody 
> has dared remove - it may have made more sense back when then it does 
> now, and has just been carried around.

Alas not: eg

->hangup
uart_hangup
uart_flush_buffer
uart_shutdown
	drops carrier and dtr
	shuts down the port
	frees the IRQ handler
	kills off the transmit buffer & tasklet

whether the resulting mess happens to still work with printk rather
depends upon what ->ops->shutdown does. Even on x86 that will drop a
remote console if the carrier is being used so you won't see printk
messages after that point as you got a modem hangup.

A re-open as a device isn't a problem (we cleared ASYNCB_INITIALIZED) -
its the printk side that kills you, and someone is going to have to read
all the embedded device consoles as they are both the most likely to
break and most used 8(

> If it actually were to cause problems with the tty->count thing, I think 
> Sergey should have seen it thanks to us still doing that old 
> CHECK_TTY_COUNT thing.

Only if you get things like crossing open/close and the port bumps the
counts internally for console uses (where hangup will then set it to zero
and break). USB still does that but the last patches to USB also pushed
hangup/console awareness into USB a bit so it knows at the hardware level
not to shut down the console device entirely.

Alan

Re: WARNING at: drivers/char/tty_ldisc.c

[OGAWA Hirofumi]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

>> So I do get the feeling that it may be just old code that simply nobody 
>> has dared remove - it may have made more sense back when then it does 
>> now, and has just been carried around.
>
> Alas not: eg
>
> ->hangup
> uart_hangup
> uart_flush_buffer
> uart_shutdown
> 	drops carrier and dtr
> 	shuts down the port
> 	frees the IRQ handler
> 	kills off the transmit buffer & tasklet
>
> whether the resulting mess happens to still work with printk rather
> depends upon what ->ops->shutdown does. Even on x86 that will drop a
> remote console if the carrier is being used so you won't see printk
> messages after that point as you got a modem hangup.
>
> A re-open as a device isn't a problem (we cleared ASYNCB_INITIALIZED) -
> its the printk side that kills you, and someone is going to have to read
> all the embedded device consoles as they are both the most likely to
> break and most used 8(

I see. I guess it is explaining the following part

	if (cons_filp) {
		if (tty->ops->close)
			for (n = 0; n < closecount; n++)
				tty->ops->close(tty, cons_filp);
	} else if (tty->ops->hangup)
		(tty->ops->hangup)(tty);

the above seem to avoid serial hangup... Luckily, I found the patch of
it. But, even before the patch, it seems we didn't

	filp->f_op = &hung_up_tty_fops;

for console stuff. But, I'm still not found why we avoid to the above...

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

commit a972c49404c18ed15db2c1cce5f414293f73c8d9
Author: Linus Torvalds <torvalds@linuxfoundation.org>
Date:   Fri Nov 23 15:16:55 2007 -0500

    Import 2.1.125pre2

diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index e8b116f..2a2f2de 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -390,7 +390,9 @@ void do_tty_hangup(void *data)
 {
 	struct tty_struct *tty = (struct tty_struct *) data;
 	struct file * filp;
+	struct file * cons_filp = NULL;
 	struct task_struct *p;
+	int    closecount = 0, n;
 
 	if (!tty)
 		return;
@@ -407,10 +409,13 @@ void do_tty_hangup(void *data)
 		if (!filp->f_dentry->d_inode)
 			continue;
 		if (filp->f_dentry->d_inode->i_rdev == CONSOLE_DEV ||
-		    filp->f_dentry->d_inode->i_rdev == SYSCONS_DEV)
+		    filp->f_dentry->d_inode->i_rdev == SYSCONS_DEV) {
+			cons_filp = filp;
 			continue;
+		}
 		if (filp->f_op != &tty_fops)
 			continue;
+		closecount++;
 		tty_fasync(-1, filp, 0);
 		filp->f_op = &hung_up_tty_fops;
 	}
@@ -470,7 +475,17 @@ void do_tty_hangup(void *data)
 	tty->session = 0;
 	tty->pgrp = -1;
 	tty->ctrl_status = 0;
-	if (tty->driver.hangup)
+	/*
+	 *	If one of the devices matches a console pointer, we
+	 *	cannot just call hangup() because that will cause
+	 *	tty->count and state->count to go out of sync.
+	 *	So we just call close() the right number of times.
+	 */
+	if (cons_filp) {
+		if (tty->driver.close)
+			for (n = 0; n < closecount; n++)
+				tty->driver.close(tty, cons_filp);
+	} else if (tty->driver.hangup)
 		(tty->driver.hangup)(tty);
 	unlock_kernel();
 }
@@ -1243,6 +1258,7 @@ retry_open:
 		if (!c)
                         return -ENODEV;
                 device = c->device(c);
+		filp->f_flags |= O_NONBLOCK; /* Don't let /dev/console block */
 		noctty = 1;
 	}
 #ifdef CONFIG_UNIX98_PTYS

Re: WARNING at: drivers/char/tty_ldisc.c

[Alan Cox]

> it. But, even before the patch, it seems we didn't
> 
> 	filp->f_op = &hung_up_tty_fops;
> 
> for console stuff. But, I'm still not found why we avoid to the above...

Not sure - I'd have to re-read all the specs. Could be that applications
writing to the console do not expect to get a hangup when the device for
the console ceases to be a controlling terminal. That would be a
behaviour change. Equally if we don't set the hung_up file operations we
can't call ->hangup().

I'd be very concerned about having the console behaviour changed. For a
late -rc I'd think it may even be better to leak an ldisc instance in
that case providing end users can't cause a repeated leak of that form.

(ie if its got a use count when we got to free it we just look the other
way or queue the kfree)

The more I look at this the more risky all the changes look except for
switching it to refcount properly.

We risk changing the behaviour of consoles in ways that might break apps
and certainly break our behaviour. We end up risking messing up serial
consoles and trying to printk to down devices.

Proper refcounting on the other hand fixes the real problem in a
controlled fashion without hard to analyse and define behaviour changes.

Alan

[PATCH 0/2] proper tty-ldisc refcounting (was Re: WARNING at: drivers/char/tty_ldisc.c)

[Linus Torvalds]

On Sun, 2 Aug 2009, Linus Torvalds wrote:
> 
> > You could just finish the ldisc refcounting. The last set of patches you
> > had off me split tty->ldisc from struct tty ready to do exactly that and
> > I don't think there is anything left that stops it happening now (It was
> > just not ready in time)
> 
> I considered it, and it didn't look horrible (the thing really is pretty 
> self-contained in tty_ldisc_try() and tty_ldisc_deref()).

Here we are.

It wasn't a straight conversion, because the old code really didn't think 
of the refcounts as lifetimes, but it wasn't too bad either. And doing the 
proper refcounting makes all the stupid "wait for idle" go away, so it 
actually removes code:

 drivers/char/tty_ldisc.c  |  145 ++++++++++++++------------------------------
 include/linux/tty_ldisc.h |    2 +-
 2 files changed, 47 insertions(+), 100 deletions(-)

and generally simplifies the logic.

That said, looking through the code as I did this, I consciously avoided 
doing some other cleanups that really should be done some day. The code is 
chock-full of crazy stuff, where we just do

	o_ldisc = tty->ldisc;

with dubious locking. None of that is _new_ though, and most of it is in 
the "replace one ldisc with another" code. And for all I know, maybe it's 
all fine, it's just very much not _obviously_ correct.

As far as I can tell, this short series should not introduce any new 
problems, but hey, maybe it leaks ldisc references like mad because I made 
some silly mistake. It's a _fairly_ straightforward cleanup, but it's a 
big cnnceptual change to go from a model with a "wait until idle and then 
free" to a model of "count users and free on last use", and I could easily 
have screwed up something.

"It works for me"(tm), including a shutdown/reboot cycle. 

Sergey, mind testing? You seem to be very good at consistently triggering 
odd things in the tty layer that few other people seem to ever hit.

Greg - I've signed off on these, but I wasn't planning on committing them 
to my master branch. So perhaps you could do these as the new tty 
maintainer, assuming we get an ack from Alan and testing by Sergey.

		Linus

[PATCH 1/2] tty-ldisc: make refcount be atomic_t 'users' count

[Linus Torvalds]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Mon, 3 Aug 2009 10:07:09 -0700
Subject: [PATCH 1/2] tty-ldisc: make refcount be atomic_t 'users' count

This is pure preparation of changing the ldisc reference counting to be
a true refcount that defines the lifetime of the ldisc.  But this is a
purely syntactic change for now to make the next steps easier.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

This patch should make no semantic changes at all. But I wanted to make 
the ldisc refcount be an atomic (I will be touching it without locks soon 
enough), and I wanted to rename it so that there isn't quite as much 
confusion between 'ldo->refcount' (ldisk operations refcount) and 
'ld->refcount' (ldisc refcount itself) in the same file.

So it's now an atomic 'ld->users' count. It still starts at zero, despite 
having a reference from 'tty->ldisc', but that will change once we turn it 
into a _real_ refcount.

 drivers/char/tty_ldisc.c  |   18 ++++++++----------
 include/linux/tty_ldisc.h |    2 +-
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/char/tty_ldisc.c b/drivers/char/tty_ldisc.c
index acd76b7..fd175e6 100644
--- a/drivers/char/tty_ldisc.c
+++ b/drivers/char/tty_ldisc.c
@@ -142,7 +142,7 @@ static struct tty_ldisc *tty_ldisc_try_get(int disc)
 			/* lock it */
 			ldops->refcount++;
 			ld->ops = ldops;
-			ld->refcount = 0;
+			atomic_set(&ld->users, 0);
 			err = 0;
 		}
 	}
@@ -206,7 +206,7 @@ static void tty_ldisc_put(struct tty_ldisc *ld)
 	ldo->refcount--;
 	module_put(ldo->owner);
 	spin_unlock_irqrestore(&tty_ldisc_lock, flags);
-	WARN_ON(ld->refcount);
+	WARN_ON(atomic_read(&ld->users));
 	kfree(ld);
 }
 
@@ -297,7 +297,7 @@ static int tty_ldisc_try(struct tty_struct *tty)
 	spin_lock_irqsave(&tty_ldisc_lock, flags);
 	ld = tty->ldisc;
 	if (test_bit(TTY_LDISC, &tty->flags)) {
-		ld->refcount++;
+		atomic_inc(&ld->users);
 		ret = 1;
 	}
 	spin_unlock_irqrestore(&tty_ldisc_lock, flags);
@@ -324,7 +324,7 @@ struct tty_ldisc *tty_ldisc_ref_wait(struct tty_struct *tty)
 {
 	/* wait_event is a macro */
 	wait_event(tty_ldisc_wait, tty_ldisc_try(tty));
-	WARN_ON(tty->ldisc->refcount == 0);
+	WARN_ON(atomic_read(&tty->ldisc->users) == 0);
 	return tty->ldisc;
 }
 EXPORT_SYMBOL_GPL(tty_ldisc_ref_wait);
@@ -365,11 +365,9 @@ void tty_ldisc_deref(struct tty_ldisc *ld)
 	BUG_ON(ld == NULL);
 
 	spin_lock_irqsave(&tty_ldisc_lock, flags);
-	if (ld->refcount == 0)
+	if (atomic_read(&ld->users) == 0)
 		printk(KERN_ERR "tty_ldisc_deref: no references.\n");
-	else
-		ld->refcount--;
-	if (ld->refcount == 0)
+	else if (atomic_dec_and_test(&ld->users))
 		wake_up(&tty_ldisc_wait);
 	spin_unlock_irqrestore(&tty_ldisc_lock, flags);
 }
@@ -536,10 +534,10 @@ static int tty_ldisc_wait_idle(struct tty_struct *tty)
 {
 	unsigned long flags;
 	spin_lock_irqsave(&tty_ldisc_lock, flags);
-	while (tty->ldisc->refcount) {
+	while (atomic_read(&tty->ldisc->users)) {
 		spin_unlock_irqrestore(&tty_ldisc_lock, flags);
 		if (wait_event_timeout(tty_ldisc_wait,
-				tty->ldisc->refcount == 0, 5 * HZ) == 0)
+				atomic_read(&tty->ldisc->users) == 0, 5 * HZ) == 0)
 			return -EBUSY;
 		spin_lock_irqsave(&tty_ldisc_lock, flags);
 	}
diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h
index 40f38d8..0c4ee9b 100644
--- a/include/linux/tty_ldisc.h
+++ b/include/linux/tty_ldisc.h
@@ -144,7 +144,7 @@ struct tty_ldisc_ops {
 
 struct tty_ldisc {
 	struct tty_ldisc_ops *ops;
-	int refcount;
+	atomic_t users;
 };
 
 #define TTY_LDISC_MAGIC	0x5403
-- 
1.6.4.21.g73b866

[PATCH 2/2] tty-ldisc: turn ldisc user count into a proper refcount

[Linus Torvalds]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Mon, 3 Aug 2009 10:38:46 -0700
Subject: [PATCH 2/2] tty-ldisc: turn ldisc user count into a proper refcount

By using the user count for the actual lifetime rules, we can get rid of
the silly "wait_for_idle" logic, because any busy ldisc will
automatically stay around until the last user releases it.  This avoids
a host of odd issues, and simplifies the code.

So now, when the last ldisc reference is dropped, we just release the
ldisc operations struct reference, and free the ldisc.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

So this is obviously the real meat of the changes.

It looks obvious enough, and it does work for me, but the counting _could_ 
be off. It probably isn't (bad counting in the new version would generally 
imply that the old code did something really bad, like free an ldisc with 
a non-zero count), but it does need some testing, and preferably somebody 
looking at it.

With this change, both 'tty_ldisc_put()' and 'tty_ldisc_deref()' are just 
aliases for the new ref-counting 'put_ldisc()'. Both of them decrement the 
ldisc user count and free it if it goes down to zero. They're identical
functions, in other words.

But the reason they still exist as sepate functions is that one of them 
was exported (tty_ldisc_deref) and had a stupid name (so I don't want to 
use it as the main name), and the other one was used in multiple places 
(and I didn't want to make the patch larger just to rename the users).

In addition to the refcounting, I did do some minimal cleanup. For 
example, now "tty_ldisc_try()" actually returns the ldisc it got under the 
lock, rather than returning true/false and then the caller would look up 
the ldisc again (now without the protection of the lock).

That said, there's tons of dubious use of 'tty->ldisc' without obviously 
proper locking or refcounting left. I expressly did _not_ want to try to 
fix it all, keeping the patch minimal. There may or may not be bugs in 
that kind of code, but they wouldn't be _new_ bugs.

That said, even if the bugs aren't new, the timing and lifetime will 
change. For example, some silly code may depend on the 'tty->ldisc' 
pointer not changing because they hold a refcount on the 'ldisc'. And 
that's no longer true - if you hold a ref on the ldisc, the 'ldisc' itself 
is safe, but tty->ldisc may change. 

So the proper locking (remains) to hold tty->ldisc_mutex if you expect 
tty->ldisc to be stable. That's not really a _new_ rule, but it's an 
example of something that the old code might have unintentionally depended 
on and hidden bugs.

Whatever. The patch _looks_ sensible to me. The only users of ldisc->users 
are:
 - get_ldisc() - atomically increment the count

 - put_ldisc() - atomically decrements the count and releases if zero

 - tty_ldisc_try_get() - creates the ldisc, and sets the count to 1.
   The ldisc should then either be released, or be attached to a tty.

Comments? Testing?

 drivers/char/tty_ldisc.c |  143 +++++++++++++++-------------------------------
 1 files changed, 46 insertions(+), 97 deletions(-)

diff --git a/drivers/char/tty_ldisc.c b/drivers/char/tty_ldisc.c
index fd175e6..be55dfc 100644
--- a/drivers/char/tty_ldisc.c
+++ b/drivers/char/tty_ldisc.c
@@ -48,6 +48,34 @@ static DECLARE_WAIT_QUEUE_HEAD(tty_ldisc_wait);
 /* Line disc dispatch table */
 static struct tty_ldisc_ops *tty_ldiscs[NR_LDISCS];
 
+static inline struct tty_ldisc *get_ldisc(struct tty_ldisc *ld)
+{
+	if (ld)
+		atomic_inc(&ld->users);
+	return ld;
+}
+
+static inline void put_ldisc(struct tty_ldisc *ld)
+{
+	if (WARN_ON_ONCE(!ld))
+		return;
+
+	/*
+	 * If this is the last user, free the ldisc, and
+	 * release the ldisc ops.
+	 */
+	if (atomic_dec_and_test(&ld->users)) {
+		unsigned long flags;
+		struct tty_ldisc_ops *ldo = ld->ops;
+
+		kfree(ld);
+		spin_lock_irqsave(&tty_ldisc_lock, flags);
+		ldo->refcount--;
+		module_put(ldo->owner);
+		spin_unlock_irqrestore(&tty_ldisc_lock, flags);
+	}
+}
+
 /**
  *	tty_register_ldisc	-	install a line discipline
  *	@disc: ldisc number
@@ -142,7 +170,7 @@ static struct tty_ldisc *tty_ldisc_try_get(int disc)
 			/* lock it */
 			ldops->refcount++;
 			ld->ops = ldops;
-			atomic_set(&ld->users, 0);
+			atomic_set(&ld->users, 1);
 			err = 0;
 		}
 	}
@@ -181,35 +209,6 @@ static struct tty_ldisc *tty_ldisc_get(int disc)
 	return ld;
 }
 
-/**
- *	tty_ldisc_put		-	drop ldisc reference
- *	@ld: ldisc
- *
- *	Drop a reference to a line discipline. Manage refcounts and
- *	module usage counts. Free the ldisc once the recount hits zero.
- *
- *	Locking:
- *		takes tty_ldisc_lock to guard against ldisc races
- */
-
-static void tty_ldisc_put(struct tty_ldisc *ld)
-{
-	unsigned long flags;
-	int disc = ld->ops->num;
-	struct tty_ldisc_ops *ldo;
-
-	BUG_ON(disc < N_TTY || disc >= NR_LDISCS);
-
-	spin_lock_irqsave(&tty_ldisc_lock, flags);
-	ldo = tty_ldiscs[disc];
-	BUG_ON(ldo->refcount == 0);
-	ldo->refcount--;
-	module_put(ldo->owner);
-	spin_unlock_irqrestore(&tty_ldisc_lock, flags);
-	WARN_ON(atomic_read(&ld->users));
-	kfree(ld);
-}
-
 static void *tty_ldiscs_seq_start(struct seq_file *m, loff_t *pos)
 {
 	return (*pos < NR_LDISCS) ? pos : NULL;
@@ -234,7 +233,7 @@ static int tty_ldiscs_seq_show(struct seq_file *m, void *v)
 	if (IS_ERR(ld))
 		return 0;
 	seq_printf(m, "%-10s %2d\n", ld->ops->name ? ld->ops->name : "???", i);
-	tty_ldisc_put(ld);
+	put_ldisc(ld);
 	return 0;
 }
 
@@ -288,20 +287,17 @@ static void tty_ldisc_assign(struct tty_struct *tty, struct tty_ldisc *ld)
  *	Locking: takes tty_ldisc_lock
  */
 
-static int tty_ldisc_try(struct tty_struct *tty)
+static struct tty_ldisc *tty_ldisc_try(struct tty_struct *tty)
 {
 	unsigned long flags;
 	struct tty_ldisc *ld;
-	int ret = 0;
 
 	spin_lock_irqsave(&tty_ldisc_lock, flags);
-	ld = tty->ldisc;
-	if (test_bit(TTY_LDISC, &tty->flags)) {
-		atomic_inc(&ld->users);
-		ret = 1;
-	}
+	ld = NULL;
+	if (test_bit(TTY_LDISC, &tty->flags))
+		ld = get_ldisc(tty->ldisc);
 	spin_unlock_irqrestore(&tty_ldisc_lock, flags);
-	return ret;
+	return ld;
 }
 
 /**
@@ -322,10 +318,11 @@ static int tty_ldisc_try(struct tty_struct *tty)
 
 struct tty_ldisc *tty_ldisc_ref_wait(struct tty_struct *tty)
 {
+	struct tty_ldisc *ld;
+
 	/* wait_event is a macro */
-	wait_event(tty_ldisc_wait, tty_ldisc_try(tty));
-	WARN_ON(atomic_read(&tty->ldisc->users) == 0);
-	return tty->ldisc;
+	wait_event(tty_ldisc_wait, (ld = tty_ldisc_try(tty)) != NULL);
+	return ld;
 }
 EXPORT_SYMBOL_GPL(tty_ldisc_ref_wait);
 
@@ -342,9 +339,7 @@ EXPORT_SYMBOL_GPL(tty_ldisc_ref_wait);
 
 struct tty_ldisc *tty_ldisc_ref(struct tty_struct *tty)
 {
-	if (tty_ldisc_try(tty))
-		return tty->ldisc;
-	return NULL;
+	return tty_ldisc_try(tty);
 }
 EXPORT_SYMBOL_GPL(tty_ldisc_ref);
 
@@ -360,19 +355,15 @@ EXPORT_SYMBOL_GPL(tty_ldisc_ref);
 
 void tty_ldisc_deref(struct tty_ldisc *ld)
 {
-	unsigned long flags;
-
-	BUG_ON(ld == NULL);
-
-	spin_lock_irqsave(&tty_ldisc_lock, flags);
-	if (atomic_read(&ld->users) == 0)
-		printk(KERN_ERR "tty_ldisc_deref: no references.\n");
-	else if (atomic_dec_and_test(&ld->users))
-		wake_up(&tty_ldisc_wait);
-	spin_unlock_irqrestore(&tty_ldisc_lock, flags);
+	put_ldisc(ld);
 }
 EXPORT_SYMBOL_GPL(tty_ldisc_deref);
 
+static inline void tty_ldisc_put(struct tty_ldisc *ld)
+{
+	put_ldisc(ld);
+}
+
 /**
  *	tty_ldisc_enable	-	allow ldisc use
  *	@tty: terminal to activate ldisc on
@@ -521,31 +512,6 @@ static int tty_ldisc_halt(struct tty_struct *tty)
 }
 
 /**
- *	tty_ldisc_wait_idle	-	wait for the ldisc to become idle
- *	@tty: tty to wait for
- *
- *	Wait for the line discipline to become idle. The discipline must
- *	have been halted for this to guarantee it remains idle.
- *
- *	tty_ldisc_lock protects the ref counts currently.
- */
-
-static int tty_ldisc_wait_idle(struct tty_struct *tty)
-{
-	unsigned long flags;
-	spin_lock_irqsave(&tty_ldisc_lock, flags);
-	while (atomic_read(&tty->ldisc->users)) {
-		spin_unlock_irqrestore(&tty_ldisc_lock, flags);
-		if (wait_event_timeout(tty_ldisc_wait,
-				atomic_read(&tty->ldisc->users) == 0, 5 * HZ) == 0)
-			return -EBUSY;
-		spin_lock_irqsave(&tty_ldisc_lock, flags);
-	}
-	spin_unlock_irqrestore(&tty_ldisc_lock, flags);
-	return 0;
-}
-
-/**
  *	tty_set_ldisc		-	set line discipline
  *	@tty: the terminal to set
  *	@ldisc: the line discipline
@@ -640,14 +606,6 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 
 	flush_scheduled_work();
 
-	/* Let any existing reference holders finish */
-	retval = tty_ldisc_wait_idle(tty);
-	if (retval < 0) {
-		clear_bit(TTY_LDISC_CHANGING, &tty->flags);
-		tty_ldisc_put(new_ldisc);
-		return retval;
-	}
-
 	mutex_lock(&tty->ldisc_mutex);
 	if (test_bit(TTY_HUPPED, &tty->flags)) {
 		/* We were raced by the hangup method. It will have stomped
@@ -793,7 +751,6 @@ void tty_ldisc_hangup(struct tty_struct *tty)
 		if (tty->ldisc) {	/* Not yet closed */
 			/* Switch back to N_TTY */
 			tty_ldisc_halt(tty);
-			tty_ldisc_wait_idle(tty);
 			tty_ldisc_reinit(tty);
 			/* At this point we have a closed ldisc and we want to
 			   reopen it. We could defer this to the next open but
@@ -858,14 +815,6 @@ void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
 	tty_ldisc_halt(tty);
 	flush_scheduled_work();
 
-	/*
-	 * Wait for any short term users (we know they are just driver
-	 * side waiters as the file is closing so user count on the file
-	 * side is zero.
-	 */
-
-	tty_ldisc_wait_idle(tty);
-
 	mutex_lock(&tty->ldisc_mutex);
 	/*
 	 * Now kill off the ldisc
-- 
1.6.4.21.g73b866

Re: [PATCH 2/2] tty-ldisc: turn ldisc user count into a proper refcount

[Alan Cox]

Looks good to me on first review.

Acked-by: Alan Cox <alan@linux.intel.com>

Re: [PATCH 2/2] tty-ldisc: turn ldisc user count into a proper refcount

[OGAWA Hirofumi]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> Whatever. The patch _looks_ sensible to me. The only users of ldisc->users 
> are:
>  - get_ldisc() - atomically increment the count
>
>  - put_ldisc() - atomically decrements the count and releases if zero
>
>  - tty_ldisc_try_get() - creates the ldisc, and sets the count to 1.
>    The ldisc should then either be released, or be attached to a tty.
>
> Comments? Testing?

This works for me. And it seems there is no refcount leak in this
shutdown case.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 0/2] proper tty-ldisc refcounting (was Re: WARNING at: drivers/char/tty_ldisc.c)

[Greg KH]

On Mon, Aug 03, 2009 at 10:55:34AM -0700, Linus Torvalds wrote:
> 
> 
> On Sun, 2 Aug 2009, Linus Torvalds wrote:
> > 
> > > You could just finish the ldisc refcounting. The last set of patches you
> > > had off me split tty->ldisc from struct tty ready to do exactly that and
> > > I don't think there is anything left that stops it happening now (It was
> > > just not ready in time)
> > 
> > I considered it, and it didn't look horrible (the thing really is pretty 
> > self-contained in tty_ldisc_try() and tty_ldisc_deref()).
> 
> Here we are.
> 
> It wasn't a straight conversion, because the old code really didn't think 
> of the refcounts as lifetimes, but it wasn't too bad either. And doing the 
> proper refcounting makes all the stupid "wait for idle" go away, so it 
> actually removes code:
> 
>  drivers/char/tty_ldisc.c  |  145 ++++++++++++++------------------------------
>  include/linux/tty_ldisc.h |    2 +-
>  2 files changed, 47 insertions(+), 100 deletions(-)
> 
> and generally simplifies the logic.
> 
> That said, looking through the code as I did this, I consciously avoided 
> doing some other cleanups that really should be done some day. The code is 
> chock-full of crazy stuff, where we just do
> 
> 	o_ldisc = tty->ldisc;
> 
> with dubious locking. None of that is _new_ though, and most of it is in 
> the "replace one ldisc with another" code. And for all I know, maybe it's 
> all fine, it's just very much not _obviously_ correct.
> 
> As far as I can tell, this short series should not introduce any new 
> problems, but hey, maybe it leaks ldisc references like mad because I made 
> some silly mistake. It's a _fairly_ straightforward cleanup, but it's a 
> big cnnceptual change to go from a model with a "wait until idle and then 
> free" to a model of "count users and free on last use", and I could easily 
> have screwed up something.
> 
> "It works for me"(tm), including a shutdown/reboot cycle. 
> 
> Sergey, mind testing? You seem to be very good at consistently triggering 
> odd things in the tty layer that few other people seem to ever hit.
> 
> Greg - I've signed off on these, but I wasn't planning on committing them 
> to my master branch. So perhaps you could do these as the new tty 
> maintainer, assuming we get an ack from Alan and testing by Sergey.

Ok, I'll queue them up if they pass Sergey's testing.

thanks,

greg k-h

Re: [PATCH 0/2] proper tty-ldisc refcounting (was Re: WARNING at: drivers/char/tty_ldisc.c)

[Linus Torvalds]

On Mon, 3 Aug 2009, Greg KH wrote:
> 
> Ok, I'll queue them up if they pass Sergey's testing.

Btw, looking at my email, I think the commentary explanations I wrote for 
you guys (under the "---") might as well be lifted up into the actual 
commit comments. They're valid long-term commentary on some of the issues 
that remain.

		Linus

Re: [PATCH 0/2] proper tty-ldisc refcounting (was Re: WARNING at: drivers/char/tty_ldisc.c)

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 152 bytes --]

On (08/03/09 11:18), Greg KH wrote:
> Ok, I'll queue them up if they pass Sergey's testing.
> 

I've been away. Sorry.
Will test ASAP.

	Sergey

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 315 bytes --]

Re: [PATCH 0/2] proper tty-ldisc refcounting (was Re: WARNING at: drivers/char/tty_ldisc.c)

[Linus Torvalds]

On Tue, 4 Aug 2009, Sergey Senozhatsky wrote:

> On (08/03/09 11:18), Greg KH wrote:
> > Ok, I'll queue them up if they pass Sergey's testing.
> > 
> 
> I've been away. Sorry.
> Will test ASAP.

Absolutely nothing to be sorry about - you've been an exemplary tester 
with very low latency. Thanks a lot.

Ogawa found a race in my original 2/2, and Greg has a small fix pending, 
but that almost certainly won't realistically matter for any real-life 
testing, so you don't really need to worry about it. 

But I'll forward that patch (and another couple cleanup patch) for you for 
testing after I've verified it myself. But don't feel like you have to 
worry about those extra patches - testing the initial refcount handling is 
the thing that matters most, the thing I have pending really is just 
details.

		Linus

[PATCH 3/2] tty-ldisc: be more careful in 'put_ldisc' locking

[Linus Torvalds]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Mon, 3 Aug 2009 14:55:24 -0700
Subject: [PATCH 3/2] tty-ldisc: be more careful in 'put_ldisc' locking

Use 'atomic_dec_and_lock()' to make sure that we always hold the
tty_ldisc_lock when the ldisc count goes to zero. That way we can never
race against 'tty_ldisc_try()' increasing the count again.

Reported-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 drivers/char/tty_ldisc.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

Ok, this is strictly speaking a bugfix, but the race is so unlikely that I 
doubt that you'll see it in testing.

So testing just patches 1-2 is fine, but this is a good idea on top of 
them.

I'll be sending out two further patches after this that are pure cleanups 
(numbered 4/2 and 5/2). Again, testing them would be wonderful, but not 
essential - they're not as interesting or important as 1-2 were.

On Mon, 3 Aug 2009, Linus Torvalds wrote:
> 
> Ogawa found a race in my original 2/2, and Greg has a small fix pending, 
> but that almost certainly won't realistically matter for any real-life 
> testing, so you don't really need to worry about it. 
> 
> But I'll forward that patch (and another couple cleanup patch) for you for 
> testing after I've verified it myself. But don't feel like you have to 
> worry about those extra patches - testing the initial refcount handling is 
> the thing that matters most, the thing I have pending really is just 
> details.
> 
> 		Linus
> 

diff --git a/drivers/char/tty_ldisc.c b/drivers/char/tty_ldisc.c
index be55dfc..1733d34 100644
--- a/drivers/char/tty_ldisc.c
+++ b/drivers/char/tty_ldisc.c
@@ -55,25 +55,32 @@ static inline struct tty_ldisc *get_ldisc(struct tty_ldisc *ld)
 	return ld;
 }
 
-static inline void put_ldisc(struct tty_ldisc *ld)
+static void put_ldisc(struct tty_ldisc *ld)
 {
+	unsigned long flags;
+
 	if (WARN_ON_ONCE(!ld))
 		return;
 
 	/*
 	 * If this is the last user, free the ldisc, and
 	 * release the ldisc ops.
+	 *
+	 * We really want an "atomic_dec_and_lock_irqsave()",
+	 * but we don't have it, so this does it by hand.
 	 */
-	if (atomic_dec_and_test(&ld->users)) {
-		unsigned long flags;
+	local_irq_save(flags);
+	if (atomic_dec_and_lock(&ld->users, &tty_ldisc_lock)) {
 		struct tty_ldisc_ops *ldo = ld->ops;
 
-		kfree(ld);
-		spin_lock_irqsave(&tty_ldisc_lock, flags);
 		ldo->refcount--;
 		module_put(ldo->owner);
 		spin_unlock_irqrestore(&tty_ldisc_lock, flags);
+
+		kfree(ld);
+		return;
 	}
+	local_irq_restore(flags);
 }
 
 /**
-- 
1.6.4.21.g73b866

[PATCH 4/2] tty-ldisc: make /proc/tty/ldiscs use ldisc_ops instead of ldiscs

[Linus Torvalds]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Mon, 3 Aug 2009 15:11:29 -0700
Subject: [PATCH 4/2] tty-ldisc: make /proc/tty/ldiscs use ldisc_ops instead of ldiscs

The /proc/tty/ldiscs file is totally and utterly un-interested in the
"struct tty_ldisc" structures, and only cares about the underlying ldisc
operations.

So don't make it create a dummy 'struct ldisc' only to get a pointer to
the operations, and then destroy it.  Instead, we split up the function
'tty_ldisc_try_get()', and create a 'get_ldops()' helper that just looks
up the ldisc operations based on the ldisc number.

That makes the code simpler to read (smaller and more well-defined
helper functions), and allows the /proc functions to avoid creating that
useless dummy only to immediately free it again.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

Pure cleanup. This adds a bit more lines than it removes, but that's 
just due to comments and just the new function declarations from splitting 
things up. It doesn't really add any code.

And the next patch will actually remove code, and relies on this cleanup.

 drivers/char/tty_ldisc.c |   65 +++++++++++++++++++++++++++------------------
 1 files changed, 39 insertions(+), 26 deletions(-)

diff --git a/drivers/char/tty_ldisc.c b/drivers/char/tty_ldisc.c
index 1733d34..8106514 100644
--- a/drivers/char/tty_ldisc.c
+++ b/drivers/char/tty_ldisc.c
@@ -145,6 +145,34 @@ int tty_unregister_ldisc(int disc)
 }
 EXPORT_SYMBOL(tty_unregister_ldisc);
 
+static struct tty_ldisc_ops *get_ldops(int disc)
+{
+	unsigned long flags;
+	struct tty_ldisc_ops *ldops, *ret;
+
+	spin_lock_irqsave(&tty_ldisc_lock, flags);
+	ret = ERR_PTR(-EINVAL);
+	ldops = tty_ldiscs[disc];
+	if (ldops) {
+		ret = ERR_PTR(-EAGAIN);
+		if (try_module_get(ldops->owner)) {
+			ldops->refcount++;
+			ret = ldops;
+		}
+	}
+	spin_unlock_irqrestore(&tty_ldisc_lock, flags);
+	return ret;
+}
+
+static void put_ldops(struct tty_ldisc_ops *ldops)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&tty_ldisc_lock, flags);
+	ldops->refcount--;
+	module_put(ldops->owner);
+	spin_unlock_irqrestore(&tty_ldisc_lock, flags);
+}
 
 /**
  *	tty_ldisc_try_get	-	try and reference an ldisc
@@ -156,36 +184,21 @@ EXPORT_SYMBOL(tty_unregister_ldisc);
 
 static struct tty_ldisc *tty_ldisc_try_get(int disc)
 {
-	unsigned long flags;
 	struct tty_ldisc *ld;
 	struct tty_ldisc_ops *ldops;
-	int err = -EINVAL;
 
 	ld = kmalloc(sizeof(struct tty_ldisc), GFP_KERNEL);
 	if (ld == NULL)
 		return ERR_PTR(-ENOMEM);
 
-	spin_lock_irqsave(&tty_ldisc_lock, flags);
-	ld->ops = NULL;
-	ldops = tty_ldiscs[disc];
-	/* Check the entry is defined */
-	if (ldops) {
-		/* If the module is being unloaded we can't use it */
-		if (!try_module_get(ldops->owner))
-			err = -EAGAIN;
-		else {
-			/* lock it */
-			ldops->refcount++;
-			ld->ops = ldops;
-			atomic_set(&ld->users, 1);
-			err = 0;
-		}
-	}
-	spin_unlock_irqrestore(&tty_ldisc_lock, flags);
-	if (err) {
+	ldops = get_ldops(disc);
+	if (IS_ERR(ldops)) {
 		kfree(ld);
-		return ERR_PTR(err);
+		return ERR_CAST(ldops);
 	}
+
+	ld->ops = ldops;
+	atomic_set(&ld->users, 1);
 	return ld;
 }
 
@@ -234,13 +247,13 @@ static void tty_ldiscs_seq_stop(struct seq_file *m, void *v)
 static int tty_ldiscs_seq_show(struct seq_file *m, void *v)
 {
 	int i = *(loff_t *)v;
-	struct tty_ldisc *ld;
+	struct tty_ldisc_ops *ldops;
 
-	ld = tty_ldisc_try_get(i);
-	if (IS_ERR(ld))
+	ldops = get_ldops(i);
+	if (IS_ERR(ldops))
 		return 0;
-	seq_printf(m, "%-10s %2d\n", ld->ops->name ? ld->ops->name : "???", i);
-	put_ldisc(ld);
+	seq_printf(m, "%-10s %2d\n", ldops->name ? ldops->name : "???", i);
+	put_ldops(ldops);
 	return 0;
 }
 
-- 
1.6.4.21.g73b866

[PATCH 5/2] tty-ldisc: get rid of tty_ldisc_try_get() helper function

[Linus Torvalds]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Mon, 3 Aug 2009 15:30:29 -0700
Subject: [PATCH 5/2] tty-ldisc: get rid of tty_ldisc_try_get() helper function

Now that the /proc/tty/ldiscs handling doesn't play games with 'struct
ldisc' any more, the only remaining user of 'tty_ldisc_try_get()' is
'tty_ldisc_get()' (note the lack of 'try').

And we're actually much better off folding the logic directly into that
file, since the 'try' part was always about trying to get the ldisc
operations, not the ldisc itself: and making that explicit inside of
'tty_ldisc_get()' clarifies the whole semantics.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

Just consolidation and cleanup.

 drivers/char/tty_ldisc.c |   51 ++++++++++++++++++----------------------------
 1 files changed, 20 insertions(+), 31 deletions(-)

diff --git a/drivers/char/tty_ldisc.c b/drivers/char/tty_ldisc.c
index 8106514..706de25 100644
--- a/drivers/char/tty_ldisc.c
+++ b/drivers/char/tty_ldisc.c
@@ -175,34 +175,6 @@ static void put_ldops(struct tty_ldisc_ops *ldops)
 }
 
 /**
- *	tty_ldisc_try_get	-	try and reference an ldisc
- *	@disc: ldisc number
- *
- *	Attempt to open and lock a line discipline into place. Return
- *	the line discipline refcounted or an error.
- */
-
-static struct tty_ldisc *tty_ldisc_try_get(int disc)
-{
-	struct tty_ldisc *ld;
-	struct tty_ldisc_ops *ldops;
-
-	ld = kmalloc(sizeof(struct tty_ldisc), GFP_KERNEL);
-	if (ld == NULL)
-		return ERR_PTR(-ENOMEM);
-
-	ldops = get_ldops(disc);
-	if (IS_ERR(ldops)) {
-		kfree(ld);
-		return ERR_CAST(ldops);
-	}
-
-	ld->ops = ldops;
-	atomic_set(&ld->users, 1);
-	return ld;
-}
-
-/**
  *	tty_ldisc_get		-	take a reference to an ldisc
  *	@disc: ldisc number
  *
@@ -218,14 +190,31 @@ static struct tty_ldisc *tty_ldisc_try_get(int disc)
 static struct tty_ldisc *tty_ldisc_get(int disc)
 {
 	struct tty_ldisc *ld;
+	struct tty_ldisc_ops *ldops;
 
 	if (disc < N_TTY || disc >= NR_LDISCS)
 		return ERR_PTR(-EINVAL);
-	ld = tty_ldisc_try_get(disc);
-	if (IS_ERR(ld)) {
+
+	/*
+	 * Get the ldisc ops - we may need to request them to be loaded
+	 * dynamically and try again.
+	 */
+	ldops = get_ldops(disc);
+	if (IS_ERR(ldops)) {
 		request_module("tty-ldisc-%d", disc);
-		ld = tty_ldisc_try_get(disc);
+		ldops = get_ldops(disc);
+		if (IS_ERR(ldops))
+			return ERR_CAST(ldops);
+	}
+
+	ld = kmalloc(sizeof(struct tty_ldisc), GFP_KERNEL);
+	if (ld == NULL) {
+		put_ldops(ldops);
+		return ERR_PTR(-ENOMEM);
 	}
+
+	ld->ops = ldops;
+	atomic_set(&ld->users, 1);
 	return ld;
 }
 
-- 
1.6.4.21.g73b866

Re: proper tty-ldisc refcounting (was Re: WARNING at: drivers/char/tty_ldisc.c)

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 827 bytes --]

On (08/03/09 11:18), Greg KH wrote:
> Ok, I'll queue them up if they pass Sergey's testing.
> 
  
patch -p1 < tty_patch_1_2 
patching file drivers/char/tty_ldisc.c
patching file include/linux/tty_ldisc.h
  
patch -p1 < tty_patch_2_2 
patching file drivers/char/tty_ldisc.c
  
patch -p1 < tty_patch_3_2 
patching file drivers/char/tty_ldisc.c
  
patch -p1 < tty_patch_4_2
patching file drivers/char/tty_ldisc.c
  
patch -p1 < tty_patch_5_2 
patching file drivers/char/tty_ldisc.c
  
Works for me (SMP/non-SMP x86 builds). Great job.
  
Tested-by: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
  
  

On (08/03/09 15:25), Linus Torvalds wrote:
> Absolutely nothing to be sorry about - you've been an exemplary tester
> with very low latency. Thanks a lot.
Thanks a lot, Linus.


        Sergey

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 315 bytes --]

Re: proper tty-ldisc refcounting (was Re: WARNING at: drivers/char/tty_ldisc.c)

[Linus Torvalds]

On Tue, 4 Aug 2009, Sergey Senozhatsky wrote:
>   
> Works for me (SMP/non-SMP x86 builds). Great job.
>   
> Tested-by: Sergey Senozhatsky <sergey.senozhatsky@mail.by>

Thanks.

Greg - feel free to treat 4/2 and 5/2 any way you want. They are obvious 
enough (no subtle issues) that I'll happily pull them into 2.6.31 too: 
relative to the initial refcount changes (which it looks like we'll need 
to do just to fix the regression) they are pretty safe.

But at the same time, they can equally well go into some later tree for 
the next merge window. So whatever you want to do.

			Linus

Re: proper tty-ldisc refcounting (was Re: WARNING at: drivers/char/tty_ldisc.c)

[Greg KH]

On Mon, Aug 03, 2009 at 05:56:07PM -0700, Linus Torvalds wrote:
> 
> 
> On Tue, 4 Aug 2009, Sergey Senozhatsky wrote:
> >   
> > Works for me (SMP/non-SMP x86 builds). Great job.
> >   
> > Tested-by: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
> 
> Thanks.
> 
> Greg - feel free to treat 4/2 and 5/2 any way you want. They are obvious 
> enough (no subtle issues) that I'll happily pull them into 2.6.31 too: 
> relative to the initial refcount changes (which it looks like we'll need 
> to do just to fix the regression) they are pretty safe.
> 
> But at the same time, they can equally well go into some later tree for 
> the next merge window. So whatever you want to do.

I'll queue them up for .32 as they don't fix a bug that people are
hitting from what I can tell.

thanks,

greg k-h

Re: proper tty-ldisc refcounting (was Re: WARNING at: drivers/char/tty_ldisc.c)

[Greg KH]

On Mon, Aug 03, 2009 at 08:53:19PM -0700, Greg KH wrote:
> On Mon, Aug 03, 2009 at 05:56:07PM -0700, Linus Torvalds wrote:
> > 
> > 
> > On Tue, 4 Aug 2009, Sergey Senozhatsky wrote:
> > >   
> > > Works for me (SMP/non-SMP x86 builds). Great job.
> > >   
> > > Tested-by: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
> > 
> > Thanks.
> > 
> > Greg - feel free to treat 4/2 and 5/2 any way you want. They are obvious 
> > enough (no subtle issues) that I'll happily pull them into 2.6.31 too: 
> > relative to the initial refcount changes (which it looks like we'll need 
> > to do just to fix the regression) they are pretty safe.
> > 
> > But at the same time, they can equally well go into some later tree for 
> > the next merge window. So whatever you want to do.
> 
> I'll queue them up for .32 as they don't fix a bug that people are
> hitting from what I can tell.

Oh wait, the original problem, single user mode.  Hm, we need all of
these to fix that problem?  Or just the first one?

thanks,

greg k-h

Re: proper tty-ldisc refcounting (was Re: WARNING at: drivers/char/tty_ldisc.c)

[Linus Torvalds]

On Mon, 3 Aug 2009, Greg KH wrote:
> 
> Oh wait, the original problem, single user mode.  Hm, we need all of
> these to fix that problem?  Or just the first one?

Patches 1-3 should fix that one. 4-5 are just cleanups with no semantic 
changes.

		Linus

Re: proper tty-ldisc refcounting (was Re: WARNING at: drivers/char/tty_ldisc.c)

[Greg KH]

On Mon, Aug 03, 2009 at 11:19:53PM -0700, Linus Torvalds wrote:
> 
> 
> On Mon, 3 Aug 2009, Greg KH wrote:
> > 
> > Oh wait, the original problem, single user mode.  Hm, we need all of
> > these to fix that problem?  Or just the first one?
> 
> Patches 1-3 should fix that one. 4-5 are just cleanups with no semantic 
> changes.

Ok, but due to the lateness of the release cycle, is it worth it to add
those 3 right now?  Or do we just take the BUG_ON() out as it's pretty
harmless while shutting down in single user mode?

What do you think?

greg k-h

Re: proper tty-ldisc refcounting (was Re: WARNING at: drivers/char/tty_ldisc.c)

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]

On (08/04/09 00:23), Greg KH wrote:
> On Mon, Aug 03, 2009 at 11:19:53PM -0700, Linus Torvalds wrote:
> > 
> > 
> > On Mon, 3 Aug 2009, Greg KH wrote:
> > > 
> > > Oh wait, the original problem, single user mode.  Hm, we need all of
> > > these to fix that problem?  Or just the first one?
> > 
> > Patches 1-3 should fix that one. 4-5 are just cleanups with no semantic 
> > changes.
> 
> Ok, but due to the lateness of the release cycle, is it worth it to add
> those 3 right now?  Or do we just take the BUG_ON() out as it's pretty
> harmless while shutting down in single user mode?
> 
> What do you think?
> 

I don't think that take the BUG_ON out is the best we can do. 

If I understand correctly, this one (tty_ldisc.c:209):
static void tty_ldisc_put(struct tty_ldisc *ld)
{
	[...]
	spin_unlock_irqrestore(&tty_ldisc_lock, flags);
-	WARN_ON(ld->refcount);
	kfree(ld);
}

It's better to leave everything as is in that case and wait 32 (to my mind).

"there may certainly be more tty breakage there too", Linus
and WARN_ON may be quite usefull.

	Sergey

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 315 bytes --]

Re: proper tty-ldisc refcounting (was Re: WARNING at: drivers/char/tty_ldisc.c)

[Greg KH]

On Tue, Aug 04, 2009 at 12:12:37PM +0300, Sergey Senozhatsky wrote:
> On (08/04/09 00:23), Greg KH wrote:
> > On Mon, Aug 03, 2009 at 11:19:53PM -0700, Linus Torvalds wrote:
> > > 
> > > 
> > > On Mon, 3 Aug 2009, Greg KH wrote:
> > > > 
> > > > Oh wait, the original problem, single user mode.  Hm, we need all of
> > > > these to fix that problem?  Or just the first one?
> > > 
> > > Patches 1-3 should fix that one. 4-5 are just cleanups with no semantic 
> > > changes.
> > 
> > Ok, but due to the lateness of the release cycle, is it worth it to add
> > those 3 right now?  Or do we just take the BUG_ON() out as it's pretty
> > harmless while shutting down in single user mode?
> > 
> > What do you think?
> > 
> 
> I don't think that take the BUG_ON out is the best we can do. 
> 
> If I understand correctly, this one (tty_ldisc.c:209):
> static void tty_ldisc_put(struct tty_ldisc *ld)
> {
> 	[...]
> 	spin_unlock_irqrestore(&tty_ldisc_lock, flags);
> -	WARN_ON(ld->refcount);
> 	kfree(ld);
> }
> 
> It's better to leave everything as is in that case and wait 32 (to my mind).

Sorry, you are correct, it was a WARN_ON call that caused the warning.

One other option is to put these changes into .32, and add them back to
the .31 -stable tree once it goes into Linus's tree and no one has any
reported problems.

thanks,

greg k-h

Re: proper tty-ldisc refcounting (was Re: WARNING at: drivers/char/tty_ldisc.c)

[Linus Torvalds]

On Tue, 4 Aug 2009, Greg KH wrote:
> 
> Ok, but due to the lateness of the release cycle, is it worth it to add
> those 3 right now?  Or do we just take the BUG_ON() out as it's pretty
> harmless while shutting down in single user mode?
> 
> What do you think?

If the WARN_ON() happens, it's not just that we have a refcount being off, 
we'll also have memory corruption and a potential oops a bit later. 

Why? Simply because somebody will be touching that 'struct ldisc', even if 
it will be just a decrement of the word that contained the refcount. So we 
have a pretty much guaranteed use-after-free scenario.

So taking out the WARN_ON() is the wrong thing. In that case it would 
probably be better to just leave the WARN_ON(), and at least know "ok, bad 
things happened". 

So I think we have a few options:

 - leave things as-is, leave the WARN_ON(), and know that it's very rare, 
   but that bad things can happen when it triggers.

   The thing I really dislike about this one is that yes, it's rare, but 
   I could see it be user-triggerable. Users do have access to 
   /dev/console when they are logged in at the console.

 - Change the old code from

	WARN_ON(ld->refcount);
	kfree(ld);

   to

	if (!WARN_ON_ONCE(ld->refcount))
		kfree(ld);

   which at least doesn't free the ldisc if it is in use. So now you have 
   a memory leak, but at least hopefully no actual corruption and 
   use-after-free. It's still a bug, but it won't be causing other bugs 
   down the line (except for running out of memory if you can trigger it 
   really easily, but that's unlikely, and I think preferable to unknown 
   problems - even if the unknown problems are very unlikely)

 - Take the three patches now.

I suspect we should take the three now. All of the issues are due to 
totally rewritten code since 2.6.30 - I suspect the risk from new bugs 
from that refcounting series is _smaller_ that the risk of bugs from the 
original ldisc rewrite (commit c65c9bc3e), and if there are bugs, I 
suspect the three patches are more likely to help than to hurt.

			Linus

Re: proper tty-ldisc refcounting (was Re: WARNING at: drivers/char/tty_ldisc.c)

[Greg KH]

On Tue, Aug 04, 2009 at 08:40:20AM -0700, Linus Torvalds wrote:
> I suspect we should take the three now. All of the issues are due to 
> totally rewritten code since 2.6.30 - I suspect the risk from new bugs 
> from that refcounting series is _smaller_ that the risk of bugs from the 
> original ldisc rewrite (commit c65c9bc3e), and if there are bugs, I 
> suspect the three patches are more likely to help than to hurt.

Ok, let me make up a git queue for you to pull them from and test it
first.  It will take a few hours, I have to play taxi for the kids
different summer camps this morning.

thanks,

greg k-h

Re: WARNING at: drivers/char/tty_ldisc.c

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 2096 bytes --]

On (08/02/09 13:20), Linus Torvalds wrote:
> On Sun, 2 Aug 2009, Sergey Senozhatsky wrote:
> > 
> > non-SMP system 'fails' as well.
> 
> Ahh, can you trigger this reliably? Is it 100% of the time when you shut 
> down from single user mode? Or just occasionally?
> 
Yes, 100% in both SMP and non-SMP builds.

> I take it that you can't get a trace with sysrq-T because nothing gets 
> logged, 
TRUE

> and you don't have a serial port console? 
I don't have a serial port console.


I think there is no need in 
	mutex_unlock(&tty->ldisc_mutex); // HACK
	...
	mutex_lock(&tty->ldisc_mutex); // HACK

since we have tty_ldisc.c:794
	mutex_lock(&tty->ldisc_mutex);			<<
        if (tty->ldisc) { /* Not yet closed */

Anyway, code looks like this:
//JUST FOR TEST!

if (tty->driver->flags & TTY_DRIVER_RESET_TERMIOS) {
		/* Avoid racing set_ldisc or tty_ldisc_release */
		//mutex_lock(&tty->ldisc_mutex);		// Commented
		if (tty->ldisc) {	/* Not yet closed */
			int work = 0;
			/* Switch back to N_TTY */
			mutex_unlock(&tty->ldisc_mutex);        // HACK

			work = tty_ldisc_halt(tty);		//to be sure
			if (work) {
				WARN_ON(0 != 1);
				wait_event_timeout(tty_ldisc_wait,
					   (1 == 0), 15 * HZ);	//Just to read what is written on the screen
			}
			
			if (tty_ldisc_wait_idle(tty) != 0) {
				WARN_ON(0 != 1);
				wait_event_timeout(tty_ldisc_wait,
					   (1 == 0), 15 * HZ);	//Just to read what is written on the screen
			}
			mutex_lock(&tty->ldisc_mutex);          // HACK
			tty_ldisc_reinit(tty);
			/* At this point we have a closed ldisc and we want to
			   reopen it. We could defer this to the next open but
			   it means auditing a lot of other paths so this is
			   a FIXME */
			WARN_ON(tty_ldisc_open(tty, tty->ldisc));
			tty_ldisc_enable(tty);
		}
		mutex_unlock(&tty->ldisc_mutex);
		tty_reset_termios(tty);
	}

This code gave me WARNINGS:
tty_io.c:1267		//tty_reopen: WARN_ON(!test_bit(TTY_LDISC, &tty->flags));
tty_ldisc.c:808		//tty_ldisc_wait_idle
tty_ldisc.c:209		//tty_ldisc_put


	Sergey

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 315 bytes --]