-Deepa

On May 22, 2019, at 8:05 AM, Oleg Nesterov oleg@redhat.com wrote:

...

On 05/21, Deepa Dinamani wrote:

Note that this patch returns interrupted errors (EINTR, ERESTARTNOHAND, etc) only when there is no other error. If there is a signal and an error like EINVAL, the syscalls return -EINVAL rather than the interrupted error codes.

Ugh. I need to re-check, but at first glance I really dislike this change.

I think we can fix the problem _and_ simplify the code. Something like below. The patch is obviously incomplete, it changes only only one caller of set_user_sigmask(), epoll_pwait() to explain what I mean. restore_user_sigmask() should simply die. Although perhaps another helper makes sense to add WARN_ON(test_tsk_restore_sigmask() && !signal_pending).

restore_user_sigmask() was added because of all the variants of these syscalls we added because of y2038 as noted in commit message:

signal: Add restore_user_sigmask()

Refactor the logic to restore the sigmask before the syscall returns into an api. This is useful for versions of syscalls that pass in the sigmask and expect the current->sigmask to be changed during the execution and restored after the execution of the syscall.

With the advent of new y2038 syscalls in the subsequent patches, we add two more new versions of the syscalls (for pselect, ppoll and io_pgetevents) in addition to the existing native and compat versions. Adding such an api reduces the logic that would need to be replicated otherwise.

diff --git a/fs/eventpoll.c b/fs/eventpoll.c index 4a0e98d..85f56e4 100644 --- a/fs/eventpoll.c +++ b/fs/eventpoll.c @@ -2318,19 +2318,19 @@ SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct epoll_event __user *, events, size_t, sigsetsize) { int error;

• sigset_t ksigmask, sigsaved;

  /*

  • If the caller wants a certain signal mask to be set during the wait,
  • we apply it here.

  */

• error = set_user_sigmask(sigmask, &ksigmask, &sigsaved, sigsetsize);

• error = set_user_sigmask(sigmask, sigsetsize); if (error) return error;

  error = do_epoll_wait(epfd, events, maxevents, timeout);

• restore_user_sigmask(sigmask, &sigsaved);

• if (error != -EINTR)

As you address all the other syscalls this condition becomes more and more complicated.

•    restore_saved_sigmask();
  
  

  return error;

} diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h index e412c09..1e82ae0 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -416,7 +416,6 @@ void task_join_group_stop(struct task_struct *task); static inline void set_restore_sigmask(void) { set_thread_flag(TIF_RESTORE_SIGMASK);

• WARN_ON(!test_thread_flag(TIF_SIGPENDING));

So you always want do_signal() to be called? You will have to check each architecture's implementation of do_signal() to check if that has any side effects.

Although this is not what the patch is solving. What we want is to adjust return codes on all these syscalls to user and not drop signals. Please check v2/v3 of the patch. I've updated the commit text to provide more context into what is actually being fixed here.

If we really want to simplify, we should rewrite all the internal logic of all the ppoll, epoll_pwait, io_pgetevent syscall internal handling where we set the error code. As new versions of syscalls were added, the internal logic got reworked rather hapazardly. But, as the current issue points out, these are delicate changes.

-Deepa

}

