[PATCH] tty: Fix SIGTTOU not sent with tcflush()

[PATCH] tty: Fix SIGTTOU not sent with tcflush()

[Peter Hurley]

Commit 'e7f3880cd9b98c5bf9391ae7acdec82b75403776'
  tty: Fix recursive deadlock in tty_perform_flush()
introduced a regression where tcflush() does not generate
SIGTTOU for background process groups.

Make sure ioctl(TCFLSH) calls tty_check_change() when
invoked from the line discipline.

Cc: stable@vger.kernel.org # v3.10+
Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index 3500d41..088b4ca 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -1201,6 +1201,9 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file,
 		}
 		return 0;
 	case TCFLSH:
+		retval = tty_check_change(tty);
+		if (retval)
+			return retval;
 		return __tty_perform_flush(tty, arg);
 	default:
 		/* Try the mode commands */
-- 
1.8.1.2

Re: [PATCH] tty: Fix SIGTTOU not sent with tcflush()

[Greg Kroah-Hartman]

On Wed, Sep 25, 2013 at 08:13:04PM -0400, Peter Hurley wrote:
> Commit 'e7f3880cd9b98c5bf9391ae7acdec82b75403776'
>   tty: Fix recursive deadlock in tty_perform_flush()
> introduced a regression where tcflush() does not generate
> SIGTTOU for background process groups.
> 
> Make sure ioctl(TCFLSH) calls tty_check_change() when
> invoked from the line discipline.
> 
> Cc: stable@vger.kernel.org # v3.10+
> Reported-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  drivers/tty/tty_ioctl.c | 3 +++
>  1 file changed, 3 insertions(+)

Thanks for tracking this down, I'll queue it up tomorrow.

greg k-h

Re: [PATCH] tty: Fix SIGTTOU not sent with tcflush()

[Oleg Nesterov]

Thanks Peter.

Well. I'am afraid my testing was wrong, because Karel reports
it fixes the problem...

