Skip to contentSkip to conversation

x86: do_signal: simplify the TS_RESTORE_SIGMASK logic

PATCH In review Oleg Nesterov 3 participants 6 messages Jul 10, 2011 – Jul 15, 2011 x86

lore
Share & tools
lore

Oleg Nesterov

author
#0 lore raw

PATCH 1/1 x86: do_signal: simplify the TS_RESTORE_SIGMASK logic

1. do_signal() looks at TS_RESTORE_SIGMASK and calculates the mask which should be stored in the signal frame, then it passes "oldset" to the callees, down to setup_rt_frame().

This is ugly, setup_rt_frame() can do this itself and nobody else needs this sigset_t. Move this code into setup_rt_frame.

2. do_signal() also clears TS_RESTORE_SIGMASK if handle_signal() succeeds.

We can move this to setup_rt_frame() as well, this avoids the unnecessary checks and makes the logic more clear.

3. use set_current_blocked() instead of sigprocmask(SIG_SETMASK), sigprocmask() should be avoided.

Signed-off-by Oleg Nesterov <[email protected]>

---

 arch/x86/kernel/signal.c |   29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)
arch/x86/kernel/signal.c renamed from arch/x86/kernel/signal.c~2_restore_sigmask +10 -19
@@ -653,11 +653,15 @@ int ia32_setup_frame(int sig, struct k_s
 
 static int
 setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
-	       sigset_t *set, struct pt_regs *regs)
+		struct pt_regs *regs)
 {
 	int usig = signr_convert(sig);
+	sigset_t *set = &current->blocked;
 	int ret;
 
+	if (current_thread_info()->status & TS_RESTORE_SIGMASK)
+		set = &current->saved_sigmask;
+
 	/* Set up the stack frame */
 	if (is_ia32) {
 		if (ka->sa.sa_flags & SA_SIGINFO)

@@ -672,12 +676,13 @@ setup_rt_frame(int sig, struct k_sigacti
 		return -EFAULT;
 	}
 
+	current_thread_info()->status &= ~TS_RESTORE_SIGMASK;
 	return ret;
 }
 
 static int
 handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka,
-	      sigset_t *oldset, struct pt_regs *regs)
+		struct pt_regs *regs)
 {
 	sigset_t blocked;
 	int ret;

@@ -712,7 +717,7 @@ handle_signal(unsigned long sig, siginfo
 	    likely(test_and_clear_thread_flag(TIF_FORCED_TF)))
 		regs->flags &= ~X86_EFLAGS_TF;
 
-	ret = setup_rt_frame(sig, ka, info, oldset, regs);
+	ret = setup_rt_frame(sig, ka, info, regs);
 
 	if (ret)
 		return ret;

@@ -758,7 +763,6 @@ static void do_signal(struct pt_regs *re
 	struct k_sigaction ka;
 	siginfo_t info;
 	int signr;
-	sigset_t *oldset;
 
 	/*
 	 * We want the common case to go fast, which is why we may in certain

@@ -770,23 +774,10 @@ static void do_signal(struct pt_regs *re
 	if (!user_mode(regs))
 		return;
 
-	if (current_thread_info()->status & TS_RESTORE_SIGMASK)
-		oldset = &current->saved_sigmask;
-	else
-		oldset = &current->blocked;
-
 	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
 	if (signr > 0) {
 		/* Whee! Actually deliver the signal.  */
-		if (handle_signal(signr, &info, &ka, oldset, regs) == 0) {
-			/*
-			 * A signal was successfully delivered; the saved
-			 * sigmask will have been stored in the signal frame,
-			 * and will be restored by sigreturn, so we can simply
-			 * clear the TS_RESTORE_SIGMASK flag.
-			 */
-			current_thread_info()->status &= ~TS_RESTORE_SIGMASK;
-		}
+		handle_signal(signr, &info, &ka, regs);
 		return;
 	}
 

@@ -814,7 +805,7 @@ static void do_signal(struct pt_regs *re
 	 */
 	if (current_thread_info()->status & TS_RESTORE_SIGMASK) {
 		current_thread_info()->status &= ~TS_RESTORE_SIGMASK;
-		sigprocmask(SIG_SETMASK, &current->saved_sigmask, NULL);
+		set_current_blocked(&current->saved_sigmask);
 	}
 }
#1 lore raw

arch/x86/kernel/signal.c setup_rt_frame(int sig, struct k_sigacti · line 680

Quoted 9 lines · Oleg Nesterov

2. do_signal() also clears TS_RESTORE_SIGMASK if handle_signal() succeeds.

We can move this to setup_rt_frame() as well, this avoids the unnecessary checks and makes the logic more clear.

arch/x86/kernel/signal.c renamed from arch/x86/kernel/signal.c~2_restore_sigmask +1 -0
+	current_thread_info()->status &= ~TS_RESTORE_SIGMASK;
 	return ret;

This is broken. If setup_rt_frame() fails, you don't want to do that.

author
#2 lore raw
Quoted 6 lines · Al Viro
Quoted 3 lines · Oleg Nesterov

2. do_signal() also clears TS_RESTORE_SIGMASK if handle_signal() succeeds.

Quoted 8 lines

arch/x86/kernel/signal.c setup_rt_frame(int sig, struct k_sigacti · line 680

Quoted 6 lines

We can move this to setup_rt_frame() as well, this avoids the unnecessary checks and makes the logic more clear.

arch/x86/kernel/signal.c renamed from arch/x86/kernel/signal.c~2_restore_sigmask +1 -0
+	current_thread_info()->status &= ~TS_RESTORE_SIGMASK;
 	return ret;

This is broken.

You know, I was also going to change this "return ret", "return 0" looks more clear to me. But then I should have renamed "ret", and I decided to leave it alone.

Quoted 1 line

If setup_rt_frame() fails, you don't want to do that.

Sure. Please look at the code, it does

	if (ret) {
		force_sigsegv(sig, current);
		return -EFAULT;
	}

	current_thread_info()->status &= ~TS_RESTORE_SIGMASK;
	return ret;

We clear TS_RESTORE_SIGMASK only if we return 0.

Oleg.

Reviewed-by on v1 1/1
#3 lore raw
In reply to Oleg Nesterov
Quoted 7 lines · On Sun, 10 Jul 2011 20:22:03 +0200 Oleg Nesterov <[email protected]> wrote:

1. do_signal() looks at TS_RESTORE_SIGMASK and calculates the mask which should be stored in the signal frame, then it passes "oldset" to the callees, down to setup_rt_frame().

This is ugly, setup_rt_frame() can do this itself and nobody else needs this sigset_t. Move this code into setup_rt_frame.

Quoted 8 lines

2. do_signal() also clears TS_RESTORE_SIGMASK if handle_signal() succeeds.

We can move this to setup_rt_frame() as well, this avoids the unnecessary checks and makes the logic more clear.

3. use set_current_blocked() instead of sigprocmask(SIG_SETMASK), sigprocmask() should be avoided.

Could you please mention commit e6fa16ab "signal: sigprocmask() should do retarget_shared_pending()", since it's not immediately obvious in this changelog why sigprocmask() should be avoided.

Quoted 1 line
Signed-off-by Oleg Nesterov <[email protected]>

FWIW,

Reviewed-by Matt Fleming <[email protected]>
Signature · 1 line
Matt Fleming, Intel Open Source Technology Center
author
#4 lore raw
Quoted 17 lines · Matt Fleming
Quoted 13 lines · On Sun, 10 Jul 2011 20:22:03 +0200 Oleg Nesterov <[email protected]> wrote:

1. do_signal() looks at TS_RESTORE_SIGMASK and calculates the mask which should be stored in the signal frame, then it passes "oldset" to the callees, down to setup_rt_frame().

This is ugly, setup_rt_frame() can do this itself and nobody else needs this sigset_t. Move this code into setup_rt_frame.

2. do_signal() also clears TS_RESTORE_SIGMASK if handle_signal() succeeds.

We can move this to setup_rt_frame() as well, this avoids the unnecessary checks and makes the logic more clear.

Quoted 6 lines
Quoted 2 lines

3. use set_current_blocked() instead of sigprocmask(SIG_SETMASK), sigprocmask() should be avoided.

Could you please mention commit e6fa16ab "signal: sigprocmask() should do retarget_shared_pending()", since it's not immediately obvious in this changelog why sigprocmask() should be avoided.

Well, sigprocmask(SIG_SETMASK) is fine from the correctness pov, it calls set_current_blocked().

sigprocmask() should be avoided because it is strange interface. It has numeruos callers, but in fact almost all of them could use set_current_blocked() (ignoring sys_rt_sigprocmask).

Linus suggested to simply kill sigprocmask(). I am not sure, but at least it shouldn't be abused and its last argument is confusing.

Quoted 1 line
Reviewed-by Matt Fleming <[email protected]>

Thanks for looking!

Oleg.

author
#5 lore raw

[tip:x86/signal] x86, do_signal: Simplify the TS_RESTORE_SIGMASK logic

In reply to Oleg Nesterov
Commit-ID:  9b429620740945363b746414e8b9a84b8119914c
Gitweb:     http://git.kernel.org/tip/9b429620740945363b746414e8b9a84b8119914c
Author:     Oleg Nesterov <[email protected]>
AuthorDate: Sun, 10 Jul 2011 20:22:03 +0200
Committer:  H. Peter Anvin <[email protected]>
CommitDate: Thu, 14 Jul 2011 21:22:11 -0700

x86, do_signal: Simplify the TS_RESTORE_SIGMASK logic

1. do_signal() looks at TS_RESTORE_SIGMASK and calculates the mask which should be stored in the signal frame, then it passes "oldset" to the callees, down to setup_rt_frame().

This is ugly, setup_rt_frame() can do this itself and nobody else needs this sigset_t. Move this code into setup_rt_frame.

2. do_signal() also clears TS_RESTORE_SIGMASK if handle_signal() succeeds.

We can move this to setup_rt_frame() as well, this avoids the unnecessary checks and makes the logic more clear.

3. use set_current_blocked() instead of sigprocmask(SIG_SETMASK), sigprocmask() should be avoided.

Signed-off-by Oleg Nesterov <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by H. Peter Anvin <[email protected]>
---
 arch/x86/kernel/signal.c |   29 ++++++++++-------------------
 1 files changed, 10 insertions(+), 19 deletions(-)
arch/x86/kernel/signal.c +10 -19
index bf9345d..8c55f97 100644
@@ -651,11 +651,15 @@ int ia32_setup_frame(int sig, struct k_sigaction *ka,
 
 static int
 setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
-	       sigset_t *set, struct pt_regs *regs)
+		struct pt_regs *regs)
 {
 	int usig = signr_convert(sig);
+	sigset_t *set = &current->blocked;
 	int ret;
 
+	if (current_thread_info()->status & TS_RESTORE_SIGMASK)
+		set = &current->saved_sigmask;
+
 	/* Set up the stack frame */
 	if (is_ia32) {
 		if (ka->sa.sa_flags & SA_SIGINFO)

@@ -670,12 +674,13 @@ setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
 		return -EFAULT;
 	}
 
+	current_thread_info()->status &= ~TS_RESTORE_SIGMASK;
 	return ret;
 }
 
 static int
 handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka,
-	      sigset_t *oldset, struct pt_regs *regs)
+		struct pt_regs *regs)
 {
 	sigset_t blocked;
 	int ret;

@@ -710,7 +715,7 @@ handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka,
 	    likely(test_and_clear_thread_flag(TIF_FORCED_TF)))
 		regs->flags &= ~X86_EFLAGS_TF;
 
