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

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

[Shachar Shemesh]

From: Shachar Shemesh <shachar@liveu.tv>

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 <shachar@liveu.tv>
---
Tested with 3.2.20 kernel.

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 24b95db..a662a24 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -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);

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

[Greg KH]

On Sun, Jul 01, 2012 at 09:53:19AM +0300, Shachar Shemesh wrote:
> From: Shachar Shemesh <shachar@liveu.tv>
> 
> 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 <shachar@liveu.tv>
> ---
> Tested with 3.2.20 kernel.
> 
> diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
> index 24b95db..a662a24 100644
> --- a/drivers/tty/tty_ldisc.c
> +++ b/drivers/tty/tty_ldisc.c
> @@ -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)) {

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

Care to try again?

greg k-h

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

[Shachar Shemesh]

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

On 07/07/2012 12:24 AM, Greg KH wrote:
> On Sun, Jul 01, 2012 at 09:53:19AM +0300, Shachar Shemesh wrote:
>> From: Shachar Shemesh <shachar@liveu.tv>
>>
>> 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 <shachar@liveu.tv>
>> ---
>> Tested with 3.2.20 kernel.
>>
> 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.

Shachar


[-- Attachment #2: pppduinterruptible.patch --]
[-- Type: text/x-diff, Size: 495 bytes --]

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 24b95db..a662a24 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -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);

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

[Greg KH]

On Sun, Jul 08, 2012 at 11:58:46AM +0300, Shachar Shemesh wrote:
> On 07/07/2012 12:24 AM, Greg KH wrote:
> >On Sun, Jul 01, 2012 at 09:53:19AM +0300, Shachar Shemesh wrote:
> >>From: Shachar Shemesh <shachar@liveu.tv>
> >>
> >>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 <shachar@liveu.tv>
> >>---
> >>Tested with 3.2.20 kernel.
> >>
> >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

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

[Shachar Shemesh]

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

On 07/09/2012 07:44 PM, Greg KH wrote:
> 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

After a few tests, it seems the only reliable way to get my mailer to 
not munge the tabs is an attachment. I've included the entire details 
inside the attachment. Hopefully, that will be parsable to everyone.

Shachar

[-- Attachment #2: pppduninterruptible.patch --]
[-- Type: text/x-diff, Size: 880 bytes --]

From: Shachar Shemesh <shachar@liveu.tv>

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 <shachar@liveu.tv>
---
Tested with 3.2.20 kernel.

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 24b95db..a662a24 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -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);

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

[Jiri Slaby]

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

On 07/10/2012 06:54 AM, Shachar Shemesh wrote:
> From: Shachar Shemesh <shachar@liveu.tv>
> 
> 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 <shachar@liveu.tv>
> ---
> Tested with 3.2.20 kernel.
> 
> diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
> index 24b95db..a662a24 100644
> --- a/drivers/tty/tty_ldisc.c
> +++ b/drivers/tty/tty_ldisc.c
> @@ -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,
-- 
js
suse labs

[-- Attachment #2: 0001-TTY-forbid-setting-ldisc-on-HUPPED-tty-step-1.patch --]
[-- Type: text/x-patch, Size: 4129 bytes --]

>From b3e5a7f778be46b4b8dd38d5565010755ccafd67 Mon Sep 17 00:00:00 2001
From: Jiri Slaby <jslaby@suse.cz>
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 <jslaby@suse.cz>
Cc: Shachar Shemesh <shachar@liveu.tv>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
 drivers/tty/tty_ldisc.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index f4e6754..7cca3fd 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -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

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

[Sasha Levin]

On 09/19/2012 09:25 PM, Jiri Slaby wrote:
> On 07/10/2012 06:54 AM, Shachar Shemesh wrote:
>> > From: Shachar Shemesh <shachar@liveu.tv>
>> > 
>> > 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 <shachar@liveu.tv>
>> > ---
>> > Tested with 3.2.20 kernel.
>> > 
>> > diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
>> > index 24b95db..a662a24 100644
>> > --- a/drivers/tty/tty_ldisc.c
>> > +++ b/drivers/tty/tty_ldisc.c
>> > @@ -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.

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
[  482.880904]  [<ffffffff839ef284>] ? _raw_spin_unlock_irqrestore+0x84/0xb0
[  482.881406]  [<ffffffff81b931fc>] disassociate_ctty+0x6c/0x230
[  482.882141]  [<ffffffff8110dc98>] do_exit+0x3d8/0xa90
[  482.882803]  [<ffffffff8117a5dd>] ? trace_hardirqs_on+0xd/0x10
[  482.883564]  [<ffffffff8110e414>] do_group_exit+0x84/0xd0
[  482.884261]  [<ffffffff8110e472>] sys_exit_group+0x12/0x20
[  482.884975]  [<ffffffff839f0ce8>] tracesys+0xe1/0xe6
[  482.885618] 1 lock held by init/1:
[  482.886059]  #0:  (&tty->ldisc_mutex){+.+.+.}, at: [<ffffffff81b99b02>]
tty_ldisc_hangup+0x122/0x320
[  482.941519] rcu-torture: rtc: ffffffff8647dc30 ver: 746 tfle: 0 rta: 746
rtaf: 0 rtf: 745 rtmbe: 0 rtbke: 0 rtbre: 0 rtbf: 0 rtb: 0 nt: 5116 onoff:
0/0:0/0 -1,0:-1,0 0:0 (HZ=100) barrier: 0/0:0
[  482.941519] rcu-torture: Reader Pipe:  664658 22 0 0 0 0 0 0 0 0 0
[  482.941519] rcu-torture: Reader Batch:  664669 11 0 0 0 0 0 0 0 0 0
[  482.941519] rcu-torture: Free-Block Circulation:  745 745 745 745 745 745 745
745 745 745 0
[  542.942392] rcu-torture: rtc: ffffffff8647dc30 ver: 746 tfle: 0 rta: 746
rtaf: 0 rtf: 745 rtmbe: 0 rtbke: 0 rtbre: 0 rtbf: 0 rtb: 0 nt: 5116 onoff:
0/0:0/0 -1,0:-1,0 0:0 (HZ=100) barrier: 0/0:0
[  542.942392] rcu-torture: Reader Pipe:  664658 22 0 0 0 0 0 0 0 0 0
[  542.942392] rcu-torture: Reader Batch:  664669 11 0 0 0 0 0 0 0 0 0
[  542.942392] rcu-torture: Free-Block Circulation:  745 745 745 745 745 745 745
745 745 745 0
[  547.040453] kworker/u:3 (4274) used greatest stack depth: 3200 bytes left
[  602.880374] INFO: task init:1 blocked for more than 120 seconds.
[  602.883834] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
message.
[  602.886219] init            D ffff88000d618000  3424     1      0 0x00000002
[  602.887347]  ffff88000d5b9c28 0000000000000002 ffff88000d5b9be8 ffffffff8114ff65
[  602.888383]  ffff88000d5b9fd8 ffff88000d5b9fd8 ffff88000d5b9fd8 ffff88000d5b9fd8
[  602.889554]  ffff88000d618000 ffff88000d5b0000 ffff88000d5b08f0 7fffffffffffffff
[  602.890851] Call Trace:
[  602.891240]  [<ffffffff8114ff65>] ? sched_clock_local+0x25/0xa0
[  602.892128]  [<ffffffff839edb35>] schedule+0x55/0x60
[  602.892902]  [<ffffffff839ebab5>] schedule_timeout+0x45/0x360
[  602.893738]  [<ffffffff839ef25d>] ? _raw_spin_unlock_irqrestore+0x5d/0xb0
[  602.894713]  [<ffffffff8117a5dd>] ? trace_hardirqs_on+0xd/0x10
[  602.895656]  [<ffffffff839ef284>] ? _raw_spin_unlock_irqrestore+0x84/0xb0
[  602.896680]  [<ffffffff81136f97>] ? prepare_to_wait+0x77/0x90
[  602.897557]  [<ffffffff81b98ef6>] tty_ldisc_wait_idle.isra.7+0x76/0xb0
[  602.898509]  [<ffffffff81137170>] ? abort_exclusive_wait+0xb0/0xb0
[  602.899415]  [<ffffffff81b99bab>] tty_ldisc_hangup+0x1cb/0x320
[  602.900363]  [<ffffffff81b90d52>] ? __tty_hangup+0x122/0x430
[  602.901241]  [<ffffffff81b90d5a>] __tty_hangup+0x12a/0x430
[  602.902052]  [<ffffffff839ef284>] ? _raw_spin_unlock_irqrestore+0x84/0xb0
[  602.903020]  [<ffffffff81b931fc>] disassociate_ctty+0x6c/0x230
[  602.903848]  [<ffffffff8110dc98>] do_exit+0x3d8/0xa90
[  602.904642]  [<ffffffff8117a5dd>] ? trace_hardirqs_on+0xd/0x10
[  602.905518]  [<ffffffff8110e414>] do_group_exit+0x84/0xd0
[  602.906427]  [<ffffffff8110e472>] sys_exit_group+0x12/0x20
[  602.907449]  [<ffffffff839f0ce8>] tracesys+0xe1/0xe6
[  602.908579] 1 lock held by init/1:
[  602.909293]  #0:  (&tty->ldisc_mutex){+.+.+.}, at: [<ffffffff81b99b02>]
tty_ldisc_hangup+0x122/0x320

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

[Jiri Slaby]

On 09/22/2012 10:35 AM, Sasha Levin wrote:
> On 09/19/2012 09:25 PM, Jiri Slaby wrote:
>> On 07/10/2012 06:54 AM, Shachar Shemesh wrote:
>>>> From: Shachar Shemesh <shachar@liveu.tv>
>>>>
>>>> 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 <shachar@liveu.tv>
>>>> ---
>>>> Tested with 3.2.20 kernel.
>>>>
>>>> diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
>>>> index 24b95db..a662a24 100644
>>>> --- a/drivers/tty/tty_ldisc.c
>>>> +++ b/drivers/tty/tty_ldisc.c
>>>> @@ -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.
> 
> This patch also causes hangs on newer kernels. Can it be reverted please?

Just for the record, how reproducible is this? IOW can you 100% say that
the hangs are gone if you revert the patch?

Could you identify the process sitting on the tty you are trying to hang up?

> [  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

BTW that also means that my proposed patch will cause the same hangup
and we should proceed to step 2 suggested in the same patch. Given
nobody noticed in the past 3 years, which is another supporting
argument. But let's first investigate what is going on.

thanks,
-- 
js
suse labs

Re: tty ldisc: Close/Reopen race prevention should check the proper flag

[Džiugas Baltrūnas]

Hello,

It appears that the original patch [1] was committed in 3.6 tree, but
not in 3.2. Therefore I can confirm that with the 3.2.33 kernel and
without having the patch applied, I still see the pppd process going
into the "uninterruptible sleep" state occasionally. I'm using Huawei
3G modems (E173u-2, E3131, E353u-2) and typically I'm facing the issue
when there is a poor radio signal. My watchdog daemon forks pppd
process using the following command line:

pppd /dev/ttyUSB0 460800 unit 0 connect "/usr/sbin/chat -V -v -s ABORT
'BUSY' ABORT 'NO CARRIER' ABORT 'VOICE' ABORT 'NO DIALTONE' ABORT 'NO
DIAL TONE' ABORT 'NO ANSWER' ABORT 'DELAYED' REPORT CONNECT TIMEOUT 6
'' 'ATQ0' 'OK-AT-OK' 'ATZ' TIMEOUT 3 'OK\d-AT-OK' 'ATI' 'OK' 'ATZ'
'OK' 'ATQ0 V1 E1 S0=0 &C1 &D2' 'OK-AT-OK'
'AT+CGDCONT=1,\"IP\",\"internet\"' 'OK' 'AT+CGREG=2' 'OK' 'ATD*99#'
TIMEOUT 30 CONNECT ''" nodetach debug lock crtscts modem noauth novj
nobsdcomp nodefaultroute noipdefault lcp-echo-interval 2
lcp-echo-failure 3 child-timeout 5

The same watchdog daemon monitors whether a pppX interface has an IP
address assigned and if not, it sends the SIGTERM signal to the pppd
process, waits for it to exit and then restarts it by forking another
pppd instance. This is usually (but now always) the case when the pppd
process is blocked in the "uninterruptible sleep" state and the kernel
message (INFO: task pppd:PID blocked for more than 120 seconds).
Additionally, both with and without the patch applied, chat process
(forked by pppd) is usually left orphan and ignores SIGTERM (but dies
after SIGKILL).

I'm wondering if there are any reasons why this patch is not present
in the 3.2 tree and I'm also looking forward for the 'right' patch to
address the issue. When the pppd process is the "uninterruptible
sleep" state, the only workaround that we have is to reset modems
using a secondary serial interface for AT commands, so that the USB
device reconnection is triggered.

Regards,
Džiugas Baltrūnas

[1] https://github.com/torvalds/linux/commit/40c9f61eae9098212b6906f29f30f08f7a19b5e2

On 19 September 2012 21:25, Jiri Slaby <jslaby@suse.cz> wrote:
> On 07/10/2012 06:54 AM, Shachar Shemesh wrote:
>> From: Shachar Shemesh <shachar@liveu.tv>
>>
>> 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 <shachar@liveu.tv>
>> ---
>> Tested with 3.2.20 kernel.
>>
>> diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
>> index 24b95db..a662a24 100644
>> --- a/drivers/tty/tty_ldisc.c
>> +++ b/drivers/tty/tty_ldisc.c
>> @@ -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,
>
>
> >From b3e5a7f778be46b4b8dd38d5565010755ccafd67 Mon Sep 17 00:00:00 2001
> From: Jiri Slaby <jslaby@suse.cz>
> 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 <jslaby@suse.cz>
> Cc: Shachar Shemesh <shachar@liveu.tv>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> ---
>  drivers/tty/tty_ldisc.c | 37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
> index f4e6754..7cca3fd 100644
> --- a/drivers/tty/tty_ldisc.c
> +++ b/drivers/tty/tty_ldisc.c
> @@ -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
>