Skip to content

Commit

Permalink
signal: remove the wrong signal_pending() check in restore_user_sigma…
Browse files Browse the repository at this point in the history 
…sk()

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

Commit 854a6ed ("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.

Eric said:

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

Link: http://lkml.kernel.org/r/[email protected]
Fixes: 854a6ed ("signal: Add restore_user_sigmask()")
Signed-off-by: Oleg Nesterov <[email protected]>
Reported-by: Eric Wong <[email protected]>
Tested-by: Eric Wong <[email protected]>
Acked-by: "Eric W. Biederman" <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
Acked-by: Deepa Dinamani <[email protected]>
Cc: Michael Kerrisk <[email protected]>
Cc: Jens Axboe <[email protected]>
Cc: Davidlohr Bueso <[email protected]>
Cc: Jason Baron <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Al Viro <[email protected]>
Cc: David Laight <[email protected]>
Cc: <[email protected]>	[5.0+]
Signed-off-by: Andrew Morton <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>
  • Loading branch information
@oleg-nesterov @torvalds
oleg-nesterov authored and torvalds committed Jun 29, 2019
1 parent 867bfa4 commit 97abc88
Show file tree
Hide file tree
Showing 6 changed files with 36 additions and 28 deletions.
28 changes: 20 additions & 8 deletions fs/aio.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
Expand All @@ -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;
Expand All @@ -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)))
Expand All @@ -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;
Expand Down Expand Up @@ -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))
Expand All @@ -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;
Expand All @@ -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))
Expand All @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions fs/eventpoll.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand Down
7 changes: 4 additions & 3 deletions fs/io_uring.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
18 changes: 6 additions & 12 deletions fs/select.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion include/linux/signal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
5 changes: 3 additions & 2 deletions kernel/signal.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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;
Expand Down

0 comments on commit 97abc88

Please sign in to comment.