static inline void clear_tsk_restore_sigmask(struct task_struct *tsk) @@ -447,7 +446,6 @@ static inline bool test_and_clear_restore_sigmask(void) static inline void set_restore_sigmask(void) { current->restore_sigmask = true;

• WARN_ON(!test_thread_flag(TIF_SIGPENDING));

} static inline void clear_tsk_restore_sigmask(struct task_struct *tsk) { diff --git a/include/linux/signal.h b/include/linux/signal.h index 9702016..887cea6 100644 --- a/include/linux/signal.h +++ b/include/linux/signal.h @@ -273,8 +273,7 @@ extern int group_send_sig_info(int sig, struct kernel_siginfo *info, struct task_struct *p, enum pid_type type); extern int __group_send_sig_info(int, struct kernel_siginfo *, struct task_struct *); extern int sigprocmask(int, sigset_t *, sigset_t *); -extern int set_user_sigmask(const sigset_t __user *usigmask, sigset_t *set,

• sigset_t *oldset, size_t sigsetsize);

+extern int set_user_sigmask(const sigset_t __user *umask, size_t sigsetsize); extern void restore_user_sigmask(const void __user *usigmask, sigset_t *sigsaved); extern void set_current_blocked(sigset_t *); diff --git a/kernel/signal.c b/kernel/signal.c index 227ba17..76f4f9a 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -2801,19 +2801,21 @@ EXPORT_SYMBOL(sigprocmask);

• This is useful for syscalls such as ppoll, pselect, io_pgetevents and
• epoll_pwait where a new sigmask is passed from userland for the syscalls.

*/ -int set_user_sigmask(const sigset_t __user *usigmask, sigset_t *set,

•         sigset_t *oldset, size_t sigsetsize)
  

+int set_user_sigmask(const sigset_t __user *umask, size_t sigsetsize) {

• if (!usigmask)

• sigset_t *kmask;

• if (!umask) return 0;

  if (sigsetsize != sizeof(sigset_t)) return -EINVAL;

• if (copy_from_user(set, usigmask, sizeof(sigset_t)))

• if (copy_from_user(kmask, umask, sizeof(sigset_t))) return -EFAULT;

• *oldset = current->blocked;
• set_current_blocked(set);

• set_restore_sigmask();

• current->saved_sigmask = current->blocked;

• set_current_blocked(kmask);

  return 0;

} @@ -2840,39 +2842,6 @@ int set_compat_user_sigmask(const compat_sigset_t __user *usigmask, EXPORT_SYMBOL(set_compat_user_sigmask); #endif

-/*

• • restore_user_sigmask:
• • usigmask: sigmask passed in from userland.
• • sigsaved: saved sigmask when the syscall started and changed the sigmask to

• •       usigmask.
    

• • This is useful for syscalls such as ppoll, pselect, io_pgetevents and
• • epoll_pwait where a new sigmask is passed in from userland for the syscalls.
• */

-void restore_user_sigmask(const void __user *usigmask, sigset_t *sigsaved) -{

• if (!usigmask)

•    return;
  

• /*

• * When signals are pending, do not restore them here.
  

• * Restoring sigmask here can lead to delivering signals that the above
  

• * syscalls are intended to block because of the sigmask passed in.
  

• */
  

• if (signal_pending(current)) {

•    current->saved_sigmask = *sigsaved;
  

•    set_restore_sigmask();
  

•    return;
  

• }
• /*

• * This is needed because the fast syscall return path does not restore
  

• * saved_sigmask when signals are not pending.
  

• */
  

• set_current_blocked(sigsaved);

-} -EXPORT_SYMBOL(restore_user_sigmask);

/**

• sys_rt_sigprocmask - change the list of currently blocked signals
• @how: whether to add, remove, or set signals

On Wed, May 22, 2019 at 9:14 AM Oleg Nesterov oleg@redhat.com wrote:

On 05/22, Deepa Dinamani wrote:

...

-Deepa

...

On May 22, 2019, at 8:05 AM, Oleg Nesterov oleg@redhat.com wrote:

...

On 05/21, Deepa Dinamani wrote:

Note that this patch returns interrupted errors (EINTR, ERESTARTNOHAND, etc) only when there is no other error. If there is a signal and an error like EINVAL, the syscalls return -EINVAL rather than the interrupted error codes.

Ugh. I need to re-check, but at first glance I really dislike this change.

I think we can fix the problem _and_ simplify the code. Something like below. The patch is obviously incomplete, it changes only only one caller of set_user_sigmask(), epoll_pwait() to explain what I mean. restore_user_sigmask() should simply die. Although perhaps another helper makes sense to add WARN_ON(test_tsk_restore_sigmask() && !signal_pending).

restore_user_sigmask() was added because of all the variants of these syscalls we added because of y2038 as noted in commit message:

signal: Add restore_user_sigmask()

Refactor the logic to restore the sigmask before the syscall
returns into an api.
This is useful for versions of syscalls that pass in the
sigmask and expect the current->sigmask to be changed during
the execution and restored after the execution of the syscall.

With the advent of new y2038 syscalls in the subsequent patches,
we add two more new versions of the syscalls (for pselect, ppoll
and io_pgetevents) in addition to the existing native and compat
versions. Adding such an api reduces the logic that would need to
be replicated otherwise.

Again, I need to re-check, will continue tomorrow. But so far I am not sure this helper can actually help.

...

...

--- a/fs/eventpoll.c +++ b/fs/eventpoll.c @@ -2318,19 +2318,19 @@ SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct epoll_event __user *, events, size_t, sigsetsize) { int error;

• sigset_t ksigmask, sigsaved;

  /*

  • If the caller wants a certain signal mask to be set during the wait,
  • we apply it here.

  */

• error = set_user_sigmask(sigmask, &ksigmask, &sigsaved, sigsetsize);

• error = set_user_sigmask(sigmask, sigsetsize); if (error) return error;

  error = do_epoll_wait(epfd, events, maxevents, timeout);

• restore_user_sigmask(sigmask, &sigsaved);

• if (error != -EINTR)

As you address all the other syscalls this condition becomes more and more complicated.

May be.

...

...

--- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -416,7 +416,6 @@ void task_join_group_stop(struct task_struct *task); static inline void set_restore_sigmask(void) { set_thread_flag(TIF_RESTORE_SIGMASK);

• WARN_ON(!test_thread_flag(TIF_SIGPENDING));

So you always want do_signal() to be called?

Why do you think so? No. This is just to avoid the warning, because with the patch I sent set_restore_sigmask() is called "in advance".

...

You will have to check each architecture's implementation of do_signal() to check if that has any side effects.

I don't think so.

Why not?

...

Although this is not what the patch is solving.

Sure. But you know, after I tried to read the changelog, I am not sure I understand what exactly you are trying to fix. Could you please explain this part

    The behavior
    before 854a6ed56839a was that the signals were dropped after the error
    code was decided. This resulted in lost signals but the userspace did not
    notice it

? I fail to understand it, sorry. It looks as if the code was already buggy before that commit and it could miss a signal or something like this, but I do not see how.

Did you read the explanation pointed to in the commit text? :

https://lore.kernel.org/linux-fsdevel/20190427093319.sgicqik2oqkez3wk@dcvr/

Let me know what part you don't understand and I can explain more.

It would be better to understand the isssue before we start discussing the fix.

-Deepa

On 05/28, Deepa Dinamani wrote:

I agree that signal handller being called and return value not being altered is an issue with other syscalls also. I was just wondering if some userspace code assumption would be assuming this. This is not a kernel bug.

But, I do not think we have an understanding of what was wrong in 854a6ed56839a anymore since you pointed out that my assumption was not correct that the signal handler being called without errno being set is wrong.

Deepa, sorry, I simply can't parse the above... most probably because of my bad English.

One open question: this part of epoll_pwait was already broken before 854a6ed56839a. Do you agree?

if (err == -EINTR) { memcpy(&current->saved_sigmask, &sigsaved, sizeof(sigsaved)); set_restore_sigmask(); } else set_current_blocked(&sigsaved);

I do not understand why do you think this part was broken :/

Or, I could revert the signal_pending() check and provide a fix something like below(not a complete patch)

...

-void restore_user_sigmask(const void __user *usigmask, sigset_t *sigsaved) +int restore_user_sigmask(const void __user *usigmask, sigset_t *sigsaved, int sig_pending) {

    if (!usigmask)
           return;

    /*
     * When signals are pending, do not restore them here.
     * Restoring sigmask here can lead to delivering signals that the above
     * syscalls are intended to block because of the sigmask passed in.
     */

•   if (sig_pending) {
            current->saved_sigmask = *sigsaved;
            set_restore_sigmask();
           return;
       }
  
  

@@ -2330,7 +2330,8 @@ SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct epoll_event __user *, events,

    error = do_epoll_wait(epfd, events, maxevents, timeout);

•   restore_user_sigmask(sigmask, &sigsaved);
  

•   signal_detected = restore_user_sigmask(sigmask, &sigsaved,
  

error == -EINTR);

I fail to understand this pseudo-code, sorry. In particular, do not understand why restore_user_sigmask() needs to return a boolean.

The only thing I _seem to_ understand is the "sig_pending" flag passed by the caller which replaces the signal_pending() check. Yes, this is what I think we should do, and this is what I tried to propose from the very beginning in my 1st email in this thread.

Oleg.

Arnd Bergmann arnd@arndb.de writes:

On Wed, May 29, 2019 at 6:12 PM Oleg Nesterov oleg@redhat.com wrote:

...

Al, Linus, Eric, please help.

The previous discussion was very confusing, we simply can not understand each other.

To me everything looks very simple and clear, but perhaps I missed something obvious? Please correct me.

Thanks for the elaborate explanation in this patch, it all starts making sense to me now. I also looked at your patch in detail and thought I had found a few mistakes at first but those all turned out to be mistakes in my reading.

...

See the compile-tested patch at the end. Of course, the new _xxx() helpers should be renamed somehow. fs/aio.c doesn't look right with or without this patch, but iiuc this is what it did before 854a6ed56839a.

I think this is a nice simplification, but it would help not to mix up the minimal regression fix with the rewrite of those functions. For the stable kernels, I think we want just the addition of the 'bool interrupted' argument to restore_user_sigmask() to close the race that was introduced 854a6ed56839a. Following up on that for future kernels, your patch improves the readability, but we can probably take it even further.

...

•   ret = set_user_sigmask(ksig.sigmask, &ksigmask, &sigsaved, ksig.sigsetsize);
  

•   ret = set_xxx(ksig.sigmask, ksig.sigsetsize);
    if (ret)
            return ret;
  
    ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);
  

•   restore_user_sigmask(ksig.sigmask, &sigsaved);
  

•   if (signal_pending(current) && !ret)
  

•   interrupted = signal_pending(current);
  

•   update_xxx(interrupted);
  

Maybe name this

       restore_saved_sigmask_if(!interrupted);

and make restore_saved_sigmask_if() an inline function next to restore_saved_sigmask()?

...

@@ -2201,13 +2205,15 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents, if (usig && copy_from_user(&ksig, usig, sizeof(ksig))) return -EFAULT;

•   ret = set_compat_user_sigmask(ksig.sigmask, &ksigmask, &sigsaved, ksig.sigsetsize);
  

•   ret = set_compat_xxx(ksig.sigmask, ksig.sigsetsize);
    if (ret)
            return ret;
  

With some of the recent discussions about compat syscall handling, I now think that we want to just fold set_compat_user_sigmask() into set_user_sigmask() (whatever they get called in the end) with an in_compat_syscall() conditional inside it, and completely get rid of the COMPAT_SYSCALL_DEFINEx() definitions for those system calls for which this is the only difference.

Unfortunately we still need the time32/time64 distinction, but removing syscall handlers is a significant cleanup here already, and we can move most of the function body of sys_io_pgetevents() into do_io_getevents() in the process. Same for some of the other calls.

Not sure about the order of the cleanups, but probably something like this would work:

1. fix the race (to be backported)
2. unify set_compat_user_sigmask/set_user_sigmask
3. remove unneeded compat handlers
4. replace restore_user_sigmask with restore_saved_sigmask_if()
5. also unify compat_get_fd_set()/get_fd_set() and kill off compat select() variants.

Are new system calls added preventing a revert of the patch in question for stable kernels?

Eric

Oleg Nesterov oleg@redhat.com writes:

This is the minimal fix for stable, I'll send cleanups later.

The commit 854a6ed56839a40f6b5d02a2962f48841482eec4 ("signal: Add restore_user_sigmask()") introduced the visible change which breaks user-space: a signal temporary unblocked by set_user_sigmask() can be delivered even if the caller returns success or timeout.

Change restore_user_sigmask() to accept the additional "interrupted" argument which should be used instead of signal_pending() check, and update the callers.

Reported-by: Eric Wong e@80x24.org Fixes: 854a6ed56839a40f6b5d02a2962f48841482eec4 ("signal: Add restore_user_sigmask()") cc: stable@vger.kernel.org (v5.0+) Signed-off-by: Oleg Nesterov oleg@redhat.com

Acked-by: "Eric W. Biederman" ebiederm@xmission.com

I have read through the patch and it looks good.

For clarity. I don't think this is required by posix, or fundamentally to remove the races in select. It is what linux has always done and we have applications who care so I agree this fix is needed.

Further in any case where the semantic change that this patch rolls back (aka where allowing a signal to be delivered and the select like call to complete) would be advantage we can do as well if not better by using signalfd.

Michael is there any chance we can get this guarantee of the linux implementation of pselect and friends clearly documented. The guarantee that if the system call completes successfully we are guaranteed that no signal that is unblocked by using sigmask will be delivered?

Eric

---

fs/aio.c | 28 ++++++++++++++++++++-------- fs/eventpoll.c | 4 ++-- fs/io_uring.c | 7 ++++--- fs/select.c | 18 ++++++------------ include/linux/signal.h | 2 +- kernel/signal.c | 5 +++-- 6 files changed, 36 insertions(+), 28 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c index 3490d1f..c1e581d 100644 --- a/fs/aio.c +++ b/fs/aio.c @@ -2095,6 +2095,7 @@ SYSCALL_DEFINE6(io_pgetevents, struct __aio_sigset ksig = { NULL, }; sigset_t ksigmask, sigsaved; struct timespec64 ts;

• bool interrupted; int ret;

if (timeout && unlikely(get_timespec64(&ts, timeout))) @@ -2108,8 +2109,10 @@ SYSCALL_DEFINE6(io_pgetevents, return ret; ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);

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

return ret; @@ -2128,6 +2131,7 @@ SYSCALL_DEFINE6(io_pgetevents_time32, struct __aio_sigset ksig = { NULL, }; sigset_t ksigmask, sigsaved; struct timespec64 ts;

• bool interrupted; int ret;

if (timeout && unlikely(get_old_timespec32(&ts, timeout))) @@ -2142,8 +2146,10 @@ SYSCALL_DEFINE6(io_pgetevents_time32, return ret; ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);

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

return ret; @@ -2193,6 +2199,7 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents, struct __compat_aio_sigset ksig = { NULL, }; sigset_t ksigmask, sigsaved; struct timespec64 t;

• bool interrupted; int ret;

if (timeout && get_old_timespec32(&t, timeout)) @@ -2206,8 +2213,10 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents, return ret; ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);

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

return ret; @@ -2226,6 +2235,7 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents_time64, struct __compat_aio_sigset ksig = { NULL, }; sigset_t ksigmask, sigsaved; struct timespec64 t;

• bool interrupted; int ret;

if (timeout && get_timespec64(&t, timeout)) @@ -2239,8 +2249,10 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents_time64, return ret; ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);

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

return ret; diff --git a/fs/eventpoll.c b/fs/eventpoll.c index c6f5131..4c74c76 100644 --- a/fs/eventpoll.c +++ b/fs/eventpoll.c @@ -2325,7 +2325,7 @@ SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct epoll_event __user *, events, error = do_epoll_wait(epfd, events, maxevents, timeout);

• restore_user_sigmask(sigmask, &sigsaved);

• restore_user_sigmask(sigmask, &sigsaved, error == -EINTR);

return error; } @@ -2350,7 +2350,7 @@ COMPAT_SYSCALL_DEFINE6(epoll_pwait, int, epfd, err = do_epoll_wait(epfd, events, maxevents, timeout);

• restore_user_sigmask(sigmask, &sigsaved);

• restore_user_sigmask(sigmask, &sigsaved, err == -EINTR);

return err; } diff --git a/fs/io_uring.c b/fs/io_uring.c index 0fbb486..1147c5d 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -2201,11 +2201,12 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events, } ret = wait_event_interruptible(ctx->wait, io_cqring_events(ring) >= min_events);

• if (ret == -ERESTARTSYS)

• ret = -EINTR;
  

if (sig)

• restore_user_sigmask(sig, &sigsaved);
  

• restore_user_sigmask(sig, &sigsaved, ret == -ERESTARTSYS);
  

• if (ret == -ERESTARTSYS)

• ret = -EINTR;
  

return READ_ONCE(ring->r.head) == READ_ONCE(ring->r.tail) ? ret : 0; } diff --git a/fs/select.c b/fs/select.c index 6cbc9ff..a4d8f6e 100644 --- a/fs/select.c +++ b/fs/select.c @@ -758,10 +758,9 @@ static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp, return ret; ret = core_sys_select(n, inp, outp, exp, to);

