* [PATCH 0/1] tty: set_termios/set_termiox should not return -EINTR
@ 2013-01-29 19:07 Oleg Nesterov
2013-01-29 19:07 ` [PATCH 1/1] " Oleg Nesterov
0 siblings, 1 reply; 8+ messages in thread
From: Oleg Nesterov @ 2013-01-29 19:07 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby; +Cc: Lingzhu Xiang, Roman Rakus, linux-kernel
Hello.
The patch looks really trivial and I even cc'ed -stable.
But it needs the review from someone who understands the drivers/tty
code, perhaps there is a reason why we should not restart TCGETA/etc.
I do not even know what set_termios() actually does ;)
Oleg.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/1] tty: set_termios/set_termiox should not return -EINTR
2013-01-29 19:07 [PATCH 0/1] tty: set_termios/set_termiox should not return -EINTR Oleg Nesterov
@ 2013-01-29 19:07 ` Oleg Nesterov
2013-01-29 19:35 ` Jiri Slaby
0 siblings, 1 reply; 8+ messages in thread
From: Oleg Nesterov @ 2013-01-29 19:07 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby; +Cc: Lingzhu Xiang, Roman Rakus, linux-kernel
See https://bugzilla.redhat.com/show_bug.cgi?id=904907
read command causes bash to abort with double free or corruption (out).
A simple test-case from Roman:
// Compile the reproducer and send sigchld ti that process.
// EINTR occurs even if SA_RESTART flag is set.
void handler(int sig)
{
}
main()
{
struct sigaction act;
act.sa_handler = handler;
act.sa_flags = SA_RESTART;
sigaction (SIGCHLD, &act, 0);
struct termio ttp;
ioctl(0, TCGETA, &ttp);
while(1)
{
if (ioctl(0, TCSETAW, ttp) < 0)
{
if (errno == EINTR)
{
fprintf(stderr, "BUG!"); return(1);
}
}
}
}
Change set_termios/set_termiox to return -ERESTARTSYS to fix this
particular problem.
I didn't dare to change other EINTR's in drivers/tty/, but they look
equally wrong.
Reported-by: Roman Rakus <rrakus@redhat.com>
Reported-by: Lingzhu Xiang <lxiang@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Cc: stable@kernel.org
---
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -617,7 +617,7 @@ static int set_termios(struct tty_struct *tty, void __user *arg, int opt)
if (opt & TERMIOS_WAIT) {
tty_wait_until_sent(tty, 0);
if (signal_pending(current))
- return -EINTR;
+ return -ERESTARTSYS;
}
tty_set_termios(tty, &tmp_termios);
@@ -684,7 +684,7 @@ static int set_termiox(struct tty_struct *tty, void __user *arg, int opt)
if (opt & TERMIOS_WAIT) {
tty_wait_until_sent(tty, 0);
if (signal_pending(current))
- return -EINTR;
+ return -ERESTARTSYS;
}
mutex_lock(&tty->termios_mutex);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] tty: set_termios/set_termiox should not return -EINTR
2013-01-29 19:07 ` [PATCH 1/1] " Oleg Nesterov
@ 2013-01-29 19:35 ` Jiri Slaby
2013-01-29 19:49 ` Oleg Nesterov
0 siblings, 1 reply; 8+ messages in thread
From: Jiri Slaby @ 2013-01-29 19:35 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Greg Kroah-Hartman, Lingzhu Xiang, Roman Rakus, linux-kernel
On 01/29/2013 08:07 PM, Oleg Nesterov wrote:
> See https://bugzilla.redhat.com/show_bug.cgi?id=904907
> read command causes bash to abort with double free or corruption (out).
>
> A simple test-case from Roman:
>
> // Compile the reproducer and send sigchld ti that process.
> // EINTR occurs even if SA_RESTART flag is set.
>
> void handler(int sig)
> {
> }
>
> main()
> {
> struct sigaction act;
> act.sa_handler = handler;
> act.sa_flags = SA_RESTART;
> sigaction (SIGCHLD, &act, 0);
> struct termio ttp;
> ioctl(0, TCGETA, &ttp);
> while(1)
> {
> if (ioctl(0, TCSETAW, ttp) < 0)
> {
> if (errno == EINTR)
> {
> fprintf(stderr, "BUG!"); return(1);
> }
> }
> }
> }
>
> Change set_termios/set_termiox to return -ERESTARTSYS to fix this
> particular problem.
This looks reasonable. However given the link above says:
You are not authorized to access bug #904907.
the description above is poor. What problem exactly does this fix? Why
this should go to stable at all?
--
js
suse labs
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] tty: set_termios/set_termiox should not return -EINTR
2013-01-29 19:35 ` Jiri Slaby
@ 2013-01-29 19:49 ` Oleg Nesterov
2013-01-29 22:01 ` Jiri Slaby
0 siblings, 1 reply; 8+ messages in thread
From: Oleg Nesterov @ 2013-01-29 19:49 UTC (permalink / raw)
To: Jiri Slaby; +Cc: Greg Kroah-Hartman, Lingzhu Xiang, Roman Rakus, linux-kernel
On 01/29, Jiri Slaby wrote:
>
> On 01/29/2013 08:07 PM, Oleg Nesterov wrote:
> >
> > Change set_termios/set_termiox to return -ERESTARTSYS to fix this
> > particular problem.
>
> This looks reasonable. However given the link above says:
> You are not authorized to access bug #904907.
> the description above is poor. What problem exactly does this fix?
A syscall must never return EINTR unless it can not be restarted
(or it should not be restarted by, say, historical reasons).
In this case ioctl(TCSETAW) returns -EINTR even if it is interrupted
by the signal which has the SA_RESTART handler. This doesn't look right
no matter what.
> Why this should go to stable at all?
OK, this is up to you.
But if this patch is correct, perhaps it should be backported. This
-EINTR breaks /bin/bash which doesn't expect it, this leads to
"*** glibc detected *** ./bash-4.1.2-14.el6/bin/bash: double free or corruption (out):"
Perhaps /bin/bash is buggy too, I do not know. Probably Roman and
Lingzhu can tell more.
Oleg.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] tty: set_termios/set_termiox should not return -EINTR
2013-01-29 19:49 ` Oleg Nesterov
@ 2013-01-29 22:01 ` Jiri Slaby
2013-01-30 11:46 ` Oleg Nesterov
2013-01-30 13:01 ` Roman Rakus
0 siblings, 2 replies; 8+ messages in thread
From: Jiri Slaby @ 2013-01-29 22:01 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Greg Kroah-Hartman, Lingzhu Xiang, Roman Rakus, linux-kernel
On 01/29/2013 08:49 PM, Oleg Nesterov wrote:
> On 01/29, Jiri Slaby wrote:
>>
>> On 01/29/2013 08:07 PM, Oleg Nesterov wrote:
>>>
>>> Change set_termios/set_termiox to return -ERESTARTSYS to fix this
>>> particular problem.
>>
>> This looks reasonable. However given the link above says:
>> You are not authorized to access bug #904907.
>> the description above is poor. What problem exactly does this fix?
>
> A syscall must never return EINTR unless it can not be restarted
> (or it should not be restarted by, say, historical reasons).
>
> In this case ioctl(TCSETAW) returns -EINTR even if it is interrupted
> by the signal which has the SA_RESTART handler. This doesn't look right
> no matter what.
Yes, and more, it is against POSIX. I don't dispute the correctness of
the patch at all.
>> Why this should go to stable at all?
>
> OK, this is up to you.
>
> But if this patch is correct, perhaps it should be backported. This
> -EINTR breaks /bin/bash which doesn't expect it, this leads to
> "*** glibc detected *** ./bash-4.1.2-14.el6/bin/bash: double free or corruption (out):"
>
> Perhaps /bin/bash is buggy too, I do not know. Probably Roman and
> Lingzhu can tell more.
But I really want to hear more details here (the commit log deserves
that). E.g. why it started causing problems right now. The code is there
like forever.
--
js
suse labs
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] tty: set_termios/set_termiox should not return -EINTR
2013-01-29 22:01 ` Jiri Slaby
@ 2013-01-30 11:46 ` Oleg Nesterov
2013-01-30 11:51 ` Oleg Nesterov
2013-01-30 13:01 ` Roman Rakus
1 sibling, 1 reply; 8+ messages in thread
From: Oleg Nesterov @ 2013-01-30 11:46 UTC (permalink / raw)
To: Jiri Slaby; +Cc: Greg Kroah-Hartman, Lingzhu Xiang, Roman Rakus, linux-kernel
On 01/29, Jiri Slaby wrote:
>
> On 01/29/2013 08:49 PM, Oleg Nesterov wrote:
> >
> > Perhaps /bin/bash is buggy too, I do not know. Probably Roman and
> > Lingzhu can tell more.
>
> But I really want to hear more details here (the commit log deserves
> that). E.g. why it started causing problems right now.
I have no idea, I only saw the test-case yesterday.
Perhaps it never caused the problems before, or perhaps it was never
reported.
> The code is there
> like forever.
Sure. Lingzhu noticed bug on 2.6.32.
Oleg.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] tty: set_termios/set_termiox should not return -EINTR
2013-01-30 11:46 ` Oleg Nesterov
@ 2013-01-30 11:51 ` Oleg Nesterov
0 siblings, 0 replies; 8+ messages in thread
From: Oleg Nesterov @ 2013-01-30 11:51 UTC (permalink / raw)
To: Jiri Slaby; +Cc: Greg Kroah-Hartman, Lingzhu Xiang, Roman Rakus, linux-kernel
On 01/30, Oleg Nesterov wrote:
>
> On 01/29, Jiri Slaby wrote:
> >
> > On 01/29/2013 08:49 PM, Oleg Nesterov wrote:
> > >
> > > Perhaps /bin/bash is buggy too, I do not know. Probably Roman and
> > > Lingzhu can tell more.
> >
> > But I really want to hear more details here (the commit log deserves
> > that). E.g. why it started causing problems right now.
>
> I have no idea, I only saw the test-case yesterday.
But if you ask how this affects /bin/bash, I can quote the description
from Lingzhu,
sigchld.sh (reproducer):
#!/bin/bash
( while :; do kill -CHLD $$ 2>&- || break; done ) &
while :; do
read -p 1 -t 0.3 -d ' '
read -p 2
done
Double free happens in read_builtin, here
FREE (tofree);
-> xfree (orig_input_string);
return (retval);
result:
sigchld.sh: line 4: read: error setting terminal attributes: Interrupted system call
1
*** glibc detected *** ./bash-4.1.2-14.el6/bin/bash: double free or corruption (out): 0x00000000020f45b0 ***
======= Backtrace: =========
(...)
Oleg.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] tty: set_termios/set_termiox should not return -EINTR
2013-01-29 22:01 ` Jiri Slaby
2013-01-30 11:46 ` Oleg Nesterov
@ 2013-01-30 13:01 ` Roman Rakus
1 sibling, 0 replies; 8+ messages in thread
From: Roman Rakus @ 2013-01-30 13:01 UTC (permalink / raw)
To: Jiri Slaby; +Cc: Oleg Nesterov, Greg Kroah-Hartman, Lingzhu Xiang, linux-kernel
On 01/29/2013 11:01 PM, Jiri Slaby wrote:
> E.g. why it started causing problems right now. The code is there
> like forever.
Because right now the situation occurs. It's not common to receive
signal during syscall. But when it happens, it should work as documented.
RR
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-01-30 13:01 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-01-29 19:07 [PATCH 0/1] tty: set_termios/set_termiox should not return -EINTR Oleg Nesterov
2013-01-29 19:07 ` [PATCH 1/1] " Oleg Nesterov
2013-01-29 19:35 ` Jiri Slaby
2013-01-29 19:49 ` Oleg Nesterov
2013-01-29 22:01 ` Jiri Slaby
2013-01-30 11:46 ` Oleg Nesterov
2013-01-30 11:51 ` Oleg Nesterov
2013-01-30 13:01 ` Roman Rakus
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).