[RFC PATCH 0/5]: Removing saved_sigmask

[RFC PATCH 0/5]: Removing saved_sigmask

[Eric W. Biederman]

While reviewing Oleg's patches I realized a bunch of the logic around
saved_sigmask was redundant.  So I dug just to see what I could see.

I turns out that real_blocked and saved_sigmask were different
implementations of the same idea for slightly different purposes.
Which means we only need either real_blocked or saved_sigmask.

I chose real_blocked as it has just a little bit of code associated
with it to disable optimizations on the signal sending path that do
not apply to blocked signals.

I did a little bit of cleanup of the users.  Modified the core to keep
real_blocked in sync with blocked except while a clever system call that
like pselect or sigtimedwait is running.

After the dust cleared this allowed restore_sigmask and all of the logic
to keep it valid to be removed entirely.

I have only done the most cursory of testing at this point.  Does anyone
have any thoughts in cleaning up the code in this direction?

Eric W. Biederman (5):
      signal: Teach sigsuspend to use set_user_sigmask
      signal/kvm:  Stop using sigprocmask in kvm_sigset_(activate|deactivate)
      signal: Always keep real_blocked in sync with blocked
      signal: Remove saved_sigmask
      signal: Remove the unnecessary restore_sigmask flag

 arch/arc/include/asm/thread_info.h       |  1 -
 arch/arm/include/asm/thread_info.h       |  1 -
 arch/arm64/include/asm/thread_info.h     |  1 -
 arch/c6x/include/asm/thread_info.h       |  1 -
 arch/csky/include/asm/thread_info.h      |  2 -
 arch/h8300/include/asm/thread_info.h     |  1 -
 arch/hexagon/include/asm/thread_info.h   |  1 -
 arch/m68k/include/asm/thread_info.h      |  1 -
 arch/mips/include/asm/thread_info.h      |  1 -
 arch/nds32/include/asm/thread_info.h     |  2 -
 arch/nios2/include/asm/thread_info.h     |  2 -
 arch/riscv/include/asm/thread_info.h     |  1 -
 arch/s390/include/asm/thread_info.h      |  1 -
 arch/sparc/include/asm/thread_info_32.h  |  1 -
 arch/um/include/asm/thread_info.h        |  1 -
 arch/unicore32/include/asm/thread_info.h |  1 -
 arch/xtensa/include/asm/thread_info.h    |  1 -
 include/linux/sched.h                    |  5 --
 include/linux/sched/signal.h             | 84 +-------------------------------
 kernel/ptrace.c                          | 15 ++----
 kernel/signal.c                          | 56 +++++++++------------
 virt/kvm/kvm_main.c                      | 11 +----
 22 files changed, 31 insertions(+), 160 deletions(-)

Eric

[RFC PATCH 0/5]: Removing saved_sigmask

[Eric W. Biederman]

While reviewing Oleg's patches I realized a bunch of the logic around
saved_sigmask was redundant.  So I dug just to see what I could see.

I turns out that real_blocked and saved_sigmask were different
implementations of the same idea for slightly different purposes.
Which means we only need either real_blocked or saved_sigmask.

I chose real_blocked as it has just a little bit of code associated
with it to disable optimizations on the signal sending path that do
not apply to blocked signals.

I did a little bit of cleanup of the users.  Modified the core to keep
real_blocked in sync with blocked except while a clever system call that
like pselect or sigtimedwait is running.

After the dust cleared this allowed restore_sigmask and all of the logic
to keep it valid to be removed entirely.

I have only done the most cursory of testing at this point.  Does anyone
have any thoughts in cleaning up the code in this direction?

Eric W. Biederman (5):
      signal: Teach sigsuspend to use set_user_sigmask
      signal/kvm:  Stop using sigprocmask in kvm_sigset_(activate|deactivate)
      signal: Always keep real_blocked in sync with blocked
      signal: Remove saved_sigmask
      signal: Remove the unnecessary restore_sigmask flag

 arch/arc/include/asm/thread_info.h       |  1 -
 arch/arm/include/asm/thread_info.h       |  1 -
 arch/arm64/include/asm/thread_info.h     |  1 -
 arch/c6x/include/asm/thread_info.h       |  1 -
 arch/csky/include/asm/thread_info.h      |  2 -
 arch/h8300/include/asm/thread_info.h     |  1 -
 arch/hexagon/include/asm/thread_info.h   |  1 -
 arch/m68k/include/asm/thread_info.h      |  1 -
 arch/mips/include/asm/thread_info.h      |  1 -
 arch/nds32/include/asm/thread_info.h     |  2 -
 arch/nios2/include/asm/thread_info.h     |  2 -
 arch/riscv/include/asm/thread_info.h     |  1 -
 arch/s390/include/asm/thread_info.h      |  1 -
 arch/sparc/include/asm/thread_info_32.h  |  1 -
 arch/um/include/asm/thread_info.h        |  1 -
 arch/unicore32/include/asm/thread_info.h |  1 -
 arch/xtensa/include/asm/thread_info.h    |  1 -
 include/linux/sched.h                    |  5 --
 include/linux/sched/signal.h             | 84 +-------------------------------
 kernel/ptrace.c                          | 15 ++----
 kernel/signal.c                          | 56 +++++++++------------
 virt/kvm/kvm_main.c                      | 11 +----
 22 files changed, 31 insertions(+), 160 deletions(-)

Eric

[RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Eric W. Biederman]

The sigsuspend system call overrides the signal mask just
like all of the other users of set_user_sigmask, so convert
it to use the same helpers.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/signal.c | 43 +++++++++++++++++++------------------------
 1 file changed, 19 insertions(+), 24 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 6f72cff043f0..bfa36320a4f7 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2932,6 +2932,15 @@ int sigprocmask(int how, sigset_t *set, sigset_t *oldset)
 }
 EXPORT_SYMBOL(sigprocmask);
 
+static int set_sigmask(sigset_t *kmask)
+{
+	set_restore_sigmask();
+	current->saved_sigmask = current->blocked;
+	set_current_blocked(kmask);
+
+	return 0;
+}
+
 /*
  * The api helps set app-provided sigmasks.
  *
@@ -2952,11 +2961,7 @@ int set_user_sigmask(const sigset_t __user *umask, size_t sigsetsize)
 	if (copy_from_user(&kmask, umask, sizeof(sigset_t)))
 		return -EFAULT;
 
-	set_restore_sigmask();
-	current->saved_sigmask = current->blocked;
-	set_current_blocked(&kmask);
-
-	return 0;
+	return set_sigmask(&kmask);
 }
 
 #ifdef CONFIG_COMPAT
@@ -2972,11 +2977,7 @@ int set_compat_user_sigmask(const compat_sigset_t __user *umask,
 	if (get_compat_sigset(&kmask, umask))
 		return -EFAULT;
 
-	set_restore_sigmask();
-	current->saved_sigmask = current->blocked;
-	set_current_blocked(&kmask);
-
-	return 0;
+	return set_sigmask(&kmask);
 }
 #endif
 
@@ -4409,14 +4410,10 @@ SYSCALL_DEFINE0(pause)
 
 static int sigsuspend(sigset_t *set)
 {
-	current->saved_sigmask = current->blocked;
-	set_current_blocked(set);
-
 	while (!signal_pending(current)) {
 		__set_current_state(TASK_INTERRUPTIBLE);
 		schedule();
 	}
-	set_restore_sigmask();
 	return -ERESTARTNOHAND;
 }
 
@@ -4430,12 +4427,10 @@ SYSCALL_DEFINE2(rt_sigsuspend, sigset_t __user *, unewset, size_t, sigsetsize)
 {
 	sigset_t newset;
 
-	/* XXX: Don't preclude handling different sized sigset_t's.  */
-	if (sigsetsize != sizeof(sigset_t))
-		return -EINVAL;
-
-	if (copy_from_user(&newset, unewset, sizeof(newset)))
+	set_user_sigmask(unewset, sigsetsize);
+	if (!unewset)
 		return -EFAULT;
+
 	return sigsuspend(&newset);
 }
  
@@ -4444,12 +4439,10 @@ COMPAT_SYSCALL_DEFINE2(rt_sigsuspend, compat_sigset_t __user *, unewset, compat_
 {
 	sigset_t newset;
 
-	/* XXX: Don't preclude handling different sized sigset_t's.  */
-	if (sigsetsize != sizeof(sigset_t))
-		return -EINVAL;
-
-	if (get_compat_sigset(&newset, unewset))
+	set_compat_user_sigmask(unewset, sigsetsize);
+	if (!unewset)
 		return -EFAULT;
+
 	return sigsuspend(&newset);
 }
 #endif
@@ -4459,6 +4452,7 @@ SYSCALL_DEFINE1(sigsuspend, old_sigset_t, mask)
 {
 	sigset_t blocked;
 	siginitset(&blocked, mask);
+	set_sigmask(&blocked);
 	return sigsuspend(&blocked);
 }
 #endif
@@ -4467,6 +4461,7 @@ SYSCALL_DEFINE3(sigsuspend, int, unused1, int, unused2, old_sigset_t, mask)
 {
 	sigset_t blocked;
 	siginitset(&blocked, mask);
+	set_sigmask(&blocked);
 	return sigsuspend(&blocked);
 }
 #endif
-- 
2.21.0.dirty

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

[RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Eric W. Biederman]

The sigsuspend system call overrides the signal mask just
like all of the other users of set_user_sigmask, so convert
it to use the same helpers.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/signal.c | 43 +++++++++++++++++++------------------------
 1 file changed, 19 insertions(+), 24 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 6f72cff043f0..bfa36320a4f7 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2932,6 +2932,15 @@ int sigprocmask(int how, sigset_t *set, sigset_t *oldset)
 }
 EXPORT_SYMBOL(sigprocmask);
 
+static int set_sigmask(sigset_t *kmask)
+{
+	set_restore_sigmask();
+	current->saved_sigmask = current->blocked;
+	set_current_blocked(kmask);
+
+	return 0;
+}
+
 /*
  * The api helps set app-provided sigmasks.
  *
@@ -2952,11 +2961,7 @@ int set_user_sigmask(const sigset_t __user *umask, size_t sigsetsize)
 	if (copy_from_user(&kmask, umask, sizeof(sigset_t)))
 		return -EFAULT;
 
-	set_restore_sigmask();
-	current->saved_sigmask = current->blocked;
-	set_current_blocked(&kmask);
-
-	return 0;
+	return set_sigmask(&kmask);
 }
 
 #ifdef CONFIG_COMPAT
@@ -2972,11 +2977,7 @@ int set_compat_user_sigmask(const compat_sigset_t __user *umask,
 	if (get_compat_sigset(&kmask, umask))
 		return -EFAULT;
 
-	set_restore_sigmask();
-	current->saved_sigmask = current->blocked;
-	set_current_blocked(&kmask);
-
-	return 0;
+	return set_sigmask(&kmask);
 }
 #endif
 
@@ -4409,14 +4410,10 @@ SYSCALL_DEFINE0(pause)
 
 static int sigsuspend(sigset_t *set)
 {
-	current->saved_sigmask = current->blocked;
-	set_current_blocked(set);
-
 	while (!signal_pending(current)) {
 		__set_current_state(TASK_INTERRUPTIBLE);
 		schedule();
 	}
-	set_restore_sigmask();
 	return -ERESTARTNOHAND;
 }
 
@@ -4430,12 +4427,10 @@ SYSCALL_DEFINE2(rt_sigsuspend, sigset_t __user *, unewset, size_t, sigsetsize)
 {
 	sigset_t newset;
 
-	/* XXX: Don't preclude handling different sized sigset_t's.  */
-	if (sigsetsize != sizeof(sigset_t))
-		return -EINVAL;
-
-	if (copy_from_user(&newset, unewset, sizeof(newset)))
+	set_user_sigmask(unewset, sigsetsize);
+	if (!unewset)
 		return -EFAULT;
+
 	return sigsuspend(&newset);
 }
  
@@ -4444,12 +4439,10 @@ COMPAT_SYSCALL_DEFINE2(rt_sigsuspend, compat_sigset_t __user *, unewset, compat_
 {
 	sigset_t newset;
 
-	/* XXX: Don't preclude handling different sized sigset_t's.  */
-	if (sigsetsize != sizeof(sigset_t))
-		return -EINVAL;
-
-	if (get_compat_sigset(&newset, unewset))
+	set_compat_user_sigmask(unewset, sigsetsize);
+	if (!unewset)
 		return -EFAULT;
+
 	return sigsuspend(&newset);
 }
 #endif
@@ -4459,6 +4452,7 @@ SYSCALL_DEFINE1(sigsuspend, old_sigset_t, mask)
 {
 	sigset_t blocked;
 	siginitset(&blocked, mask);
+	set_sigmask(&blocked);
 	return sigsuspend(&blocked);
 }
 #endif
@@ -4467,6 +4461,7 @@ SYSCALL_DEFINE3(sigsuspend, int, unused1, int, unused2, old_sigset_t, mask)
 {
 	sigset_t blocked;
 	siginitset(&blocked, mask);
+	set_sigmask(&blocked);
 	return sigsuspend(&blocked);
 }
 #endif
-- 
2.21.0.dirty

[RFC PATCH 2/5] signal/kvm: Stop using sigprocmask in kvm_sigset_(activate|deactivate)

[Eric W. Biederman]

Instead of jumping through hoops call __set_current_blocked directly.
As well as directly manipulating real_blocked.

This is in preparation for modifying the code to always keep
real_blocked in sync with blocked except in the rare cases when the
kernel needs to override it.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 virt/kvm/kvm_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index f0d13d9d125d..8575a1010bfc 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2240,7 +2240,8 @@ void kvm_sigset_activate(struct kvm_vcpu *vcpu)
 	 * ->real_blocked don't care as long ->real_blocked is always a subset
 	 * of ->blocked.
 	 */
-	sigprocmask(SIG_SETMASK, &vcpu->sigset, &current->real_blocked);
+	current->real_blocked = current->blocked;
+	__set_current_blocked(&vcpu->sigset);
 }
 
 void kvm_sigset_deactivate(struct kvm_vcpu *vcpu)
@@ -2248,7 +2249,7 @@ void kvm_sigset_deactivate(struct kvm_vcpu *vcpu)
 	if (!vcpu->sigset_active)
 		return;
 
-	sigprocmask(SIG_SETMASK, &current->real_blocked, NULL);
+	__set_current_blocked(&current->real_blocked);
 	sigemptyset(&current->real_blocked);
 }
 
-- 
2.21.0.dirty

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

[RFC PATCH 2/5] signal/kvm: Stop using sigprocmask in kvm_sigset_(activate|deactivate)

[Eric W. Biederman]

Instead of jumping through hoops call __set_current_blocked directly.
As well as directly manipulating real_blocked.

This is in preparation for modifying the code to always keep
real_blocked in sync with blocked except in the rare cases when the
kernel needs to override it.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 virt/kvm/kvm_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index f0d13d9d125d..8575a1010bfc 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2240,7 +2240,8 @@ void kvm_sigset_activate(struct kvm_vcpu *vcpu)
 	 * ->real_blocked don't care as long ->real_blocked is always a subset
 	 * of ->blocked.
 	 */
-	sigprocmask(SIG_SETMASK, &vcpu->sigset, &current->real_blocked);
+	current->real_blocked = current->blocked;
+	__set_current_blocked(&vcpu->sigset);
 }
 
 void kvm_sigset_deactivate(struct kvm_vcpu *vcpu)
@@ -2248,7 +2249,7 @@ void kvm_sigset_deactivate(struct kvm_vcpu *vcpu)
 	if (!vcpu->sigset_active)
 		return;
 
-	sigprocmask(SIG_SETMASK, &current->real_blocked, NULL);
+	__set_current_blocked(&current->real_blocked);
 	sigemptyset(&current->real_blocked);
 }
 
-- 
2.21.0.dirty

[RFC PATCH 3/5] signal: Always keep real_blocked in sync with blocked

[Eric W. Biederman]

Except where we temporarily override blocked always keep real_blocked
in sync with blocked.

By always setting real_blocked when we set blocked this allows
some slight efficiency and simplifications, by not having
to save blocked.

This also preparse the code for the removal of saved_sigmask.  That
should result in a massive simplification.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/ptrace.c     |  1 +
 kernel/signal.c     | 14 ++++++++++----
 virt/kvm/kvm_main.c |  8 --------
 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 6f357f4fc859..6507d700d70f 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -968,6 +968,7 @@ int ptrace_request(struct task_struct *child, long request,
 		 */
 		spin_lock_irq(&child->sighand->siglock);
 		child->blocked = new_set;
+		child->real_blocked = new_set;
 		spin_unlock_irq(&child->sighand->siglock);
 
 		clear_tsk_restore_sigmask(child);
diff --git a/kernel/signal.c b/kernel/signal.c
index bfa36320a4f7..fcd84f4a93c9 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2878,6 +2878,9 @@ void set_current_blocked(sigset_t *newset)
 {
 	sigdelsetmask(newset, sigmask(SIGKILL) | sigmask(SIGSTOP));
 	__set_current_blocked(newset);
+
+	/* Lockless, only current can change ->real_blocked, never from irq */
+	current->real_blocked = *newset;
 }
 
 void __set_current_blocked(const sigset_t *newset)
@@ -2928,15 +2931,20 @@ int sigprocmask(int how, sigset_t *set, sigset_t *oldset)
 	}
 
 	__set_current_blocked(&newset);