• restore_user_sigmask(sigmask, &sigsaved, ret == -ERESTARTNOHAND); ret = poll_select_copy_remaining(&end_time, tsp, type, ret);

• restore_user_sigmask(sigmask, &sigsaved);
• return ret;

} @@ -1106,8 +1105,7 @@ SYSCALL_DEFINE5(ppoll, struct pollfd __user *, ufds, unsigned int, nfds, ret = do_sys_poll(ufds, nfds, to);

• restore_user_sigmask(sigmask, &sigsaved);

• restore_user_sigmask(sigmask, &sigsaved, ret == -EINTR); /* We can restart this syscall, usually */ if (ret == -EINTR) ret = -ERESTARTNOHAND;

@@ -1142,8 +1140,7 @@ SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds, unsigned int, nfds, ret = do_sys_poll(ufds, nfds, to);

• restore_user_sigmask(sigmask, &sigsaved);

• restore_user_sigmask(sigmask, &sigsaved, ret == -EINTR); /* We can restart this syscall, usually */ if (ret == -EINTR) ret = -ERESTARTNOHAND;

@@ -1350,10 +1347,9 @@ static long do_compat_pselect(int n, compat_ulong_t __user *inp, return ret; ret = compat_core_sys_select(n, inp, outp, exp, to);

• restore_user_sigmask(sigmask, &sigsaved, ret == -ERESTARTNOHAND); ret = poll_select_copy_remaining(&end_time, tsp, type, ret);

• restore_user_sigmask(sigmask, &sigsaved);
• return ret;

} @@ -1425,8 +1421,7 @@ COMPAT_SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds, ret = do_sys_poll(ufds, nfds, to);

• restore_user_sigmask(sigmask, &sigsaved);

• restore_user_sigmask(sigmask, &sigsaved, ret == -EINTR); /* We can restart this syscall, usually */ if (ret == -EINTR) ret = -ERESTARTNOHAND;

@@ -1461,8 +1456,7 @@ COMPAT_SYSCALL_DEFINE5(ppoll_time64, struct pollfd __user *, ufds, ret = do_sys_poll(ufds, nfds, to);

• restore_user_sigmask(sigmask, &sigsaved);

• restore_user_sigmask(sigmask, &sigsaved, ret == -EINTR); /* We can restart this syscall, usually */ if (ret == -EINTR) ret = -ERESTARTNOHAND;

diff --git a/include/linux/signal.h b/include/linux/signal.h index 9702016..78c2bb3 100644 --- a/include/linux/signal.h +++ b/include/linux/signal.h @@ -276,7 +276,7 @@ extern int sigprocmask(int, sigset_t *, sigset_t *); extern int set_user_sigmask(const sigset_t __user *usigmask, sigset_t *set, sigset_t *oldset, size_t sigsetsize); extern void restore_user_sigmask(const void __user *usigmask,

• 		 sigset_t *sigsaved);
  

• 		 sigset_t *sigsaved, bool interrupted);
  

extern void set_current_blocked(sigset_t *); extern void __set_current_blocked(const sigset_t *); extern int show_unhandled_signals; diff --git a/kernel/signal.c b/kernel/signal.c index 328a01e..aa6a6f1 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -2912,7 +2912,8 @@ EXPORT_SYMBOL(set_compat_user_sigmask);

• This is useful for syscalls such as ppoll, pselect, io_pgetevents and
• epoll_pwait where a new sigmask is passed in from userland for the syscalls.

*/ -void restore_user_sigmask(const void __user *usigmask, sigset_t *sigsaved) +void restore_user_sigmask(const void __user *usigmask, sigset_t *sigsaved,

• 		bool interrupted)
  

{ if (!usigmask) @@ -2922,7 +2923,7 @@ void restore_user_sigmask(const void __user *usigmask, sigset_t *sigsaved) * Restoring sigmask here can lead to delivering signals that the above * syscalls are intended to block because of the sigmask passed in. */

• if (signal_pending(current)) {

• if (interrupted) { current->saved_sigmask = *sigsaved; set_restore_sigmask(); return;

Oleg Nesterov oleg@redhat.com wrote:

This is the minimal fix for stable, I'll send cleanups later.

The commit 854a6ed56839a40f6b5d02a2962f48841482eec4 ("signal: Add restore_user_sigmask()") introduced the visible change which breaks user-space: a signal temporary unblocked by set_user_sigmask() can be delivered even if the caller returns success or timeout.

Change restore_user_sigmask() to accept the additional "interrupted" argument which should be used instead of signal_pending() check, and update the callers.

Reported-by: Eric Wong e@80x24.org Fixes: 854a6ed56839a40f6b5d02a2962f48841482eec4 ("signal: Add restore_user_sigmask()") cc: stable@vger.kernel.org (v5.0+) Signed-off-by: Oleg Nesterov oleg@redhat.com

Thanks, for epoll_pwait on top of Linux v5.1.7 and cmogstored v1.7.0:

Tested-by: Eric Wong e@80x24.org

(cmogstored v1.7.1 already works around this when it sees a 0 return value (but not >0, yet...))

diff --git a/fs/io_uring.c b/fs/io_uring.c index 0fbb486..1147c5d 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -2201,11 +2201,12 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events, } ret = wait_event_interruptible(ctx->wait, io_cqring_events(ring) >= min_events);

• if (ret == -ERESTARTSYS)

• ret = -EINTR;
  

if (sig)

• restore_user_sigmask(sig, &sigsaved);
  

• restore_user_sigmask(sig, &sigsaved, ret == -ERESTARTSYS);
  

• if (ret == -ERESTARTSYS)

• ret = -EINTR;
  

return READ_ONCE(ring->r.head) == READ_ONCE(ring->r.tail) ? ret : 0; }

That io_uring bit didn't apply cleanly to stable, since stable is missing fdb288a679cdf6a71f3c1ae6f348ba4dae742681 ("io_uring: use wait_event_interruptible for cq_wait conditional wait") and related commits.

In any case, I'm not using io_uring anywhere, yet (and probably won't, since I'll still need threads to deal with open/unlink/rename on slow JBOD HDDs).

task->saved_sigmask and ->restore_sigmask are only used in the ret-from- syscall paths. This means that set_user_sigmask() can save ->blocked in ->saved_sigmask and do set_restore_sigmask() to indicate that ->blocked was modified.

This way the callers do not need 2 sigset_t's passed to set/restore and restore_user_sigmask() renamed to restore_saved_sigmask_unless() turns into the trivial helper which just calls restore_saved_sigmask().

Signed-off-by: Oleg Nesterov oleg@redhat.com --- fs/aio.c | 20 +++++-------- fs/eventpoll.c | 12 +++----- fs/io_uring.c | 11 ++----- fs/select.c | 34 ++++++++-------------- include/linux/compat.h | 3 +- include/linux/sched/signal.h | 12 ++++++-- include/linux/signal.h | 4 --- kernel/signal.c | 69 ++++++++++++-------------------------------- 8 files changed, 57 insertions(+), 108 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c index c1e581d..8200f97 100644 --- a/fs/aio.c +++ b/fs/aio.c @@ -2093,7 +2093,6 @@ SYSCALL_DEFINE6(io_pgetevents, const struct __aio_sigset __user *, usig) { struct __aio_sigset ksig = { NULL, }; - sigset_t ksigmask, sigsaved; struct timespec64 ts; bool interrupted; int ret; @@ -2104,14 +2103,14 @@ SYSCALL_DEFINE6(io_pgetevents, if (usig && copy_from_user(&ksig, usig, sizeof(ksig))) return -EFAULT;

- ret = set_user_sigmask(ksig.sigmask, &ksigmask, &sigsaved, ksig.sigsetsize); + ret = set_user_sigmask(ksig.sigmask, ksig.sigsetsize); if (ret) return ret;

ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);

interrupted = signal_pending(current); - restore_user_sigmask(ksig.sigmask, &sigsaved, interrupted); + restore_saved_sigmask_unless(interrupted); if (interrupted && !ret) ret = -ERESTARTNOHAND;

@@ -2129,7 +2128,6 @@ SYSCALL_DEFINE6(io_pgetevents_time32, const struct __aio_sigset __user *, usig) { struct __aio_sigset ksig = { NULL, }; - sigset_t ksigmask, sigsaved; struct timespec64 ts; bool interrupted; int ret; @@ -2141,14 +2139,14 @@ SYSCALL_DEFINE6(io_pgetevents_time32, return -EFAULT;

- ret = set_user_sigmask(ksig.sigmask, &ksigmask, &sigsaved, ksig.sigsetsize); + ret = set_user_sigmask(ksig.sigmask, ksig.sigsetsize); if (ret) return ret;

ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);

interrupted = signal_pending(current); - restore_user_sigmask(ksig.sigmask, &sigsaved, interrupted); + restore_saved_sigmask_unless(interrupted); if (interrupted && !ret) ret = -ERESTARTNOHAND;

@@ -2197,7 +2195,6 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents, const struct __compat_aio_sigset __user *, usig) { struct __compat_aio_sigset ksig = { NULL, }; - sigset_t ksigmask, sigsaved; struct timespec64 t; bool interrupted; int ret; @@ -2208,14 +2205,14 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents, if (usig && copy_from_user(&ksig, usig, sizeof(ksig))) return -EFAULT;

- ret = set_compat_user_sigmask(ksig.sigmask, &ksigmask, &sigsaved, ksig.sigsetsize); + ret = set_compat_user_sigmask(ksig.sigmask, ksig.sigsetsize); if (ret) return ret;

ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);

interrupted = signal_pending(current); - restore_user_sigmask(ksig.sigmask, &sigsaved, interrupted); + restore_saved_sigmask_unless(interrupted); if (interrupted && !ret) ret = -ERESTARTNOHAND;

@@ -2233,7 +2230,6 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents_time64, const struct __compat_aio_sigset __user *, usig) { struct __compat_aio_sigset ksig = { NULL, }; - sigset_t ksigmask, sigsaved; struct timespec64 t; bool interrupted; int ret; @@ -2244,14 +2240,14 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents_time64, if (usig && copy_from_user(&ksig, usig, sizeof(ksig))) return -EFAULT;

- ret = set_compat_user_sigmask(ksig.sigmask, &ksigmask, &sigsaved, ksig.sigsetsize); + ret = set_compat_user_sigmask(ksig.sigmask, ksig.sigsetsize); if (ret) return ret;

ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);

interrupted = signal_pending(current); - restore_user_sigmask(ksig.sigmask, &sigsaved, interrupted); + restore_saved_sigmask_unless(interrupted); if (interrupted && !ret) ret = -ERESTARTNOHAND;

diff --git a/fs/eventpoll.c b/fs/eventpoll.c index 4c74c76..0f9c073 100644 --- a/fs/eventpoll.c +++ b/fs/eventpoll.c @@ -2313,19 +2313,17 @@ SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct epoll_event __user *, events, size_t, sigsetsize) { int error; - sigset_t ksigmask, sigsaved;

/* * If the caller wants a certain signal mask to be set during the wait, * we apply it here. */ - error = set_user_sigmask(sigmask, &ksigmask, &sigsaved, sigsetsize); + error = set_user_sigmask(sigmask, sigsetsize); if (error) return error;

error = do_epoll_wait(epfd, events, maxevents, timeout); - - restore_user_sigmask(sigmask, &sigsaved, error == -EINTR); + restore_saved_sigmask_unless(error == -EINTR);

return error; } @@ -2338,19 +2336,17 @@ COMPAT_SYSCALL_DEFINE6(epoll_pwait, int, epfd, compat_size_t, sigsetsize) { long err; - sigset_t ksigmask, sigsaved;

/* * If the caller wants a certain signal mask to be set during the wait, * we apply it here. */ - err = set_compat_user_sigmask(sigmask, &ksigmask, &sigsaved, sigsetsize); + err = set_compat_user_sigmask(sigmask, sigsetsize); if (err) return err;

err = do_epoll_wait(epfd, events, maxevents, timeout); - - restore_user_sigmask(sigmask, &sigsaved, err == -EINTR); + restore_saved_sigmask_unless(err == -EINTR);

return err; } diff --git a/fs/io_uring.c b/fs/io_uring.c index 1147c5d..ad7b9bc 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -2180,7 +2180,6 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events, const sigset_t __user *sig, size_t sigsz) { struct io_cq_ring *ring = ctx->cq_ring; - sigset_t ksigmask, sigsaved; int ret;

if (io_cqring_events(ring) >= min_events) @@ -2190,21 +2189,17 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events, #ifdef CONFIG_COMPAT if (in_compat_syscall()) ret = set_compat_user_sigmask((const compat_sigset_t __user *)sig, - &ksigmask, &sigsaved, sigsz); + sigsz); else #endif - ret = set_user_sigmask(sig, &ksigmask, - &sigsaved, sigsz); + ret = set_user_sigmask(sig, sigsz);

if (ret) return ret; }

