Re: [PATCH 1/1] signal: Adjust error codes according to restore_user_sigmask()
On Tue, May 21, 2019 at 5:35 PM Deepa Dinamani wrote: > > > > > It's been 2 weeks and this fix hasn't appeared in mmots / mmotm. > > > > I also noticed it's missing Cc: for stable@ (below) > > > > > > Why is a -stable backport needed? I see some talk above about lost > > > signals but it is unclear whether these are being observed after fixing > > > the regression caused by 854a6ed56839a. > > > > I guess Deepa's commit messages wasn't clear... > > I suggest prepending this as the first paragraph to Deepa's > > original message: > > > > This fixes a bug introduced with 854a6ed56839a which caused > > EINTR to not be reported to userspace on epoll_pwait. Failure > > to report EINTR to userspace caused problems with user code > > which relies on EINTR to run signal handlers. > > This is not what the patch fixed. > > The notable change is userspace is that now whenever a signal is > delivered, the return value is adjusted to reflect the signal > delivery. > Prior to this patch, there was a window, however small it might have > been, when the signal was delivered but the errono was not adjusted > appropriately. > This is because of the regression caused by 854a6ed56839a, which > extended the window of delivery of signals that was delivered to > userspace. > The patch also fixes more than sys_epoll_pwait(). > > I will post a follow up patch. > > > > > > IOW, can we please have a changelog which has a clear and complete > > > description of the user-visible effects of the change. > > > > > > And please Cc Oleg. > > I will cc Oleg. Also the commit message was brief because the issue was explained in the link that was quoted in the commit message. Detailed issue discussion permalink: https://lore.kernel.org/linux-fsdevel/20190427093319.sgicqik2oqkez3wk@dcvr/ -Deepa
Re: [PATCH 1/1] signal: Adjust error codes according to restore_user_sigmask()
> > > It's been 2 weeks and this fix hasn't appeared in mmots / mmotm. > > > I also noticed it's missing Cc: for stable@ (below) > > > > Why is a -stable backport needed? I see some talk above about lost > > signals but it is unclear whether these are being observed after fixing > > the regression caused by 854a6ed56839a. > > I guess Deepa's commit messages wasn't clear... > I suggest prepending this as the first paragraph to Deepa's > original message: > > This fixes a bug introduced with 854a6ed56839a which caused > EINTR to not be reported to userspace on epoll_pwait. Failure > to report EINTR to userspace caused problems with user code > which relies on EINTR to run signal handlers. This is not what the patch fixed. The notable change is userspace is that now whenever a signal is delivered, the return value is adjusted to reflect the signal delivery. Prior to this patch, there was a window, however small it might have been, when the signal was delivered but the errono was not adjusted appropriately. This is because of the regression caused by 854a6ed56839a, which extended the window of delivery of signals that was delivered to userspace. The patch also fixes more than sys_epoll_pwait(). I will post a follow up patch. > > > IOW, can we please have a changelog which has a clear and complete > > description of the user-visible effects of the change. > > > > And please Cc Oleg. I will cc Oleg.
Re: [PATCH 1/1] signal: Adjust error codes according to restore_user_sigmask()
Andrew Morton wrote:
> On Tue, 21 May 2019 09:25:51 + Eric Wong wrote:
>
> > 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 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
> > > Fixes: 854a6ed56839a40f6b5d02a2962f48841482eec4 ("signal: Add
> > > restore_user_sigmask()")
> > > Signed-off-by: Deepa Dinamani
> > > Reviewed-by: Davidlohr Bueso
>
> (top-posting fixed).
>
> > It's been 2 weeks and this fix hasn't appeared in mmots / mmotm.
> > I also noticed it's missing Cc: for stable@ (below)
>
> Why is a -stable backport needed? I see some talk above about lost
> signals but it is unclear whether these are being observed after fixing
> the regression caused by 854a6ed56839a.
I guess Deepa's commit messages wasn't clear...
I suggest prepending this as the first paragraph to Deepa's
original message:
This fixes a bug introduced with 854a6ed56839a which caused
EINTR to not be reported to userspace on epoll_pwait. Failure
to report EINTR to userspace caused problems with user code
which relies on EINTR to run signal handlers.
> IOW, can we please have a changelog which has a clear and complete
> description of the user-visible effects of the change.
>
> And please Cc Oleg.
Re: [PATCH 1/1] signal: Adjust error codes according to restore_user_sigmask()
On Tue, 21 May 2019 09:25:51 + Eric Wong wrote:
> 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 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
> > Fixes: 854a6ed56839a40f6b5d02a2962f48841482eec4 ("signal: Add
> > restore_user_sigmask()")
> > Signed-off-by: Deepa Dinamani
> > Reviewed-by: Davidlohr Bueso
(top-posting fixed).
> It's been 2 weeks and this fix hasn't appeared in mmots / mmotm.
> I also noticed it's missing Cc: for stable@ (below)
Why is a -stable backport needed? I see some talk above about lost
signals but it is unclear whether these are being observed after fixing
the regression caused by 854a6ed56839a.
IOW, can we please have a changelog which has a clear and complete
description of the user-visible effects of the change.
And please Cc Oleg.
Re: [PATCH 1/1] signal: Adjust error codes according to restore_user_sigmask()
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 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
> Fixes: 854a6ed56839a40f6b5d02a2962f48841482eec4 ("signal: Add
> restore_user_sigmask()")
> Signed-off-by: Deepa Dinamani
> Reviewed-by: Davidlohr Bueso
Cc: # 5.0.x
Cc: # 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_tksigmask, sigsaved;
> struct timespec64 ts;
> - int ret;
> + int ret, signal_detected;
>
> if (timeout && unlikely(get_timespec64(, timeout)))
> return -EFAULT;
> @@ -2108,8 +2108,8 @@ SYSCALL_DEFINE6(io_pgetevents,
> return ret;
>
> ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? : NULL);
> - restore_user_sigmask(ksig.sigmask, );
> - if (signal_pending(current) && !ret)
> + signal_detected = restore_user_sigmask(ksig.sigmask, );
> + if (signal_detected && !ret)
> ret = -ERESTARTNOHAND;
>
> return ret;
> @@ -2128,7 +2128,7 @@ SYSCALL_DEFINE6(io_pgetevents_time32,
> struct __aio_sigset ksig = { NULL, };
> sigset_tksigmask, sigsaved;
> struct timespec64 ts;
> - int ret;
> + int ret, signal_detected;
>
> if (timeout && unlikely(get_old_timespec32(, 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 ? : NULL);
> - restore_user_sigmask(ksig.sigmask, );
> - if (signal_pending(current) && !ret)
> + signal_detected = restore_user_sigmask(ksig.sigmask, );
> + 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(, 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 ? : NULL);
> - restore_user_sigmask(ksig.sigmask, );
> - if (signal_pending(current) && !ret)
> + signal_detected = restore_user_sigmask(ksig.sigmask, );
> + 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(, 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 ? : NULL);
> - restore_user_sigmask(ksig.sigmask, );
> - if (signal_pending(current) && !ret)
>
[PATCH 1/1] signal: Adjust error codes according to restore_user_sigmask()
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
Fixes: 854a6ed56839a40f6b5d02a2962f48841482eec4 ("signal: Add
restore_user_sigmask()")
Signed-off-by: Deepa Dinamani
Reviewed-by: Davidlohr Bueso
---
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_tksigmask, sigsaved;
struct timespec64 ts;
- int ret;
+ int ret, signal_detected;
if (timeout && unlikely(get_timespec64(, timeout)))
return -EFAULT;
@@ -2108,8 +2108,8 @@ SYSCALL_DEFINE6(io_pgetevents,
return ret;
ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? : NULL);
- restore_user_sigmask(ksig.sigmask, );
- if (signal_pending(current) && !ret)
+ signal_detected = restore_user_sigmask(ksig.sigmask, );
+ if (signal_detected && !ret)
ret = -ERESTARTNOHAND;
return ret;
@@ -2128,7 +2128,7 @@ SYSCALL_DEFINE6(io_pgetevents_time32,
struct __aio_sigset ksig = { NULL, };
sigset_tksigmask, sigsaved;
struct timespec64 ts;
- int ret;
+ int ret, signal_detected;
if (timeout && unlikely(get_old_timespec32(, 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 ? : NULL);
- restore_user_sigmask(ksig.sigmask, );
- if (signal_pending(current) && !ret)
+ signal_detected = restore_user_sigmask(ksig.sigmask, );
+ 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(, 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 ? : NULL);
- restore_user_sigmask(ksig.sigmask, );
- if (signal_pending(current) && !ret)
+ signal_detected = restore_user_sigmask(ksig.sigmask, );
+ 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(, 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 ? : NULL);
- restore_user_sigmask(ksig.sigmask, );
- if (signal_pending(current) && !ret)
+ signal_detected = restore_user_sigmask(ksig.sigmask, );
+ 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
@@
