[PATCH 1/1] Char: TTY, restore tty_ldisc_wait_idle

[PATCH 1/1] Char: TTY, restore tty_ldisc_wait_idle

[Jiri Slaby]

It was removed in 65b770468e98 (tty-ldisc: turn ldisc user count into
a proper refcount), but we need to wait for last user to quit the
ldisc before we close it in tty_set_ldisc.

Otherwise weird things start to happen. There might be processes
waiting in tty_read->n_tty_read on tty->read_wait for input to appear
and at that moment, a change of ldisc is fatal. n_tty_close is called,
it frees read_buf and the waiting process is still in the middle of
reading and goes nuts after it is woken.

Previously we prevented close to happen when others are in ldisc ops
by tty_ldisc_wait_idle in tty_set_ldisc. But the commit above removed
that. So revoke the change and test whether there is 1 user (=we), and
allow the close then.

I don't understand why tty_ldisc_lock would be needed when it's an
atomic variable, so this is a lockless tty_ldisc_wait_idle.

NOTE: This is an untested patch, because I cannot reproduce myself
(partly because I haven't tried yet) and because I want to hear back
from you.

NOT-signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/char/tty_ldisc.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/drivers/char/tty_ldisc.c b/drivers/char/tty_ldisc.c
index 412f977..86032b5 100644
--- a/drivers/char/tty_ldisc.c
+++ b/drivers/char/tty_ldisc.c
@@ -47,6 +47,7 @@
 
 static DEFINE_SPINLOCK(tty_ldisc_lock);
 static DECLARE_WAIT_QUEUE_HEAD(tty_ldisc_wait);
+static DECLARE_WAIT_QUEUE_HEAD(tty_ldisc_idle);
 /* Line disc dispatch table */
 static struct tty_ldisc_ops *tty_ldiscs[NR_LDISCS];
 
@@ -83,6 +84,7 @@ static void put_ldisc(struct tty_ldisc *ld)
 		return;
 	}
 	local_irq_restore(flags);
+	wake_up(&tty_ldisc_idle);
 }
 
 /**
@@ -531,6 +533,23 @@ 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.
+ */
+static int tty_ldisc_wait_idle(struct tty_struct *tty)
+{
+	int ret;
+	ret = wait_event_interruptible_timeout(tty_ldisc_idle,
+			atomic_read(&tty->ldisc->users) == 1, 5 * HZ);
+	if (ret < 0)
+		return ret;
+	return ret > 0 ? 0 : -EBUSY;
+}
+
+/**
  *	tty_set_ldisc		-	set line discipline
  *	@tty: the terminal to set
  *	@ldisc: the line discipline
@@ -634,6 +653,13 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 
 	flush_scheduled_work();
 
+	retval = tty_ldisc_wait_idle(tty);
+	if (retval) {
+		clear_bit(TTY_LDISC_CHANGING, &tty->flags);
+		tty_ldisc_put(new_ldisc);
+		return retval;
+	}
+
 	tty_lock();
 	mutex_lock(&tty->ldisc_mutex);
 	if (test_bit(TTY_HUPPED, &tty->flags)) {
-- 
1.7.3.1

Re: [PATCH 1/1] Char: TTY, restore tty_ldisc_wait_idle

[Linus Torvalds]

On Thu, Oct 21, 2010 at 6:58 AM, Jiri Slaby <jslaby@suse.cz> wrote:
> It was removed in 65b770468e98 (tty-ldisc: turn ldisc user count into
> a proper refcount), but we need to wait for last user to quit the
> ldisc before we close it in tty_set_ldisc.
>
> Otherwise weird things start to happen. There might be processes
> waiting in tty_read->n_tty_read on tty->read_wait for input to appear
> and at that moment, a change of ldisc is fatal. n_tty_close is called,
> it frees read_buf and the waiting process is still in the middle of
> reading and goes nuts after it is woken.

Hmm. Looks reasonable. And the waiting is outside the lock, so there
aren't any of the problem cases that caused the original changes. And
we don't need the lock, because the TTY_LDISC_CHANGING bit will
protect against anything new coming in, so we don't have races with
the count going up afterwards.

And you're right about the lockless approach being reasonable inside
the testing code too - it's atomic as you say, and we don't touch/care
about anything else.

So I don't have any objections, apart from thinking that the ldisc
code is apparently still too fragile if this is needed.  But the ldisc
change is so special that I don't think this is a unreasonable hack.
Even if it _is_ a bit of a hack still.

So feel free to add an acked-by: from me. Whoever saw the problem
should probably test the patch first, though.

                                Linus

Re: [PATCH 1/1] Char: TTY, restore tty_ldisc_wait_idle

[Jiri Slaby]

On 10/21/2010 04:17 PM, Linus Torvalds wrote:
> On Thu, Oct 21, 2010 at 6:58 AM, Jiri Slaby <jslaby@suse.cz> wrote:
>> It was removed in 65b770468e98 (tty-ldisc: turn ldisc user count into
>> a proper refcount), but we need to wait for last user to quit the
>> ldisc before we close it in tty_set_ldisc.
>>
>> Otherwise weird things start to happen. There might be processes
>> waiting in tty_read->n_tty_read on tty->read_wait for input to appear
>> and at that moment, a change of ldisc is fatal. n_tty_close is called,
>> it frees read_buf and the waiting process is still in the middle of
>> reading and goes nuts after it is woken.
> 
> Hmm. Looks reasonable. And the waiting is outside the lock, so there
> aren't any of the problem cases that caused the original changes. And
> we don't need the lock, because the TTY_LDISC_CHANGING bit will
> protect against anything new coming in, so we don't have races with
> the count going up afterwards.
> 
> And you're right about the lockless approach being reasonable inside
> the testing code too - it's atomic as you say, and we don't touch/care
> about anything else.
> 
> So I don't have any objections, apart from thinking that the ldisc
> code is apparently still too fragile if this is needed.  But the ldisc
> change is so special that I don't think this is a unreasonable hack.
> Even if it _is_ a bit of a hack still.

Actually the fail path handling should be more than in the patch.
Otherwise I get warnings here and there (TTY_LDISC is not set in
tty_open). Something like the diff below.

> So feel free to add an acked-by: from me. Whoever saw the problem
> should probably test the patch first, though.

Ok, thanks for the review, I fwded to people who hit the bug.





---
@@ -654,14 +659,16 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 	flush_scheduled_work();

 	retval = tty_ldisc_wait_idle(tty);
+
+	tty_lock();
+	mutex_lock(&tty->ldisc_mutex);
+
+	/* handle wait idle failure locked */
 	if (retval) {
-		clear_bit(TTY_LDISC_CHANGING, &tty->flags);
 		tty_ldisc_put(new_ldisc);
-		return retval;
+		goto enable;
 	}

-	tty_lock();
-	mutex_lock(&tty->ldisc_mutex);
 	if (test_bit(TTY_HUPPED, &tty->flags)) {
 		/* We were raced by the hangup method. It will have stomped
 		   the ldisc data and closed the ldisc down */
@@ -695,6 +702,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)

 	tty_ldisc_put(o_ldisc);

+enable:
 	/*
 	 *	Allow ldisc referencing to occur again
 	 */


thanks,
-- 
js