+
+	/* Lockless, only current can change ->real_blocked, never from irq */
+	tsk->real_blocked = newset;
 	return 0;
 }
 EXPORT_SYMBOL(sigprocmask);
 
-static int set_sigmask(sigset_t *kmask)
+static int set_sigmask(sigset_t *newset)
 {
 	set_restore_sigmask();
 	current->saved_sigmask = current->blocked;
-	set_current_blocked(kmask);
+
+	sigdelsetmask(newset, sigmask(SIGKILL) | sigmask(SIGSTOP));
+	__set_current_blocked(newset);
 
 	return 0;
 }
@@ -3440,7 +3448,6 @@ static int do_sigtimedwait(const sigset_t *which, kernel_siginfo_t *info,
 		 * they arrive. Unblocking is always fine, we can avoid
 		 * set_current_blocked().
 		 */
-		tsk->real_blocked = tsk->blocked;
 		sigandsets(&tsk->blocked, &tsk->blocked, &mask);
 		recalc_sigpending();
 		spin_unlock_irq(&tsk->sighand->siglock);
@@ -3450,7 +3457,6 @@ static int do_sigtimedwait(const sigset_t *which, kernel_siginfo_t *info,
 							 HRTIMER_MODE_REL);
 		spin_lock_irq(&tsk->sighand->siglock);
 		__set_task_blocked(tsk, &tsk->real_blocked);
-		sigemptyset(&tsk->real_blocked);
 		sig = dequeue_signal(tsk, &mask, info);
 	}
 	spin_unlock_irq(&tsk->sighand->siglock);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 8575a1010bfc..4bfed018574a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2234,13 +2234,6 @@ void kvm_sigset_activate(struct kvm_vcpu *vcpu)
 	if (!vcpu->sigset_active)
 		return;
 
-	/*
-	 * This does a lockless modification of ->real_blocked, which is fine
-	 * because, only current can change ->real_blocked and all readers of
-	 * ->real_blocked don't care as long ->real_blocked is always a subset
-	 * of ->blocked.
-	 */
-	current->real_blocked = current->blocked;
 	__set_current_blocked(&vcpu->sigset);
 }
 
@@ -2250,7 +2243,6 @@ void kvm_sigset_deactivate(struct kvm_vcpu *vcpu)
 		return;
 
 	__set_current_blocked(&current->real_blocked);
-	sigemptyset(&current->real_blocked);
 }
 
 static void grow_halt_poll_ns(struct kvm_vcpu *vcpu)
-- 
2.21.0.dirty

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

[RFC PATCH 3/5] signal: Always keep real_blocked in sync with blocked

[Eric W. Biederman]

Except where we temporarily override blocked always keep real_blocked
in sync with blocked.

By always setting real_blocked when we set blocked this allows
some slight efficiency and simplifications, by not having
to save blocked.

This also preparse the code for the removal of saved_sigmask.  That
should result in a massive simplification.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/ptrace.c     |  1 +
 kernel/signal.c     | 14 ++++++++++----
 virt/kvm/kvm_main.c |  8 --------
 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 6f357f4fc859..6507d700d70f 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -968,6 +968,7 @@ int ptrace_request(struct task_struct *child, long request,
 		 */
 		spin_lock_irq(&child->sighand->siglock);
 		child->blocked = new_set;
+		child->real_blocked = new_set;
 		spin_unlock_irq(&child->sighand->siglock);
 
 		clear_tsk_restore_sigmask(child);
diff --git a/kernel/signal.c b/kernel/signal.c
index bfa36320a4f7..fcd84f4a93c9 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2878,6 +2878,9 @@ void set_current_blocked(sigset_t *newset)
 {
 	sigdelsetmask(newset, sigmask(SIGKILL) | sigmask(SIGSTOP));
 	__set_current_blocked(newset);
+
+	/* Lockless, only current can change ->real_blocked, never from irq */
+	current->real_blocked = *newset;
 }
 
 void __set_current_blocked(const sigset_t *newset)
@@ -2928,15 +2931,20 @@ int sigprocmask(int how, sigset_t *set, sigset_t *oldset)
 	}
 
 	__set_current_blocked(&newset);
+
+	/* Lockless, only current can change ->real_blocked, never from irq */
+	tsk->real_blocked = newset;
 	return 0;
 }
 EXPORT_SYMBOL(sigprocmask);
 
-static int set_sigmask(sigset_t *kmask)
+static int set_sigmask(sigset_t *newset)
 {
 	set_restore_sigmask();
 	current->saved_sigmask = current->blocked;
-	set_current_blocked(kmask);
+
+	sigdelsetmask(newset, sigmask(SIGKILL) | sigmask(SIGSTOP));
+	__set_current_blocked(newset);
 
 	return 0;
 }
@@ -3440,7 +3448,6 @@ static int do_sigtimedwait(const sigset_t *which, kernel_siginfo_t *info,
 		 * they arrive. Unblocking is always fine, we can avoid
 		 * set_current_blocked().
 		 */
-		tsk->real_blocked = tsk->blocked;
 		sigandsets(&tsk->blocked, &tsk->blocked, &mask);
 		recalc_sigpending();
 		spin_unlock_irq(&tsk->sighand->siglock);
@@ -3450,7 +3457,6 @@ static int do_sigtimedwait(const sigset_t *which, kernel_siginfo_t *info,
 							 HRTIMER_MODE_REL);
 		spin_lock_irq(&tsk->sighand->siglock);
 		__set_task_blocked(tsk, &tsk->real_blocked);
-		sigemptyset(&tsk->real_blocked);
 		sig = dequeue_signal(tsk, &mask, info);
 	}
 	spin_unlock_irq(&tsk->sighand->siglock);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 8575a1010bfc..4bfed018574a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2234,13 +2234,6 @@ void kvm_sigset_activate(struct kvm_vcpu *vcpu)
 	if (!vcpu->sigset_active)
 		return;
 
-	/*
-	 * This does a lockless modification of ->real_blocked, which is fine
-	 * because, only current can change ->real_blocked and all readers of
-	 * ->real_blocked don't care as long ->real_blocked is always a subset
-	 * of ->blocked.
-	 */
-	current->real_blocked = current->blocked;
 	__set_current_blocked(&vcpu->sigset);
 }
 
@@ -2250,7 +2243,6 @@ void kvm_sigset_deactivate(struct kvm_vcpu *vcpu)
 		return;
 
 	__set_current_blocked(&current->real_blocked);
-	sigemptyset(&current->real_blocked);
 }
 
 static void grow_halt_poll_ns(struct kvm_vcpu *vcpu)
-- 
2.21.0.dirty

[RFC PATCH 4/5] signal: Remove saved_sigmask

[Eric W. Biederman]

