Subject: [PATCH] tty ldisc: Close/Reopen race prevention should check the proper flag

From: Shachar Shemesh <[email protected]>

Commit acfa747b introduced the TTY_HUPPING flag to distinguish closed TTY from currently closing ones. The test in tty_set_ldisc still remained pointing at the old flag. This causes pppd to sometimes lapse into uninterruptible sleep when killed and restarted.

Signed-off-by Shachar Shemesh <[email protected]>

---
Tested with 3.2.20 kernel.

drivers/tty/tty_ldisc.c +0 -0

index 24b95db..a662a24 100644

Quoted 8 lines · Shachar Shemesh

From: Shachar Shemesh <[email protected]>

Commit acfa747b introduced the TTY_HUPPING flag to distinguish closed TTY from currently closing ones. The test in tty_set_ldisc still remained pointing at the old flag. This causes pppd to sometimes lapse into uninterruptible sleep when killed and restarted.

Quoted 4 lines

Signed-off-by Shachar Shemesh <[email protected]>

---
Tested with 3.2.20 kernel.

All tabs are converted to spaces and make this patch impossible to apply :(

Care to try again?

Quoted 13 lines · Greg KH

Quoted 12 lines · Shachar Shemesh

From: Shachar Shemesh <[email protected]>

Commit acfa747b introduced the TTY_HUPPING flag to distinguish closed TTY from currently closing ones. The test in tty_set_ldisc still remained pointing at the old flag. This causes pppd to sometimes lapse into uninterruptible sleep when killed and restarted.

Signed-off-by Shachar Shemesh <[email protected]>

---
Tested with 3.2.20 kernel.

Quoted 4 lines

All tabs are converted to spaces and make this patch impossible to apply :(

Care to try again?

I'm sorry, I'm sending this as an attachment. I've verified that my mailer sends this as plain text, so it should be okay.

Yes, that worked, but then I would have to edit the body to include the above information in the patch properly.

So, care to resend it all in a "clean" format that I can apply it in?

thanks,

greg k-h

From: Shachar Shemesh <[email protected]>

Commit acfa747b introduced the TTY_HUPPING flag to distinguish closed TTY from currently closing ones. The test in tty_set_ldisc still remained pointing at the old flag. This causes pppd to sometimes lapse into uninterruptible sleep when killed and restarted.

Signed-off-by Shachar Shemesh <[email protected]>

---
Tested with 3.2.20 kernel.

drivers/tty/tty_ldisc.c +0 -0

index 24b95db..a662a24 100644

From b3e5a7f778be46b4b8dd38d5565010755ccafd67 Mon Sep 17 00:00:00 2001

Quoted 20 lines · Shachar Shemesh

Quoted 20 lines

From: Shachar Shemesh <[email protected]>

Commit acfa747b introduced the TTY_HUPPING flag to distinguish closed TTY from currently closing ones. The test in tty_set_ldisc still remained pointing at the old flag. This causes pppd to sometimes lapse into uninterruptible sleep when killed and restarted.

Signed-off-by Shachar Shemesh <[email protected]>

---
Tested with 3.2.20 kernel.

drivers/tty/tty_ldisc.c +0 -0

index 24b95db..a662a24 100644
@@ -658,7 +658,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 		goto enable;
 	}
 

Quoted 5 lines

drivers/tty/tty_ldisc.c int tty_set_ldisc(struct tty_struct *tty, int ldisc) · line 664

Quoted 5 lines

drivers/tty/tty_ldisc.c +1 -1

-	if (test_bit(TTY_HUPPED, &tty->flags)) {
+	if (test_bit(TTY_HUPPING, &tty->flags)) {
 		/* We were raced by the hangup method. It will have stomped
 		   the ldisc data and closed the ldisc down */
 		clear_bit(TTY_LDISC_CHANGING, &tty->flags);

Yes, that makes the issue go away, but does not seem to be right too. There are two issues I see:
* TTY_HUPPED has no use now. That is incorrect. Here should be a test for both flags, I think.
* The change forces the set_ldisc path to always re-open the ldisc even if it the terminal is HUPPED.

Quoted 21 lines · Jiri Slaby

Quoted 20 lines · Shachar Shemesh

Quoted 20 lines

From: Shachar Shemesh <[email protected]>

Commit acfa747b introduced the TTY_HUPPING flag to distinguish closed TTY from currently closing ones. The test in tty_set_ldisc still remained pointing at the old flag. This causes pppd to sometimes lapse into uninterruptible sleep when killed and restarted.

Signed-off-by Shachar Shemesh <[email protected]>

---
Tested with 3.2.20 kernel.

drivers/tty/tty_ldisc.c +0 -0

index 24b95db..a662a24 100644
@@ -658,7 +658,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 		goto enable;
 	}
 

Quoted 11 lines

Quoted 5 lines

drivers/tty/tty_ldisc.c int tty_set_ldisc(struct tty_struct *tty, int ldisc) · line 664

Quoted 5 lines

drivers/tty/tty_ldisc.c +1 -1

-	if (test_bit(TTY_HUPPED, &tty->flags)) {
+	if (test_bit(TTY_HUPPING, &tty->flags)) {
 		/* We were raced by the hangup method. It will have stomped
 		   the ldisc data and closed the ldisc down */
 		clear_bit(TTY_LDISC_CHANGING, &tty->flags);

Yes, that makes the issue go away, but does not seem to be right too. There are two issues I see:
* TTY_HUPPED has no use now. That is incorrect. Here should be a test for both flags, I think.
* The change forces the set_ldisc path to always re-open the ldisc even if it the terminal is HUPPED.

This patch also causes hangs on newer kernels. Can it be reverted please?

[  482.860279] INFO: task init:1 blocked for more than 120 seconds.
[  482.864244] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this

message.

[  482.867175] init            D ffff88000d618000  3424     1      0 0x00000002
[  482.869321]  ffff88000d5b9c28 0000000000000002 ffff88000d5b9be8 ffffffff8114ff65
[  482.870387]  ffff88000d5b9fd8 ffff88000d5b9fd8 ffff88000d5b9fd8 ffff88000d5b9fd8
[  482.871419]  ffff88000d618000 ffff88000d5b0000 ffff88000d5b08f0 7fffffffffffffff
[  482.872143] Call Trace:
[  482.872336]  [<ffffffff8114ff65>] ? sched_clock_local+0x25/0xa0
[  482.872796]  [<ffffffff839edb35>] schedule+0x55/0x60
[  482.873433]  [<ffffffff839ebab5>] schedule_timeout+0x45/0x360
[  482.874134]  [<ffffffff839ef25d>] ? _raw_spin_unlock_irqrestore+0x5d/0xb0

[  482.874752]  [<ffffffff8117a5dd>] ? trace_hardirqs_on+0xd/0x10
[  482.875835]  [<ffffffff839ef284>] ? _raw_spin_unlock_irqrestore+0x84/0xb0
[  482.876744]  [<ffffffff81136f97>] ? prepare_to_wait+0x77/0x90
[  482.877485]  [<ffffffff81b98ef6>] tty_ldisc_wait_idle.isra.7+0x76/0xb0
[  482.878428]  [<ffffffff81137170>] ? abort_exclusive_wait+0xb0/0xb0
[  482.879239]  [<ffffffff81b99bab>] tty_ldisc_hangup+0x1cb/0x320
[  482.879988]  [<ffffffff81b90d52>] ? __tty_hangup+0x122/0x430
[  482.880491]  [<ffffffff81b90d5a>] __tty_hangup+0x12a/0x430

Quoted 17 lines · Shachar Shemesh

From: Shachar Shemesh <[email protected]>

Commit acfa747b introduced the TTY_HUPPING flag to distinguish closed TTY from currently closing ones. The test in tty_set_ldisc still remained pointing at the old flag. This causes pppd to sometimes lapse into uninterruptible sleep when killed and restarted.

Signed-off-by Shachar Shemesh <[email protected]>

---
Tested with 3.2.20 kernel.

drivers/tty/tty_ldisc.c +0 -0

index 24b95db..a662a24 100644

drivers/tty/tty_ldisc.c int tty_set_ldisc(struct tty_struct *tty, int ldisc) · line 664

Quoted 8 lines

drivers/tty/tty_ldisc.c +1 -1

@@ -658,7 +658,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
              goto enable;
      }
 
-     if (test_bit(TTY_HUPPED, &tty->flags)) {
+     if (test_bit(TTY_HUPPING, &tty->flags)) {
              /* We were raced by the hangup method. It will have stomped
                 the ldisc data and closed the ldisc down */
              clear_bit(TTY_LDISC_CHANGING, &tty->flags);

Yes, that makes the issue go away, but does not seem to be right too. There are two issues I see:
* TTY_HUPPED has no use now. That is incorrect. Here should be a test for both flags, I think.
* The change forces the set_ldisc path to always re-open the ldisc even if it the terminal is HUPPED.

The bug is not in the kernel. It was in login(1) (util-linux). And it should be fixed by now. See:
http://git.kernel.org/?p=utils/util-linux/util-linux.git;a=commitdiff;h=2e7035646eb85851171cc2e989bfa858a4f00cd4

I'm for an in-fact revert of the patch. But temporarily I would still return EIO. However let's do it even after the reopen is done, so that we get rid of the user-visible regression. Like in the attached patch. The regression was introduced by commit c65c9bc3e (tty: rewrite the ldisc locking). That is !three! years ago. And that is suspicious in itself.

BTW Why pppd thinks it is a good idea to ignore EIO from TIOCSETD ioctl?

regards,

Quoted 1 line

From b3e5a7f778be46b4b8dd38d5565010755ccafd67 Mon Sep 17 00:00:00 2001

From: Jiri Slaby <[email protected]>
Date: Wed, 19 Sep 2012 20:23:59 +0200
Subject: [PATCH 1/1] TTY: forbid setting ldisc on HUPPED tty, step 1

Commit c65c9bc3e (tty: rewrite the ldisc locking) introduced a check into tty_set_ldisc whether the terminal is already hung. In that case it returns -EIO.

Later, commit aa3c8af86 (tty ldisc: Close/Reopen race prevention should check the proper flag) effectively disabled the check by replacing HUPPED by HUPPING flag. Those are two different flags and the former check was actually correct. In principle we revert here to the previous behavior plus we add a check if we are HUPPING. Because it makes no sense to change ldisc when some other thread is making a HUP but it had to unlock BTM temporarily.

The bug which commit aa3c8af86 above tried to avoid lays in fact in login(1). It was fixed by util-linux commit 2e7035646 (login: close tty before vhangup()) already. It was reproducible for example by the following setup:
1) add a user ppp with shell set to /usr/bin/pppd
2) run agetty on ttyS0 to wait for a user
3) user connects via modem on ttyS0, types ppp to getty
4) 'login ppp' is executed
5) login performs hangup without properly closing ttyS0
6) login executes pppd
7) pppd does TIOCSETD attempt to N_PPP
8) kernel denies step 7) as ttyS0 is HUPPED
9) pppd calls some N_PPP ldisc's ioctls regardless -- uninterruptible sleep in tty_ioctl, trying to have a ref on tty->ldisc

