Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1316902 > unrolled thread

[PATCH] signals: work around random wakeups in sigsuspend()

Back to article view | Back to linux.kernel

Contents

  [PATCH] signals: work around random wakeups in sigsuspend() Sasha Levin <sasha.levin@oracle.com> - 2016-01-25 17:00 +0100
    Re: [PATCH] signals: work around random wakeups in sigsuspend() Oleg Nesterov <oleg@redhat.com> - 2016-01-25 20:10 +0100
      Re: [PATCH] signals: work around random wakeups in sigsuspend() Peter Zijlstra <peterz@infradead.org> - 2016-01-25 20:40 +0100
    Re: [PATCH] signals: work around random wakeups in sigsuspend() Andrew Morton <akpm@linux-foundation.org> - 2016-01-25 22:40 +0100
      Re: [PATCH] signals: work around random wakeups in sigsuspend() Oleg Nesterov <oleg@redhat.com> - 2016-01-26 22:20 +0100
        Re: [PATCH] signals: work around random wakeups in sigsuspend() Peter Zijlstra <peterz@infradead.org> - 2016-01-27 09:50 +0100
          Re: [PATCH] signals: work around random wakeups in sigsuspend() Oleg Nesterov <oleg@redhat.com> - 2016-01-27 17:50 +0100
            Re: [PATCH] signals: work around random wakeups in sigsuspend() Peter Zijlstra <peterz@infradead.org> - 2016-01-27 19:00 +0100
            Re: [PATCH] signals: work around random wakeups in sigsuspend() Andrew Morton <akpm@linux-foundation.org> - 2016-01-27 19:40 +0100
              Re: [PATCH] signals: work around random wakeups in sigsuspend() Oleg Nesterov <oleg@redhat.com> - 2016-01-27 22:10 +0100
          Re: [PATCH] signals: work around random wakeups in sigsuspend() Sasha Levin <sasha.levin@oracle.com> - 2016-01-27 18:30 +0100
    Re: [PATCH] signals: work around random wakeups in sigsuspend() Ingo Molnar <mingo@kernel.org> - 2016-01-26 07:50 +0100
      Re: [PATCH] signals: work around random wakeups in sigsuspend() Oleg Nesterov <oleg@redhat.com> - 2016-01-26 16:10 +0100

#1316902 — [PATCH] signals: work around random wakeups in sigsuspend()

A random wakeup can get us out of sigsuspend() without TIF_SIGPENDING
being set.

Avoid that by making sure we were signaled, like sys_pause() does.

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 kernel/signal.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 5da9180..3256c7e 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3528,8 +3528,10 @@ static int sigsuspend(sigset_t *set)
 	current->saved_sigmask = current->blocked;
 	set_current_blocked(set);
 
-	__set_current_state(TASK_INTERRUPTIBLE);
-	schedule();
+	while (!signal_pending(current)) {
+		__set_current_state(TASK_INTERRUPTIBLE);
+		schedule();
+	}
 	set_restore_sigmask();
 	return -ERESTARTNOHAND;
 }
-- 
1.7.10.4

[toc] | [next] | [standalone]

#1317216

On 01/25, Sasha Levin wrote:
>
> A random wakeup can get us out of sigsuspend() without TIF_SIGPENDING
> being set.

and TIF_RESTORE_SIGMASK is just wrong in this case. I'd say this is the
bugfix, not work-around ;)

> Avoid that by making sure we were signaled, like sys_pause() does.
>
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>

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

Thanks Sasha.


> ---
>  kernel/signal.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 5da9180..3256c7e 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3528,8 +3528,10 @@ static int sigsuspend(sigset_t *set)
>  	current->saved_sigmask = current->blocked;
>  	set_current_blocked(set);
>  
> -	__set_current_state(TASK_INTERRUPTIBLE);
> -	schedule();
> +	while (!signal_pending(current)) {
> +		__set_current_state(TASK_INTERRUPTIBLE);
> +		schedule();
> +	}
>  	set_restore_sigmask();
>  	return -ERESTARTNOHAND;
>  }
> -- 
> 1.7.10.4
> 