We have real_sigmask that serves the same purpose and is always kept
uptodate.  Remove a field from the task structure and a conditional
from the signal delivery code by removing this field.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 include/linux/sched.h        |  2 --
 include/linux/sched/signal.h |  9 +++------
 kernel/ptrace.c              | 12 ++----------
 kernel/signal.c              |  2 --
 4 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 11837410690f..520efbd355be 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -864,8 +864,6 @@ struct task_struct {
 	struct sighand_struct		*sighand;
 	sigset_t			blocked;
 	sigset_t			real_blocked;
-	/* Restored if set_restore_sigmask() was used: */
-	sigset_t			saved_sigmask;
 	struct sigpending		pending;
 	unsigned long			sas_ss_sp;
 	size_t				sas_ss_size;
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 97f6a6a35a81..78678de45278 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -407,7 +407,7 @@ void task_join_group_stop(struct task_struct *task);
  */
 
 /**
- * set_restore_sigmask() - make sure saved_sigmask processing gets done
+ * set_restore_sigmask() - make sure real_sigmask processing gets done
  *
  * This sets TIF_RESTORE_SIGMASK and ensures that the arch signal code
  * will run before returning to user mode, to process the flag.  For
@@ -479,7 +479,7 @@ static inline bool test_and_clear_restore_sigmask(void)
 static inline void restore_saved_sigmask(void)
 {
 	if (test_and_clear_restore_sigmask())
-		__set_current_blocked(&current->saved_sigmask);
+		__set_current_blocked(&current->real_blocked);
 }
 
 extern int set_user_sigmask(const sigset_t __user *umask, size_t sigsetsize);
@@ -494,10 +494,7 @@ static inline void restore_saved_sigmask_unless(bool interrupted)
 
 static inline sigset_t *sigmask_to_save(void)
 {
-	sigset_t *res = &current->blocked;
-	if (unlikely(test_restore_sigmask()))
-		res = &current->saved_sigmask;
-	return res;
+	return &current->real_blocked;
 }
 
 static inline int kill_cad_pid(int sig, int priv)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 6507d700d70f..5ed6126e1cc5 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -925,26 +925,18 @@ int ptrace_request(struct task_struct *child, long request,
 			ret = ptrace_setsiginfo(child, &siginfo);
 		break;
 
-	case PTRACE_GETSIGMASK: {
-		sigset_t *mask;
-
+	case PTRACE_GETSIGMASK:
 		if (addr != sizeof(sigset_t)) {
 			ret = -EINVAL;
 			break;
 		}
 
-		if (test_tsk_restore_sigmask(child))
-			mask = &child->saved_sigmask;
-		else
-			mask = &child->blocked;
-
-		if (copy_to_user(datavp, mask, sizeof(sigset_t)))
+		if (copy_to_user(datavp, &child->real_blocked, sizeof(sigset_t)))
 			ret = -EFAULT;
 		else
 			ret = 0;
 
 		break;
-	}
 
 	case PTRACE_SETSIGMASK: {
 		sigset_t new_set;
diff --git a/kernel/signal.c b/kernel/signal.c
index fcd84f4a93c9..c37d4f209699 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2941,8 +2941,6 @@ EXPORT_SYMBOL(sigprocmask);
 static int set_sigmask(sigset_t *newset)
 {
 	set_restore_sigmask();
-	current->saved_sigmask = current->blocked;
-
 	sigdelsetmask(newset, sigmask(SIGKILL) | sigmask(SIGSTOP));
 	__set_current_blocked(newset);
 
-- 
2.21.0.dirty

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

[RFC PATCH 4/5] signal: Remove saved_sigmask

[Eric W. Biederman]

We have real_sigmask that serves the same purpose and is always kept
uptodate.  Remove a field from the task structure and a conditional
from the signal delivery code by removing this field.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 include/linux/sched.h        |  2 --
 include/linux/sched/signal.h |  9 +++------
 kernel/ptrace.c              | 12 ++----------
 kernel/signal.c              |  2 --
 4 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 11837410690f..520efbd355be 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -864,8 +864,6 @@ struct task_struct {
 	struct sighand_struct		*sighand;
 	sigset_t			blocked;
 	sigset_t			real_blocked;
-	/* Restored if set_restore_sigmask() was used: */
-	sigset_t			saved_sigmask;
 	struct sigpending		pending;
 	unsigned long			sas_ss_sp;
 	size_t				sas_ss_size;
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 97f6a6a35a81..78678de45278 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -407,7 +407,7 @@ void task_join_group_stop(struct task_struct *task);
  */
 
 /**
- * set_restore_sigmask() - make sure saved_sigmask processing gets done
+ * set_restore_sigmask() - make sure real_sigmask processing gets done
  *
  * This sets TIF_RESTORE_SIGMASK and ensures that the arch signal code
  * will run before returning to user mode, to process the flag.  For
@@ -479,7 +479,7 @@ static inline bool test_and_clear_restore_sigmask(void)
 static inline void restore_saved_sigmask(void)
 {
 	if (test_and_clear_restore_sigmask())
-		__set_current_blocked(&current->saved_sigmask);
+		__set_current_blocked(&current->real_blocked);
 }
 
 extern int set_user_sigmask(const sigset_t __user *umask, size_t sigsetsize);
@@ -494,10 +494,7 @@ static inline void restore_saved_sigmask_unless(bool interrupted)
 
 static inline sigset_t *sigmask_to_save(void)
 {
-	sigset_t *res = &current->blocked;
-	if (unlikely(test_restore_sigmask()))
-		res = &current->saved_sigmask;
-	return res;
+	return &current->real_blocked;
 }
 
 static inline int kill_cad_pid(int sig, int priv)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 6507d700d70f..5ed6126e1cc5 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -925,26 +925,18 @@ int ptrace_request(struct task_struct *child, long request,
 			ret = ptrace_setsiginfo(child, &siginfo);
 		break;
 
-	case PTRACE_GETSIGMASK: {
-		sigset_t *mask;
-
+	case PTRACE_GETSIGMASK:
 		if (addr != sizeof(sigset_t)) {
 			ret = -EINVAL;
 			break;
 		}
 
-		if (test_tsk_restore_sigmask(child))
-			mask = &child->saved_sigmask;
-		else
-			mask = &child->blocked;
-
-		if (copy_to_user(datavp, mask, sizeof(sigset_t)))
+		if (copy_to_user(datavp, &child->real_blocked, sizeof(sigset_t)))
 			ret = -EFAULT;
 		else
 			ret = 0;
 
 		break;
-	}
 
 	case PTRACE_SETSIGMASK: {
 		sigset_t new_set;
diff --git a/kernel/signal.c b/kernel/signal.c
index fcd84f4a93c9..c37d4f209699 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2941,8 +2941,6 @@ EXPORT_SYMBOL(sigprocmask);
 static int set_sigmask(sigset_t *newset)
 {
 	set_restore_sigmask();
-	current->saved_sigmask = current->blocked;
-
 	sigdelsetmask(newset, sigmask(SIGKILL) | sigmask(SIGSTOP));
 	__set_current_blocked(newset);
 
-- 
2.21.0.dirty

[RFC PATCH 5/5] signal: Remove the unnecessary restore_sigmask flag

[Eric W. Biederman]

__set_current_blocked already has a test to see if the new
value and the old value of the signal mask differ.  Both
real_blocked and blocked should be on the same cache line,
and are a single word compare on 64bit.

Historically real_blocked or saved_sigmask could be garbage
so a test had to be used that always tested valid bits.
Now that real_blocked is alwasy valid there is no need
to test something different.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/arc/include/asm/thread_info.h       |  1 -
 arch/arm/include/asm/thread_info.h       |  1 -
 arch/arm64/include/asm/thread_info.h     |  1 -
 arch/c6x/include/asm/thread_info.h       |  1 -
 arch/csky/include/asm/thread_info.h      |  2 -
 arch/h8300/include/asm/thread_info.h     |  1 -
 arch/hexagon/include/asm/thread_info.h   |  1 -
 arch/m68k/include/asm/thread_info.h      |  1 -
 arch/mips/include/asm/thread_info.h      |  1 -
 arch/nds32/include/asm/thread_info.h     |  2 -
 arch/nios2/include/asm/thread_info.h     |  2 -
 arch/riscv/include/asm/thread_info.h     |  1 -
 arch/s390/include/asm/thread_info.h      |  1 -
 arch/sparc/include/asm/thread_info_32.h  |  1 -
 arch/um/include/asm/thread_info.h        |  1 -
 arch/unicore32/include/asm/thread_info.h |  1 -
 arch/xtensa/include/asm/thread_info.h    |  1 -
 include/linux/sched.h                    |  3 -
 include/linux/sched/signal.h             | 79 +-----------------------
 kernel/ptrace.c                          |  2 -
 kernel/signal.c                          |  7 ---
 21 files changed, 1 insertion(+), 110 deletions(-)

diff --git a/arch/arc/include/asm/thread_info.h b/arch/arc/include/asm/thread_info.h
index c85947bac5e5..bde0f76a986b 100644
--- a/arch/arc/include/asm/thread_info.h
+++ b/arch/arc/include/asm/thread_info.h
@@ -77,7 +77,6 @@ static inline __attribute_const__ struct thread_info *current_thread_info(void)
  * - pending work-to-be-done flags are in LSW
  * - other flags in MSW
  */
-#define TIF_RESTORE_SIGMASK	0	/* restore sig mask in do_signal() */
 #define TIF_NOTIFY_RESUME	1	/* resumption notification requested */
 #define TIF_SIGPENDING		2	/* signal pending */
 #define TIF_NEED_RESCHED	3	/* rescheduling necessary */
diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index 286eb61c632b..d1ee97031c21 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -147,7 +147,6 @@ extern int vfp_restore_user_hwstate(struct user_vfp *,
 #define TIF_NOHZ		12	/* in adaptive nohz mode */
 #define TIF_USING_IWMMXT	17
 #define TIF_MEMDIE		18	/* is terminating due to OOM killer */
-#define TIF_RESTORE_SIGMASK	20
 
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
 #define _TIF_NEED_RESCHED	(1 << TIF_NEED_RESCHED)
diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index eb3ef73e07cf..bb8fdc9f78d2 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -93,7 +93,6 @@ void arch_release_task_struct(struct task_struct *tsk);
 #define TIF_SECCOMP		11
 #define TIF_MEMDIE		18	/* is terminating due to OOM killer */
 #define TIF_FREEZE		19
-#define TIF_RESTORE_SIGMASK	20
 #define TIF_SINGLESTEP		21
 #define TIF_32BIT		22	/* 32bit process */
 #define TIF_SVE			23	/* Scalable Vector Extension in use */
diff --git a/arch/c6x/include/asm/thread_info.h b/arch/c6x/include/asm/thread_info.h
index 59a5697fe0f3..229d365ffa9d 100644
--- a/arch/c6x/include/asm/thread_info.h
+++ b/arch/c6x/include/asm/thread_info.h
@@ -84,7 +84,6 @@ struct thread_info *current_thread_info(void)
 #define TIF_NOTIFY_RESUME	1	/* resumption notification requested */
 #define TIF_SIGPENDING		2	/* signal pending */
 #define TIF_NEED_RESCHED	3	/* rescheduling necessary */
-#define TIF_RESTORE_SIGMASK	4	/* restore signal mask in do_signal() */
 
 #define TIF_MEMDIE		17	/* OOM killer killed process */
 
diff --git a/arch/csky/include/asm/thread_info.h b/arch/csky/include/asm/thread_info.h
index 0b546a55a8bf..f7261865e9dc 100644
--- a/arch/csky/include/asm/thread_info.h
+++ b/arch/csky/include/asm/thread_info.h
@@ -59,7 +59,6 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_SYSCALL_AUDIT	5	/* syscall auditing */
 #define TIF_POLLING_NRFLAG	16	/* poll_idle() is TIF_NEED_RESCHED */
 #define TIF_MEMDIE		18      /* is terminating due to OOM killer */
-#define TIF_RESTORE_SIGMASK	20	/* restore signal mask in do_signal() */
 #define TIF_SECCOMP		21	/* secure computing */
 
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
@@ -70,7 +69,6 @@ static inline struct thread_info *current_thread_info(void)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
 #define _TIF_POLLING_NRFLAG	(1 << TIF_POLLING_NRFLAG)
 #define _TIF_MEMDIE		(1 << TIF_MEMDIE)
-#define _TIF_RESTORE_SIGMASK	(1 << TIF_RESTORE_SIGMASK)
 #define _TIF_SECCOMP		(1 << TIF_SECCOMP)
 
 #endif	/* _ASM_CSKY_THREAD_INFO_H */
diff --git a/arch/h8300/include/asm/thread_info.h b/arch/h8300/include/asm/thread_info.h
index 0cdaa302d3d2..8d5d5063628a 100644
--- a/arch/h8300/include/asm/thread_info.h
+++ b/arch/h8300/include/asm/thread_info.h
@@ -68,7 +68,6 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_NEED_RESCHED	2	/* rescheduling necessary */
 #define TIF_SINGLESTEP		3	/* singlestepping active */
 #define TIF_MEMDIE		4	/* is terminating due to OOM killer */
-#define TIF_RESTORE_SIGMASK	5	/* restore signal mask in do_signal() */
 #define TIF_NOTIFY_RESUME	6	/* callback before returning to user */
 #define TIF_SYSCALL_AUDIT	7	/* syscall auditing active */
 #define TIF_SYSCALL_TRACEPOINT	8	/* for ftrace syscall instrumentation */
diff --git a/arch/hexagon/include/asm/thread_info.h b/arch/hexagon/include/asm/thread_info.h
index f41f9c6f0e31..2d1183690bc5 100644
--- a/arch/hexagon/include/asm/thread_info.h
+++ b/arch/hexagon/include/asm/thread_info.h
@@ -107,7 +107,6 @@ register struct thread_info *__current_thread_info asm(QUOTED_THREADINFO_REG);
 #define TIF_SIGPENDING          2       /* signal pending */
 #define TIF_NEED_RESCHED        3       /* rescheduling necessary */
 #define TIF_SINGLESTEP          4       /* restore ss @ return to usr mode */
-#define TIF_RESTORE_SIGMASK     6       /* restore sig mask in do_signal() */
 /* true if poll_idle() is polling TIF_NEED_RESCHED */
 #define TIF_MEMDIE              17      /* OOM killer killed process */
 
diff --git a/arch/m68k/include/asm/thread_info.h b/arch/m68k/include/asm/thread_info.h
index 015f1ca38305..d620cb4f240e 100644
--- a/arch/m68k/include/asm/thread_info.h
+++ b/arch/m68k/include/asm/thread_info.h
@@ -66,6 +66,5 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_DELAYED_TRACE	14	/* single step a syscall */
 #define TIF_SYSCALL_TRACE	15	/* syscall trace active */
 #define TIF_MEMDIE		16	/* is terminating due to OOM killer */
-#define TIF_RESTORE_SIGMASK	18	/* restore signal mask in do_signal */
 
 #endif	/* _ASM_M68K_THREAD_INFO_H */
diff --git a/arch/mips/include/asm/thread_info.h b/arch/mips/include/asm/thread_info.h
index 4993db40482c..b2327b51c141 100644
--- a/arch/mips/include/asm/thread_info.h
+++ b/arch/mips/include/asm/thread_info.h
@@ -97,7 +97,6 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_SECCOMP		4	/* secure computing */
 #define TIF_NOTIFY_RESUME	5	/* callback before returning to user */
 #define TIF_UPROBE		6	/* breakpointed or singlestepping */
-#define TIF_RESTORE_SIGMASK	9	/* restore signal mask in do_signal() */
 #define TIF_USEDFPU		16	/* FPU was used by this task this quantum (SMP) */
 #define TIF_MEMDIE		18	/* is terminating due to OOM killer */
 #define TIF_NOHZ		19	/* in adaptive nohz mode */
diff --git a/arch/nds32/include/asm/thread_info.h b/arch/nds32/include/asm/thread_info.h
index c135111ec44e..79914ce56cd9 100644
--- a/arch/nds32/include/asm/thread_info.h
+++ b/arch/nds32/include/asm/thread_info.h
@@ -52,7 +52,6 @@ struct thread_info {
 #define TIF_POLLING_NRFLAG	17
 #define TIF_MEMDIE		18
 #define TIF_FREEZE		19
-#define TIF_RESTORE_SIGMASK	20
 
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
 #define _TIF_NEED_RESCHED	(1 << TIF_NEED_RESCHED)
@@ -61,7 +60,6 @@ struct thread_info {
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_POLLING_NRFLAG	(1 << TIF_POLLING_NRFLAG)
 #define _TIF_FREEZE		(1 << TIF_FREEZE)
-#define _TIF_RESTORE_SIGMASK	(1 << TIF_RESTORE_SIGMASK)
 
 /*
  * Change these and you break ASM code in entry-common.S
diff --git a/arch/nios2/include/asm/thread_info.h b/arch/nios2/include/asm/thread_info.h
index 7349a4fa635b..b17309c3a5b3 100644
--- a/arch/nios2/include/asm/thread_info.h
+++ b/arch/nios2/include/asm/thread_info.h
@@ -86,7 +86,6 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_MEMDIE		4	/* is terminating due to OOM killer */
 #define TIF_SECCOMP		5	/* secure computing */
 #define TIF_SYSCALL_AUDIT	6	/* syscall auditing active */
-#define TIF_RESTORE_SIGMASK	9	/* restore signal mask in do_signal() */
 
 #define TIF_POLLING_NRFLAG	16	/* true if poll_idle() is polling
 					   TIF_NEED_RESCHED */
@@ -97,7 +96,6 @@ static inline struct thread_info *current_thread_info(void)
 #define _TIF_NEED_RESCHED	(1 << TIF_NEED_RESCHED)
 #define _TIF_SECCOMP		(1 << TIF_SECCOMP)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
-#define _TIF_RESTORE_SIGMASK	(1 << TIF_RESTORE_SIGMASK)
 #define _TIF_POLLING_NRFLAG	(1 << TIF_POLLING_NRFLAG)
 
 /* work to do on interrupt/exception return */
diff --git a/arch/riscv/include/asm/thread_info.h b/arch/riscv/include/asm/thread_info.h
index 9c039870019b..6a1e393b4b5f 100644
--- a/arch/riscv/include/asm/thread_info.h
+++ b/arch/riscv/include/asm/thread_info.h
@@ -79,7 +79,6 @@ struct thread_info {
 #define TIF_NOTIFY_RESUME	1	/* callback before returning to user */
 #define TIF_SIGPENDING		2	/* signal pending */
 #define TIF_NEED_RESCHED	3	/* rescheduling necessary */
-#define TIF_RESTORE_SIGMASK	4	/* restore signal mask in do_signal() */
 #define TIF_MEMDIE		5	/* is terminating due to OOM killer */
 #define TIF_SYSCALL_TRACEPOINT  6       /* syscall tracepoint instrumentation */
 #define TIF_SYSCALL_AUDIT	7	/* syscall auditing */
diff --git a/arch/s390/include/asm/thread_info.h b/arch/s390/include/asm/thread_info.h
index ce4e17c9aad6..6a1d1636016b 100644
--- a/arch/s390/include/asm/thread_info.h
+++ b/arch/s390/include/asm/thread_info.h
@@ -71,7 +71,6 @@ void arch_setup_new_exec(void);
 
 #define TIF_31BIT		16	/* 32bit process */
 #define TIF_MEMDIE		17	/* is terminating due to OOM killer */
-#define TIF_RESTORE_SIGMASK	18	/* restore signal mask in do_signal() */
 #define TIF_SINGLE_STEP		19	/* This task is single stepped */
 #define TIF_BLOCK_STEP		20	/* This task is block stepped */
 #define TIF_UPROBE_SINGLESTEP	21	/* This task is uprobe single stepped */
diff --git a/arch/sparc/include/asm/thread_info_32.h b/arch/sparc/include/asm/thread_info_32.h
index 548b366165dd..567f472c730b 100644
--- a/arch/sparc/include/asm/thread_info_32.h
+++ b/arch/sparc/include/asm/thread_info_32.h
@@ -103,7 +103,6 @@ register struct thread_info *current_thread_info_reg asm("g6");
 #define TIF_NOTIFY_RESUME	1	/* callback before returning to user */
 #define TIF_SIGPENDING		2	/* signal pending */
 #define TIF_NEED_RESCHED	3	/* rescheduling necessary */
-#define TIF_RESTORE_SIGMASK	4	/* restore signal mask in do_signal() */
 #define TIF_USEDFPU		8	/* FPU was used by this task
 					 * this quantum (SMP) */
 #define TIF_POLLING_NRFLAG	9	/* true if poll_idle() is polling
diff --git a/arch/um/include/asm/thread_info.h b/arch/um/include/asm/thread_info.h
index 4eecd960ee8c..565b7db73083 100644
--- a/arch/um/include/asm/thread_info.h
+++ b/arch/um/include/asm/thread_info.h
@@ -60,7 +60,6 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_RESTART_BLOCK	4
 #define TIF_MEMDIE		5	/* is terminating due to OOM killer */
 #define TIF_SYSCALL_AUDIT	6
-#define TIF_RESTORE_SIGMASK	7
 #define TIF_NOTIFY_RESUME	8
 #define TIF_SECCOMP		9	/* secure computing */
 
diff --git a/arch/unicore32/include/asm/thread_info.h b/arch/unicore32/include/asm/thread_info.h
index 5fb728f3b49a..0f38f6072839 100644
--- a/arch/unicore32/include/asm/thread_info.h
+++ b/arch/unicore32/include/asm/thread_info.h
@@ -119,7 +119,6 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_NOTIFY_RESUME	2	/* callback before returning to user */
 #define TIF_SYSCALL_TRACE	8
 #define TIF_MEMDIE		18
-#define TIF_RESTORE_SIGMASK	20
 
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
 #define _TIF_NEED_RESCHED	(1 << TIF_NEED_RESCHED)
diff --git a/arch/xtensa/include/asm/thread_info.h b/arch/xtensa/include/asm/thread_info.h
index f092cc3f4e66..27350241615c 100644
--- a/arch/xtensa/include/asm/thread_info.h
+++ b/arch/xtensa/include/asm/thread_info.h
@@ -108,7 +108,6 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_SINGLESTEP		3	/* restore singlestep on return to user mode */
 #define TIF_SYSCALL_TRACEPOINT	4	/* syscall tracepoint instrumentation */
 #define TIF_MEMDIE		5	/* is terminating due to OOM killer */
-#define TIF_RESTORE_SIGMASK	6	/* restore signal mask in do_signal() */
 #define TIF_NOTIFY_RESUME	7	/* callback before returning to user */
 #define TIF_DB_DISABLED		8	/* debug trap disabled for syscall */
 
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 520efbd355be..a5284f5ccb7c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -713,9 +713,6 @@ struct task_struct {
 	/* Bit to tell LSMs we're in execve(): */
 	unsigned			in_execve:1;
 	unsigned			in_iowait:1;
-#ifndef TIF_RESTORE_SIGMASK
-	unsigned			restore_sigmask:1;
-#endif
 #ifdef CONFIG_MEMCG
 	unsigned			in_user_fault:1;
 #endif
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 78678de45278..ac34566e762f 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -400,86 +400,9 @@ static inline void ptrace_signal_wake_up(struct task_struct *t, bool resume)
 
 void task_join_group_stop(struct task_struct *task);
 
-#ifdef TIF_RESTORE_SIGMASK
-/*
- * Legacy restore_sigmask accessors.  These are inefficient on
- * SMP architectures because they require atomic operations.
- */
-
-/**
- * set_restore_sigmask() - make sure real_sigmask processing gets done
- *
- * This sets TIF_RESTORE_SIGMASK and ensures that the arch signal code
- * will run before returning to user mode, to process the flag.  For
- * all callers, TIF_SIGPENDING is already set or it's no harm to set
- * it.  TIF_RESTORE_SIGMASK need not be in the set of bits that the
- * arch code will notice on return to user mode, in case those bits
- * are scarce.  We set TIF_SIGPENDING here to ensure that the arch
- * signal code always gets run when TIF_RESTORE_SIGMASK is set.
- */
-static inline void set_restore_sigmask(void)
-{
-	set_thread_flag(TIF_RESTORE_SIGMASK);
-}
-
-static inline void clear_tsk_restore_sigmask(struct task_struct *task)
-{
-	clear_tsk_thread_flag(task, TIF_RESTORE_SIGMASK);
-}
-
-static inline void clear_restore_sigmask(void)
-{
-	clear_thread_flag(TIF_RESTORE_SIGMASK);
-}
-static inline bool test_tsk_restore_sigmask(struct task_struct *task)
-{
-	return test_tsk_thread_flag(task, TIF_RESTORE_SIGMASK);
-}
-static inline bool test_restore_sigmask(void)
-{
-	return test_thread_flag(TIF_RESTORE_SIGMASK);
-}
-static inline bool test_and_clear_restore_sigmask(void)
-{
-	return test_and_clear_thread_flag(TIF_RESTORE_SIGMASK);
-}
-
-#else	/* TIF_RESTORE_SIGMASK */
-
-/* Higher-quality implementation, used if TIF_RESTORE_SIGMASK doesn't exist. */
-static inline void set_restore_sigmask(void)
-{
-	current->restore_sigmask = true;
-}
-static inline void clear_tsk_restore_sigmask(struct task_struct *task)
-{
-	task->restore_sigmask = false;
-}
-static inline void clear_restore_sigmask(void)
-{
-	current->restore_sigmask = false;
-}
-static inline bool test_restore_sigmask(void)
-{
-	return current->restore_sigmask;
-}
-static inline bool test_tsk_restore_sigmask(struct task_struct *task)
-{
-	return task->restore_sigmask;
-}
-static inline bool test_and_clear_restore_sigmask(void)
-{
-	if (!current->restore_sigmask)
-		return false;
-	current->restore_sigmask = false;
-	return true;
-}
-#endif
-
 static inline void restore_saved_sigmask(void)
 {
-	if (test_and_clear_restore_sigmask())
-		__set_current_blocked(&current->real_blocked);
+	__set_current_blocked(&current->real_blocked);
 }
 
 extern int set_user_sigmask(const sigset_t __user *umask, size_t sigsetsize);
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 5ed6126e1cc5..08e25bbb04f4 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -963,8 +963,6 @@ int ptrace_request(struct task_struct *child, long request,
 		child->real_blocked = new_set;
 		spin_unlock_irq(&child->sighand->siglock);
 
-		clear_tsk_restore_sigmask(child);
-
 		ret = 0;
 		break;
 	}
diff --git a/kernel/signal.c b/kernel/signal.c
index c37d4f209699..1358e6bf02d3 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2734,12 +2734,6 @@ static void signal_delivered(struct ksignal *ksig, int stepping)
 {
 	sigset_t blocked;
 
-	/* A signal was successfully delivered, and the
-	   saved sigmask was stored on the signal frame,
-	   and will be restored by sigreturn.  So we can
-	   simply clear the restore sigmask flag.  */
-	clear_restore_sigmask();
-
 	sigorsets(&blocked, &current->blocked, &ksig->ka.sa.sa_mask);
 	if (!(ksig->ka.sa.sa_flags & SA_NODEFER))
 		sigaddset(&blocked, ksig->sig);
@@ -2940,7 +2934,6 @@ EXPORT_SYMBOL(sigprocmask);
 
 static int set_sigmask(sigset_t *newset)
 {
-	set_restore_sigmask();
 	sigdelsetmask(newset, sigmask(SIGKILL) | sigmask(SIGSTOP));
 	__set_current_blocked(newset);
 
-- 
2.21.0.dirty

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

[RFC PATCH 5/5] signal: Remove the unnecessary restore_sigmask flag

[Eric W. Biederman]

__set_current_blocked already has a test to see if the new
value and the old value of the signal mask differ.  Both
real_blocked and blocked should be on the same cache line,
and are a single word compare on 64bit.

Historically real_blocked or saved_sigmask could be garbage
so a test had to be used that always tested valid bits.
Now that real_blocked is alwasy valid there is no need
to test something different.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/arc/include/asm/thread_info.h       |  1 -
 arch/arm/include/asm/thread_info.h       |  1 -
 arch/arm64/include/asm/thread_info.h     |  1 -
 arch/c6x/include/asm/thread_info.h       |  1 -
 arch/csky/include/asm/thread_info.h      |  2 -
 arch/h8300/include/asm/thread_info.h     |  1 -
 arch/hexagon/include/asm/thread_info.h   |  1 -
 arch/m68k/include/asm/thread_info.h      |  1 -
 arch/mips/include/asm/thread_info.h      |  1 -
 arch/nds32/include/asm/thread_info.h     |  2 -
 arch/nios2/include/asm/thread_info.h     |  2 -
 arch/riscv/include/asm/thread_info.h     |  1 -
 arch/s390/include/asm/thread_info.h      |  1 -
 arch/sparc/include/asm/thread_info_32.h  |  1 -
 arch/um/include/asm/thread_info.h        |  1 -
 arch/unicore32/include/asm/thread_info.h |  1 -
 arch/xtensa/include/asm/thread_info.h    |  1 -
 include/linux/sched.h                    |  3 -
 include/linux/sched/signal.h             | 79 +-----------------------
 kernel/ptrace.c                          |  2 -
 kernel/signal.c                          |  7 ---
 21 files changed, 1 insertion(+), 110 deletions(-)

diff --git a/arch/arc/include/asm/thread_info.h b/arch/arc/include/asm/thread_info.h
index c85947bac5e5..bde0f76a986b 100644
--- a/arch/arc/include/asm/thread_info.h
+++ b/arch/arc/include/asm/thread_info.h
@@ -77,7 +77,6 @@ static inline __attribute_const__ struct thread_info *current_thread_info(void)
  * - pending work-to-be-done flags are in LSW
  * - other flags in MSW
  */
-#define TIF_RESTORE_SIGMASK	0	/* restore sig mask in do_signal() */
 #define TIF_NOTIFY_RESUME	1	/* resumption notification requested */
 #define TIF_SIGPENDING		2	/* signal pending */
 #define TIF_NEED_RESCHED	3	/* rescheduling necessary */
diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index 286eb61c632b..d1ee97031c21 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -147,7 +147,6 @@ extern int vfp_restore_user_hwstate(struct user_vfp *,
 #define TIF_NOHZ		12	/* in adaptive nohz mode */
 #define TIF_USING_IWMMXT	17
 #define TIF_MEMDIE		18	/* is terminating due to OOM killer */
-#define TIF_RESTORE_SIGMASK	20
 
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
 #define _TIF_NEED_RESCHED	(1 << TIF_NEED_RESCHED)
diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index eb3ef73e07cf..bb8fdc9f78d2 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -93,7 +93,6 @@ void arch_release_task_struct(struct task_struct *tsk);
 #define TIF_SECCOMP		11
 #define TIF_MEMDIE		18	/* is terminating due to OOM killer */
 #define TIF_FREEZE		19
-#define TIF_RESTORE_SIGMASK	20
 #define TIF_SINGLESTEP		21
 #define TIF_32BIT		22	/* 32bit process */
 #define TIF_SVE			23	/* Scalable Vector Extension in use */
diff --git a/arch/c6x/include/asm/thread_info.h b/arch/c6x/include/asm/thread_info.h
index 59a5697fe0f3..229d365ffa9d 100644
--- a/arch/c6x/include/asm/thread_info.h
+++ b/arch/c6x/include/asm/thread_info.h
@@ -84,7 +84,6 @@ struct thread_info *current_thread_info(void)
 #define TIF_NOTIFY_RESUME	1	/* resumption notification requested */
 #define TIF_SIGPENDING		2	/* signal pending */
 #define TIF_NEED_RESCHED	3	/* rescheduling necessary */
-#define TIF_RESTORE_SIGMASK	4	/* restore signal mask in do_signal() */
 
 #define TIF_MEMDIE		17	/* OOM killer killed process */
 
diff --git a/arch/csky/include/asm/thread_info.h b/arch/csky/include/asm/thread_info.h
index 0b546a55a8bf..f7261865e9dc 100644
--- a/arch/csky/include/asm/thread_info.h
+++ b/arch/csky/include/asm/thread_info.h
@@ -59,7 +59,6 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_SYSCALL_AUDIT	5	/* syscall auditing */
 #define TIF_POLLING_NRFLAG	16	/* poll_idle() is TIF_NEED_RESCHED */
 #define TIF_MEMDIE		18      /* is terminating due to OOM killer */
-#define TIF_RESTORE_SIGMASK	20	/* restore signal mask in do_signal() */
 #define TIF_SECCOMP		21	/* secure computing */
 
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
@@ -70,7 +69,6 @@ static inline struct thread_info *current_thread_info(void)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
 #define _TIF_POLLING_NRFLAG	(1 << TIF_POLLING_NRFLAG)
 #define _TIF_MEMDIE		(1 << TIF_MEMDIE)
-#define _TIF_RESTORE_SIGMASK	(1 << TIF_RESTORE_SIGMASK)
 #define _TIF_SECCOMP		(1 << TIF_SECCOMP)
 
 #endif	/* _ASM_CSKY_THREAD_INFO_H */
diff --git a/arch/h8300/include/asm/thread_info.h b/arch/h8300/include/asm/thread_info.h
index 0cdaa302d3d2..8d5d5063628a 100644
--- a/arch/h8300/include/asm/thread_info.h
+++ b/arch/h8300/include/asm/thread_info.h
@@ -68,7 +68,6 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_NEED_RESCHED	2	/* rescheduling necessary */
 #define TIF_SINGLESTEP		3	/* singlestepping active */
 #define TIF_MEMDIE		4	/* is terminating due to OOM killer */
-#define TIF_RESTORE_SIGMASK	5	/* restore signal mask in do_signal() */
 #define TIF_NOTIFY_RESUME	6	/* callback before returning to user */
 #define TIF_SYSCALL_AUDIT	7	/* syscall auditing active */
 #define TIF_SYSCALL_TRACEPOINT	8	/* for ftrace syscall instrumentation */
diff --git a/arch/hexagon/include/asm/thread_info.h b/arch/hexagon/include/asm/thread_info.h
index f41f9c6f0e31..2d1183690bc5 100644
--- a/arch/hexagon/include/asm/thread_info.h
+++ b/arch/hexagon/include/asm/thread_info.h
@@ -107,7 +107,6 @@ register struct thread_info *__current_thread_info asm(QUOTED_THREADINFO_REG);
 #define TIF_SIGPENDING          2       /* signal pending */
 #define TIF_NEED_RESCHED        3       /* rescheduling necessary */
 #define TIF_SINGLESTEP          4       /* restore ss @ return to usr mode */
-#define TIF_RESTORE_SIGMASK     6       /* restore sig mask in do_signal() */
 /* true if poll_idle() is polling TIF_NEED_RESCHED */
 #define TIF_MEMDIE              17      /* OOM killer killed process */
 
diff --git a/arch/m68k/include/asm/thread_info.h b/arch/m68k/include/asm/thread_info.h
index 015f1ca38305..d620cb4f240e 100644
--- a/arch/m68k/include/asm/thread_info.h
+++ b/arch/m68k/include/asm/thread_info.h
@@ -66,6 +66,5 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_DELAYED_TRACE	14	/* single step a syscall */
 #define TIF_SYSCALL_TRACE	15	/* syscall trace active */
 #define TIF_MEMDIE		16	/* is terminating due to OOM killer */
-#define TIF_RESTORE_SIGMASK	18	/* restore signal mask in do_signal */
 
 #endif	/* _ASM_M68K_THREAD_INFO_H */
diff --git a/arch/mips/include/asm/thread_info.h b/arch/mips/include/asm/thread_info.h
index 4993db40482c..b2327b51c141 100644
--- a/arch/mips/include/asm/thread_info.h
+++ b/arch/mips/include/asm/thread_info.h
@@ -97,7 +97,6 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_SECCOMP		4	/* secure computing */
 #define TIF_NOTIFY_RESUME	5	/* callback before returning to user */
 #define TIF_UPROBE		6	/* breakpointed or singlestepping */
-#define TIF_RESTORE_SIGMASK	9	/* restore signal mask in do_signal() */
 #define TIF_USEDFPU		16	/* FPU was used by this task this quantum (SMP) */
 #define TIF_MEMDIE		18	/* is terminating due to OOM killer */
 #define TIF_NOHZ		19	/* in adaptive nohz mode */
diff --git a/arch/nds32/include/asm/thread_info.h b/arch/nds32/include/asm/thread_info.h
index c135111ec44e..79914ce56cd9 100644
--- a/arch/nds32/include/asm/thread_info.h
+++ b/arch/nds32/include/asm/thread_info.h
@@ -52,7 +52,6 @@ struct thread_info {
 #define TIF_POLLING_NRFLAG	17
 #define TIF_MEMDIE		18
 #define TIF_FREEZE		19
-#define TIF_RESTORE_SIGMASK	20
 
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
 #define _TIF_NEED_RESCHED	(1 << TIF_NEED_RESCHED)
@@ -61,7 +60,6 @@ struct thread_info {
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_POLLING_NRFLAG	(1 << TIF_POLLING_NRFLAG)
 #define _TIF_FREEZE		(1 << TIF_FREEZE)
-#define _TIF_RESTORE_SIGMASK	(1 << TIF_RESTORE_SIGMASK)
 
 /*
  * Change these and you break ASM code in entry-common.S
diff --git a/arch/nios2/include/asm/thread_info.h b/arch/nios2/include/asm/thread_info.h
index 7349a4fa635b..b17309c3a5b3 100644
--- a/arch/nios2/include/asm/thread_info.h
+++ b/arch/nios2/include/asm/thread_info.h
@@ -86,7 +86,6 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_MEMDIE		4	/* is terminating due to OOM killer */
 #define TIF_SECCOMP		5	/* secure computing */
 #define TIF_SYSCALL_AUDIT	6	/* syscall auditing active */
-#define TIF_RESTORE_SIGMASK	9	/* restore signal mask in do_signal() */
 
 #define TIF_POLLING_NRFLAG	16	/* true if poll_idle() is polling
 					   TIF_NEED_RESCHED */
@@ -97,7 +96,6 @@ static inline struct thread_info *current_thread_info(void)
 #define _TIF_NEED_RESCHED	(1 << TIF_NEED_RESCHED)
 #define _TIF_SECCOMP		(1 << TIF_SECCOMP)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
-#define _TIF_RESTORE_SIGMASK	(1 << TIF_RESTORE_SIGMASK)
 #define _TIF_POLLING_NRFLAG	(1 << TIF_POLLING_NRFLAG)
 
 /* work to do on interrupt/exception return */
diff --git a/arch/riscv/include/asm/thread_info.h b/arch/riscv/include/asm/thread_info.h
index 9c039870019b..6a1e393b4b5f 100644
--- a/arch/riscv/include/asm/thread_info.h
+++ b/arch/riscv/include/asm/thread_info.h
@@ -79,7 +79,6 @@ struct thread_info {
 #define TIF_NOTIFY_RESUME	1	/* callback before returning to user */
 #define TIF_SIGPENDING		2	/* signal pending */
 #define TIF_NEED_RESCHED	3	/* rescheduling necessary */
-#define TIF_RESTORE_SIGMASK	4	/* restore signal mask in do_signal() */
 #define TIF_MEMDIE		5	/* is terminating due to OOM killer */
 #define TIF_SYSCALL_TRACEPOINT  6       /* syscall tracepoint instrumentation */
 #define TIF_SYSCALL_AUDIT	7	/* syscall auditing */
diff --git a/arch/s390/include/asm/thread_info.h b/arch/s390/include/asm/thread_info.h
index ce4e17c9aad6..6a1d1636016b 100644
--- a/arch/s390/include/asm/thread_info.h
+++ b/arch/s390/include/asm/thread_info.h
@@ -71,7 +71,6 @@ void arch_setup_new_exec(void);
 
 #define TIF_31BIT		16	/* 32bit process */
 #define TIF_MEMDIE		17	/* is terminating due to OOM killer */
-#define TIF_RESTORE_SIGMASK	18	/* restore signal mask in do_signal() */
 #define TIF_SINGLE_STEP		19	/* This task is single stepped */
 #define TIF_BLOCK_STEP		20	/* This task is block stepped */
 #define TIF_UPROBE_SINGLESTEP	21	/* This task is uprobe single stepped */
diff --git a/arch/sparc/include/asm/thread_info_32.h b/arch/sparc/include/asm/thread_info_32.h
index 548b366165dd..567f472c730b 100644
--- a/arch/sparc/include/asm/thread_info_32.h
+++ b/arch/sparc/include/asm/thread_info_32.h
@@ -103,7 +103,6 @@ register struct thread_info *current_thread_info_reg asm("g6");
 #define TIF_NOTIFY_RESUME	1	/* callback before returning to user */
 #define TIF_SIGPENDING		2	/* signal pending */
 #define TIF_NEED_RESCHED	3	/* rescheduling necessary */
-#define TIF_RESTORE_SIGMASK	4	/* restore signal mask in do_signal() */
 #define TIF_USEDFPU		8	/* FPU was used by this task
 					 * this quantum (SMP) */
 #define TIF_POLLING_NRFLAG	9	/* true if poll_idle() is polling
diff --git a/arch/um/include/asm/thread_info.h b/arch/um/include/asm/thread_info.h
index 4eecd960ee8c..565b7db73083 100644
--- a/arch/um/include/asm/thread_info.h
+++ b/arch/um/include/asm/thread_info.h
@@ -60,7 +60,6 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_RESTART_BLOCK	4
 #define TIF_MEMDIE		5	/* is terminating due to OOM killer */
 #define TIF_SYSCALL_AUDIT	6
-#define TIF_RESTORE_SIGMASK	7
 #define TIF_NOTIFY_RESUME	8
 #define TIF_SECCOMP		9	/* secure computing */
 
diff --git a/arch/unicore32/include/asm/thread_info.h b/arch/unicore32/include/asm/thread_info.h
index 5fb728f3b49a..0f38f6072839 100644
--- a/arch/unicore32/include/asm/thread_info.h
+++ b/arch/unicore32/include/asm/thread_info.h
@@ -119,7 +119,6 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_NOTIFY_RESUME	2	/* callback before returning to user */
 #define TIF_SYSCALL_TRACE	8
 #define TIF_MEMDIE		18
-#define TIF_RESTORE_SIGMASK	20
 
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
 #define _TIF_NEED_RESCHED	(1 << TIF_NEED_RESCHED)
diff --git a/arch/xtensa/include/asm/thread_info.h b/arch/xtensa/include/asm/thread_info.h
index f092cc3f4e66..27350241615c 100644
--- a/arch/xtensa/include/asm/thread_info.h
+++ b/arch/xtensa/include/asm/thread_info.h
@@ -108,7 +108,6 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_SINGLESTEP		3	/* restore singlestep on return to user mode */
 #define TIF_SYSCALL_TRACEPOINT	4	/* syscall tracepoint instrumentation */
 #define TIF_MEMDIE		5	/* is terminating due to OOM killer */
-#define TIF_RESTORE_SIGMASK	6	/* restore signal mask in do_signal() */
 #define TIF_NOTIFY_RESUME	7	/* callback before returning to user */
 #define TIF_DB_DISABLED		8	/* debug trap disabled for syscall */
 
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 520efbd355be..a5284f5ccb7c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -713,9 +713,6 @@ struct task_struct {
 	/* Bit to tell LSMs we're in execve(): */
 	unsigned			in_execve:1;
 	unsigned			in_iowait:1;
-#ifndef TIF_RESTORE_SIGMASK
-	unsigned			restore_sigmask:1;
-#endif
 #ifdef CONFIG_MEMCG
 	unsigned			in_user_fault:1;
 #endif
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 78678de45278..ac34566e762f 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -400,86 +400,9 @@ static inline void ptrace_signal_wake_up(struct task_struct *t, bool resume)
 
 void task_join_group_stop(struct task_struct *task);
 
-#ifdef TIF_RESTORE_SIGMASK
-/*
- * Legacy restore_sigmask accessors.  These are inefficient on
- * SMP architectures because they require atomic operations.
- */
-
-/**
- * set_restore_sigmask() - make sure real_sigmask processing gets done
- *
- * This sets TIF_RESTORE_SIGMASK and ensures that the arch signal code
- * will run before returning to user mode, to process the flag.  For
- * all callers, TIF_SIGPENDING is already set or it's no harm to set
- * it.  TIF_RESTORE_SIGMASK need not be in the set of bits that the
- * arch code will notice on return to user mode, in case those bits
- * are scarce.  We set TIF_SIGPENDING here to ensure that the arch
- * signal code always gets run when TIF_RESTORE_SIGMASK is set.
- */
-static inline void set_restore_sigmask(void)
-{
-	set_thread_flag(TIF_RESTORE_SIGMASK);
-}
-
-static inline void clear_tsk_restore_sigmask(struct task_struct *task)
-{
-	clear_tsk_thread_flag(task, TIF_RESTORE_SIGMASK);
-}
-
-static inline void clear_restore_sigmask(void)
-{
-	clear_thread_flag(TIF_RESTORE_SIGMASK);
-}
-static inline bool test_tsk_restore_sigmask(struct task_struct *task)
-{
-	return test_tsk_thread_flag(task, TIF_RESTORE_SIGMASK);
-}
-static inline bool test_restore_sigmask(void)
-{
-	return test_thread_flag(TIF_RESTORE_SIGMASK);
-}
-static inline bool test_and_clear_restore_sigmask(void)
-{
-	return test_and_clear_thread_flag(TIF_RESTORE_SIGMASK);
-}
-
-#else	/* TIF_RESTORE_SIGMASK */
-
-/* Higher-quality implementation, used if TIF_RESTORE_SIGMASK doesn't exist. */
-static inline void set_restore_sigmask(void)
-{
-	current->restore_sigmask = true;
-}
-static inline void clear_tsk_restore_sigmask(struct task_struct *task)
-{
-	task->restore_sigmask = false;
-}
-static inline void clear_restore_sigmask(void)
-{
-	current->restore_sigmask = false;
-}
-static inline bool test_restore_sigmask(void)
-{
-	return current->restore_sigmask;
-}
-static inline bool test_tsk_restore_sigmask(struct task_struct *task)
-{
-	return task->restore_sigmask;
-}
-static inline bool test_and_clear_restore_sigmask(void)
-{
-	if (!current->restore_sigmask)
-		return false;
-	current->restore_sigmask = false;
-	return true;
-}
-#endif
-
 static inline void restore_saved_sigmask(void)
 {
-	if (test_and_clear_restore_sigmask())
-		__set_current_blocked(&current->real_blocked);
+	__set_current_blocked(&current->real_blocked);
 }
 
 extern int set_user_sigmask(const sigset_t __user *umask, size_t sigsetsize);
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 5ed6126e1cc5..08e25bbb04f4 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -963,8 +963,6 @@ int ptrace_request(struct task_struct *child, long request,
 		child->real_blocked = new_set;
 		spin_unlock_irq(&child->sighand->siglock);
 
-		clear_tsk_restore_sigmask(child);
-
 		ret = 0;
 		break;
 	}
diff --git a/kernel/signal.c b/kernel/signal.c
index c37d4f209699..1358e6bf02d3 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2734,12 +2734,6 @@ static void signal_delivered(struct ksignal *ksig, int stepping)
 {
 	sigset_t blocked;
 
-	/* A signal was successfully delivered, and the
-	   saved sigmask was stored on the signal frame,
-	   and will be restored by sigreturn.  So we can
-	   simply clear the restore sigmask flag.  */
-	clear_restore_sigmask();
-
 	sigorsets(&blocked, &current->blocked, &ksig->ka.sa.sa_mask);
 	if (!(ksig->ka.sa.sa_flags & SA_NODEFER))
 		sigaddset(&blocked, ksig->sig);
@@ -2940,7 +2934,6 @@ EXPORT_SYMBOL(sigprocmask);
 
 static int set_sigmask(sigset_t *newset)
 {
-	set_restore_sigmask();
 	sigdelsetmask(newset, sigmask(SIGKILL) | sigmask(SIGSTOP));
 	__set_current_blocked(newset);
 
-- 
2.21.0.dirty

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Linus Torvalds]

On Fri, Jun 7, 2019 at 2:41 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> The sigsuspend system call overrides the signal mask just
> like all of the other users of set_user_sigmask, so convert
> it to use the same helpers.

Me likey.

Whole series looks good to me, but that's just from looking at the
patches. Maybe testing shows problems..

              Linus

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Linus Torvalds]

On Fri, Jun 7, 2019 at 2:41 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> The sigsuspend system call overrides the signal mask just
> like all of the other users of set_user_sigmask, so convert
> it to use the same helpers.

Me likey.

Whole series looks good to me, but that's just from looking at the
patches. Maybe testing shows problems..

              Linus

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Oleg Nesterov]

On 06/07, Eric W. Biederman wrote:
>
> +static int set_sigmask(sigset_t *kmask)
> +{
> +	set_restore_sigmask();
> +	current->saved_sigmask = current->blocked;
> +	set_current_blocked(kmask);
> +
> +	return 0;
> +}

I was going to do the same change except my version returns void ;)

So ACK.


As for 2-5, sorry I can't read them today, will do tomorrow.

But at first glance... yes, we can remove TIF_RESTORE_SIGMASK.

As for "remove saved_sigmask" I have some concerns... At least this
means a user-visible change iiuc. Say, pselect unblocks a fatal signal.
Say, SIGINT without a handler. Suppose SIGINT comes after set_sigmask().

Before this change the process will be killed.

After this change it will be killed or not. It won't be killed if
do_select() finds an already ready fd without blocking, or it finds a
ready fd right after SIGINT interrupts poll_schedule_timeout().

And _to me_ the new behaviour makes more sense. But when it comes to
user-visible changes you can never know if it breaks something or not.

Oleg.

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Oleg Nesterov]

On 06/07, Eric W. Biederman wrote:
>
> +static int set_sigmask(sigset_t *kmask)
> +{
> +	set_restore_sigmask();
> +	current->saved_sigmask = current->blocked;
> +	set_current_blocked(kmask);
> +
> +	return 0;
> +}

I was going to do the same change except my version returns void ;)

So ACK.


As for 2-5, sorry I can't read them today, will do tomorrow.

But at first glance... yes, we can remove TIF_RESTORE_SIGMASK.

As for "remove saved_sigmask" I have some concerns... At least this
means a user-visible change iiuc. Say, pselect unblocks a fatal signal.
Say, SIGINT without a handler. Suppose SIGINT comes after set_sigmask().

Before this change the process will be killed.

After this change it will be killed or not. It won't be killed if
do_select() finds an already ready fd without blocking, or it finds a
ready fd right after SIGINT interrupts poll_schedule_timeout().

And _to me_ the new behaviour makes more sense. But when it comes to
user-visible changes you can never know if it breaks something or not.

Oleg.

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> writes:

> On 06/07, Eric W. Biederman wrote:
>>
>> +static int set_sigmask(sigset_t *kmask)
>> +{
>> +	set_restore_sigmask();
>> +	current->saved_sigmask = current->blocked;
>> +	set_current_blocked(kmask);
>> +
>> +	return 0;
>> +}
>
> I was going to do the same change except my version returns void ;)
>
> So ACK.
>
>
> As for 2-5, sorry I can't read them today, will do tomorrow.
>
> But at first glance... yes, we can remove TIF_RESTORE_SIGMASK.
>
> As for "remove saved_sigmask" I have some concerns... At least this
> means a user-visible change iiuc. Say, pselect unblocks a fatal signal.
> Say, SIGINT without a handler. Suppose SIGINT comes after set_sigmask().
>
> Before this change the process will be killed.
>
> After this change it will be killed or not. It won't be killed if
> do_select() finds an already ready fd without blocking, or it finds a
> ready fd right after SIGINT interrupts poll_schedule_timeout().

Yes.  Because having the signal set in real_blocked disables the
immediate kill optimization, and the signal has to be delivered before
we decide to kill the process.  Which matters because as you say if
nothing checks signal_pending() when the signals are unblocked we might
not attempt to deliver the signal.

So it is a matter of timing.

If we have both a signal and a file descriptor become ready
at the same time I would call that a race.  Either could
wake up the process and depending on the exact time we could
return either one.

So it is possible that today if the signal came just after the file
descriptor ,the code might have made it to restore_saved_sigmask_unless,
before __send_signal runs.

I see the concern.  I think in a matter like this we try it.  Make
the patches clean so people can bisect the problem.  Then if someone
runs into this problem we revert the offending patches.

If it looks like bisection won't cleanly reveal the potential problem
please let me know.

Personally I don't think anyone sane would intentionally depend on this
and I don't think there is a sufficiently reliable way to depend on this
by accident that people would actually be depending on it.

> And _to me_ the new behaviour makes more sense. But when it comes to
> user-visible changes you can never know if it breaks something or not.

True.

The set of applications is larger than any developer can reasonably test.

Eric

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> writes:

> On 06/07, Eric W. Biederman wrote:
>>
>> +static int set_sigmask(sigset_t *kmask)
>> +{
>> +	set_restore_sigmask();
>> +	current->saved_sigmask = current->blocked;
>> +	set_current_blocked(kmask);
>> +
>> +	return 0;
>> +}
>
> I was going to do the same change except my version returns void ;)
>
> So ACK.
>
>
> As for 2-5, sorry I can't read them today, will do tomorrow.
>
> But at first glance... yes, we can remove TIF_RESTORE_SIGMASK.
>
> As for "remove saved_sigmask" I have some concerns... At least this
> means a user-visible change iiuc. Say, pselect unblocks a fatal signal.
> Say, SIGINT without a handler. Suppose SIGINT comes after set_sigmask().
>
> Before this change the process will be killed.
>
> After this change it will be killed or not. It won't be killed if
> do_select() finds an already ready fd without blocking, or it finds a
> ready fd right after SIGINT interrupts poll_schedule_timeout().

Yes.  Because having the signal set in real_blocked disables the
immediate kill optimization, and the signal has to be delivered before
we decide to kill the process.  Which matters because as you say if
nothing checks signal_pending() when the signals are unblocked we might
not attempt to deliver the signal.

So it is a matter of timing.

If we have both a signal and a file descriptor become ready
at the same time I would call that a race.  Either could
wake up the process and depending on the exact time we could
return either one.

So it is possible that today if the signal came just after the file
descriptor ,the code might have made it to restore_saved_sigmask_unless,
before __send_signal runs.

I see the concern.  I think in a matter like this we try it.  Make
the patches clean so people can bisect the problem.  Then if someone
runs into this problem we revert the offending patches.

If it looks like bisection won't cleanly reveal the potential problem
please let me know.

Personally I don't think anyone sane would intentionally depend on this
and I don't think there is a sufficiently reliable way to depend on this
by accident that people would actually be depending on it.

> And _to me_ the new behaviour makes more sense. But when it comes to
> user-visible changes you can never know if it breaks something or not.

True.

The set of applications is larger than any developer can reasonably test.

Eric

RE: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[David Laight]

From: Eric W. Biederman
> Sent: 10 June 2019 22:21
...
> >
> > As for "remove saved_sigmask" I have some concerns... At least this
> > means a user-visible change iiuc. Say, pselect unblocks a fatal signal.
> > Say, SIGINT without a handler. Suppose SIGINT comes after set_sigmask().
> >
> > Before this change the process will be killed.
> >
> > After this change it will be killed or not. It won't be killed if
> > do_select() finds an already ready fd without blocking, or it finds a
> > ready fd right after SIGINT interrupts poll_schedule_timeout().
> 
> Yes.  Because having the signal set in real_blocked disables the
> immediate kill optimization, and the signal has to be delivered before
> we decide to kill the process.  Which matters because as you say if
> nothing checks signal_pending() when the signals are unblocked we might
> not attempt to deliver the signal.
> 
> So it is a matter of timing.
> 
> If we have both a signal and a file descriptor become ready
> at the same time I would call that a race.  Either could
> wake up the process and depending on the exact time we could
> return either one.
> 
> So it is possible that today if the signal came just after the file
> descriptor ,the code might have made it to restore_saved_sigmask_unless,
> before __send_signal runs.
> 
> I see the concern.  I think in a matter like this we try it.  Make
> the patches clean so people can bisect the problem.  Then if someone
> runs into this problem we revert the offending patches.

If I have an application that has a loop with a pselect call that
enables SIGINT (without a handler) and, for whatever reason,
one of the fd is always 'ready' then I'd expect a SIGINT
(from ^C) to terminate the program.

A quick test program:

#include <sys/time.h>
#include <sys/types.h>
#include <unistd.h>

#include <sys/select.h>
#include <signal.h>

int main(int argc, char **argv)
{
        fd_set readfds;
        sigset_t sig_int;
        struct timespec delay = {1, 0};

        sigfillset(&sig_int);
        sigdelset(&sig_int, SIGINT);

        sighold(SIGINT);

        for (;;) {
                FD_ZERO(&readfds);
                FD_SET(0, &readfds);
                pselect(1, &readfds, NULL, NULL, &delay, &sig_int);

                poll(0,0,1000);
        }
}

Run under strace to see what is happening and send SIGINT from a different terminal.
The program sleeps for a second in each of the pselect() and poll() calls.
Send a SIGINT and in terminates after pselect() returns ERESTARTNOHAND.

Run again, this time press enter - making fd 0 readable.
pselect() returns 1, but the program still exits.
(Tested on a 5.1.0-rc5 kernel.)

If a signal handler were defined it should be called instead.

FWIW is ERESTARTNOHAND actually sane here?
If I've used setitimer() to get SIGALARM generated every second I'd
expect select() to return EINTR every second even if I don't
have a SIGALARM handler?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[David Laight]

From: Eric W. Biederman
> Sent: 10 June 2019 22:21
...
> >
> > As for "remove saved_sigmask" I have some concerns... At least this
> > means a user-visible change iiuc. Say, pselect unblocks a fatal signal.
> > Say, SIGINT without a handler. Suppose SIGINT comes after set_sigmask().
> >
> > Before this change the process will be killed.
> >
> > After this change it will be killed or not. It won't be killed if
> > do_select() finds an already ready fd without blocking, or it finds a
> > ready fd right after SIGINT interrupts poll_schedule_timeout().
> 
> Yes.  Because having the signal set in real_blocked disables the
> immediate kill optimization, and the signal has to be delivered before
> we decide to kill the process.  Which matters because as you say if
> nothing checks signal_pending() when the signals are unblocked we might
> not attempt to deliver the signal.
> 
> So it is a matter of timing.
> 
> If we have both a signal and a file descriptor become ready
> at the same time I would call that a race.  Either could
> wake up the process and depending on the exact time we could
> return either one.
> 
> So it is possible that today if the signal came just after the file
> descriptor ,the code might have made it to restore_saved_sigmask_unless,
> before __send_signal runs.
> 
> I see the concern.  I think in a matter like this we try it.  Make
> the patches clean so people can bisect the problem.  Then if someone
> runs into this problem we revert the offending patches.

If I have an application that has a loop with a pselect call that
enables SIGINT (without a handler) and, for whatever reason,
one of the fd is always 'ready' then I'd expect a SIGINT
(from ^C) to terminate the program.

A quick test program:

#include <sys/time.h>
#include <sys/types.h>
#include <unistd.h>

#include <sys/select.h>
#include <signal.h>

int main(int argc, char **argv)
{
        fd_set readfds;
        sigset_t sig_int;
        struct timespec delay = {1, 0};

        sigfillset(&sig_int);
        sigdelset(&sig_int, SIGINT);

        sighold(SIGINT);

        for (;;) {
                FD_ZERO(&readfds);
                FD_SET(0, &readfds);
                pselect(1, &readfds, NULL, NULL, &delay, &sig_int);

                poll(0,0,1000);
        }
}

Run under strace to see what is happening and send SIGINT from a different terminal.
The program sleeps for a second in each of the pselect() and poll() calls.
Send a SIGINT and in terminates after pselect() returns ERESTARTNOHAND.

Run again, this time press enter - making fd 0 readable.
pselect() returns 1, but the program still exits.
(Tested on a 5.1.0-rc5 kernel.)

If a signal handler were defined it should be called instead.

FWIW is ERESTARTNOHAND actually sane here?
If I've used setitimer() to get SIGALARM generated every second I'd
expect select() to return EINTR every second even if I don't
have a SIGALARM handler?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[David Laight]

From: David Laight
> Sent: 11 June 2019 10:52
...
> If I have an application that has a loop with a pselect call that
> enables SIGINT (without a handler) and, for whatever reason,
> one of the fd is always 'ready' then I'd expect a SIGINT
> (from ^C) to terminate the program.
> 
> A quick test program:
> 
> #include <sys/time.h>
> #include <sys/types.h>
> #include <unistd.h>
> 
> #include <sys/select.h>
> #include <signal.h>
> 
> int main(int argc, char **argv)
> {
>         fd_set readfds;
>         sigset_t sig_int;
>         struct timespec delay = {1, 0};
> 
>         sigfillset(&sig_int);
>         sigdelset(&sig_int, SIGINT);
> 
>         sighold(SIGINT);
> 
>         for (;;) {
>                 FD_ZERO(&readfds);
>                 FD_SET(0, &readfds);
>                 pselect(1, &readfds, NULL, NULL, &delay, &sig_int);
> 
>                 poll(0,0,1000);
>         }
> }
> 
> Run under strace to see what is happening and send SIGINT from a different terminal.
> The program sleeps for a second in each of the pselect() and poll() calls.
> Send a SIGINT and in terminates after pselect() returns ERESTARTNOHAND.
> 
> Run again, this time press enter - making fd 0 readable.
> pselect() returns 1, but the program still exits.
> (Tested on a 5.1.0-rc5 kernel.)
> 
> If a signal handler were defined it should be called instead.

If I add a signal handler for SIGINT it is called when pselect()
returns regardless of the return value.

If I setup SIGUSR1/2 the same way as SIGINT and get the SIGINT
handler to sighold() and then raise both of them, the USR1/2
handlers are both called on the next pselect() call.
(Without the extra sighold() the handlers are called when kill()
returns.)

I'd expect the epoll functions to work the same way.

sigtimedwait is different though - it returns the number of the
pending signal (and doesn't call the handler).
So if two signals are pending neither handler should be called.
The second signal would be returned on the following sigtimedwait call.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[David Laight]

From: David Laight
> Sent: 11 June 2019 10:52
...
> If I have an application that has a loop with a pselect call that
> enables SIGINT (without a handler) and, for whatever reason,
> one of the fd is always 'ready' then I'd expect a SIGINT
> (from ^C) to terminate the program.
> 
> A quick test program:
> 
> #include <sys/time.h>
> #include <sys/types.h>
> #include <unistd.h>
> 
> #include <sys/select.h>
> #include <signal.h>
> 
> int main(int argc, char **argv)
> {
>         fd_set readfds;
>         sigset_t sig_int;
>         struct timespec delay = {1, 0};
> 
>         sigfillset(&sig_int);
>         sigdelset(&sig_int, SIGINT);
> 
>         sighold(SIGINT);
> 
>         for (;;) {
>                 FD_ZERO(&readfds);
>                 FD_SET(0, &readfds);
>                 pselect(1, &readfds, NULL, NULL, &delay, &sig_int);
> 
>                 poll(0,0,1000);
>         }
> }
> 
> Run under strace to see what is happening and send SIGINT from a different terminal.
> The program sleeps for a second in each of the pselect() and poll() calls.
> Send a SIGINT and in terminates after pselect() returns ERESTARTNOHAND.
> 
> Run again, this time press enter - making fd 0 readable.
> pselect() returns 1, but the program still exits.
> (Tested on a 5.1.0-rc5 kernel.)
> 
> If a signal handler were defined it should be called instead.

If I add a signal handler for SIGINT it is called when pselect()
returns regardless of the return value.

If I setup SIGUSR1/2 the same way as SIGINT and get the SIGINT
handler to sighold() and then raise both of them, the USR1/2
handlers are both called on the next pselect() call.
(Without the extra sighold() the handlers are called when kill()
returns.)

I'd expect the epoll functions to work the same way.

sigtimedwait is different though - it returns the number of the
pending signal (and doesn't call the handler).
So if two signals are pending neither handler should be called.
The second signal would be returned on the following sigtimedwait call.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[David Laight]

From: David Laight
> Sent: 11 June 2019 10:52
...
> FWIW is ERESTARTNOHAND actually sane here?
> If I've used setitimer() to get SIGALARM generated every second I'd
> expect select() to return EINTR every second even if I don't
> have a SIGALARM handler?

Actually no - after sigset(SIGALRM, SIG_IGN) I'd expect absolutely
nothing to happen when kill(pid, SIGALRM) is called.

However if I run:

	struct itimerval itimerval = {{1, 0}, {1, 0}};
	setitimer(ITIMER_REAL, &itimerval, NULL);
	sigset(SIGALRM, SIG_IGN);

	poll(0, 0, big_timeout);

I see (with strace) poll() repeatedly returning ERESTART_RESTARTBLOCK
and being restarted.
Replacing poll() with pselect() returns ERESTARTNOHAND.
(In both cases the timeout must be updated since the application
does see the timeout.)

I'm sure other unix kernels completely ignore signals set to SIG_IGN.
So this restart dance just isn't needed.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

RE: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[David Laight]

From: David Laight
> Sent: 11 June 2019 10:52
...
> FWIW is ERESTARTNOHAND actually sane here?
> If I've used setitimer() to get SIGALARM generated every second I'd
> expect select() to return EINTR every second even if I don't
> have a SIGALARM handler?

Actually no - after sigset(SIGALRM, SIG_IGN) I'd expect absolutely
nothing to happen when kill(pid, SIGALRM) is called.

However if I run:

	struct itimerval itimerval = {{1, 0}, {1, 0}};
	setitimer(ITIMER_REAL, &itimerval, NULL);
	sigset(SIGALRM, SIG_IGN);

	poll(0, 0, big_timeout);

I see (with strace) poll() repeatedly returning ERESTART_RESTARTBLOCK
and being restarted.
Replacing poll() with pselect() returns ERESTARTNOHAND.
(In both cases the timeout must be updated since the application
does see the timeout.)

I'm sure other unix kernels completely ignore signals set to SIG_IGN.
So this restart dance just isn't needed.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Oleg Nesterov]

On 06/10, Eric W. Biederman wrote:
>
> Personally I don't think anyone sane would intentionally depend on this
> and I don't think there is a sufficiently reliable way to depend on this
> by accident that people would actually be depending on it.

Agreed.

As I said I like these changes and I see nothing wrong. To me they fix the
current behaviour, or at least make it more consistent.

But perhaps this should be documented in the changelog? To make it clear
that this change was intentional.

Oleg.

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Oleg Nesterov]

On 06/10, Eric W. Biederman wrote:
>
> Personally I don't think anyone sane would intentionally depend on this
> and I don't think there is a sufficiently reliable way to depend on this
> by accident that people would actually be depending on it.

Agreed.

As I said I like these changes and I see nothing wrong. To me they fix the
current behaviour, or at least make it more consistent.

But perhaps this should be documented in the changelog? To make it clear
that this change was intentional.

Oleg.

Re: [RFC PATCH 0/5]: Removing saved_sigmask

[Oleg Nesterov]

On 06/07, Eric W. Biederman wrote:
>
> Eric W. Biederman (5):
>       signal: Teach sigsuspend to use set_user_sigmask
>       signal/kvm:  Stop using sigprocmask in kvm_sigset_(activate|deactivate)
>       signal: Always keep real_blocked in sync with blocked
>       signal: Remove saved_sigmask
>       signal: Remove the unnecessary restore_sigmask flag

Reviewed-by: Oleg Nesterov <oleg@redhat.com>



I guess this should be routed via -mm tree? This depends on
signal-simplify-set_user_sigmask-restore_user_sigmask.patch

Oleg.

Re: [RFC PATCH 0/5]: Removing saved_sigmask

[Oleg Nesterov]

On 06/07, Eric W. Biederman wrote:
>
> Eric W. Biederman (5):
>       signal: Teach sigsuspend to use set_user_sigmask
>       signal/kvm:  Stop using sigprocmask in kvm_sigset_(activate|deactivate)
>       signal: Always keep real_blocked in sync with blocked
>       signal: Remove saved_sigmask
>       signal: Remove the unnecessary restore_sigmask flag

Reviewed-by: Oleg Nesterov <oleg@redhat.com>



I guess this should be routed via -mm tree? This depends on
signal-simplify-set_user_sigmask-restore_user_sigmask.patch

Oleg.

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> writes:

> On 06/10, Eric W. Biederman wrote:
>>
>> Personally I don't think anyone sane would intentionally depend on this
>> and I don't think there is a sufficiently reliable way to depend on this
>> by accident that people would actually be depending on it.
>
> Agreed.
>
> As I said I like these changes and I see nothing wrong. To me they fix the
> current behaviour, or at least make it more consistent.
>
> But perhaps this should be documented in the changelog? To make it clear
> that this change was intentional.

Good point.  I had not documented it because I thought I was only
disabling an optimization.

Eric

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> writes:

> On 06/10, Eric W. Biederman wrote:
>>
>> Personally I don't think anyone sane would intentionally depend on this
>> and I don't think there is a sufficiently reliable way to depend on this
>> by accident that people would actually be depending on it.
>
> Agreed.
>
> As I said I like these changes and I see nothing wrong. To me they fix the
> current behaviour, or at least make it more consistent.
>
> But perhaps this should be documented in the changelog? To make it clear
> that this change was intentional.

Good point.  I had not documented it because I thought I was only
disabling an optimization.

Eric

RE: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[David Laight]

From: Oleg Nesterov [mailto:oleg@redhat.com]
> Sent: 11 June 2019 19:56
> On 06/10, Eric W. Biederman wrote:
> >
> > Personally I don't think anyone sane would intentionally depend on this
> > and I don't think there is a sufficiently reliable way to depend on this
> > by accident that people would actually be depending on it.
> 
> Agreed.
> 
> As I said I like these changes and I see nothing wrong. To me they fix the
> current behaviour, or at least make it more consistent.
> 
> But perhaps this should be documented in the changelog? To make it clear
> that this change was intentional.

What happens if you run the test program I posted yesterday after the changes?

It looks like pselect() and epoll_pwait() operated completely differently.
pselect() would always calls the signal handlers.
epoll_pwait() only calls them when EINTR is returned.
So changing epoll_pwait() and pselect() to work the same way
is bound to break some applications.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[David Laight]

From: Oleg Nesterov [mailto:oleg@redhat.com]
> Sent: 11 June 2019 19:56
> On 06/10, Eric W. Biederman wrote:
> >
> > Personally I don't think anyone sane would intentionally depend on this
> > and I don't think there is a sufficiently reliable way to depend on this
> > by accident that people would actually be depending on it.
> 
> Agreed.
> 
> As I said I like these changes and I see nothing wrong. To me they fix the
> current behaviour, or at least make it more consistent.
> 
> But perhaps this should be documented in the changelog? To make it clear
> that this change was intentional.

What happens if you run the test program I posted yesterday after the changes?

It looks like pselect() and epoll_pwait() operated completely differently.
pselect() would always calls the signal handlers.
epoll_pwait() only calls them when EINTR is returned.
So changing epoll_pwait() and pselect() to work the same way
is bound to break some applications.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Eric W. Biederman]

David Laight <David.Laight@ACULAB.COM> writes:

> From: David Laight
>> Sent: 11 June 2019 10:52
> ...
>> FWIW is ERESTARTNOHAND actually sane here?
>> If I've used setitimer() to get SIGALARM generated every second I'd
>> expect select() to return EINTR every second even if I don't
>> have a SIGALARM handler?
>
> Actually no - after sigset(SIGALRM, SIG_IGN) I'd expect absolutely
> nothing to happen when kill(pid, SIGALRM) is called.
>
> However if I run:
>
> 	struct itimerval itimerval = {{1, 0}, {1, 0}};
> 	setitimer(ITIMER_REAL, &itimerval, NULL);
> 	sigset(SIGALRM, SIG_IGN);
>
> 	poll(0, 0, big_timeout);
>
> I see (with strace) poll() repeatedly returning ERESTART_RESTARTBLOCK
> and being restarted.
> Replacing poll() with pselect() returns ERESTARTNOHAND.
> (In both cases the timeout must be updated since the application
> does see the timeout.)
>
> I'm sure other unix kernels completely ignore signals set to SIG_IGN.
> So this restart dance just isn't needed.

We also ignore such signals except when the signal is blocked when it is
received.

We don't currently but it would be perfectly legitimate for
set_current_blocked to dequeue and drop signals that have become
unblocked whose handler is SIG_IGN.

The dance would still be needed for the rare case where TIF_SIGPENDING
gets set for a non-signal.

Dropping the signal while it is blocked if it's handler is SIG_IGN looks
like a bad idea.

Eric








--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Eric W. Biederman]

David Laight <David.Laight@ACULAB.COM> writes:

> From: David Laight
>> Sent: 11 June 2019 10:52
> ...
>> FWIW is ERESTARTNOHAND actually sane here?
>> If I've used setitimer() to get SIGALARM generated every second I'd
>> expect select() to return EINTR every second even if I don't
>> have a SIGALARM handler?
>
> Actually no - after sigset(SIGALRM, SIG_IGN) I'd expect absolutely
> nothing to happen when kill(pid, SIGALRM) is called.
>
> However if I run:
>
> 	struct itimerval itimerval = {{1, 0}, {1, 0}};
> 	setitimer(ITIMER_REAL, &itimerval, NULL);
> 	sigset(SIGALRM, SIG_IGN);
>
> 	poll(0, 0, big_timeout);
>
> I see (with strace) poll() repeatedly returning ERESTART_RESTARTBLOCK
> and being restarted.
> Replacing poll() with pselect() returns ERESTARTNOHAND.
> (In both cases the timeout must be updated since the application
> does see the timeout.)
>
> I'm sure other unix kernels completely ignore signals set to SIG_IGN.
> So this restart dance just isn't needed.

We also ignore such signals except when the signal is blocked when it is
received.

We don't currently but it would be perfectly legitimate for
set_current_blocked to dequeue and drop signals that have become
unblocked whose handler is SIG_IGN.

The dance would still be needed for the rare case where TIF_SIGPENDING
gets set for a non-signal.

Dropping the signal while it is blocked if it's handler is SIG_IGN looks
like a bad idea.

Eric

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Eric W. Biederman]

David Laight <David.Laight@ACULAB.COM> writes:

> From: David Laight
>> Sent: 11 June 2019 10:52
> ...
>> If I have an application that has a loop with a pselect call that
>> enables SIGINT (without a handler) and, for whatever reason,
>> one of the fd is always 'ready' then I'd expect a SIGINT
>> (from ^C) to terminate the program.
>> 
>> A quick test program:
>> 
>> #include <sys/time.h>
>> #include <sys/types.h>
>> #include <unistd.h>
>> 
>> #include <sys/select.h>
>> #include <signal.h>
>> 
>> int main(int argc, char **argv)
>> {
>>         fd_set readfds;
>>         sigset_t sig_int;
>>         struct timespec delay = {1, 0};
>> 
>>         sigfillset(&sig_int);
>>         sigdelset(&sig_int, SIGINT);
>> 
>>         sighold(SIGINT);
>> 
>>         for (;;) {
>>                 FD_ZERO(&readfds);
>>                 FD_SET(0, &readfds);
>>                 pselect(1, &readfds, NULL, NULL, &delay, &sig_int);
>> 
>>                 poll(0,0,1000);
>>         }
>> }
>> 
>> Run under strace to see what is happening and send SIGINT from a different terminal.
>> The program sleeps for a second in each of the pselect() and poll() calls.
>> Send a SIGINT and in terminates after pselect() returns ERESTARTNOHAND.
>> 
>> Run again, this time press enter - making fd 0 readable.
>> pselect() returns 1, but the program still exits.
>> (Tested on a 5.1.0-rc5 kernel.)
>> 
>> If a signal handler were defined it should be called instead.
>
> If I add a signal handler for SIGINT it is called when pselect()
> returns regardless of the return value.

That is odd.  Is this with Oleg's fix applied?

Eric

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Eric W. Biederman]

David Laight <David.Laight@ACULAB.COM> writes:

> From: David Laight
>> Sent: 11 June 2019 10:52
> ...
>> If I have an application that has a loop with a pselect call that
>> enables SIGINT (without a handler) and, for whatever reason,
>> one of the fd is always 'ready' then I'd expect a SIGINT
>> (from ^C) to terminate the program.
>> 
>> A quick test program:
>> 
>> #include <sys/time.h>
>> #include <sys/types.h>
>> #include <unistd.h>
>> 
>> #include <sys/select.h>
>> #include <signal.h>
>> 
>> int main(int argc, char **argv)
>> {
>>         fd_set readfds;
>>         sigset_t sig_int;
>>         struct timespec delay = {1, 0};
>> 
>>         sigfillset(&sig_int);
>>         sigdelset(&sig_int, SIGINT);
>> 
>>         sighold(SIGINT);
>> 
>>         for (;;) {
>>                 FD_ZERO(&readfds);
>>                 FD_SET(0, &readfds);
>>                 pselect(1, &readfds, NULL, NULL, &delay, &sig_int);
>> 
>>                 poll(0,0,1000);
>>         }
>> }
>> 
>> Run under strace to see what is happening and send SIGINT from a different terminal.
>> The program sleeps for a second in each of the pselect() and poll() calls.
>> Send a SIGINT and in terminates after pselect() returns ERESTARTNOHAND.
>> 
>> Run again, this time press enter - making fd 0 readable.
>> pselect() returns 1, but the program still exits.
>> (Tested on a 5.1.0-rc5 kernel.)
>> 
>> If a signal handler were defined it should be called instead.
>
> If I add a signal handler for SIGINT it is called when pselect()
> returns regardless of the return value.

That is odd.  Is this with Oleg's fix applied?

Eric

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Eric W. Biederman]

David Laight <David.Laight@ACULAB.COM> writes:

> From: Oleg Nesterov [mailto:oleg@redhat.com]
>> Sent: 11 June 2019 19:56
>> On 06/10, Eric W. Biederman wrote:
>> >
>> > Personally I don't think anyone sane would intentionally depend on this
>> > and I don't think there is a sufficiently reliable way to depend on this
>> > by accident that people would actually be depending on it.
>> 
>> Agreed.
>> 
>> As I said I like these changes and I see nothing wrong. To me they fix the
>> current behaviour, or at least make it more consistent.
>> 
>> But perhaps this should be documented in the changelog? To make it clear
>> that this change was intentional.
>
> What happens if you run the test program I posted yesterday after the changes?
>
> It looks like pselect() and epoll_pwait() operated completely differently.
> pselect() would always calls the signal handlers.
> epoll_pwait() only calls them when EINTR is returned.
> So changing epoll_pwait() and pselect() to work the same way
> is bound to break some applications.

That is not the change we are discussing.  We are looking at making
pselect and epoll_pwait act a little more like sigtimedwait.

In particular we are discussiong signals whose handler is SIG_DFL and
whose default disposition is to kill the process, such as SIGINT.

When those signals are delivered and they are not blocked, we take
an optimized path in complete_signal and start the process tear down.

That early start of process tear down does not happen if the signal is
blocked or it happens to be in real_blocked (from sigtimedwait).

This matters is the small time window where the signal is received while
the process has temporarily unblocked the signal, and the signal is not
detected by the code and blocked and oridinarily would not be delivered
until next time because of restore_sigmask_unless.

If the tear down has started early.  Even if we would not have returned
the process normally the signal can kill the process.  AKA epoll_pwait
can today result in it's caller being killed without -EINTR being
returned.

My change fixes that behavior and disables the early process tear down
for those signals, by having real_blocked match the set of signals
that are normally blocked by the process.  The result should be
the signal will have to wait for the next call that temporarily
unblocked the process.

The real benefit is that that the code becomes more comprehensible.

It is my patch that titled: "signal: Always keep real_blocked in sync
with blocked" that causes that to happen.

Eric

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Eric W. Biederman]

David Laight <David.Laight@ACULAB.COM> writes:

> From: Oleg Nesterov [mailto:oleg@redhat.com]
>> Sent: 11 June 2019 19:56
>> On 06/10, Eric W. Biederman wrote:
>> >
>> > Personally I don't think anyone sane would intentionally depend on this
>> > and I don't think there is a sufficiently reliable way to depend on this
>> > by accident that people would actually be depending on it.
>> 
>> Agreed.
>> 
>> As I said I like these changes and I see nothing wrong. To me they fix the
>> current behaviour, or at least make it more consistent.
>> 
>> But perhaps this should be documented in the changelog? To make it clear
>> that this change was intentional.
>
> What happens if you run the test program I posted yesterday after the changes?
>
> It looks like pselect() and epoll_pwait() operated completely differently.
> pselect() would always calls the signal handlers.
> epoll_pwait() only calls them when EINTR is returned.
> So changing epoll_pwait() and pselect() to work the same way
> is bound to break some applications.

That is not the change we are discussing.  We are looking at making
pselect and epoll_pwait act a little more like sigtimedwait.

In particular we are discussiong signals whose handler is SIG_DFL and
whose default disposition is to kill the process, such as SIGINT.

When those signals are delivered and they are not blocked, we take
an optimized path in complete_signal and start the process tear down.

That early start of process tear down does not happen if the signal is
blocked or it happens to be in real_blocked (from sigtimedwait).

This matters is the small time window where the signal is received while
the process has temporarily unblocked the signal, and the signal is not
detected by the code and blocked and oridinarily would not be delivered
until next time because of restore_sigmask_unless.

If the tear down has started early.  Even if we would not have returned
the process normally the signal can kill the process.  AKA epoll_pwait
can today result in it's caller being killed without -EINTR being
returned.

My change fixes that behavior and disables the early process tear down
for those signals, by having real_blocked match the set of signals
that are normally blocked by the process.  The result should be
the signal will have to wait for the next call that temporarily
unblocked the process.

The real benefit is that that the code becomes more comprehensible.

It is my patch that titled: "signal: Always keep real_blocked in sync
with blocked" that causes that to happen.

Eric

RE: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[David Laight]

From: Eric W. Biederman
> Sent: 12 June 2019 13:56
> David Laight <David.Laight@ACULAB.COM> writes:
> 
> > From: David Laight
> >> Sent: 11 June 2019 10:52
> > ...
> >> If I have an application that has a loop with a pselect call that
> >> enables SIGINT (without a handler) and, for whatever reason,
> >> one of the fd is always 'ready' then I'd expect a SIGINT
> >> (from ^C) to terminate the program.
> >>
> >> A quick test program:
> >>
> >> #include <sys/time.h>
> >> #include <sys/types.h>
> >> #include <unistd.h>
> >>
> >> #include <sys/select.h>
> >> #include <signal.h>
> >>
> >> int main(int argc, char **argv)
> >> {
> >>         fd_set readfds;
> >>         sigset_t sig_int;
> >>         struct timespec delay = {1, 0};
> >>
> >>         sigfillset(&sig_int);
> >>         sigdelset(&sig_int, SIGINT);
> >>
> >>         sighold(SIGINT);
> >>
> >>         for (;;) {
> >>                 FD_ZERO(&readfds);
> >>                 FD_SET(0, &readfds);
> >>                 pselect(1, &readfds, NULL, NULL, &delay, &sig_int);
> >>
> >>                 poll(0,0,1000);
> >>         }
> >> }
> >>
> >> Run under strace to see what is happening and send SIGINT from a different terminal.
> >> The program sleeps for a second in each of the pselect() and poll() calls.
> >> Send a SIGINT and in terminates after pselect() returns ERESTARTNOHAND.
> >>
> >> Run again, this time press enter - making fd 0 readable.
> >> pselect() returns 1, but the program still exits.
> >> (Tested on a 5.1.0-rc5 kernel.)
> >>
> >> If a signal handler were defined it should be called instead.
> >
> > If I add a signal handler for SIGINT it is called when pselect()
> > returns regardless of the return value.
> 
> That is odd.  Is this with Oleg's fix applied?

No it is a 5.1.0-rc5 kernel with no related local patches.
So it is the 'historic' behaviour of pselect().
But not the original one! Under 2.6.22-5-31 the signal handler isn't caller
when pselect() returns 1.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

RE: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[David Laight]

From: Eric W. Biederman
> Sent: 12 June 2019 13:56
> David Laight <David.Laight@ACULAB.COM> writes:
> 
> > From: David Laight
> >> Sent: 11 June 2019 10:52
> > ...
> >> If I have an application that has a loop with a pselect call that
> >> enables SIGINT (without a handler) and, for whatever reason,
> >> one of the fd is always 'ready' then I'd expect a SIGINT
> >> (from ^C) to terminate the program.
> >>
> >> A quick test program:
> >>
> >> #include <sys/time.h>
> >> #include <sys/types.h>
> >> #include <unistd.h>
> >>
> >> #include <sys/select.h>
> >> #include <signal.h>
> >>
> >> int main(int argc, char **argv)
> >> {
> >>         fd_set readfds;
> >>         sigset_t sig_int;
> >>         struct timespec delay = {1, 0};
> >>
> >>         sigfillset(&sig_int);
> >>         sigdelset(&sig_int, SIGINT);
> >>
> >>         sighold(SIGINT);
> >>
> >>         for (;;) {
> >>                 FD_ZERO(&readfds);
> >>                 FD_SET(0, &readfds);
> >>                 pselect(1, &readfds, NULL, NULL, &delay, &sig_int);
> >>
> >>                 poll(0,0,1000);
> >>         }
> >> }
> >>
> >> Run under strace to see what is happening and send SIGINT from a different terminal.
> >> The program sleeps for a second in each of the pselect() and poll() calls.
> >> Send a SIGINT and in terminates after pselect() returns ERESTARTNOHAND.
> >>
> >> Run again, this time press enter - making fd 0 readable.
> >> pselect() returns 1, but the program still exits.
> >> (Tested on a 5.1.0-rc5 kernel.)
> >>
> >> If a signal handler were defined it should be called instead.
> >
> > If I add a signal handler for SIGINT it is called when pselect()
> > returns regardless of the return value.
> 
> That is odd.  Is this with Oleg's fix applied?

No it is a 5.1.0-rc5 kernel with no related local patches.
So it is the 'historic' behaviour of pselect().
But not the original one! Under 2.6.22-5-31 the signal handler isn't caller
when pselect() returns 1.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Oleg Nesterov]

On 06/12, David Laight wrote:
>
> > > If I add a signal handler for SIGINT it is called when pselect()
> > > returns regardless of the return value.
> >
> > That is odd.  Is this with Oleg's fix applied?
>
> No it is a 5.1.0-rc5 kernel with no related local patches.
> So it is the 'historic' behaviour of pselect().

No, this is not historic behaviour,

> But not the original one! Under 2.6.22-5-31 the signal handler isn't caller
> when pselect() returns 1.

This is historic behaviour.

And it was broken by 854a6ed56839a4 ("signal: Add restore_user_sigmask()").

And this is what we already discussed many, many times in this thread ;)

