Re: [PATCH] signal: remove the wrong signal_pending() check in restore_user_sigmask()

[ebiederm@xmission.com (Eric W. Biederman)]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Tue, Jun 4, 2019 at 6:41 AM Oleg Nesterov <oleg@redhat.com> wrote:
>>
>> This is the minimal fix for stable, I'll send cleanups later.
>
> Ugh. I htink this is correct, but I wish we had a better and more
> intuitive interface.
>
> In particular, since restore_user_sigmask() basically wants to check
> for "signal_pending()" anyway (to decide if the mask should be
> restored by signal handling or by that function), I really get the
> feeling that a lot of these patterns like

Linus that checking for signal_pending() in restore_user_sigmask is the
bug that caused the regression.

>> -       restore_user_sigmask(ksig.sigmask, &sigsaved);
>> -       if (signal_pending(current) && !ret)
>> +
>> +       interrupted = signal_pending(current);
>> +       restore_user_sigmask(ksig.sigmask, &sigsaved, interrupted);
>> +       if (interrupted && !ret)
>>                 ret = -ERESTARTNOHAND;
>
> are wrong to begin with, and we really should aim for an interface
> which says "tell me whether you completed the system call, and I'll
> give you an error return if not".

The pattern you are pointing out is specific to io_pgetevents and it's
variations.  It does look buggy to me but not for the reason you point
out, but instead because it does not appear to let a pending signal
cause io_pgetevents to return early.

I suspect we should fix that and have do_io_getevents return
-EINTR or -ERESTARTNOHAND like everyone else.

The concept of interrupted (aka return -EINTR to userspace) is truly
fundamental to the current semantics.  We effectively put a normally
blocked signal that was triggered back if we won't be returning -EINTR
to userspace.

> How about we make restore_user_sigmask() take two return codes: the
> 'ret' we already have, and the return we would get if there is a
> signal pending and w're currently returning zero.
>
> IOW, I think the above could become
>
>         ret = restore_user_sigmask(ksig.sigmask, &sigsaved, ret, -ERESTARTHAND);
>
> instead if we just made the right interface decision.
>
> Hmm?

At best I think that is a cleanup that will complicate creating a simple
straight forward regression fix.

Unless I am misreading things that is optimizing the interface for
dealing with broken code.

So can we please get this fix in and then look at cleaning up and
simplifying this code.

Eric

p.s. A rather compelling cleanup is to:

- Leave the signal mask alone.
- Register with signalfd_wqh for wake ups.
- Have a helper

   int signal_pending_sigmask(sigset_t *blocked)
   {
   	struct task_struct *tsk = current;
   	int ret = 0;
   	spin_lock_irq(&tsk->sighand->siglock);
        if (next_signal(&tsk->pending, blocked) ||
            next_signal(&tsk->signal->pending, blocked)) {
        	ret = -ERESTARTHAND;
                if (!sigequalsets(&tsk->blocked, blocked)) {
                	tsk->saved_sigmask = tsk->blocked;
                	__set_task_blocked(tsk, blocked);
                        set_restore_sigmask();
		}
        }
        spin_unlock_irq(&tsk->sighand->siglock);
        return ret;
   }
  
- Use that helper instead of signal_pending() in the various
  sleep functions.
- Possibly get the signal mask from tsk instead of passing it into
  all of the helpers.

Eric