ret = wait_event_interruptible(ctx->wait, io_cqring_events(ring) >= min_events); - - if (sig) - restore_user_sigmask(sig, &sigsaved, ret == -ERESTARTSYS); - + restore_saved_sigmask_unless(ret == -ERESTARTSYS); if (ret == -ERESTARTSYS) ret = -EINTR;

diff --git a/fs/select.c b/fs/select.c index a4d8f6e..1fc1b24 100644 --- a/fs/select.c +++ b/fs/select.c @@ -730,7 +730,6 @@ static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp, const sigset_t __user *sigmask, size_t sigsetsize, enum poll_time_type type) { - sigset_t ksigmask, sigsaved; struct timespec64 ts, end_time, *to = NULL; int ret;

@@ -753,12 +752,12 @@ static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp, return -EINVAL; }

- ret = set_user_sigmask(sigmask, &ksigmask, &sigsaved, sigsetsize); + ret = set_user_sigmask(sigmask, sigsetsize); if (ret) return ret;

ret = core_sys_select(n, inp, outp, exp, to); - restore_user_sigmask(sigmask, &sigsaved, ret == -ERESTARTNOHAND); + restore_saved_sigmask_unless(ret == -ERESTARTNOHAND); ret = poll_select_copy_remaining(&end_time, tsp, type, ret);

return ret; @@ -1086,7 +1085,6 @@ SYSCALL_DEFINE5(ppoll, struct pollfd __user *, ufds, unsigned int, nfds, struct __kernel_timespec __user *, tsp, const sigset_t __user *, sigmask, size_t, sigsetsize) { - sigset_t ksigmask, sigsaved; struct timespec64 ts, end_time, *to = NULL; int ret;

@@ -1099,17 +1097,16 @@ SYSCALL_DEFINE5(ppoll, struct pollfd __user *, ufds, unsigned int, nfds, return -EINVAL; }

- ret = set_user_sigmask(sigmask, &ksigmask, &sigsaved, sigsetsize); + ret = set_user_sigmask(sigmask, sigsetsize); if (ret) return ret;

ret = do_sys_poll(ufds, nfds, to);

- restore_user_sigmask(sigmask, &sigsaved, ret == -EINTR); + restore_saved_sigmask_unless(ret == -EINTR); /* We can restart this syscall, usually */ if (ret == -EINTR) ret = -ERESTARTNOHAND; - ret = poll_select_copy_remaining(&end_time, tsp, PT_TIMESPEC, ret);

return ret; @@ -1121,7 +1118,6 @@ SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds, unsigned int, nfds, struct old_timespec32 __user *, tsp, const sigset_t __user *, sigmask, size_t, sigsetsize) { - sigset_t ksigmask, sigsaved; struct timespec64 ts, end_time, *to = NULL; int ret;

@@ -1134,17 +1130,16 @@ SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds, unsigned int, nfds, return -EINVAL; }

- ret = set_user_sigmask(sigmask, &ksigmask, &sigsaved, sigsetsize); + ret = set_user_sigmask(sigmask, sigsetsize); if (ret) return ret;

ret = do_sys_poll(ufds, nfds, to);

- restore_user_sigmask(sigmask, &sigsaved, ret == -EINTR); + restore_saved_sigmask_unless(ret == -EINTR); /* We can restart this syscall, usually */ if (ret == -EINTR) ret = -ERESTARTNOHAND; - ret = poll_select_copy_remaining(&end_time, tsp, PT_OLD_TIMESPEC, ret);