Oleg.

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Oleg Nesterov]

On 06/12, David Laight wrote:
>
> > > If I add a signal handler for SIGINT it is called when pselect()
> > > returns regardless of the return value.
> >
> > That is odd.  Is this with Oleg's fix applied?
>
> No it is a 5.1.0-rc5 kernel with no related local patches.
> So it is the 'historic' behaviour of pselect().

No, this is not historic behaviour,

> But not the original one! Under 2.6.22-5-31 the signal handler isn't caller
> when pselect() returns 1.

This is historic behaviour.

And it was broken by 854a6ed56839a4 ("signal: Add restore_user_sigmask()").

And this is what we already discussed many, many times in this thread ;)

Oleg.

RE: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[David Laight]

From: Oleg Nesterov
> Sent: 12 June 2019 14:35
> On 06/12, David Laight wrote:
> >
> > > > If I add a signal handler for SIGINT it is called when pselect()
> > > > returns regardless of the return value.
> > >
> > > That is odd.  Is this with Oleg's fix applied?
> >
> > No it is a 5.1.0-rc5 kernel with no related local patches.
> > So it is the 'historic' behaviour of pselect().
> 
> No, this is not historic behaviour,
> 
> > But not the original one! Under 2.6.22-5-31 the signal handler isn't caller
> > when pselect() returns 1.
> 
> This is historic behaviour.
> 
> And it was broken by 854a6ed56839a4 ("signal: Add restore_user_sigmask()").
> 
> And this is what we already discussed many, many times in this thread ;)

