signal: Adjust error codes according to restore_user_sigmask()
diff mbox series

Message ID 20190503034205.12121-1-deepa.kernel@gmail.com
State New
Headers show
Series
  • signal: Adjust error codes according to restore_user_sigmask()
Related show

Commit Message

Deepa Dinamani May 3, 2019, 3:42 a.m. UTC
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 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.

Reported-by: Eric Wong <e@80x24.org>
Fixes: 854a6ed56839a40f6b5d02a2962f48841482eec4 ("signal: Add restore_user_sigmask()")
Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
---
Sorry, I was trying a new setup at work. I should have tested it.
My bad, I've checked this one.
I've removed the questionable reported-by, since we're not sure if
it is actually the same issue.

 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        |  8 +++++---
 6 files changed, 55 insertions(+), 39 deletions(-)

Comments

Eric Wong May 3, 2019, 6:34 a.m. UTC | #1
Deepa Dinamani <deepa.kernel@gmail.com> wrote:
> Sorry, I was trying a new setup at work. I should have tested it.
> My bad, I've checked this one.

Thanks.  This is good w.r.t. epoll_pwait and ppoll when applied
to 5.0.11 (no fs/io_uring.c).

Can't think of anything which uses pselect or aio on my system;
but it looks right to me.

> I've removed the questionable reported-by, since we're not sure if
> it is actually the same issue.

Yes, I hope Omar can test this, too.

Thanks again, all!
Deepa Dinamani May 3, 2019, 6:21 p.m. UTC | #2
On Thu, May 2, 2019 at 11:34 PM Eric Wong <e@80x24.org> wrote:
>
> Deepa Dinamani <deepa.kernel@gmail.com> wrote:
> > Sorry, I was trying a new setup at work. I should have tested it.
> > My bad, I've checked this one.
>
> Thanks.  This is good w.r.t. epoll_pwait and ppoll when applied
> to 5.0.11 (no fs/io_uring.c).

Thanks. Al, would you be picking up this patch? I can resend it.

> Can't think of anything which uses pselect or aio on my system;
> but it looks right to me.
>
> > I've removed the questionable reported-by, since we're not sure if
> > it is actually the same issue.
>
> Yes, I hope Omar can test this, too.

Omar, would you be able to test this?

Thanks,
Deepa
Davidlohr Bueso May 3, 2019, 7:51 p.m. UTC | #3
This patch could also be routed via akpm, adding him.

On Thu, 02 May 2019, Deepa Dinamani 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 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.

Thanks for doing this; I've reviewed the epoll bits (along with the overall
idea) and it seems like a sane alternative to reverting the offending patch.

Feel free to add:

Reviewed-by: Davidlohr Bueso <dbueso@suse.de>

A small nit, I think we should be a bit more verbose about the return semantics
of restore_user_sigmask()... see at the end.

>
>Reported-by: Eric Wong <e@80x24.org>
>Fixes: 854a6ed56839a40f6b5d02a2962f48841482eec4 ("signal: Add restore_user_sigmask()")
>Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>

>---
>Sorry, I was trying a new setup at work. I should have tested it.
>My bad, I've checked this one.
>I've removed the questionable reported-by, since we're not sure if
>it is actually the same issue.
>
> 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        |  8 +++++---
> 6 files changed, 55 insertions(+), 39 deletions(-)
>
>diff --git a/fs/aio.c b/fs/aio.c
>index 38b741aef0bf..7de2f7573d55 100644
>--- a/fs/aio.c
>+++ b/fs/aio.c
>@@ -2133,7 +2133,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;
>@@ -2146,8 +2146,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;
>@@ -2166,7 +2166,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;
>@@ -2180,8 +2180,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;
>@@ -2231,7 +2231,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;
>@@ -2244,8 +2244,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;
>@@ -2264,7 +2264,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;
>@@ -2277,8 +2277,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 e2bd77da5e21..e107e299c3f3 100644
>--- a/fs/io_uring.c
>+++ b/fs/io_uring.c
>@@ -1965,7 +1965,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();
>@@ -1996,8 +1996,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..348dbe5e6dd0 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 3a9e41197d46..4f8b49a7b058 100644
>--- a/kernel/signal.c
>+++ b/kernel/signal.c
>@@ -2845,15 +2845,16 @@ 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.

How about:

"
Callers must carefully coordinate between signal_pending() checks between the
actual system call and restore_user_sigmask() - for which a new pending signal
may come in between calls and therefore must consider this for returning a proper
error code.

Returns 1 in case a signal pending is detected, otherwise 0.
"

>  *
>  * 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
>@@ -2862,7 +2863,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;
> 	}
>
> 	/*
>@@ -2870,6 +2871,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.21.0.593.g511ec345e18-goog
>
Deepa Dinamani May 3, 2019, 10:53 p.m. UTC | #4
The original patch was merged through the tip tree. Adding tglx just in case.

I will post the revised patch to everyone on this thread.

> >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 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.
>
> Thanks for doing this; I've reviewed the epoll bits (along with the overall
> idea) and it seems like a sane alternative to reverting the offending patch.

Sorry maybe the description wasn't clear. What I actually am saying is
that all these syscalls were dropping signals before and
854a6ed56839a4 actually did things right by making sure they did not
do so.
But, there was a bug in that it did not communicate to userspace when
the error code was not already set.
However, we could still argue that the check and flipping of the mask
isn't atomic and there is still a way this can theoretically happen.
But, this will also mean that these syscalls will slow down further.
But, they are already expected to be slow so maybe it doesn't matter.
I will note this down in the commit text.
I don't think reverting was an alternative. 854a6ed56839a4 exposed a
bug that was already there.

> Feel free to add:
>
> Reviewed-by: Davidlohr Bueso <dbueso@suse.de>
>
> A small nit, I think we should be a bit more verbose about the return semantics
> of restore_user_sigmask()... see at the end.
>
> >
> >Reported-by: Eric Wong <e@80x24.org>
> >Fixes: 854a6ed56839a40f6b5d02a2962f48841482eec4 ("signal: Add restore_user_sigmask()")
> >Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
> >--- a/kernel/signal.c
> >+++ b/kernel/signal.c
> >@@ -2845,15 +2845,16 @@ 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.
>
> How about:
>
> "
> Callers must carefully coordinate between signal_pending() checks between the
> actual system call and restore_user_sigmask() - for which a new pending signal
> may come in between calls and therefore must consider this for returning a proper
> error code.
>
> Returns 1 in case a signal pending is detected, otherwise 0.

Ok, I will add more verbiage here.

Thanks,
Deepa

Patch
diff mbox series

diff --git a/fs/aio.c b/fs/aio.c
index 38b741aef0bf..7de2f7573d55 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -2133,7 +2133,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;
@@ -2146,8 +2146,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;
@@ -2166,7 +2166,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;
@@ -2180,8 +2180,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;
@@ -2231,7 +2231,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;
@@ -2244,8 +2244,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;
@@ -2264,7 +2264,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;
@@ -2277,8 +2277,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 e2bd77da5e21..e107e299c3f3 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1965,7 +1965,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();
@@ -1996,8 +1996,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..348dbe5e6dd0 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 3a9e41197d46..4f8b49a7b058 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2845,15 +2845,16 @@  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.
  *
  * 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
@@ -2862,7 +2863,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;
 	}
 
 	/*
@@ -2870,6 +2871,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);