return ret; @@ -1319,7 +1314,6 @@ static long do_compat_pselect(int n, compat_ulong_t __user *inp, void __user *tsp, compat_sigset_t __user *sigmask, compat_size_t sigsetsize, enum poll_time_type type) { - sigset_t ksigmask, sigsaved; struct timespec64 ts, end_time, *to = NULL; int ret;

@@ -1342,12 +1336,12 @@ static long do_compat_pselect(int n, compat_ulong_t __user *inp, return -EINVAL; }

- ret = set_compat_user_sigmask(sigmask, &ksigmask, &sigsaved, sigsetsize); + ret = set_compat_user_sigmask(sigmask, sigsetsize); if (ret) return ret;

ret = compat_core_sys_select(n, inp, outp, exp, to); - restore_user_sigmask(sigmask, &sigsaved, ret == -ERESTARTNOHAND); + restore_saved_sigmask_unless(ret == -ERESTARTNOHAND); ret = poll_select_copy_remaining(&end_time, tsp, type, ret);

return ret; @@ -1402,7 +1396,6 @@ COMPAT_SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds, unsigned int, nfds, struct old_timespec32 __user *, tsp, const compat_sigset_t __user *, sigmask, compat_size_t, sigsetsize) { - sigset_t ksigmask, sigsaved; struct timespec64 ts, end_time, *to = NULL; int ret;

@@ -1415,17 +1408,16 @@ COMPAT_SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds, return -EINVAL; }

- ret = set_compat_user_sigmask(sigmask, &ksigmask, &sigsaved, sigsetsize); + ret = set_compat_user_sigmask(sigmask, sigsetsize); if (ret) return ret;

ret = do_sys_poll(ufds, nfds, to);

- restore_user_sigmask(sigmask, &sigsaved, ret == -EINTR); + restore_saved_sigmask_unless(ret == -EINTR); /* We can restart this syscall, usually */ if (ret == -EINTR) ret = -ERESTARTNOHAND; - ret = poll_select_copy_remaining(&end_time, tsp, PT_OLD_TIMESPEC, ret);