My brain hurts :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[David Laight]

From: Oleg Nesterov
> Sent: 12 June 2019 14:35
> On 06/12, David Laight wrote:
> >
> > > > If I add a signal handler for SIGINT it is called when pselect()
> > > > returns regardless of the return value.
> > >
> > > That is odd.  Is this with Oleg's fix applied?
> >
> > No it is a 5.1.0-rc5 kernel with no related local patches.
> > So it is the 'historic' behaviour of pselect().
> 
> No, this is not historic behaviour,
> 
> > But not the original one! Under 2.6.22-5-31 the signal handler isn't caller
> > when pselect() returns 1.
> 
> This is historic behaviour.
> 
> And it was broken by 854a6ed56839a4 ("signal: Add restore_user_sigmask()").
> 
> And this is what we already discussed many, many times in this thread ;)

My brain hurts :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Oleg Nesterov]

On 06/11, David Laight wrote:
>
> If I have an application that has a loop with a pselect call that
> enables SIGINT (without a handler) and, for whatever reason,
> one of the fd is always 'ready' then I'd expect a SIGINT
> (from ^C) to terminate the program.

This was never true.

Before Eric's patches SIGINT can kill a process or not, depending on timing.
In particular, if SIGINT was already pending before pselect() and it finds
an already ready fd, the program won't terminate.