-	ret = setup_rt_frame(sig, ka, info, oldset, regs);
+	ret = setup_rt_frame(sig, ka, info, regs);
 
 	if (ret)
 		return ret;

@@ -765,7 +770,6 @@ static void do_signal(struct pt_regs *regs)
 	struct k_sigaction ka;
 	siginfo_t info;
 	int signr;
-	sigset_t *oldset;
 
 	/*
 	 * We want the common case to go fast, which is why we may in certain

@@ -777,23 +781,10 @@ static void do_signal(struct pt_regs *regs)
 	if (!user_mode(regs))
 		return;
 
-	if (current_thread_info()->status & TS_RESTORE_SIGMASK)
-		oldset = &current->saved_sigmask;
-	else
-		oldset = &current->blocked;
-
 	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
 	if (signr > 0) {
 		/* Whee! Actually deliver the signal.  */
-		if (handle_signal(signr, &info, &ka, oldset, regs) == 0) {
-			/*
-			 * A signal was successfully delivered; the saved
-			 * sigmask will have been stored in the signal frame,
-			 * and will be restored by sigreturn, so we can simply
-			 * clear the TS_RESTORE_SIGMASK flag.
-			 */
-			current_thread_info()->status &= ~TS_RESTORE_SIGMASK;
-		}
+		handle_signal(signr, &info, &ka, regs);
 		return;
 	}
 

@@ -821,7 +812,7 @@ static void do_signal(struct pt_regs *regs)
 	 */
 	if (current_thread_info()->status & TS_RESTORE_SIGMASK) {
 		current_thread_info()->status &= ~TS_RESTORE_SIGMASK;
-		sigprocmask(SIG_SETMASK, &current->saved_sigmask, NULL);
+		set_current_blocked(&current->saved_sigmask);
 	}
 }