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

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

[Sasha Levin]

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

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

[Oleg Nesterov]

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
> 

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

[Peter Zijlstra]

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>

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

[Al Viro]

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 ;)
> 
> > 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.

Out of curiousity - where did that stray wakeup come from?  PTRACE_KILL
used to trigger those, but that got fixed.  How does one trigger that
kind of bugs on the current kernels?

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

[Peter Zijlstra]

On Thu, Feb 25, 2016 at 03:18:52AM +0000, Al Viro wrote:
> 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 ;)
> > 
> > > 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.
> 
> Out of curiousity - where did that stray wakeup come from?  PTRACE_KILL
> used to trigger those, but that got fixed.  How does one trigger that
> kind of bugs on the current kernels?

Its a regular TASK_INTERRUPTIBLE sleep, for those spurious wakeups are
not a bug, they're pretty fundamentally allowed.

See: lkml.kernel.org/r/CA+55aFwHkOo+YGWKYROmce1-H_uG3KfEUmCkJUerTj=ojY2H6Q@mail.gmail.com

But they became a lot more common because we explicitly used the delayed
wakeup pattern described there with wake_q. See commits:

  7675104990ed ("sched: Implement lockless wake-queues")
  1d0dcb3ad9d3 ("futex: Implement lockless wakeups")

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

[Al Viro]

On Thu, Feb 25, 2016 at 09:11:44AM +0100, Peter Zijlstra wrote:

> > Out of curiousity - where did that stray wakeup come from?  PTRACE_KILL
> > used to trigger those, but that got fixed.  How does one trigger that
> > kind of bugs on the current kernels?
> 
> Its a regular TASK_INTERRUPTIBLE sleep, for those spurious wakeups are
> not a bug, they're pretty fundamentally allowed.

They are, which makes any code that doesn't expect them in such situations
buggy.

> See: lkml.kernel.org/r/CA+55aFwHkOo+YGWKYROmce1-H_uG3KfEUmCkJUerTj=ojY2H6Q@mail.gmail.com

I know.  The question is not whether the code must take them into account
(it must; it's a bug not to), it's what's a good way to trigger such bugs.
IOW, how to stress-test for such bugs?

PTRACE_KILL used to be a convenient way to arrange for a wakeup delivered
to victim engaged in something we want to stress; it doesn't do blind
wake_up_process() anymore, so that trick is gone.  Is there anything
similar?

Suppose I have a dodgy waitqueue code (pardon the redundancy) in some
filesystem.  I have some idea how to maneuver a process into such-and-such
part of that code; is there any convenient way to turn that into "... OK,
now let's add bombing it with stray wakeups"?

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

[Andrew Morton]

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> 

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

[Oleg Nesterov]

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.

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

[Peter Zijlstra]

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);

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

[Oleg Nesterov]

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.

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

[Peter Zijlstra]

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 :/

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

[Andrew Morton]

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.

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

[Oleg Nesterov]

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.

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

[Sasha Levin]

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

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

[Ingo Molnar]

* 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

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

[Oleg Nesterov]

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.