After the Eric's patches SIGINT will only kill the program if pselect() does
not find a ready fd.

And this is much more consistent. Now we can simply say that the signal will
be delivered only if pselect() fails and returns -EINTR. If it doesn't have
a handler the process will be killed, otherwise the handler will be called.

Oleg.

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Oleg Nesterov]

On 06/11, David Laight wrote:
>
> If I have an application that has a loop with a pselect call that
> enables SIGINT (without a handler) and, for whatever reason,
> one of the fd is always 'ready' then I'd expect a SIGINT
> (from ^C) to terminate the program.

This was never true.

Before Eric's patches SIGINT can kill a process or not, depending on timing.
In particular, if SIGINT was already pending before pselect() and it finds
an already ready fd, the program won't terminate.

After the Eric's patches SIGINT will only kill the program if pselect() does
not find a ready fd.

And this is much more consistent. Now we can simply say that the signal will
be delivered only if pselect() fails and returns -EINTR. If it doesn't have
a handler the process will be killed, otherwise the handler will be called.

Oleg.

RE: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[David Laight]

From: Oleg Nesterov
> Sent: 12 June 2019 14:46
> On 06/11, David Laight wrote:
> >
> > If I have an application that has a loop with a pselect call that
> > enables SIGINT (without a handler) and, for whatever reason,
> > one of the fd is always 'ready' then I'd expect a SIGINT
> > (from ^C) to terminate the program.
> 
> This was never true.
> 
> Before Eric's patches SIGINT can kill a process or not, depending on timing.
> In particular, if SIGINT was already pending before pselect() and it finds
> an already ready fd, the program won't terminate.