But. I applied this patch to my 3.11 tree (last commit is bff157b3a)
which also has the additional patch (03e12617 "tty: disassociate_ctty()
sends the extra SIGCONT"), and

	TET_CONFIG=CFG TET_ROOT=.  ./T.tcflush 4

still fails... T.tcflush was compiled on another (Karel's) machine,
perhaps there is something in libc, I do not know.

So let me ask just in case, I assume the fix below doesn't depend on
other changes I could miss?

I'll retest after git-pull and report...

On 09/25, Peter Hurley wrote:
>
> Commit 'e7f3880cd9b98c5bf9391ae7acdec82b75403776'
>   tty: Fix recursive deadlock in tty_perform_flush()
> introduced a regression where tcflush() does not generate
> SIGTTOU for background process groups.
>
> Make sure ioctl(TCFLSH) calls tty_check_change() when
> invoked from the line discipline.
>
> Cc: stable@vger.kernel.org # v3.10+
> Reported-by: Oleg Nesterov <oleg@redhat.com>

Actually Reported-by: Karel Srot <ksrot@redhat.com>

my fault, forgot to mention this.

> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  drivers/tty/tty_ioctl.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
> index 3500d41..088b4ca 100644
> --- a/drivers/tty/tty_ioctl.c
> +++ b/drivers/tty/tty_ioctl.c
> @@ -1201,6 +1201,9 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file,
>  		}
>  		return 0;
>  	case TCFLSH:
> +		retval = tty_check_change(tty);
> +		if (retval)
> +			return retval;
>  		return __tty_perform_flush(tty, arg);
>  	default:
>  		/* Try the mode commands */
> -- 
> 1.8.1.2
> 

Re: [PATCH] tty: Fix SIGTTOU not sent with tcflush()

[Oleg Nesterov]

On 09/26, Oleg Nesterov wrote:
>
> Thanks Peter.
>
> Well. I'am afraid my testing was wrong, because Karel reports
> it fixes the problem...
>
> But. I applied this patch to my 3.11 tree (last commit is bff157b3a)
> which also has the additional patch (03e12617 "tty: disassociate_ctty()
> sends the extra SIGCONT"), and
>
> 	TET_CONFIG=CFG TET_ROOT=.  ./T.tcflush 4
>
> still fails... T.tcflush was compiled on another (Karel's) machine,
> perhaps there is something in libc, I do not know.
>
> So let me ask just in case, I assume the fix below doesn't depend on
> other changes I could miss?
>
> I'll retest after git-pull and report...

Still fails under the Linus's tree + this patch.

However!!!

Tested-by: Oleg Nesterov <oleg@redhat.com>




It turns out, T.tcflush doesn't expect it can start as a process group
leader. In this case setsid() fails, then tty_open() doesn't set
signal->tty, and thus this patch makes no difference: tty_check_change()
fails because tty doesn't match signal->tty.

And indeed, this test passes if you run it under strace, or simply do

	$ TET_CONFIG=CFG TET_ROOT=. perl -e "system './T.tcflush 4'"

And damn, the fact it doesn't fail under strace doesn't allow you to
see that setsid() fails ;)

This is probably explains why Karel reported success, perhaps he
didn't run this test individually.

Thanks again Peter. Perhaps my analysis was wrong (and I do not see
setsid() in the sources), I do not really care. but perhaps tcflush.c
should be updated.

Oleg.

Re: [PATCH] tty: Fix SIGTTOU not sent with tcflush()

[Peter Hurley]

On 09/26/2013 03:10 PM, Oleg Nesterov wrote:
> On 09/26, Oleg Nesterov wrote:
>>
>> Thanks Peter.
>>
>> Well. I'am afraid my testing was wrong, because Karel reports
>> it fixes the problem...
>>
>> But. I applied this patch to my 3.11 tree (last commit is bff157b3a)
>> which also has the additional patch (03e12617 "tty: disassociate_ctty()
>> sends the extra SIGCONT"), and
>>
>> 	TET_CONFIG=CFG TET_ROOT=.  ./T.tcflush 4
>>
>> still fails... T.tcflush was compiled on another (Karel's) machine,
>> perhaps there is something in libc, I do not know.
>>
>> So let me ask just in case, I assume the fix below doesn't depend on
>> other changes I could miss?
>>
>> I'll retest after git-pull and report...
>
> Still fails under the Linus's tree + this patch.
>
> However!!!
>
> Tested-by: Oleg Nesterov <oleg@redhat.com>

Greg picked this patch up yesterday evening so it went in as-is.


> It turns out, T.tcflush doesn't expect it can start as a process group
> leader. In this case setsid() fails, then tty_open() doesn't set
> signal->tty, and thus this patch makes no difference: tty_check_change()
> fails because tty doesn't match signal->tty.
>
> And indeed, this test passes if you run it under strace, or simply do
>
> 	$ TET_CONFIG=CFG TET_ROOT=. perl -e "system './T.tcflush 4'"
>
> And damn, the fact it doesn't fail under strace doesn't allow you to
> see that setsid() fails ;)

I wondered if the test was designed for standalone.

FWIW, I looked at tcflush.c to see what test4() was doing,
wrote my own test vector to confirm the problem, instrumented my
test vector, realized what the problem was, wrote the patch,
built mainline 3.11 + this patch, re-tested the problem, confirmed
the regression went back to 3.10, prepared the patch and sent
the emails.

It's going to take me some time to figure out how best to use LSB;
all the test instrumentation makes the source hard to work with
directly.


> This is probably explains why Karel reported success, perhaps he
> didn't run this test individually.
>
> Thanks again Peter. Perhaps my analysis was wrong (and I do not see
> setsid() in the sources), I do not really care. but perhaps tcflush.c
> should be updated.

I would agree with your analysis.

Regards,
Peter Hurley

Re: [PATCH] tty: Fix SIGTTOU not sent with tcflush()

[Karel Srot]

Hello Oleg,

On Thu, 2013-09-26 at 21:10 +0200, Oleg Nesterov wrote:
> On 09/26, Oleg Nesterov wrote:
> >
> > Thanks Peter.
> >
> > Well. I'am afraid my testing was wrong, because Karel reports
> > it fixes the problem...
> >
> > But. I applied this patch to my 3.11 tree (last commit is bff157b3a)
> > which also has the additional patch (03e12617 "tty: disassociate_ctty()
> > sends the extra SIGCONT"), and
> >
> > 	TET_CONFIG=CFG TET_ROOT=.  ./T.tcflush 4
> >
> > still fails... T.tcflush was compiled on another (Karel's) machine,
> > perhaps there is something in libc, I do not know.
> >
> > So let me ask just in case, I assume the fix below doesn't depend on
> > other changes I could miss?
> >
> > I'll retest after git-pull and report...
> 
> Still fails under the Linus's tree + this patch.
> 
> However!!!
> 
> Tested-by: Oleg Nesterov <oleg@redhat.com>
> 
> 
> 
> 
> It turns out, T.tcflush doesn't expect it can start as a process group
> leader. In this case setsid() fails, then tty_open() doesn't set
> signal->tty, and thus this patch makes no difference: tty_check_change()
> fails because tty doesn't match signal->tty.
> 
> And indeed, this test passes if you run it under strace, or simply do
> 
> 	$ TET_CONFIG=CFG TET_ROOT=. perl -e "system './T.tcflush 4'"
> 
> And damn, the fact it doesn't fail under strace doesn't allow you to
> see that setsid() fails ;)
> 
> This is probably explains why Karel reported success, perhaps he
> didn't run this test individually.

This is true, I didn't execute this particular test directly but through
the LSB test suite framework.

> 
> Thanks again Peter. Perhaps my analysis was wrong (and I do not see
> setsid() in the sources), I do not really care. but perhaps tcflush.c
> should be updated.
> 
> Oleg.
> 


Karel

-- 
Karel Srot
Quality Engineer, QE BaseOS Security team

Email: ksrot@redhat.com
Web: www.cz.redhat.com
Red Hat Czech s.r.o., Purkyňova 99/71, 612 45, Brno, Czech Republic