Re: [PATCH 1/1] signal: Adjust error codes according to restore_user_sigmask()

From: Eric Wong
Date: Tue May 21 2019 - 05:28:24 EST

Next message: Tengfei Fan: "[PATCH] arm64: break while loop if task had been rescheduled"
Previous message: Niklas Cassel: "Re: [PATCH] arm64: dts: qcom: msm8996: Stop using legacy clock names"
Next in thread: Andrew Morton: "Re: [PATCH 1/1] signal: Adjust error codes according to restore_user_sigmask()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

It's been 2 weeks and this fix hasn't appeared in mmots / mmotm.
I also noticed it's missing Cc: for stable@ (below)

Deepa Dinamani <deepa.kernel@xxxxxxxxx> wrote:
> For all the syscalls that receive a sigmask from the userland,
> the user sigmask is to be in effect through the syscall execution.
> At the end of syscall, sigmask of the current process is restored
> to what it was before the switch over to user sigmask.
> But, for this to be true in practice, the sigmask should be restored
> only at the the point we change the saved_sigmask. Anything before
> that loses signals. And, anything after is just pointless as the
> signal is already lost by restoring the sigmask.
>
> The inherent issue was detected because of a regression caused by
> 854a6ed56839a.
> The patch moved the signal_pending() check closer to restoring of the
> user sigmask. But, it failed to update the error code accordingly.
>
> Detailed issue discussion permalink:
> https://lore.kernel.org/linux-fsdevel/20190427093319.sgicqik2oqkez3wk@dcvr/
>
> Note that the 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.
>
> The sys_io_uring_enter() seems to be returning success when there is
> a signal and the queue is not empty. This seems to be a bug. I will
> follow up with a separate patch for that.
>
> Reported-by: Eric Wong <e@xxxxxxxxx>
> Fixes: 854a6ed56839a40f6b5d02a2962f48841482eec4 ("signal: Add restore_user_sigmask()")
> Signed-off-by: Deepa Dinamani <deepa.kernel@xxxxxxxxx>
> Reviewed-by: Davidlohr Bueso <dbueso@xxxxxxx>

Cc: <stable@xxxxxxxxxxxxxxx> # 5.0.x
Cc: <stable@xxxxxxxxxxxxxxx> # 5.1.x