Which matches what I see on a very old Linux system.

> After the Eric's patches SIGINT will only kill the program if pselect() does
> not find a ready fd.
> 
> And this is much more consistent. Now we can simply say that the signal will
> be delivered only if pselect() fails and returns -EINTR. If it doesn't have
> a handler the process will be killed, otherwise the handler will be called.

But is it what the standards mandate?
Can anyone check how Solaris and any of the BSDs behave?
I don't have access to any solaris systems (I doubt I'll get the disk to
spin on the one in my garage).
I can check NetBSD when I get home.

The ToG page for pselect() http://pubs.opengroup.org/onlinepubs/9699919799/functions/pselect.html says:
    "If sigmask is not a null pointer, then the pselect() function shall replace
    the signal mask of the caller by the set of signals pointed to by sigmask
    before examining the descriptors, and shall restore the signal mask of the
    calling thread before returning."
Note that it says 'before examining the descriptors' not 'before blocking'.
Under the general description about signals it also says that the signal handler
will be called (or other action happen) when a pending signal is unblocked.
So unblocking SIGINT (set to SIG_DFL) prior to examining the descriptors
should be enough to cause the process to exit.
The fact that signal handlers are not called until 'return to user'
is really an implementation choice - but (IMHO) it should appear as if they
were called at the time they became unmasked.

If nothing else the man pages need a note about the standards and portability.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[David Laight]

From: Oleg Nesterov
> Sent: 12 June 2019 14:46
> On 06/11, David Laight wrote:
> >
> > If I have an application that has a loop with a pselect call that
> > enables SIGINT (without a handler) and, for whatever reason,
> > one of the fd is always 'ready' then I'd expect a SIGINT
> > (from ^C) to terminate the program.
> 
> This was never true.
> 
> Before Eric's patches SIGINT can kill a process or not, depending on timing.
> In particular, if SIGINT was already pending before pselect() and it finds
> an already ready fd, the program won't terminate.

Which matches what I see on a very old Linux system.

> After the Eric's patches SIGINT will only kill the program if pselect() does
> not find a ready fd.
> 
> And this is much more consistent. Now we can simply say that the signal will
> be delivered only if pselect() fails and returns -EINTR. If it doesn't have
> a handler the process will be killed, otherwise the handler will be called.

But is it what the standards mandate?
Can anyone check how Solaris and any of the BSDs behave?
I don't have access to any solaris systems (I doubt I'll get the disk to
spin on the one in my garage).
I can check NetBSD when I get home.

The ToG page for pselect() http://pubs.opengroup.org/onlinepubs/9699919799/functions/pselect.html says:
    "If sigmask is not a null pointer, then the pselect() function shall replace
    the signal mask of the caller by the set of signals pointed to by sigmask
    before examining the descriptors, and shall restore the signal mask of the
    calling thread before returning."
Note that it says 'before examining the descriptors' not 'before blocking'.
Under the general description about signals it also says that the signal handler
will be called (or other action happen) when a pending signal is unblocked.
So unblocking SIGINT (set to SIG_DFL) prior to examining the descriptors
should be enough to cause the process to exit.
The fact that signal handlers are not called until 'return to user'
is really an implementation choice - but (IMHO) it should appear as if they
were called at the time they became unmasked.

If nothing else the man pages need a note about the standards and portability.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Eric W. Biederman]

David Laight <David.Laight@ACULAB.COM> writes:

> From: Oleg Nesterov
>> Sent: 12 June 2019 14:46
>> On 06/11, David Laight wrote:
>> >
>> > If I have an application that has a loop with a pselect call that
>> > enables SIGINT (without a handler) and, for whatever reason,
>> > one of the fd is always 'ready' then I'd expect a SIGINT
>> > (from ^C) to terminate the program.

I think this gets into a quality of implementation.

I suspect that set_user_sigmask should do:
if (signal_pending())
	return -ERESTARNOSIGHAND; /* -EINTR that restarts if nothing was pending */

Which should be safe as nothing has blocked yet to consume any of the
timeouts, and it should ensure that none of the routines miss a signal.

>> This was never true.
>> 
>> Before Eric's patches SIGINT can kill a process or not, depending on timing.
>> In particular, if SIGINT was already pending before pselect() and it finds
>> an already ready fd, the program won't terminate.
>
> Which matches what I see on a very old Linux system.
>
>> After the Eric's patches SIGINT will only kill the program if pselect() does
>> not find a ready fd.
>> 
>> And this is much more consistent. Now we can simply say that the signal will
>> be delivered only if pselect() fails and returns -EINTR. If it doesn't have
>> a handler the process will be killed, otherwise the handler will be called.
>
> But is it what the standards mandate?
> Can anyone check how Solaris and any of the BSDs behave?
> I don't have access to any solaris systems (I doubt I'll get the disk to
> spin on the one in my garage).
> I can check NetBSD when I get home.
>
> The ToG page for pselect() http://pubs.opengroup.org/onlinepubs/9699919799/functions/pselect.html says:
>     "If sigmask is not a null pointer, then the pselect() function shall replace
>     the signal mask of the caller by the set of signals pointed to by sigmask
>     before examining the descriptors, and shall restore the signal mask of the
>     calling thread before returning."
> Note that it says 'before examining the descriptors' not 'before blocking'.
> Under the general description about signals it also says that the signal handler
> will be called (or other action happen) when a pending signal is unblocked.
> So unblocking SIGINT (set to SIG_DFL) prior to examining the descriptors
> should be enough to cause the process to exit.
> The fact that signal handlers are not called until 'return to user'
> is really an implementation choice - but (IMHO) it should appear as if they
> were called at the time they became unmasked.
>
> If nothing else the man pages need a note about the standards and portability.

I think the entire point is so that signals can be added to an event
loop in a race free way.  *cough* signalfd *cough*.  I think if you are
setting SIGINT to SIG_DFL you would leave SIGINT unblocked so it can
happen anywhere.