[toc] | [prev] | [next] | [standalone]

#1317237

On Mon, Jan 25, 2016 at 08:09:15PM +0100, Oleg Nesterov wrote:
> On 01/25, Sasha Levin wrote:
> >
> > A random wakeup can get us out of sigsuspend() without TIF_SIGPENDING
> > being set.
> 
> and TIF_RESTORE_SIGMASK is just wrong in this case. I'd say this is the
> bugfix, not work-around ;)

Agreed!

> > Avoid that by making sure we were signaled, like sys_pause() does.
> >
> > Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> 
> Acked-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

[toc] | [prev] | [next] | [standalone]

#1317316

On Mon, 25 Jan 2016 10:21:46 -0500 Sasha Levin <sasha.levin@oracle.com> wrote:

> A random wakeup can get us out of sigsuspend() without TIF_SIGPENDING
> being set.
> 
> Avoid that by making sure we were signaled, like sys_pause() does.

What we're lacking here is any description of the end-user-visible
effects of the bug.  Enough for people to be able to decide (and to
recognize!) whether their kernel needs this patch.</stdrefrain> 

[toc] | [prev] | [next] | [standalone]

#1318399

On 01/25, Andrew Morton wrote:
>
> On Mon, 25 Jan 2016 10:21:46 -0500 Sasha Levin <sasha.levin@oracle.com> wrote:
>
> > A random wakeup can get us out of sigsuspend() without TIF_SIGPENDING
> > being set.
> >
> > Avoid that by making sure we were signaled, like sys_pause() does.
>
> What we're lacking here is any description of the end-user-visible
> effects of the bug.

The warning in dmesg and -ERESTARTNOHAND which we should never return to user
space, although I bet nobody checks the error code returned by sigsuspend().

Plus, of course, sys_sigsuspend() can return while it should not.

> Enough for people to be able to decide (and to
> recognize!) whether their kernel needs this patch.</stdrefrain>

I don't think this problem is really serious, plus it is very unlikely. The
spurious return from sigsuspend() should not really hurt.

And, ironically, there is another more serious "reverse" problem ;) sigsuspend()
orany other user of -ERESTARTNOHAND can "miss" the signal, in a sense that the
kernel can wrongly restart this syscall after return from signal handler. This
is not trivial to fix..

Oleg.

[toc] | [prev] | [next] | [standalone]

#1318753

On Tue, Jan 26, 2016 at 10:10:09PM +0100, Oleg Nesterov wrote:

> And, ironically, there is another more serious "reverse" problem ;) sigsuspend()
> orany other user of -ERESTARTNOHAND can "miss" the signal, in a sense that the
> kernel can wrongly restart this syscall after return from signal handler. This
> is not trivial to fix..

So I'm not entirely sure I get what you mean there. But it did get me to
look at the patch again:

+       while (!signal_pending(current)) {
+               __set_current_state(TASK_INTERRUPTIBLE);
+               schedule();
+       }

That should very much be:

	for (;;) {
		set_current_state(TASK_INTERRUPTIBLE);
		if (signal_pending(current))
			break;
		schedule();
	}
	__set_current_state(TASK_RUNNING);

[toc] | [prev] | [next] | [standalone]

#1319152

On 01/27, Peter Zijlstra wrote:
>
> On Tue, Jan 26, 2016 at 10:10:09PM +0100, Oleg Nesterov wrote:
>
> > And, ironically, there is another more serious "reverse" problem ;) sigsuspend()
> > orany other user of -ERESTARTNOHAND can "miss" the signal, in a sense that the
> > kernel can wrongly restart this syscall after return from signal handler. This
> > is not trivial to fix..
>
> So I'm not entirely sure I get what you mean there.

Yes, sorry. When I re-read my email it really looks like "I know the secret but I
won't tell you" ;)