return ret; @@ -1437,7 +1429,6 @@ COMPAT_SYSCALL_DEFINE5(ppoll_time64, struct pollfd __user *, ufds, unsigned int, nfds, struct __kernel_timespec __user *, tsp, const compat_sigset_t __user *, sigmask, compat_size_t, sigsetsize) { - sigset_t ksigmask, sigsaved; struct timespec64 ts, end_time, *to = NULL; int ret;

@@ -1450,17 +1441,16 @@ COMPAT_SYSCALL_DEFINE5(ppoll_time64, struct pollfd __user *, ufds, return -EINVAL; }

- ret = set_compat_user_sigmask(sigmask, &ksigmask, &sigsaved, sigsetsize); + ret = set_compat_user_sigmask(sigmask, sigsetsize); if (ret) return ret;

ret = do_sys_poll(ufds, nfds, to);

- restore_user_sigmask(sigmask, &sigsaved, ret == -EINTR); + restore_saved_sigmask_unless(ret == -EINTR); /* We can restart this syscall, usually */ if (ret == -EINTR) ret = -ERESTARTNOHAND; - ret = poll_select_copy_remaining(&end_time, tsp, PT_TIMESPEC, ret);

return ret; diff --git a/include/linux/compat.h b/include/linux/compat.h index ebddcb6..16dafd9 100644 --- a/include/linux/compat.h +++ b/include/linux/compat.h @@ -138,8 +138,7 @@ typedef struct { compat_sigset_word sig[_COMPAT_NSIG_WORDS]; } compat_sigset_t;

-int set_compat_user_sigmask(const compat_sigset_t __user *usigmask, - sigset_t *set, sigset_t *oldset, +int set_compat_user_sigmask(const compat_sigset_t __user *umask, size_t sigsetsize);

struct compat_sigaction { diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h index 38a0f07..4d9c71d 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -417,7 +417,6 @@ void task_join_group_stop(struct task_struct *task); static inline void set_restore_sigmask(void) { set_thread_flag(TIF_RESTORE_SIGMASK); - WARN_ON(!test_thread_flag(TIF_SIGPENDING)); }

static inline void clear_tsk_restore_sigmask(struct task_struct *task) @@ -448,7 +447,6 @@ static inline bool test_and_clear_restore_sigmask(void) static inline void set_restore_sigmask(void) { current->restore_sigmask = true; - WARN_ON(!test_thread_flag(TIF_SIGPENDING)); } static inline void clear_tsk_restore_sigmask(struct task_struct *task) { @@ -481,6 +479,16 @@ static inline void restore_saved_sigmask(void) __set_current_blocked(&current->saved_sigmask); }

+extern int set_user_sigmask(const sigset_t __user *umask, size_t sigsetsize); + +static inline void restore_saved_sigmask_unless(bool interrupted) +{ + if (interrupted) + WARN_ON(!test_thread_flag(TIF_SIGPENDING)); + else + restore_saved_sigmask(); +} + static inline sigset_t *sigmask_to_save(void) { sigset_t *res = &current->blocked; diff --git a/include/linux/signal.h b/include/linux/signal.h index 78c2bb3..b5d99482 100644 --- a/include/linux/signal.h +++ b/include/linux/signal.h @@ -273,10 +273,6 @@ extern int group_send_sig_info(int sig, struct kernel_siginfo *info, struct task_struct *p, enum pid_type type); extern int __group_send_sig_info(int, struct kernel_siginfo *, struct task_struct *); extern int sigprocmask(int, sigset_t *, sigset_t *); -extern int set_user_sigmask(const sigset_t __user *usigmask, sigset_t *set, - sigset_t *oldset, size_t sigsetsize); -extern void restore_user_sigmask(const void __user *usigmask, - sigset_t *sigsaved, bool interrupted); extern void set_current_blocked(sigset_t *); extern void __set_current_blocked(const sigset_t *); extern int show_unhandled_signals; diff --git a/kernel/signal.c b/kernel/signal.c index aa6a6f1..51772da 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -2863,80 +2863,49 @@ EXPORT_SYMBOL(sigprocmask); * * This is useful for syscalls such as ppoll, pselect, io_pgetevents and * epoll_pwait where a new sigmask is passed from userland for the syscalls. + * + * Note that it does set_restore_sigmask() in advance, so it must be always + * paired with restore_saved_sigmask_unless() before return from syscall. */ -int set_user_sigmask(const sigset_t __user *usigmask, sigset_t *set, - sigset_t *oldset, size_t sigsetsize) +int set_user_sigmask(const sigset_t __user *umask, size_t sigsetsize) { - if (!usigmask) - return 0; + sigset_t *kmask;

+ if (!umask) + return 0; if (sigsetsize != sizeof(sigset_t)) return -EINVAL; - if (copy_from_user(set, usigmask, sizeof(sigset_t))) + if (copy_from_user(kmask, umask, sizeof(sigset_t))) return -EFAULT;

- *oldset = current->blocked; - set_current_blocked(set); + set_restore_sigmask(); + current->saved_sigmask = current->blocked; + set_current_blocked(kmask);

return 0; } -EXPORT_SYMBOL(set_user_sigmask);

#ifdef CONFIG_COMPAT -int set_compat_user_sigmask(const compat_sigset_t __user *usigmask, - sigset_t *set, sigset_t *oldset, +int set_compat_user_sigmask(const compat_sigset_t __user *umask, size_t sigsetsize) { - if (!usigmask) - return 0; + sigset_t *kmask;

+ if (!umask) + return 0; if (sigsetsize != sizeof(compat_sigset_t)) return -EINVAL; - if (get_compat_sigset(set, usigmask)) + if (get_compat_sigset(kmask, umask)) return -EFAULT;

- *oldset = current->blocked; - set_current_blocked(set); + set_restore_sigmask(); + current->saved_sigmask = current->blocked; + set_current_blocked(kmask);

return 0; } -EXPORT_SYMBOL(set_compat_user_sigmask); #endif

-/* - * restore_user_sigmask: - * usigmask: sigmask passed in from userland. - * sigsaved: saved sigmask when the syscall started and changed the sigmask to - * usigmask. - * - * This is useful for syscalls such as ppoll, pselect, io_pgetevents and - * epoll_pwait where a new sigmask is passed in from userland for the syscalls. - */ -void restore_user_sigmask(const void __user *usigmask, sigset_t *sigsaved, - bool interrupted) -{ - - if (!usigmask) - return; - /* - * When signals are pending, do not restore them here. - * Restoring sigmask here can lead to delivering signals that the above - * syscalls are intended to block because of the sigmask passed in. - */ - if (interrupted) { - current->saved_sigmask = *sigsaved; - set_restore_sigmask(); - return; - } - - /* - * This is needed because the fast syscall return path does not restore - * saved_sigmask when signals are not pending. - */ - set_current_blocked(sigsaved); -} -EXPORT_SYMBOL(restore_user_sigmask); - /** * sys_rt_sigprocmask - change the list of currently blocked signals * @how: whether to add, remove, or set signals

task->saved_sigmask and ->restore_sigmask are only used in the ret-from- syscall paths. This means that set_user_sigmask() can save ->blocked in ->saved_sigmask and do set_restore_sigmask() to indicate that ->blocked was modified.

This way the callers do not need 2 sigset_t's passed to set/restore and restore_user_sigmask() renamed to restore_saved_sigmask_unless() turns into the trivial helper which just calls restore_saved_sigmask().

Signed-off-by: Oleg Nesterov oleg@redhat.com --- fs/aio.c | 20 +++++-------- fs/eventpoll.c | 12 +++----- fs/io_uring.c | 11 ++----- fs/select.c | 34 ++++++++-------------- include/linux/compat.h | 3 +- include/linux/sched/signal.h | 12 ++++++-- include/linux/signal.h | 4 --- kernel/signal.c | 69 ++++++++++++-------------------------------- 8 files changed, 57 insertions(+), 108 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c index c1e581d..8200f97 100644 --- a/fs/aio.c +++ b/fs/aio.c @@ -2093,7 +2093,6 @@ SYSCALL_DEFINE6(io_pgetevents, const struct __aio_sigset __user *, usig) { struct __aio_sigset ksig = { NULL, }; - sigset_t ksigmask, sigsaved; struct timespec64 ts; bool interrupted; int ret; @@ -2104,14 +2103,14 @@ SYSCALL_DEFINE6(io_pgetevents, if (usig && copy_from_user(&ksig, usig, sizeof(ksig))) return -EFAULT;

- ret = set_user_sigmask(ksig.sigmask, &ksigmask, &sigsaved, ksig.sigsetsize); + ret = set_user_sigmask(ksig.sigmask, ksig.sigsetsize); if (ret) return ret;

ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);

interrupted = signal_pending(current); - restore_user_sigmask(ksig.sigmask, &sigsaved, interrupted); + restore_saved_sigmask_unless(interrupted); if (interrupted && !ret) ret = -ERESTARTNOHAND;

@@ -2129,7 +2128,6 @@ SYSCALL_DEFINE6(io_pgetevents_time32, const struct __aio_sigset __user *, usig) { struct __aio_sigset ksig = { NULL, }; - sigset_t ksigmask, sigsaved; struct timespec64 ts; bool interrupted; int ret; @@ -2141,14 +2139,14 @@ SYSCALL_DEFINE6(io_pgetevents_time32, return -EFAULT;

- ret = set_user_sigmask(ksig.sigmask, &ksigmask, &sigsaved, ksig.sigsetsize); + ret = set_user_sigmask(ksig.sigmask, ksig.sigsetsize); if (ret) return ret;

ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);

interrupted = signal_pending(current); - restore_user_sigmask(ksig.sigmask, &sigsaved, interrupted); + restore_saved_sigmask_unless(interrupted); if (interrupted && !ret) ret = -ERESTARTNOHAND;

@@ -2197,7 +2195,6 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents, const struct __compat_aio_sigset __user *, usig) { struct __compat_aio_sigset ksig = { NULL, }; - sigset_t ksigmask, sigsaved; struct timespec64 t; bool interrupted; int ret; @@ -2208,14 +2205,14 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents, if (usig && copy_from_user(&ksig, usig, sizeof(ksig))) return -EFAULT;

- ret = set_compat_user_sigmask(ksig.sigmask, &ksigmask, &sigsaved, ksig.sigsetsize); + ret = set_compat_user_sigmask(ksig.sigmask, ksig.sigsetsize); if (ret) return ret;

ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);

interrupted = signal_pending(current); - restore_user_sigmask(ksig.sigmask, &sigsaved, interrupted); + restore_saved_sigmask_unless(interrupted); if (interrupted && !ret) ret = -ERESTARTNOHAND;

@@ -2233,7 +2230,6 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents_time64, const struct __compat_aio_sigset __user *, usig) { struct __compat_aio_sigset ksig = { NULL, }; - sigset_t ksigmask, sigsaved; struct timespec64 t; bool interrupted; int ret; @@ -2244,14 +2240,14 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents_time64, if (usig && copy_from_user(&ksig, usig, sizeof(ksig))) return -EFAULT;

- ret = set_compat_user_sigmask(ksig.sigmask, &ksigmask, &sigsaved, ksig.sigsetsize); + ret = set_compat_user_sigmask(ksig.sigmask, ksig.sigsetsize); if (ret) return ret;

ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);

interrupted = signal_pending(current); - restore_user_sigmask(ksig.sigmask, &sigsaved, interrupted); + restore_saved_sigmask_unless(interrupted); if (interrupted && !ret) ret = -ERESTARTNOHAND;

diff --git a/fs/eventpoll.c b/fs/eventpoll.c index 4c74c76..0f9c073 100644 --- a/fs/eventpoll.c +++ b/fs/eventpoll.c @@ -2313,19 +2313,17 @@ SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct epoll_event __user *, events, size_t, sigsetsize) { int error; - sigset_t ksigmask, sigsaved;

/* * If the caller wants a certain signal mask to be set during the wait, * we apply it here. */ - error = set_user_sigmask(sigmask, &ksigmask, &sigsaved, sigsetsize); + error = set_user_sigmask(sigmask, sigsetsize); if (error) return error;

error = do_epoll_wait(epfd, events, maxevents, timeout); - - restore_user_sigmask(sigmask, &sigsaved, error == -EINTR); + restore_saved_sigmask_unless(error == -EINTR);

return error; } @@ -2338,19 +2336,17 @@ COMPAT_SYSCALL_DEFINE6(epoll_pwait, int, epfd, compat_size_t, sigsetsize) { long err; - sigset_t ksigmask, sigsaved;

/* * If the caller wants a certain signal mask to be set during the wait, * we apply it here. */ - err = set_compat_user_sigmask(sigmask, &ksigmask, &sigsaved, sigsetsize); + err = set_compat_user_sigmask(sigmask, sigsetsize); if (err) return err;

err = do_epoll_wait(epfd, events, maxevents, timeout); - - restore_user_sigmask(sigmask, &sigsaved, err == -EINTR); + restore_saved_sigmask_unless(err == -EINTR);

return err; } diff --git a/fs/io_uring.c b/fs/io_uring.c index 1147c5d..ad7b9bc 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -2180,7 +2180,6 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events, const sigset_t __user *sig, size_t sigsz) { struct io_cq_ring *ring = ctx->cq_ring; - sigset_t ksigmask, sigsaved; int ret;

if (io_cqring_events(ring) >= min_events) @@ -2190,21 +2189,17 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events, #ifdef CONFIG_COMPAT if (in_compat_syscall()) ret = set_compat_user_sigmask((const compat_sigset_t __user *)sig, - &ksigmask, &sigsaved, sigsz); + sigsz); else #endif - ret = set_user_sigmask(sig, &ksigmask, - &sigsaved, sigsz); + ret = set_user_sigmask(sig, sigsz);

if (ret) return ret; }

ret = wait_event_interruptible(ctx->wait, io_cqring_events(ring) >= min_events); - - if (sig) - restore_user_sigmask(sig, &sigsaved, ret == -ERESTARTSYS); - + restore_saved_sigmask_unless(ret == -ERESTARTSYS); if (ret == -ERESTARTSYS) ret = -EINTR;

diff --git a/fs/select.c b/fs/select.c index a4d8f6e..1fc1b24 100644 --- a/fs/select.c +++ b/fs/select.c @@ -730,7 +730,6 @@ static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp, const sigset_t __user *sigmask, size_t sigsetsize, enum poll_time_type type) { - sigset_t ksigmask, sigsaved; struct timespec64 ts, end_time, *to = NULL; int ret;

@@ -753,12 +752,12 @@ static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp, return -EINVAL; }

- ret = set_user_sigmask(sigmask, &ksigmask, &sigsaved, sigsetsize); + ret = set_user_sigmask(sigmask, sigsetsize); if (ret) return ret;

ret = core_sys_select(n, inp, outp, exp, to); - restore_user_sigmask(sigmask, &sigsaved, ret == -ERESTARTNOHAND); + restore_saved_sigmask_unless(ret == -ERESTARTNOHAND); ret = poll_select_copy_remaining(&end_time, tsp, type, ret);

return ret; @@ -1086,7 +1085,6 @@ SYSCALL_DEFINE5(ppoll, struct pollfd __user *, ufds, unsigned int, nfds, struct __kernel_timespec __user *, tsp, const sigset_t __user *, sigmask, size_t, sigsetsize) { - sigset_t ksigmask, sigsaved; struct timespec64 ts, end_time, *to = NULL; int ret;

@@ -1099,17 +1097,16 @@ SYSCALL_DEFINE5(ppoll, struct pollfd __user *, ufds, unsigned int, nfds, return -EINVAL; }

- ret = set_user_sigmask(sigmask, &ksigmask, &sigsaved, sigsetsize); + ret = set_user_sigmask(sigmask, sigsetsize); if (ret) return ret;

ret = do_sys_poll(ufds, nfds, to);

- restore_user_sigmask(sigmask, &sigsaved, ret == -EINTR); + restore_saved_sigmask_unless(ret == -EINTR); /* We can restart this syscall, usually */ if (ret == -EINTR) ret = -ERESTARTNOHAND; - ret = poll_select_copy_remaining(&end_time, tsp, PT_TIMESPEC, ret);

return ret; @@ -1121,7 +1118,6 @@ SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds, unsigned int, nfds, struct old_timespec32 __user *, tsp, const sigset_t __user *, sigmask, size_t, sigsetsize) { - sigset_t ksigmask, sigsaved; struct timespec64 ts, end_time, *to = NULL; int ret;

@@ -1134,17 +1130,16 @@ SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds, unsigned int, nfds, return -EINVAL; }