I can see more utility in having a SIGINT handler and you block SIGINT
except in your polling loop so you can do something deterministic when
SIGINT comes in.

Which makes it independent of my patches, but still worth fixing.

Eric

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Eric W. Biederman]

David Laight <David.Laight@ACULAB.COM> writes:

> From: Oleg Nesterov
>> Sent: 12 June 2019 14:46
>> On 06/11, David Laight wrote:
>> >
>> > If I have an application that has a loop with a pselect call that
>> > enables SIGINT (without a handler) and, for whatever reason,
>> > one of the fd is always 'ready' then I'd expect a SIGINT
>> > (from ^C) to terminate the program.

I think this gets into a quality of implementation.

I suspect that set_user_sigmask should do:
if (signal_pending())
	return -ERESTARNOSIGHAND; /* -EINTR that restarts if nothing was pending */

Which should be safe as nothing has blocked yet to consume any of the
timeouts, and it should ensure that none of the routines miss a signal.

>> This was never true.
>> 
>> Before Eric's patches SIGINT can kill a process or not, depending on timing.
>> In particular, if SIGINT was already pending before pselect() and it finds
>> an already ready fd, the program won't terminate.
>
> Which matches what I see on a very old Linux system.
>
>> After the Eric's patches SIGINT will only kill the program if pselect() does
>> not find a ready fd.
>> 
>> And this is much more consistent. Now we can simply say that the signal will
>> be delivered only if pselect() fails and returns -EINTR. If it doesn't have
>> a handler the process will be killed, otherwise the handler will be called.
>
> But is it what the standards mandate?
> Can anyone check how Solaris and any of the BSDs behave?
> I don't have access to any solaris systems (I doubt I'll get the disk to
> spin on the one in my garage).
> I can check NetBSD when I get home.
>
> The ToG page for pselect() http://pubs.opengroup.org/onlinepubs/9699919799/functions/pselect.html says:
>     "If sigmask is not a null pointer, then the pselect() function shall replace
>     the signal mask of the caller by the set of signals pointed to by sigmask
>     before examining the descriptors, and shall restore the signal mask of the
>     calling thread before returning."
> Note that it says 'before examining the descriptors' not 'before blocking'.
> Under the general description about signals it also says that the signal handler
> will be called (or other action happen) when a pending signal is unblocked.
> So unblocking SIGINT (set to SIG_DFL) prior to examining the descriptors
> should be enough to cause the process to exit.
> The fact that signal handlers are not called until 'return to user'
> is really an implementation choice - but (IMHO) it should appear as if they
> were called at the time they became unmasked.
>
> If nothing else the man pages need a note about the standards and portability.

I think the entire point is so that signals can be added to an event
loop in a race free way.  *cough* signalfd *cough*.  I think if you are
setting SIGINT to SIG_DFL you would leave SIGINT unblocked so it can
happen anywhere.

I can see more utility in having a SIGINT handler and you block SIGINT
except in your polling loop so you can do something deterministic when
SIGINT comes in.

Which makes it independent of my patches, but still worth fixing.

Eric

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Oleg Nesterov]

On 06/12, Eric W. Biederman wrote:
> David Laight <David.Laight@ACULAB.COM> writes:
>
> > From: Oleg Nesterov
> >> Sent: 12 June 2019 14:46
> >> On 06/11, David Laight wrote:
> >> >
> >> > If I have an application that has a loop with a pselect call that
> >> > enables SIGINT (without a handler) and, for whatever reason,
> >> > one of the fd is always 'ready' then I'd expect a SIGINT
> >> > (from ^C) to terminate the program.
>
> I think this gets into a quality of implementation.
>
> I suspect that set_user_sigmask should do:
> if (signal_pending())
> 	return -ERESTARNOSIGHAND; /* -EINTR that restarts if nothing was pending */
>
> Which should be safe as nothing has blocked yet to consume any of the
> timeouts, and it should ensure that none of the routines miss a signal.

Why? I don't think this makes any sense.

Perhaps we could do this _after_ set_current_blocked() for the case when
the already pending SIGINT was unblocked but a) I am not sure this would
be really better and b) I think it is too late to change this.

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Oleg Nesterov]

On 06/12, Eric W. Biederman wrote:
> David Laight <David.Laight@ACULAB.COM> writes:
>
> > From: Oleg Nesterov
> >> Sent: 12 June 2019 14:46
> >> On 06/11, David Laight wrote:
> >> >
> >> > If I have an application that has a loop with a pselect call that
> >> > enables SIGINT (without a handler) and, for whatever reason,
> >> > one of the fd is always 'ready' then I'd expect a SIGINT
> >> > (from ^C) to terminate the program.
>
> I think this gets into a quality of implementation.
>
> I suspect that set_user_sigmask should do:
> if (signal_pending())
> 	return -ERESTARNOSIGHAND; /* -EINTR that restarts if nothing was pending */
>
> Which should be safe as nothing has blocked yet to consume any of the
> timeouts, and it should ensure that none of the routines miss a signal.

Why? I don't think this makes any sense.

Perhaps we could do this _after_ set_current_blocked() for the case when
the already pending SIGINT was unblocked but a) I am not sure this would
be really better and b) I think it is too late to change this.

Oleg.

RE: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[David Laight]

From: David Laight
> Sent: 12 June 2019 15:18
> From: Oleg Nesterov
> > Sent: 12 June 2019 14:46
> > On 06/11, David Laight wrote:
> > >
> > > If I have an application that has a loop with a pselect call that
> > > enables SIGINT (without a handler) and, for whatever reason,
> > > one of the fd is always 'ready' then I'd expect a SIGINT
> > > (from ^C) to terminate the program.
> >
> > This was never true.
> >
> > Before Eric's patches SIGINT can kill a process or not, depending on timing.
> > In particular, if SIGINT was already pending before pselect() and it finds
> > an already ready fd, the program won't terminate.
> 
> Which matches what I see on a very old Linux system.
> 
> > After the Eric's patches SIGINT will only kill the program if pselect() does
> > not find a ready fd.
> >
> > And this is much more consistent. Now we can simply say that the signal will
> > be delivered only if pselect() fails and returns -EINTR. If it doesn't have
> > a handler the process will be killed, otherwise the handler will be called.
> 
> But is it what the standards mandate?
> Can anyone check how Solaris and any of the BSDs behave?
> I don't have access to any solaris systems (I doubt I'll get the disk to
> spin on the one in my garage).
> I can check NetBSD when I get home.

I tested NetBSD last night.
pselect() always calls the signal handlers even when an fd is ready.
I'm beginning to suspect that this is the 'standards conforming' behaviour.
I don't remember when pselect() was added to the ToG specs, it didn't
go through XNET while I  was going to the meetings.

	David

> 
> The ToG page for pselect() http://pubs.opengroup.org/onlinepubs/9699919799/functions/pselect.html
> says:
>     "If sigmask is not a null pointer, then the pselect() function shall replace
>     the signal mask of the caller by the set of signals pointed to by sigmask
>     before examining the descriptors, and shall restore the signal mask of the
>     calling thread before returning."
> Note that it says 'before examining the descriptors' not 'before blocking'.
> Under the general description about signals it also says that the signal handler
> will be called (or other action happen) when a pending signal is unblocked.
> So unblocking SIGINT (set to SIG_DFL) prior to examining the descriptors
> should be enough to cause the process to exit.
> The fact that signal handlers are not called until 'return to user'
> is really an implementation choice - but (IMHO) it should appear as if they
> were called at the time they became unmasked.
> 
> If nothing else the man pages need a note about the standards and portability.
> 
> 	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

RE: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[David Laight]

From: David Laight
> Sent: 12 June 2019 15:18
> From: Oleg Nesterov
> > Sent: 12 June 2019 14:46
> > On 06/11, David Laight wrote:
> > >
> > > If I have an application that has a loop with a pselect call that
> > > enables SIGINT (without a handler) and, for whatever reason,
> > > one of the fd is always 'ready' then I'd expect a SIGINT
> > > (from ^C) to terminate the program.
> >
> > This was never true.
> >
> > Before Eric's patches SIGINT can kill a process or not, depending on timing.
> > In particular, if SIGINT was already pending before pselect() and it finds
> > an already ready fd, the program won't terminate.
> 
> Which matches what I see on a very old Linux system.
> 
> > After the Eric's patches SIGINT will only kill the program if pselect() does
> > not find a ready fd.
> >
> > And this is much more consistent. Now we can simply say that the signal will
> > be delivered only if pselect() fails and returns -EINTR. If it doesn't have
> > a handler the process will be killed, otherwise the handler will be called.
> 
> But is it what the standards mandate?
> Can anyone check how Solaris and any of the BSDs behave?
> I don't have access to any solaris systems (I doubt I'll get the disk to
> spin on the one in my garage).
> I can check NetBSD when I get home.

I tested NetBSD last night.
pselect() always calls the signal handlers even when an fd is ready.
I'm beginning to suspect that this is the 'standards conforming' behaviour.
I don't remember when pselect() was added to the ToG specs, it didn't
go through XNET while I  was going to the meetings.

	David

> 
> The ToG page for pselect() http://pubs.opengroup.org/onlinepubs/9699919799/functions/pselect.html
> says:
>     "If sigmask is not a null pointer, then the pselect() function shall replace
>     the signal mask of the caller by the set of signals pointed to by sigmask
>     before examining the descriptors, and shall restore the signal mask of the
>     calling thread before returning."
> Note that it says 'before examining the descriptors' not 'before blocking'.
> Under the general description about signals it also says that the signal handler
> will be called (or other action happen) when a pending signal is unblocked.
> So unblocking SIGINT (set to SIG_DFL) prior to examining the descriptors
> should be enough to cause the process to exit.
> The fact that signal handlers are not called until 'return to user'
> is really an implementation choice - but (IMHO) it should appear as if they
> were called at the time they became unmasked.
> 
> If nothing else the man pages need a note about the standards and portability.
> 
> 	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Oleg Nesterov]

On 06/13, David Laight wrote:
>
> I tested NetBSD last night.
> pselect() always calls the signal handlers even when an fd is ready.
> I'm beginning to suspect that this is the 'standards conforming' behaviour.

May be. May be not. I have no idea.

> > The ToG page for pselect() http://pubs.opengroup.org/onlinepubs/9699919799/functions/pselect.html
> > says:
> >     "If sigmask is not a null pointer, then the pselect() function shall replace
> >     the signal mask of the caller by the set of signals pointed to by sigmask
> >     before examining the descriptors, and shall restore the signal mask of the
> >     calling thread before returning."

> > Note that it says 'before examining the descriptors' not 'before blocking'.

And you interpret this as if a pending signal should be delivered in any case,
even if pselect succeeds. Again, perhaps you are right, but to me this is simply
undocumented.

However, linux never did this. Until the commit 854a6ed56839 ("signal: Add
restore_user_sigmask()"). This commit caused regression. We had to revert it.

> > If nothing else the man pages need a note about the standards and portability.

Agreed.

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Oleg Nesterov]

On 06/13, David Laight wrote:
>
> I tested NetBSD last night.
> pselect() always calls the signal handlers even when an fd is ready.
> I'm beginning to suspect that this is the 'standards conforming' behaviour.

May be. May be not. I have no idea.

> > The ToG page for pselect() http://pubs.opengroup.org/onlinepubs/9699919799/functions/pselect.html
> > says:
> >     "If sigmask is not a null pointer, then the pselect() function shall replace
> >     the signal mask of the caller by the set of signals pointed to by sigmask
> >     before examining the descriptors, and shall restore the signal mask of the
> >     calling thread before returning."

> > Note that it says 'before examining the descriptors' not 'before blocking'.

And you interpret this as if a pending signal should be delivered in any case,
even if pselect succeeds. Again, perhaps you are right, but to me this is simply
undocumented.

However, linux never did this. Until the commit 854a6ed56839 ("signal: Add
restore_user_sigmask()"). This commit caused regression. We had to revert it.

> > If nothing else the man pages need a note about the standards and portability.

Agreed.

Oleg.

RE: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[David Laight]

From: Oleg Nesterov
> Sent: 13 June 2019 10:43
> On 06/13, David Laight wrote:
> >
> > I tested NetBSD last night.
> > pselect() always calls the signal handlers even when an fd is ready.
> > I'm beginning to suspect that this is the 'standards conforming' behaviour.
> 
> May be. May be not. I have no idea.
> 
> > > The ToG page for pselect() http://pubs.opengroup.org/onlinepubs/9699919799/functions/pselect.html
> > > says:
> > >     "If sigmask is not a null pointer, then the pselect() function shall replace
> > >     the signal mask of the caller by the set of signals pointed to by sigmask
> > >     before examining the descriptors, and shall restore the signal mask of the
> > >     calling thread before returning."
> 
> > > Note that it says 'before examining the descriptors' not 'before blocking'.
> 
> And you interpret this as if a pending signal should be delivered in any case,
> even if pselect succeeds. Again, perhaps you are right, but to me this is simply
> undocumented.

This text (from http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html) is moderately clear:
    ... if all threads within the process block delivery of the signal, the signal shall
    remain pending on the process until a thread calls a sigwait() function selecting that
    signal, a thread unblocks delivery of the signal, or the action associated with the signal
    is set to ignore the signal.

So when pselect() 'replaces the signal mask' any pending signals should be delivered.
And 'delivery' means 'call the signal handler'.
All Unix systems will defer calling the signal handler until the system call
returns, but this is not mandated by Posix.

> However, linux never did this. Until the commit 854a6ed56839 ("signal: Add
> restore_user_sigmask()"). This commit caused regression. We had to revert it.

That change wasn't expected to change the behaviour...

	David

> > > If nothing else the man pages need a note about the standards and portability.
> 
> Agreed.
> 
> Oleg.

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

RE: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[David Laight]

From: Oleg Nesterov
> Sent: 13 June 2019 10:43
> On 06/13, David Laight wrote:
> >
> > I tested NetBSD last night.
> > pselect() always calls the signal handlers even when an fd is ready.
> > I'm beginning to suspect that this is the 'standards conforming' behaviour.
> 
> May be. May be not. I have no idea.
> 
> > > The ToG page for pselect() http://pubs.opengroup.org/onlinepubs/9699919799/functions/pselect.html
> > > says:
> > >     "If sigmask is not a null pointer, then the pselect() function shall replace
> > >     the signal mask of the caller by the set of signals pointed to by sigmask
> > >     before examining the descriptors, and shall restore the signal mask of the
> > >     calling thread before returning."
> 
> > > Note that it says 'before examining the descriptors' not 'before blocking'.
> 
> And you interpret this as if a pending signal should be delivered in any case,
> even if pselect succeeds. Again, perhaps you are right, but to me this is simply
> undocumented.

This text (from http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html) is moderately clear:
    ... if all threads within the process block delivery of the signal, the signal shall
    remain pending on the process until a thread calls a sigwait() function selecting that
    signal, a thread unblocks delivery of the signal, or the action associated with the signal
    is set to ignore the signal.

So when pselect() 'replaces the signal mask' any pending signals should be delivered.
And 'delivery' means 'call the signal handler'.
All Unix systems will defer calling the signal handler until the system call
returns, but this is not mandated by Posix.

> However, linux never did this. Until the commit 854a6ed56839 ("signal: Add
> restore_user_sigmask()"). This commit caused regression. We had to revert it.

That change wasn't expected to change the behaviour...

	David

> > > If nothing else the man pages need a note about the standards and portability.
> 
> Agreed.
> 
> Oleg.

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Oleg Nesterov]

On 06/13, David Laight wrote:
>
> > And you interpret this as if a pending signal should be delivered in any case,
> > even if pselect succeeds. Again, perhaps you are right, but to me this is simply
> > undocumented.
>
> This text (from http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html) is moderately clear:
>     ... if all threads within the process block delivery of the signal, the signal shall
>     remain pending on the process until a thread calls a sigwait() function selecting that
>     signal, a thread unblocks delivery of the signal, or the action associated with the signal
>     is set to ignore the signal.
>
> So when pselect() 'replaces the signal mask' any pending signals should be delivered.

I fail to understand this logic.


> > However, linux never did this. Until the commit 854a6ed56839 ("signal: Add
> > restore_user_sigmask()"). This commit caused regression. We had to revert it.
>
> That change wasn't expected to change the behaviour...

Yes.

And the changed behaviour matched your understanding of standard. We had to
change it back.

So what do you want from me? ;)

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

[Oleg Nesterov]

On 06/13, David Laight wrote:
>
> > And you interpret this as if a pending signal should be delivered in any case,
> > even if pselect succeeds. Again, perhaps you are right, but to me this is simply
> > undocumented.
>
> This text (from http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html) is moderately clear:
>     ... if all threads within the process block delivery of the signal, the signal shall
>     remain pending on the process until a thread calls a sigwait() function selecting that
>     signal, a thread unblocks delivery of the signal, or the action associated with the signal
>     is set to ignore the signal.
>
> So when pselect() 'replaces the signal mask' any pending signals should be delivered.

I fail to understand this logic.


> > However, linux never did this. Until the commit 854a6ed56839 ("signal: Add
> > restore_user_sigmask()"). This commit caused regression. We had to revert it.
>
> That change wasn't expected to change the behaviour...

Yes.

And the changed behaviour matched your understanding of standard. We had to
change it back.

So what do you want from me? ;)

Oleg.