Now to sum up the fixes:
* The fix in util-linux is that 5 is changed to properly close all ttyS0 instances.
* The workaround from commit aa3c8af86 was that 8) succeeds.
* This patch is the same as aa3c8af86 except we still return -EIO and warn the user this will not be supported in the future. This is because we do not want to have a user-visible regression here.

"Step 1" because step two would be to immediatelly return -EIO like in the attached false branch in the patch.

Signed-off-by Jiri Slaby <[email protected]>

Cc Shachar Shemesh <[email protected]>

Cc Alan Cox <[email protected]>

---
 drivers/tty/tty_ldisc.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

drivers/tty/tty_ldisc.c +28 -10

index f4e6754..7cca3fd 100644
@@ -563,6 +563,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
        struct tty_ldisc *o_ldisc, *new_ldisc;
        int work, o_work = 0;
        struct tty_struct *o_tty;
+       int retval_hupped = 0;
 
        new_ldisc = tty_ldisc_get(ldisc);
        if (IS_ERR(new_ldisc))

@@ -659,14 +660,32 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
                goto enable;
        }
 
-       if (test_bit(TTY_HUPPING, &tty->flags)) {
-               /* We were raced by the hangup method. It will have stomped
-                  the ldisc data and closed the ldisc down */
-               clear_bit(TTY_LDISC_CHANGING, &tty->flags);
-               mutex_unlock(&tty->ldisc_mutex);
-               tty_ldisc_put(new_ldisc);
-               tty_unlock(tty);
-               return -EIO;
+       if (test_bit(TTY_HUPPED, &tty->flags) ||
+                       test_bit(TTY_HUPPING, &tty->flags)) {
+               /*
+                * Until login(1) is fixed everywhere, let's fall through.
+                * Change to goto with -EIO after 3.10.
+                */
+               if (1) {
+                       char ttyn[64], comm[TASK_COMM_LEN];
+                       printk_ratelimited(KERN_WARNING "%s: %s calls TIOCSETD on a hung TTY (%s). This is deprecated and will stop working soon.\n",
+                                       __func__, get_task_comm(comm, current),
+                                       tty_name(tty, ttyn));
+                       retval_hupped = -EIO;
+               } else {
+                       /*
+                        * We were raced by the hangup method. It will have
+                        * stomped the ldisc data and closed the ldisc down
+                        */
+                       tty_ldisc_put(new_ldisc);
+                       retval = -EIO;
+                       /*
+                        * We have to enable the original ldisc so that
+                        * processes see N_TTY and fail instead of waiting
+                        * infinitely.
+                        */
+                       goto enable;
+               }
        }
 
        /* Shutdown the current discipline. */

@@ -709,7 +728,7 @@ enable:
                schedule_work(&o_tty->port->buf.work);
        mutex_unlock(&tty->ldisc_mutex);
        tty_unlock(tty);
-       return retval;
+       return retval ? : retval_hupped;
 }
 
 /**
--

1.7.12