All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86: do_signal: simplify the TS_RESTORE_SIGMASK logic
@ 2011-07-10 18:22 Oleg Nesterov
  2011-07-10 21:08 ` Al Viro
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Oleg Nesterov @ 2011-07-10 18:22 UTC (permalink / raw)
  To: Andrew Morton; +Cc: David Howells, David Woodhouse, Ingo Molnar, linux-kernel

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 <oleg@redhat.com>
---

 arch/x86/kernel/signal.c |   29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

--- ptrace/arch/x86/kernel/signal.c~2_restore_sigmask	2011-07-10 18:06:30.000000000 +0200
+++ ptrace/arch/x86/kernel/signal.c	2011-07-10 19:45:21.000000000 +0200
@@ -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);
 	}
 }
 


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] x86: do_signal: simplify the TS_RESTORE_SIGMASK logic
  2011-07-10 18:22 [PATCH] x86: do_signal: simplify the TS_RESTORE_SIGMASK logic Oleg Nesterov
@ 2011-07-10 21:08 ` Al Viro
  2011-07-11 11:39   ` Oleg Nesterov
  2011-07-13  9:25 ` Matt Fleming
  2011-07-15  5:47 ` [tip:x86/signal] x86, do_signal: Simplify " tip-bot for Oleg Nesterov
  2 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2011-07-10 21:08 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Andrew Morton, David Howells, David Woodhouse, Ingo Molnar, linux-kernel

On Sun, Jul 10, 2011 at 08:22:03PM +0200, Oleg Nesterov wrote:

> 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.


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

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] x86: do_signal: simplify the TS_RESTORE_SIGMASK logic
  2011-07-10 21:08 ` Al Viro
@ 2011-07-11 11:39   ` Oleg Nesterov
  0 siblings, 0 replies; 6+ messages in thread
From: Oleg Nesterov @ 2011-07-11 11:39 UTC (permalink / raw)
  To: Al Viro
  Cc: Andrew Morton, David Howells, David Woodhouse, Ingo Molnar, linux-kernel

On 07/10, Al Viro wrote:
>
> On Sun, Jul 10, 2011 at 08:22:03PM +0200, Oleg Nesterov wrote:
>
> > 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.
>
>
> > +	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.

> 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.


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] x86: do_signal: simplify the TS_RESTORE_SIGMASK logic
  2011-07-10 18:22 [PATCH] x86: do_signal: simplify the TS_RESTORE_SIGMASK logic Oleg Nesterov
  2011-07-10 21:08 ` Al Viro
@ 2011-07-13  9:25 ` Matt Fleming
  2011-07-13 15:23   ` Oleg Nesterov
  2011-07-15  5:47 ` [tip:x86/signal] x86, do_signal: Simplify " tip-bot for Oleg Nesterov
  2 siblings, 1 reply; 6+ messages in thread
From: Matt Fleming @ 2011-07-13  9:25 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Andrew Morton, David Howells, David Woodhouse, Ingo Molnar, linux-kernel

On Sun, 10 Jul 2011 20:22:03 +0200
Oleg Nesterov <oleg@redhat.com> 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.
> 
> 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.

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

FWIW,

Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com>

-- 
Matt Fleming, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] x86: do_signal: simplify the TS_RESTORE_SIGMASK logic
  2011-07-13  9:25 ` Matt Fleming
@ 2011-07-13 15:23   ` Oleg Nesterov
  0 siblings, 0 replies; 6+ messages in thread
From: Oleg Nesterov @ 2011-07-13 15:23 UTC (permalink / raw)
  To: Matt Fleming
  Cc: Andrew Morton, David Howells, David Woodhouse, Ingo Molnar, linux-kernel

On 07/13, Matt Fleming wrote:
>
> On Sun, 10 Jul 2011 20:22:03 +0200
> Oleg Nesterov <oleg@redhat.com> 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.
> >
> > 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.

> Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com>

Thanks for looking!

Oleg.


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [tip:x86/signal] x86, do_signal: Simplify the TS_RESTORE_SIGMASK logic
  2011-07-10 18:22 [PATCH] x86: do_signal: simplify the TS_RESTORE_SIGMASK logic Oleg Nesterov
  2011-07-10 21:08 ` Al Viro
  2011-07-13  9:25 ` Matt Fleming
@ 2011-07-15  5:47 ` tip-bot for Oleg Nesterov
  2 siblings, 0 replies; 6+ messages in thread
From: tip-bot for Oleg Nesterov @ 2011-07-15  5:47 UTC (permalink / raw)
  To: linux-tip-commits; +Cc: linux-kernel, hpa, mingo, oleg, tglx, hpa

Commit-ID:  9b429620740945363b746414e8b9a84b8119914c
Gitweb:     http://git.kernel.org/tip/9b429620740945363b746414e8b9a84b8119914c
Author:     Oleg Nesterov <oleg@redhat.com>
AuthorDate: Sun, 10 Jul 2011 20:22:03 +0200
Committer:  H. Peter Anvin <hpa@linux.intel.com>
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 <oleg@redhat.com>
Link: http://lkml.kernel.org/r/20110710182203.GA27979@redhat.com
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/kernel/signal.c |   29 ++++++++++-------------------
 1 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index bf9345d..8c55f97 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -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);
 	}
 }
 

^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-07-15  5:47 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-10 18:22 [PATCH] x86: do_signal: simplify the TS_RESTORE_SIGMASK logic Oleg Nesterov
2011-07-10 21:08 ` Al Viro
2011-07-11 11:39   ` Oleg Nesterov
2011-07-13  9:25 ` Matt Fleming
2011-07-13 15:23   ` Oleg Nesterov
2011-07-15  5:47 ` [tip:x86/signal] x86, do_signal: Simplify " tip-bot for Oleg Nesterov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.