- ret = set_user_sigmask(sigmask, &ksigmask, &sigsaved, sigsetsize); + ret = set_user_sigmask(sigmask, sigsetsize); if (ret) return ret;

ret = do_sys_poll(ufds, nfds, to);

- restore_user_sigmask(sigmask, &sigsaved, ret == -EINTR); + restore_saved_sigmask_unless(ret == -EINTR); /* We can restart this syscall, usually */ if (ret == -EINTR) ret = -ERESTARTNOHAND; - ret = poll_select_copy_remaining(&end_time, tsp, PT_OLD_TIMESPEC, ret);

return ret; @@ -1319,7 +1314,6 @@ static long do_compat_pselect(int n, compat_ulong_t __user *inp, void __user *tsp, compat_sigset_t __user *sigmask, compat_size_t sigsetsize, enum poll_time_type type) { - sigset_t ksigmask, sigsaved; struct timespec64 ts, end_time, *to = NULL; int ret;

@@ -1342,12 +1336,12 @@ static long do_compat_pselect(int n, compat_ulong_t __user *inp, return -EINVAL; }

- ret = set_compat_user_sigmask(sigmask, &ksigmask, &sigsaved, sigsetsize); + ret = set_compat_user_sigmask(sigmask, sigsetsize); if (ret) return ret;

ret = compat_core_sys_select(n, inp, outp, exp, to); - restore_user_sigmask(sigmask, &sigsaved, ret == -ERESTARTNOHAND); + restore_saved_sigmask_unless(ret == -ERESTARTNOHAND); ret = poll_select_copy_remaining(&end_time, tsp, type, ret);

return ret; @@ -1402,7 +1396,6 @@ COMPAT_SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds, unsigned int, nfds, struct old_timespec32 __user *, tsp, const compat_sigset_t __user *, sigmask, compat_size_t, sigsetsize) { - sigset_t ksigmask, sigsaved; struct timespec64 ts, end_time, *to = NULL; int ret;

@@ -1415,17 +1408,16 @@ COMPAT_SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds, return -EINVAL; }

- ret = set_compat_user_sigmask(sigmask, &ksigmask, &sigsaved, sigsetsize); + ret = set_compat_user_sigmask(sigmask, sigsetsize); if (ret) return ret;

ret = do_sys_poll(ufds, nfds, to);

- restore_user_sigmask(sigmask, &sigsaved, ret == -EINTR); + restore_saved_sigmask_unless(ret == -EINTR); /* We can restart this syscall, usually */ if (ret == -EINTR) ret = -ERESTARTNOHAND; - ret = poll_select_copy_remaining(&end_time, tsp, PT_OLD_TIMESPEC, ret);