> ---
>
> fs/aio.c | 24 ++++++++++++------------
> fs/eventpoll.c | 14 ++++++++++----
> fs/io_uring.c | 9 ++++++---
> fs/select.c | 37 +++++++++++++++++++++----------------
> include/linux/signal.h | 2 +-
> kernel/signal.c | 13 ++++++++++---
> 6 files changed, 60 insertions(+), 39 deletions(-)
>
> diff --git a/fs/aio.c b/fs/aio.c
> index 3490d1fa0e16..ebd2b1980161 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -2095,7 +2095,7 @@ SYSCALL_DEFINE6(io_pgetevents,
> struct __aio_sigset ksig = { NULL, };
> sigset_t ksigmask, sigsaved;
> struct timespec64 ts;
> - int ret;
> + int ret, signal_detected;
>
> if (timeout && unlikely(get_timespec64(&ts, timeout)))
> return -EFAULT;
> @@ -2108,8 +2108,8 @@ 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)
> + signal_detected = restore_user_sigmask(ksig.sigmask, &sigsaved);
> + if (signal_detected && !ret)
> ret = -ERESTARTNOHAND;
>
> return ret;
> @@ -2128,7 +2128,7 @@ SYSCALL_DEFINE6(io_pgetevents_time32,
> struct __aio_sigset ksig = { NULL, };
> sigset_t ksigmask, sigsaved;
> struct timespec64 ts;
> - int ret;
> + int ret, signal_detected;
>
> if (timeout && unlikely(get_old_timespec32(&ts, timeout)))
> return -EFAULT;
> @@ -2142,8 +2142,8 @@ 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)
> + signal_detected = restore_user_sigmask(ksig.sigmask, &sigsaved);
> + if (signal_detected && !ret)
> ret = -ERESTARTNOHAND;
>
> return ret;
> @@ -2193,7 +2193,7 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents,
> struct __compat_aio_sigset ksig = { NULL, };
> sigset_t ksigmask, sigsaved;
> struct timespec64 t;
> - int ret;
> + int ret, signal_detected;
>
> if (timeout && get_old_timespec32(&t, timeout))
> return -EFAULT;
> @@ -2206,8 +2206,8 @@ 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)
> + signal_detected = restore_user_sigmask(ksig.sigmask, &sigsaved);
> + if (signal_detected && !ret)
> ret = -ERESTARTNOHAND;
>
> return ret;
> @@ -2226,7 +2226,7 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents_time64,
> struct __compat_aio_sigset ksig = { NULL, };
> sigset_t ksigmask, sigsaved;
> struct timespec64 t;
> - int ret;
> + int ret, signal_detected;
>
> if (timeout && get_timespec64(&t, timeout))
> return -EFAULT;
> @@ -2239,8 +2239,8 @@ 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)
> + signal_detected = restore_user_sigmask(ksig.sigmask, &sigsaved);
> + if (signal_detected && !ret)
> ret = -ERESTARTNOHAND;
>
> return ret;
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index 4a0e98d87fcc..fe5a0724b417 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -2317,7 +2317,7 @@ SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct epoll_event __user *, events,
> int, maxevents, int, timeout, const sigset_t __user *, sigmask,
> size_t, sigsetsize)
> {
> - int error;
> + int error, signal_detected;
> sigset_t ksigmask, sigsaved;
>
> /*
> @@ -2330,7 +2330,10 @@ 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);
> +
> + if (signal_detected && !error)
> + error = -EINTR;
>
> return error;
> }
> @@ -2342,7 +2345,7 @@ COMPAT_SYSCALL_DEFINE6(epoll_pwait, int, epfd,
> const compat_sigset_t __user *, sigmask,
> compat_size_t, sigsetsize)
> {
> - long err;
> + long err, signal_detected;
> sigset_t ksigmask, sigsaved;
>
> /*
> @@ -2355,7 +2358,10 @@ COMPAT_SYSCALL_DEFINE6(epoll_pwait, int, epfd,
>
> err = do_epoll_wait(epfd, events, maxevents, timeout);
>
> - restore_user_sigmask(sigmask, &sigsaved);
> + signal_detected = restore_user_sigmask(sigmask, &sigsaved);
> +
> + if (signal_detected && !err)
> + err = -EINTR;
>
> return err;
> }
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 452e35357865..8fd4710f371d 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -2195,7 +2195,7 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
> struct io_cq_ring *ring = ctx->cq_ring;
> sigset_t ksigmask, sigsaved;
> DEFINE_WAIT(wait);
> - int ret;
> + int ret, signal_detected;
>
> /* See comment at the top of this file */
> smp_rmb();
> @@ -2234,8 +2234,11 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
>
> finish_wait(&ctx->wait, &wait);
>
> - if (sig)
> - restore_user_sigmask(sig, &sigsaved);
> + if (sig) {
> + signal_detected = restore_user_sigmask(sig, &sigsaved);
> + if (signal_detected && !ret)
> + 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 6cbc9ff56ba0..da9cfea35159 100644
> --- a/fs/select.c
> +++ b/fs/select.c
> @@ -732,7 +732,7 @@ static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp,
> {
> sigset_t ksigmask, sigsaved;
> struct timespec64 ts, end_time, *to = NULL;
> - int ret;
> + int ret, signal_detected;
>
> if (tsp) {
> switch (type) {
> @@ -760,7 +760,9 @@ static long do_pselect(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, tsp, type, ret);
>
> - restore_user_sigmask(sigmask, &sigsaved);
> + signal_detected = restore_user_sigmask(sigmask, &sigsaved);
> + if (signal_detected && !ret)
> + ret = -EINTR;
>
> return ret;
> }
> @@ -1089,7 +1091,7 @@ SYSCALL_DEFINE5(ppoll, struct pollfd __user *, ufds, unsigned int, nfds,
> {
> sigset_t ksigmask, sigsaved;
> struct timespec64 ts, end_time, *to = NULL;
> - int ret;
> + int ret, signal_detected;
>
> if (tsp) {
> if (get_timespec64(&ts, tsp))
> @@ -1106,10 +1108,10 @@ SYSCALL_DEFINE5(ppoll, struct pollfd __user *, ufds, unsigned int, nfds,
>
> ret = do_sys_poll(ufds, nfds, to);
>
> - restore_user_sigmask(sigmask, &sigsaved);
> + signal_detected = restore_user_sigmask(sigmask, &sigsaved);
>
> /* We can restart this syscall, usually */
> - if (ret == -EINTR)
> + if (ret == -EINTR || (signal_detected && !ret))
> ret = -ERESTARTNOHAND;
>
> ret = poll_select_copy_remaining(&end_time, tsp, PT_TIMESPEC, ret);
> @@ -1125,7 +1127,7 @@ SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds, unsigned int, nfds,
> {
> sigset_t ksigmask, sigsaved;
> struct timespec64 ts, end_time, *to = NULL;
> - int ret;
> + int ret, signal_detected;
>
> if (tsp) {
> if (get_old_timespec32(&ts, tsp))
> @@ -1142,10 +1144,10 @@ SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds, unsigned int, nfds,
>
> ret = do_sys_poll(ufds, nfds, to);
>
> - restore_user_sigmask(sigmask, &sigsaved);
> + signal_detected = restore_user_sigmask(sigmask, &sigsaved);
>
> /* We can restart this syscall, usually */
> - if (ret == -EINTR)
> + if (ret == -EINTR || (signal_detected && !ret))
> ret = -ERESTARTNOHAND;
>
> ret = poll_select_copy_remaining(&end_time, tsp, PT_OLD_TIMESPEC, ret);
> @@ -1324,7 +1326,7 @@ static long do_compat_pselect(int n, compat_ulong_t __user *inp,
> {
> sigset_t ksigmask, sigsaved;
> struct timespec64 ts, end_time, *to = NULL;
> - int ret;
> + int ret, signal_detected;
>
> if (tsp) {
> switch (type) {
> @@ -1352,7 +1354,10 @@ static long do_compat_pselect(int n, compat_ulong_t __user *inp,
> ret = compat_core_sys_select(n, inp, outp, exp, to);
> ret = poll_select_copy_remaining(&end_time, tsp, type, ret);
>
> - restore_user_sigmask(sigmask, &sigsaved);
> + signal_detected = restore_user_sigmask(sigmask, &sigsaved);
> +
> + if (signal_detected && !ret)
> + ret = -EINTR;
>
> return ret;
> }
> @@ -1408,7 +1413,7 @@ COMPAT_SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds,
> {
> sigset_t ksigmask, sigsaved;
> struct timespec64 ts, end_time, *to = NULL;
> - int ret;
> + int ret, signal_detected;
>
> if (tsp) {
> if (get_old_timespec32(&ts, tsp))
> @@ -1425,10 +1430,10 @@ COMPAT_SYSCALL_DEFINE5(ppoll_time32, struct pollfd __user *, ufds,
>
> ret = do_sys_poll(ufds, nfds, to);
>
> - restore_user_sigmask(sigmask, &sigsaved);
> + signal_detected = restore_user_sigmask(sigmask, &sigsaved);
>
> /* We can restart this syscall, usually */
> - if (ret == -EINTR)
> + if (ret == -EINTR || (signal_detected && !ret))
> ret = -ERESTARTNOHAND;
>
> ret = poll_select_copy_remaining(&end_time, tsp, PT_OLD_TIMESPEC, ret);
> @@ -1444,7 +1449,7 @@ COMPAT_SYSCALL_DEFINE5(ppoll_time64, struct pollfd __user *, ufds,
> {
> sigset_t ksigmask, sigsaved;
> struct timespec64 ts, end_time, *to = NULL;
> - int ret;
> + int ret, signal_detected;
>
> if (tsp) {
> if (get_timespec64(&ts, tsp))
> @@ -1461,10 +1466,10 @@ COMPAT_SYSCALL_DEFINE5(ppoll_time64, struct pollfd __user *, ufds,
>
> ret = do_sys_poll(ufds, nfds, to);
>
> - restore_user_sigmask(sigmask, &sigsaved);
> + signal_detected = restore_user_sigmask(sigmask, &sigsaved);
>
> /* We can restart this syscall, usually */
> - if (ret == -EINTR)
> + if (ret == -EINTR || (signal_detected && !ret))
> ret = -ERESTARTNOHAND;
>
> ret = poll_select_copy_remaining(&end_time, tsp, PT_TIMESPEC, ret);
> diff --git a/include/linux/signal.h b/include/linux/signal.h
> index 9702016734b1..1d36e8629edf 100644
> --- a/include/linux/signal.h
> +++ b/include/linux/signal.h
> @@ -275,7 +275,7 @@ extern int __group_send_sig_info(int, struct kernel_siginfo *, struct task_struc
> 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,
> +extern int restore_user_sigmask(const void __user *usigmask,
> sigset_t *sigsaved);
> extern void set_current_blocked(sigset_t *);
> extern void __set_current_blocked(const sigset_t *);
> diff --git a/kernel/signal.c b/kernel/signal.c
> index e46d527ff467..ea0321b70315 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2906,15 +2906,21 @@ EXPORT_SYMBOL(set_compat_user_sigmask);
> * usigmask: sigmask passed in from userland.
> * sigsaved: saved sigmask when the syscall started and changed the sigmask to
> * usigmask.
> + * returns 1 in case a pending signal is detected.
> + *
> + * Users of the api need to adjust their return values based on whether the
> + * signal was detected here. If a signal is detected, it is delivered to the
> + * userspace. So without an error like -EINTR, userspace might fail to
> + * adjust the flow of execution.
> *
> * 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)
> +int restore_user_sigmask(const void __user *usigmask, sigset_t *sigsaved)
> {
>
> if (!usigmask)
> - return;
> + return 0;
> /*
> * When signals are pending, do not restore them here.
> * Restoring sigmask here can lead to delivering signals that the above
> @@ -2923,7 +2929,7 @@ void restore_user_sigmask(const void __user *usigmask, sigset_t *sigsaved)
> if (signal_pending(current)) {
> current->saved_sigmask = *sigsaved;
> set_restore_sigmask();
> - return;
> + return 1;
> }
>
> /*
> @@ -2931,6 +2937,7 @@ void restore_user_sigmask(const void __user *usigmask, sigset_t *sigsaved)
> * saved_sigmask when signals are not pending.
> */
> set_current_blocked(sigsaved);
> + return 0;
> }
> EXPORT_SYMBOL(restore_user_sigmask);
>
> --
> 2.17.1

Next message: Tengfei Fan: "[PATCH] arm64: break while loop if task had been rescheduled"
Previous message: Niklas Cassel: "Re: [PATCH] arm64: dts: qcom: msm8996: Stop using legacy clock names"
Next in thread: Andrew Morton: "Re: [PATCH 1/1] signal: Adjust error codes according to restore_user_sigmask()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]