The problem is simple, the fix is not. -ERESTARTNOHAND means that we should restart
if do_signal()->get_signal() returns zero. This can happen when, say, another thread
has already dequeued the group-wide signal which was the reason for TIF_SIGPENDING.
Or signal_pending() was true because of debuger, or another reason, doesn't matter.

In this case do_signal() does

			regs->ax = regs->orig_ax;
			regs->ip -= 2;

and we return to user space. Note that after that the kernel can't know that the
task is going to restart the syscall which should be interrupted by signals.

Now suppose that another signal (with the handler) comes before this task executes
the "syscall" insn. In this case sigsuspend() will restart after the task runs the
handler.

> But it did get me to
> look at the patch again:
>
> +       while (!signal_pending(current)) {
> +               __set_current_state(TASK_INTERRUPTIBLE);
> +               schedule();
> +       }
>
> That should very much be:
>
> 	for (;;) {
> 		set_current_state(TASK_INTERRUPTIBLE);
> 		if (signal_pending(current))
> 			break;
> 		schedule();
> 	}
> 	__set_current_state(TASK_RUNNING);

Why? It should work either way. Yes, signal_wakeup() can come right before
__set_current_state(TASK_INTERRUPTIBLE) but this is fine, __schedule() must not
sleep if signal_pending() == T, that is why it checks signal_pending_state().
See also the comment above smp_mb__before_spinlock() in schedule().

IOW, signal_pending() is the "special" condition, you do not need to serialize
this check with task->state setting, exactly because schedule() knows about the
signals.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1319192

On Wed, Jan 27, 2016 at 05:41:54PM +0100, Oleg Nesterov wrote:
> Why? It should work either way. Yes, signal_wakeup() can come right before
> __set_current_state(TASK_INTERRUPTIBLE) but this is fine, __schedule() must not
> sleep if signal_pending() == T,


Urgh yes, I always forget this :/

[toc] | [prev] | [next] | [standalone]

#1319304

On Wed, 27 Jan 2016 17:41:54 +0100 Oleg Nesterov <oleg@redhat.com> wrote:

> > But it did get me to
> > look at the patch again:
> >
> > +       while (!signal_pending(current)) {
> > +               __set_current_state(TASK_INTERRUPTIBLE);
> > +               schedule();
> > +       }
> >
> > That should very much be:
> >
> > 	for (;;) {
> > 		set_current_state(TASK_INTERRUPTIBLE);
> > 		if (signal_pending(current))
> > 			break;
> > 		schedule();
> > 	}
> > 	__set_current_state(TASK_RUNNING);
> 
> Why? It should work either way. Yes, signal_wakeup() can come right before
> __set_current_state(TASK_INTERRUPTIBLE) but this is fine, __schedule() must not
> sleep if signal_pending() == T, that is why it checks signal_pending_state().
> See also the comment above smp_mb__before_spinlock() in schedule().
> 
> IOW, signal_pending() is the "special" condition, you do not need to serialize
> this check with task->state setting, exactly because schedule() knows about the
> signals.

So it's non-buggy because signal_pending() is special.  But it *looks*
buggy!  And there's no comment there explaining why it looks buggy but
isn't, so someone may later come along and "fix" it for us.

[toc] | [prev] | [next] | [standalone]

#1319845

On 01/27, Andrew Morton wrote:
>
> On Wed, 27 Jan 2016 17:41:54 +0100 Oleg Nesterov <oleg@redhat.com> wrote:
>
> > IOW, signal_pending() is the "special" condition, you do not need to serialize
> > this check with task->state setting, exactly because schedule() knows about the
> > signals.
>
> So it's non-buggy because signal_pending() is special.  But it *looks*
> buggy!  And there's no comment there explaining why it looks buggy but
> isn't, so someone may later come along and "fix" it for us.

perhaps we can add a comment somewhere in sched.h to explain that a task can
never sleep with task->state == STATE if signal_pending_state(STATE) is true.

