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

[Jiri Slaby <jirislaby@gmail.com>]

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