return ret; @@ -1437,7 +1429,6 @@ COMPAT_SYSCALL_DEFINE5(ppoll_time64, struct pollfd __user *, ufds, unsigned int, nfds, struct __kernel_timespec __user *, tsp, const compat_sigset_t __user *, sigmask, compat_size_t, sigsetsize) { - sigset_t ksigmask, sigsaved; struct timespec64 ts, end_time, *to = NULL; int ret;

@@ -1450,17 +1441,16 @@ COMPAT_SYSCALL_DEFINE5(ppoll_time64, struct pollfd __user *, ufds, return -EINVAL; }

- ret = set_compat_user_sigmask(sigmask, &ksigmask, &sigsaved, sigsetsize); + ret = set_compat_user_sigmask(sigmask, sigsetsize); if (ret) return ret;

ret = do_sys_poll(ufds, nfds, to);

- restore_user_sigmask(sigmask, &sigsaved, ret == -EINTR); + restore_saved_sigmask_unless(ret == -EINTR); /* We can restart this syscall, usually */ if (ret == -EINTR) ret = -ERESTARTNOHAND; - ret = poll_select_copy_remaining(&end_time, tsp, PT_TIMESPEC, ret);

return ret; diff --git a/include/linux/compat.h b/include/linux/compat.h index ebddcb6..16dafd9 100644 --- a/include/linux/compat.h +++ b/include/linux/compat.h @@ -138,8 +138,7 @@ typedef struct { compat_sigset_word sig[_COMPAT_NSIG_WORDS]; } compat_sigset_t;

-int set_compat_user_sigmask(const compat_sigset_t __user *usigmask, - sigset_t *set, sigset_t *oldset, +int set_compat_user_sigmask(const compat_sigset_t __user *umask, size_t sigsetsize);

struct compat_sigaction { diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h index 38a0f07..4d9c71d 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -417,7 +417,6 @@ void task_join_group_stop(struct task_struct *task); static inline void set_restore_sigmask(void) { set_thread_flag(TIF_RESTORE_SIGMASK); - WARN_ON(!test_thread_flag(TIF_SIGPENDING)); }

static inline void clear_tsk_restore_sigmask(struct task_struct *task) @@ -448,7 +447,6 @@ static inline bool test_and_clear_restore_sigmask(void) static inline void set_restore_sigmask(void) { current->restore_sigmask = true; - WARN_ON(!test_thread_flag(TIF_SIGPENDING)); } static inline void clear_tsk_restore_sigmask(struct task_struct *task) { @@ -481,6 +479,16 @@ static inline void restore_saved_sigmask(void) __set_current_blocked(&current->saved_sigmask); }

+extern int set_user_sigmask(const sigset_t __user *umask, size_t sigsetsize); + +static inline void restore_saved_sigmask_unless(bool interrupted) +{ + if (interrupted) + WARN_ON(!test_thread_flag(TIF_SIGPENDING)); + else + restore_saved_sigmask(); +} + static inline sigset_t *sigmask_to_save(void) { sigset_t *res = &current->blocked; diff --git a/include/linux/signal.h b/include/linux/signal.h index 78c2bb3..b5d99482 100644 --- a/include/linux/signal.h +++ b/include/linux/signal.h @@ -273,10 +273,6 @@ extern int group_send_sig_info(int sig, struct kernel_siginfo *info, struct task_struct *p, enum pid_type type); extern int __group_send_sig_info(int, struct kernel_siginfo *, struct task_struct *); extern int sigprocmask(int, sigset_t *, sigset_t *); -extern int set_user_sigmask(const sigset_t __user *usigmask, sigset_t *set, - sigset_t *oldset, size_t sigsetsize); -extern void restore_user_sigmask(const void __user *usigmask, - sigset_t *sigsaved, bool interrupted); extern void set_current_blocked(sigset_t *); extern void __set_current_blocked(const sigset_t *); extern int show_unhandled_signals; diff --git a/kernel/signal.c b/kernel/signal.c index aa6a6f1..1a1013e1 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -2863,80 +2863,49 @@ EXPORT_SYMBOL(sigprocmask); * * This is useful for syscalls such as ppoll, pselect, io_pgetevents and * epoll_pwait where a new sigmask is passed from userland for the syscalls. + * + * Note that it does set_restore_sigmask() in advance, so it must be always + * paired with restore_saved_sigmask_unless() before return from syscall. */ -int set_user_sigmask(const sigset_t __user *usigmask, sigset_t *set, - sigset_t *oldset, size_t sigsetsize) +int set_user_sigmask(const sigset_t __user *umask, size_t sigsetsize) { - if (!usigmask) - return 0; + sigset_t kmask;

+ if (!umask) + return 0; if (sigsetsize != sizeof(sigset_t)) return -EINVAL; - if (copy_from_user(set, usigmask, sizeof(sigset_t))) + if (copy_from_user(&kmask, umask, sizeof(sigset_t))) return -EFAULT;

- *oldset = current->blocked; - set_current_blocked(set); + set_restore_sigmask(); + current->saved_sigmask = current->blocked; + set_current_blocked(&kmask);

return 0; } -EXPORT_SYMBOL(set_user_sigmask);

#ifdef CONFIG_COMPAT -int set_compat_user_sigmask(const compat_sigset_t __user *usigmask, - sigset_t *set, sigset_t *oldset, +int set_compat_user_sigmask(const compat_sigset_t __user *umask, size_t sigsetsize) { - if (!usigmask) - return 0; + sigset_t kmask;

+ if (!umask) + return 0; if (sigsetsize != sizeof(compat_sigset_t)) return -EINVAL; - if (get_compat_sigset(set, usigmask)) + if (get_compat_sigset(&kmask, umask)) return -EFAULT;

- *oldset = current->blocked; - set_current_blocked(set); + set_restore_sigmask(); + current->saved_sigmask = current->blocked; + set_current_blocked(&kmask);

return 0; } -EXPORT_SYMBOL(set_compat_user_sigmask); #endif

-/* - * restore_user_sigmask: - * usigmask: sigmask passed in from userland. - * sigsaved: saved sigmask when the syscall started and changed the sigmask to - * usigmask. - * - * This is useful for syscalls such as ppoll, pselect, io_pgetevents and - * epoll_pwait where a new sigmask is passed in from userland for the syscalls. - */ -void restore_user_sigmask(const void __user *usigmask, sigset_t *sigsaved, - bool interrupted) -{ - - if (!usigmask) - return; - /* - * When signals are pending, do not restore them here. - * Restoring sigmask here can lead to delivering signals that the above - * syscalls are intended to block because of the sigmask passed in. - */ - if (interrupted) { - current->saved_sigmask = *sigsaved; - set_restore_sigmask(); - return; - } - - /* - * This is needed because the fast syscall return path does not restore - * saved_sigmask when signals are not pending. - */ - set_current_blocked(sigsaved); -} -EXPORT_SYMBOL(restore_user_sigmask); - /** * sys_rt_sigprocmask - change the list of currently blocked signals * @how: whether to add, remove, or set signals

do_poll() returns -EINTR if interrupted and after that all its callers have to translate it into -ERESTARTNOHAND. Change do_poll() to return -ERESTARTNOHAND and update (simplify) the callers.

Note that this also unifies all users of restore_saved_sigmask_unless(), see the next patch.

Signed-off-by: Oleg Nesterov oleg@redhat.com --- fs/select.c | 30 +++++++----------------------- 1 file changed, 7 insertions(+), 23 deletions(-)

diff --git a/fs/select.c b/fs/select.c index 1fc1b24..57712c3 100644 --- a/fs/select.c +++ b/fs/select.c @@ -925,7 +925,7 @@ static int do_poll(struct poll_list *list, struct poll_wqueues *wait, if (!count) { count = wait->error; if (signal_pending(current)) - count = -EINTR; + count = -ERESTARTNOHAND; } if (count || timed_out) break; @@ -1040,7 +1040,7 @@ static long do_restart_poll(struct restart_block *restart_block)

ret = do_sys_poll(ufds, nfds, to);

- if (ret == -EINTR) { + if (ret == -ERESTARTNOHAND) { restart_block->fn = do_restart_poll; ret = -ERESTART_RESTARTBLOCK; } @@ -1061,7 +1061,7 @@ SYSCALL_DEFINE3(poll, struct pollfd __user *, ufds, unsigned int, nfds,

ret = do_sys_poll(ufds, nfds, to);

- if (ret == -EINTR) { + if (ret == -ERESTARTNOHAND) { struct restart_block *restart_block;

restart_block = &current->restart_block; @@ -1102,11 +1102,7 @@ SYSCALL_DEFINE5(ppoll, struct pollfd __user *, ufds, unsigned int, nfds, return ret;

ret = do_sys_poll(ufds, nfds, to); - - restore_saved_sigmask_unless(ret == -EINTR); - /* We can restart this syscall, usually */ - if (ret == -EINTR) - ret = -ERESTARTNOHAND; + restore_saved_sigmask_unless(ret == -ERESTARTNOHAND); ret = poll_select_copy_remaining(&end_time, tsp, PT_TIMESPEC, ret);

return ret; @@ -1135,11 +1131,7 @@ SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds, unsigned int, nfds, return ret;

ret = do_sys_poll(ufds, nfds, to); - - restore_saved_sigmask_unless(ret == -EINTR); - /* We can restart this syscall, usually */ - if (ret == -EINTR) - ret = -ERESTARTNOHAND; + restore_saved_sigmask_unless(ret == -ERESTARTNOHAND); ret = poll_select_copy_remaining(&end_time, tsp, PT_OLD_TIMESPEC, ret);

return ret; @@ -1413,11 +1405,7 @@ COMPAT_SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds, return ret;

ret = do_sys_poll(ufds, nfds, to); - - restore_saved_sigmask_unless(ret == -EINTR); - /* We can restart this syscall, usually */ - if (ret == -EINTR) - ret = -ERESTARTNOHAND; + restore_saved_sigmask_unless(ret == -ERESTARTNOHAND); ret = poll_select_copy_remaining(&end_time, tsp, PT_OLD_TIMESPEC, ret);

return ret; @@ -1446,11 +1434,7 @@ COMPAT_SYSCALL_DEFINE5(ppoll_time64, struct pollfd __user *, ufds, return ret;

ret = do_sys_poll(ufds, nfds, to); - - restore_saved_sigmask_unless(ret == -EINTR); - /* We can restart this syscall, usually */ - if (ret == -EINTR) - ret = -ERESTARTNOHAND; + restore_saved_sigmask_unless(ret == -ERESTARTNOHAND); ret = poll_select_copy_remaining(&end_time, tsp, PT_TIMESPEC, ret);

return ret;

Now that restore_saved_sigmask_unless() is always called with the same argument right before poll_select_copy_remaining() we can move it into poll_select_copy_remaining() and make it the only caller of restore() in fs/select.c.

The patch also renames poll_select_copy_remaining(), poll_select_finish() looks better after this change.

kern_select() doesn't use set_user_sigmask(), so in this case poll_select_finish() does restore_saved_sigmask_unless() "for no reason". But this won't hurt, and WARN_ON(!TIF_SIGPENDING) is still valid.

Signed-off-by: Oleg Nesterov oleg@redhat.com --- fs/select.c | 46 +++++++++++++--------------------------------- 1 file changed, 13 insertions(+), 33 deletions(-)

diff --git a/fs/select.c b/fs/select.c index 57712c3..51ceec2 100644 --- a/fs/select.c +++ b/fs/select.c @@ -294,12 +294,14 @@ enum poll_time_type { PT_OLD_TIMESPEC = 3, };

-static int poll_select_copy_remaining(struct timespec64 *end_time, - void __user *p, - enum poll_time_type pt_type, int ret) +static int poll_select_finish(struct timespec64 *end_time, + void __user *p, + enum poll_time_type pt_type, int ret) { struct timespec64 rts;

+ restore_saved_sigmask_unless(ret == -ERESTARTNOHAND); + if (!p) return ret;

@@ -714,9 +716,7 @@ static int kern_select(int n, fd_set __user *inp, fd_set __user *outp, }

ret = core_sys_select(n, inp, outp, exp, to); - ret = poll_select_copy_remaining(&end_time, tvp, PT_TIMEVAL, ret); - - return ret; + return poll_select_finish(&end_time, tvp, PT_TIMEVAL, ret); }

SYSCALL_DEFINE5(select, int, n, fd_set __user *, inp, fd_set __user *, outp, @@ -757,10 +757,7 @@ static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp, return ret;

ret = core_sys_select(n, inp, outp, exp, to); - restore_saved_sigmask_unless(ret == -ERESTARTNOHAND); - ret = poll_select_copy_remaining(&end_time, tsp, type, ret); - - return ret; + return poll_select_finish(&end_time, tsp, type, ret); }

/* @@ -1102,10 +1099,7 @@ SYSCALL_DEFINE5(ppoll, struct pollfd __user *, ufds, unsigned int, nfds, return ret;

ret = do_sys_poll(ufds, nfds, to); - restore_saved_sigmask_unless(ret == -ERESTARTNOHAND); - ret = poll_select_copy_remaining(&end_time, tsp, PT_TIMESPEC, ret); - - return ret; + return poll_select_finish(&end_time, tsp, PT_TIMESPEC, ret); }

#if defined(CONFIG_COMPAT_32BIT_TIME) && !defined(CONFIG_64BIT) @@ -1131,10 +1125,7 @@ SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds, unsigned int, nfds, return ret;

ret = do_sys_poll(ufds, nfds, to); - restore_saved_sigmask_unless(ret == -ERESTARTNOHAND); - ret = poll_select_copy_remaining(&end_time, tsp, PT_OLD_TIMESPEC, ret); - - return ret; + return poll_select_finish(&end_time, tsp, PT_OLD_TIMESPEC, ret); } #endif

@@ -1271,9 +1262,7 @@ static int do_compat_select(int n, compat_ulong_t __user *inp, }

ret = compat_core_sys_select(n, inp, outp, exp, to); - ret = poll_select_copy_remaining(&end_time, tvp, PT_OLD_TIMEVAL, ret); - - return ret; + return poll_select_finish(&end_time, tvp, PT_OLD_TIMEVAL, ret); }

COMPAT_SYSCALL_DEFINE5(select, int, n, compat_ulong_t __user *, inp, @@ -1333,10 +1322,7 @@ static long do_compat_pselect(int n, compat_ulong_t __user *inp, return ret;

ret = compat_core_sys_select(n, inp, outp, exp, to); - restore_saved_sigmask_unless(ret == -ERESTARTNOHAND); - ret = poll_select_copy_remaining(&end_time, tsp, type, ret); - - return ret; + return poll_select_finish(&end_time, tsp, type, ret); }

COMPAT_SYSCALL_DEFINE6(pselect6_time64, int, n, compat_ulong_t __user *, inp, @@ -1405,10 +1391,7 @@ COMPAT_SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds, return ret;

ret = do_sys_poll(ufds, nfds, to); - restore_saved_sigmask_unless(ret == -ERESTARTNOHAND); - ret = poll_select_copy_remaining(&end_time, tsp, PT_OLD_TIMESPEC, ret); - - return ret; + return poll_select_finish(&end_time, tsp, PT_OLD_TIMESPEC, ret); } #endif

@@ -1434,10 +1417,7 @@ COMPAT_SYSCALL_DEFINE5(ppoll_time64, struct pollfd __user *, ufds, return ret;

ret = do_sys_poll(ufds, nfds, to); - restore_saved_sigmask_unless(ret == -ERESTARTNOHAND); - ret = poll_select_copy_remaining(&end_time, tsp, PT_TIMESPEC, ret); - - return ret; + return poll_select_finish(&end_time, tsp, PT_TIMESPEC, ret); }

#endif