Every user of signal_pending() in the wait-event-like loop relies on this well-
known fact. Say, wait_event_interruptible() or __mutex_lock_common().

This is actually more about task->state, not about TIF_SIGPENDING imo.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1319169

On 01/27/2016 03:44 AM, Peter Zijlstra wrote:
> On Tue, Jan 26, 2016 at 10:10:09PM +0100, Oleg Nesterov wrote:
> 
>> And, ironically, there is another more serious "reverse" problem ;) sigsuspend()
>> orany other user of -ERESTARTNOHAND can "miss" the signal, in a sense that the
>> kernel can wrongly restart this syscall after return from signal handler. This
>> is not trivial to fix..
> 
> So I'm not entirely sure I get what you mean there. But it did get me to
> look at the patch again:
> 
> +       while (!signal_pending(current)) {
> +               __set_current_state(TASK_INTERRUPTIBLE);
> +               schedule();
> +       }
> 
> That should very much be:
> 
> 	for (;;) {
> 		set_current_state(TASK_INTERRUPTIBLE);
> 		if (signal_pending(current))
> 			break;
> 		schedule();
> 	}
> 	__set_current_state(TASK_RUNNING);

Should that be the case for sys_pause() too?


Thanks,
Sasha

[toc] | [prev] | [next] | [standalone]

#1317565

* Sasha Levin <sasha.levin@oracle.com> wrote:

> A random wakeup can get us out of sigsuspend() without TIF_SIGPENDING
> being set.
> 
> Avoid that by making sure we were signaled, like sys_pause() does.
> 
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> ---
>  kernel/signal.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 5da9180..3256c7e 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3528,8 +3528,10 @@ static int sigsuspend(sigset_t *set)
>  	current->saved_sigmask = current->blocked;
>  	set_current_blocked(set);
>  
> -	__set_current_state(TASK_INTERRUPTIBLE);
> -	schedule();
> +	while (!signal_pending(current)) {
> +		__set_current_state(TASK_INTERRUPTIBLE);
> +		schedule();
> +	}
>  	set_restore_sigmask();
>  	return -ERESTARTNOHAND;
>  }

So this does not appear to be anything new, right?

I agree with the fix, but I'm somewhat worried about the potential ABI impact: 
does anything exist out there that has learned to rely on spurious returns from 
SyS_sigsuspend() or SyS_rt_sigsuspend() system calls? These are one of the most 
frequently used system calls in signal based event loops.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1318020

On 01/26, Ingo Molnar wrote:
>
> * Sasha Levin <sasha.levin@oracle.com> wrote:
>
> > A random wakeup can get us out of sigsuspend() without TIF_SIGPENDING
> > being set.
> >
> > Avoid that by making sure we were signaled, like sys_pause() does.
> >
> > Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> > ---
> >  kernel/signal.c |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index 5da9180..3256c7e 100644
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -3528,8 +3528,10 @@ static int sigsuspend(sigset_t *set)
> >  	current->saved_sigmask = current->blocked;
> >  	set_current_blocked(set);
> >
> > -	__set_current_state(TASK_INTERRUPTIBLE);
> > -	schedule();
> > +	while (!signal_pending(current)) {
> > +		__set_current_state(TASK_INTERRUPTIBLE);
> > +		schedule();
> > +	}
> >  	set_restore_sigmask();
> >  	return -ERESTARTNOHAND;
> >  }
>
> So this does not appear to be anything new, right?
>
> I agree with the fix, but I'm somewhat worried about the potential ABI impact:
> does anything exist out there that has learned to rely on spurious returns from
> SyS_sigsuspend() or SyS_rt_sigsuspend() system calls?

Unlikely. We can even forget about set_restore_sigmask/TIF_RESTORE_SIGMASK and
WARN_ON(). We are going to return -ERESTARTNOHAND, this assumes that TIF_SIGPENDING
must be set and thus do_signal() will be called, userspace should never see this
error code. This is even documented in errno.h.

Oleg.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web