[PATCH -mm 0/2] aio: simplify/fix the usage of restore_saved_sigmask_unless()

[PATCH -mm 0/2] aio: simplify/fix the usage of restore_saved_sigmask_unless()

[Oleg Nesterov]

Absolutely untested. Benjamin, please review.

This depends on

	signal-remove-the-wrong-signal_pending-check-in-restore_user_sigmask.patch
	signal-simplify-set_user_sigmask-restore_user_sigmask.patch

Oleg.

 fs/aio.c | 32 ++++++--------------------------
 1 file changed, 6 insertions(+), 26 deletions(-)

[PATCH 1/2] aio: simplify the usage of restore_saved_sigmask_unless()

[Oleg Nesterov]

Move the signal_pending() check and restore_saved_sigmask_unless()
into do_io_getevents() and simplify the callers.

The only complication is that non-restartable io_getevents() and
io_getevents_time32() have to translate ERESTARTNOHAND into EINTR.
They do not need restore_saved_sigmask_unless(), but it is harmless
and WARN_ON(!TIF_SIGPENDING) will be usefule after the next patch.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/aio.c | 34 ++++++++--------------------------
 1 file changed, 8 insertions(+), 26 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 8200f97..944eef7 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -2034,6 +2034,7 @@ static long do_io_getevents(aio_context_t ctx_id,
 {
 	ktime_t until = ts ? timespec64_to_ktime(*ts) : KTIME_MAX;
 	struct kioctx *ioctx = lookup_ioctx(ctx_id);
+	bool interrupted;
 	long ret = -EINVAL;
 
 	if (likely(ioctx)) {
@@ -2042,6 +2043,11 @@ static long do_io_getevents(aio_context_t ctx_id,
 		percpu_ref_put(&ioctx->users);
 	}
 
+	interrupted = signal_pending(current);
+	restore_saved_sigmask_unless(interrupted);
+	if (interrupted && !ret)
+		ret = -ERESTARTNOHAND;
+
 	return ret;
 }
 
@@ -2072,7 +2078,7 @@ SYSCALL_DEFINE5(io_getevents, aio_context_t, ctx_id,
 		return -EFAULT;
 
 	ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);
-	if (!ret && signal_pending(current))
+	if (ret == -ERESTARTNOHAND)
 		ret = -EINTR;
 	return ret;
 }
@@ -2094,7 +2100,6 @@ SYSCALL_DEFINE6(io_pgetevents,
 {
 	struct __aio_sigset	ksig = { NULL, };
 	struct timespec64	ts;
-	bool interrupted;
 	int ret;
 
 	if (timeout && unlikely(get_timespec64(&ts, timeout)))
@@ -2109,11 +2114,6 @@ SYSCALL_DEFINE6(io_pgetevents,
 
 	ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);
 
-	interrupted = signal_pending(current);
-	restore_saved_sigmask_unless(interrupted);
-	if (interrupted && !ret)
-		ret = -ERESTARTNOHAND;
-
 	return ret;
 }
 
@@ -2129,7 +2129,6 @@ SYSCALL_DEFINE6(io_pgetevents_time32,
 {
 	struct __aio_sigset	ksig = { NULL, };
 	struct timespec64	ts;
-	bool interrupted;
 	int ret;
 
 	if (timeout && unlikely(get_old_timespec32(&ts, timeout)))
@@ -2145,11 +2144,6 @@ SYSCALL_DEFINE6(io_pgetevents_time32,
 
 	ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);
 
-	interrupted = signal_pending(current);
-	restore_saved_sigmask_unless(interrupted);
-	if (interrupted && !ret)
-		ret = -ERESTARTNOHAND;
-
 	return ret;
 }
 
@@ -2170,7 +2164,7 @@ SYSCALL_DEFINE5(io_getevents_time32, __u32, ctx_id,
 		return -EFAULT;
 
 	ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);
-	if (!ret && signal_pending(current))
+	if (ret == -ERESTARTNOHAND)
 		ret = -EINTR;
 	return ret;
 }
@@ -2196,7 +2190,6 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents,
 {
 	struct __compat_aio_sigset ksig = { NULL, };
 	struct timespec64 t;
-	bool interrupted;
 	int ret;
 
 	if (timeout && get_old_timespec32(&t, timeout))
@@ -2211,11 +2204,6 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents,
 
 	ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);
 
-	interrupted = signal_pending(current);
-	restore_saved_sigmask_unless(interrupted);
-	if (interrupted && !ret)
-		ret = -ERESTARTNOHAND;
-
 	return ret;
 }
 
@@ -2231,7 +2219,6 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents_time64,
 {
 	struct __compat_aio_sigset ksig = { NULL, };
 	struct timespec64 t;
-	bool interrupted;
 	int ret;
 
 	if (timeout && get_timespec64(&t, timeout))
@@ -2246,11 +2233,6 @@ COMPAT_SYSCALL_DEFINE6(io_pgetevents_time64,
 
 	ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);
 
-	interrupted = signal_pending(current);
-	restore_saved_sigmask_unless(interrupted);
-	if (interrupted && !ret)
-		ret = -ERESTARTNOHAND;
-
 	return ret;
 }
 #endif
-- 
2.5.0

Re: [PATCH 1/2] aio: simplify the usage of restore_saved_sigmask_unless()

[Linus Torvalds]

On Fri, Jun 7, 2019 at 3:32 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> The only complication is that non-restartable io_getevents() and
> io_getevents_time32() have to translate ERESTARTNOHAND into EINTR.

Are they actually nonrestartable? I think the current EINTR is just a mistake.

               Linus

Re: [PATCH 1/2] aio: simplify the usage of restore_saved_sigmask_unless()

[Linus Torvalds]

On Fri, Jun 7, 2019 at 10:33 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Are they actually nonrestartable? I think the current EINTR is just a mistake.

Oh, I guess they are, because of the relative timeout thing that
shouldn't reset to the original value.

And I don't think this is worth a ERESTAR_RESTARTTBLOCK.

          Linus

Re: [PATCH 1/2] aio: simplify the usage of restore_saved_sigmask_unless()

[Eric W. Biederman]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Fri, Jun 7, 2019 at 10:33 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> Are they actually nonrestartable? I think the current EINTR is just a mistake.
>
> Oh, I guess they are, because of the relative timeout thing that
> shouldn't reset to the original value.
>
> And I don't think this is worth a ERESTAR_RESTARTTBLOCK.

Unless I am misreading things io_pgetevents isn't restartable
either and ERESTARTNOHAND is a bug in that case.

Is the bug going the other way?

Eric

Re: [PATCH 1/2] aio: simplify the usage of restore_saved_sigmask_unless()

[Linus Torvalds]

On Fri, Jun 7, 2019 at 11:03 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Unless I am misreading things io_pgetevents isn't restartable
> either and ERESTARTNOHAND is a bug in that case.
>
> Is the bug going the other way?

Yeah, I think you're right, and that should be EINTR too.

Or at least be conditional on whether it had a timeout or not. A NULL
timeout obviously _is_ restartable. But I don't think it's worth being
clever here.

                Linus

[PATCH 2/2] aio: fix the usage of restore_saved_sigmask_unless()

[Oleg Nesterov]

do_io_getevents() is the only user of set/restore_user_sigmask which
can return success or error and deliver a signal which was temporary
unblocked by set_user_sigmask().

Change it to keep the modified sigmask (pass true to restore_unless)
only if the syscall returns ERESTARTNOHAND/EINTR. This matches all
other syscalls which modify current->blocked before wait-for-event.

Note that it doesn't even need the signal_pending() check, read_event()
wait_event_interruptible_hrtimeout() returns ERESTARTSYS if interrupted.
But it seems that read_events() needs some unrelated cleanups which
should be done first.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/aio.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 944eef7..5bdbef0 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -2034,7 +2034,6 @@ static long do_io_getevents(aio_context_t ctx_id,
 {
 	ktime_t until = ts ? timespec64_to_ktime(*ts) : KTIME_MAX;
 	struct kioctx *ioctx = lookup_ioctx(ctx_id);
-	bool interrupted;
 	long ret = -EINVAL;
 
 	if (likely(ioctx)) {
@@ -2043,10 +2042,9 @@ static long do_io_getevents(aio_context_t ctx_id,
 		percpu_ref_put(&ioctx->users);
 	}
 
-	interrupted = signal_pending(current);
-	restore_saved_sigmask_unless(interrupted);
-	if (interrupted && !ret)
+	if (!ret && signal_pending(current))
 		ret = -ERESTARTNOHAND;
+	restore_saved_sigmask_unless(ret == -ERESTARTNOHAND);
 
 	return ret;
 }
-- 
2.5.0