synchronous signal in the blocked signal context

synchronous signal in the blocked signal context

[Siddha, Suresh B]

This patch (b0423a0d9cc836b2c3d796623cd19236bfedfe63)

[PATCH] Remove duplicate code in signal.c

reverts a patch introduced by Linus long time back.

http://linux.bkbits.net:8080/linux-2.6/gnupatch@3f0621871mAhWfFZzuA74eKKLvE6OQ

Was this intentional?

With the current mainline code, SIGSEGV inside a SIGSEGV handler will endup
in linux handling endless recursive faults.

Just wondering if this has been discussed before and is intentional.

thanks,
suresh

Re: synchronous signal in the blocked signal context

[Linus Torvalds]

On Mon, 31 Jul 2006, Siddha, Suresh B wrote:
>
> This patch (b0423a0d9cc836b2c3d796623cd19236bfedfe63)
> 
> [PATCH] Remove duplicate code in signal.c
> 
> reverts a patch introduced by Linus long time back.

Good catch.

> Was this intentional?
> 
> With the current mainline code, SIGSEGV inside a SIGSEGV handler will endup
> in linux handling endless recursive faults.
> 
> Just wondering if this has been discussed before and is intentional.

It certainly wasn't discussed, and I don't think it was intentional. We 
should _not_ just unblock a blocked signal. We should kill the process, 
because sending the signal is actually very very wrong.

Paul? Should I just revert, or did you have some deeper reason for it?

		Linus

Re: synchronous signal in the blocked signal context

[Paul E. McKenney]

On Mon, Jul 31, 2006 at 09:54:47PM -0700, Linus Torvalds wrote:
> 
> 
> On Mon, 31 Jul 2006, Siddha, Suresh B wrote:
> >
> > This patch (b0423a0d9cc836b2c3d796623cd19236bfedfe63)
> > 
> > [PATCH] Remove duplicate code in signal.c
> > 
> > reverts a patch introduced by Linus long time back.
> 
> Good catch.
> 
> > Was this intentional?
> > 
> > With the current mainline code, SIGSEGV inside a SIGSEGV handler will endup
> > in linux handling endless recursive faults.
> > 
> > Just wondering if this has been discussed before and is intentional.
> 
> It certainly wasn't discussed, and I don't think it was intentional. We 
> should _not_ just unblock a blocked signal. We should kill the process, 
> because sending the signal is actually very very wrong.
> 
> Paul? Should I just revert, or did you have some deeper reason for it?

I cannot claim any deep thought on this one, so please do revert it.

Next time I submit a patch to code with which I am not intimately
familiar, I clearly need to carefully review the earlier patches.  :-/

							Thanx, Paul

Re: synchronous signal in the blocked signal context

[Linus Torvalds]

On Tue, 1 Aug 2006, Paul E. McKenney wrote:
> > 
> > Paul? Should I just revert, or did you have some deeper reason for it?
> 
> I cannot claim any deep thought on this one, so please do revert it.

Well, I do have to say that I like the notion of trying to have the _same_ 
semantics for "force_sig_info()" and "force_sig_specific()", so in that 
way your patch is fine - I just missed the fact that it changed it back to 
the old broken ones (that results in endless SIGSEGV's if the SIGSEGV 
happens when setting up the handler for the SIGSEGV and other 
"interesting" issues, where a bug can result in the user process hanging 
instead of just killing it outright).

However, I wonder if the _proper_ fix is to just either remove 
"force_sig_specific()" entirely, or just make that one match the semantics 
of "force_sig_info()" instead (rather than doing it the other way - change 
for_sig_specific() to match force_sig_info()).

force_sig_info() has only two uses, and both should be ok with the 
force_sig_specific() semantics, since they are for SIGSTOP and SIGKILL 
respectively, and those should not be blockable unless you're a kernel 
thread (and I don't think either of them could validly ever be used with 
kernel threads anyway), so doing it the other way around _should_ be ok.

Paul, Suresh, would something like this work for you instead?

		Linus
----
diff --git a/kernel/signal.c b/kernel/signal.c
index 7fe874d..bfdb568 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -791,22 +791,31 @@ out:
 /*
  * Force a signal that the process can't ignore: if necessary
  * we unblock the signal and change any SIG_IGN to SIG_DFL.
+ *
+ * Note: If we unblock the signal, we always reset it to SIG_DFL,
+ * since we do not want to have a signal handler that was blocked
+ * be invoked when user space had explicitly blocked it.
+ *
+ * We don't want to have recursive SIGSEGV's etc, for example.
  */
-
 int
 force_sig_info(int sig, struct siginfo *info, struct task_struct *t)
 {
 	unsigned long int flags;
-	int ret;
+	int ret, blocked, ignored;
+	struct k_sigaction *action;
 
 	spin_lock_irqsave(&t->sighand->siglock, flags);
-	if (t->sighand->action[sig-1].sa.sa_handler == SIG_IGN) {
-		t->sighand->action[sig-1].sa.sa_handler = SIG_DFL;
-	}
-	if (sigismember(&t->blocked, sig)) {
-		sigdelset(&t->blocked, sig);
+	action = &t->sighand->action[sig-1];
+	ignored = action->sa.sa_handler == SIG_IGN;
+	blocked = sigismember(&t->blocked, sig);
+	if (blocked || ignored) {
+		action->sa.sa_handler = SIG_DFL;
+		if (blocked) {
+			sigdelset(&t->blocked, sig);
+			recalc_sigpending_tsk(t);
+		}
 	}
-	recalc_sigpending_tsk(t);
 	ret = specific_send_sig_info(sig, info, t);
 	spin_unlock_irqrestore(&t->sighand->siglock, flags);
 

Re: synchronous signal in the blocked signal context

[Siddha, Suresh B]

On Tue, Aug 01, 2006 at 08:25:12AM -0700, Linus Torvalds wrote:
> 
> 
> On Tue, 1 Aug 2006, Paul E. McKenney wrote:
> > > 
> > > Paul? Should I just revert, or did you have some deeper reason for it?
> > 
> > I cannot claim any deep thought on this one, so please do revert it.
> 
> Well, I do have to say that I like the notion of trying to have the _same_ 
> semantics for "force_sig_info()" and "force_sig_specific()", so in that 
> way your patch is fine - I just missed the fact that it changed it back to 
> the old broken ones (that results in endless SIGSEGV's if the SIGSEGV 
> happens when setting up the handler for the SIGSEGV and other 
> "interesting" issues, where a bug can result in the user process hanging 
> instead of just killing it outright).
> 
> However, I wonder if the _proper_ fix is to just either remove 
> "force_sig_specific()" entirely, or just make that one match the semantics 
> of "force_sig_info()" instead (rather than doing it the other way - change 
> for_sig_specific() to match force_sig_info()).
> 
> force_sig_info() has only two uses, and both should be ok with the 
> force_sig_specific() semantics, since they are for SIGSTOP and SIGKILL 
> respectively, and those should not be blockable unless you're a kernel 
> thread (and I don't think either of them could validly ever be used with 
> kernel threads anyway), so doing it the other way around _should_ be ok.
> 
> Paul, Suresh, would something like this work for you instead?

Yes.

Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>

> 
> 		Linus
> ----
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 7fe874d..bfdb568 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -791,22 +791,31 @@ out:
>  /*
>   * Force a signal that the process can't ignore: if necessary
>   * we unblock the signal and change any SIG_IGN to SIG_DFL.
> + *
> + * Note: If we unblock the signal, we always reset it to SIG_DFL,
> + * since we do not want to have a signal handler that was blocked
> + * be invoked when user space had explicitly blocked it.
> + *
> + * We don't want to have recursive SIGSEGV's etc, for example.
>   */
> -
>  int
>  force_sig_info(int sig, struct siginfo *info, struct task_struct *t)
>  {
>  	unsigned long int flags;
> -	int ret;
> +	int ret, blocked, ignored;
> +	struct k_sigaction *action;
>  
>  	spin_lock_irqsave(&t->sighand->siglock, flags);
> -	if (t->sighand->action[sig-1].sa.sa_handler == SIG_IGN) {
> -		t->sighand->action[sig-1].sa.sa_handler = SIG_DFL;
> -	}
> -	if (sigismember(&t->blocked, sig)) {
> -		sigdelset(&t->blocked, sig);
> +	action = &t->sighand->action[sig-1];
> +	ignored = action->sa.sa_handler == SIG_IGN;
> +	blocked = sigismember(&t->blocked, sig);
> +	if (blocked || ignored) {
> +		action->sa.sa_handler = SIG_DFL;
> +		if (blocked) {
> +			sigdelset(&t->blocked, sig);
> +			recalc_sigpending_tsk(t);
> +		}
>  	}
> -	recalc_sigpending_tsk(t);
>  	ret = specific_send_sig_info(sig, info, t);
>  	spin_unlock_irqrestore(&t->sighand->siglock, flags);
>  

Re: synchronous signal in the blocked signal context

[Paul E. McKenney]

On Tue, Aug 01, 2006 at 08:25:12AM -0700, Linus Torvalds wrote:
> 
> 
> On Tue, 1 Aug 2006, Paul E. McKenney wrote:
> > > 
> > > Paul? Should I just revert, or did you have some deeper reason for it?
> > 
> > I cannot claim any deep thought on this one, so please do revert it.
> 
> Well, I do have to say that I like the notion of trying to have the _same_ 
> semantics for "force_sig_info()" and "force_sig_specific()", so in that 
> way your patch is fine - I just missed the fact that it changed it back to 
> the old broken ones (that results in endless SIGSEGV's if the SIGSEGV 
> happens when setting up the handler for the SIGSEGV and other 
> "interesting" issues, where a bug can result in the user process hanging 
> instead of just killing it outright).

I guess I am glad I was not -totally- insane when submitting the
original patch.  ;-)

> However, I wonder if the _proper_ fix is to just either remove 
> "force_sig_specific()" entirely, or just make that one match the semantics 
> of "force_sig_info()" instead (rather than doing it the other way - change 
> for_sig_specific() to match force_sig_info()).

One question -- the original (2.6.14 or thereabouts) version of
force_sig_info() would do the sigdelset() and recalc_sig_pending()
even if the signal was not blocked, while your patch below would
do sigdelset()/recalc_sig_pending() only if the signal was blocked,
even if it was not ignored.  Not sure this matters, but thought I
should ask.

> force_sig_info() has only two uses, and both should be ok with the 

s/force_sig_info/force_sig_specific/?  I see >100 uses of force_sig_info().

> force_sig_specific() semantics, since they are for SIGSTOP and SIGKILL 
> respectively, and those should not be blockable unless you're a kernel 
> thread (and I don't think either of them could validly ever be used with 
> kernel threads anyway), so doing it the other way around _should_ be ok.

OK, SIGSTOP and SIGKILL cannot be ignored or blocked.  So wouldn't
they end up skipping the recalc_sig_pending() in the new code,
where they would have ended up executing it in the 2.6.14 version
of force_sig_specific()?

Assuming I am at least semi-sane, one possible way to fix shown below.

						Thanx, Paul

> Paul, Suresh, would something like this work for you instead?
> 
> 		Linus
> ----
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 7fe874d..bfdb568 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -791,22 +791,31 @@ out:
>  /*
>   * Force a signal that the process can't ignore: if necessary
>   * we unblock the signal and change any SIG_IGN to SIG_DFL.
> + *
> + * Note: If we unblock the signal, we always reset it to SIG_DFL,
> + * since we do not want to have a signal handler that was blocked
> + * be invoked when user space had explicitly blocked it.
> + *
> + * We don't want to have recursive SIGSEGV's etc, for example.
>   */
> -
>  int
>  force_sig_info(int sig, struct siginfo *info, struct task_struct *t)
>  {
>  	unsigned long int flags;
> -	int ret;
> +	int ret, blocked, ignored;

	int alwaysfatal;

> +	struct k_sigaction *action;
>  
>  	spin_lock_irqsave(&t->sighand->siglock, flags);
> -	if (t->sighand->action[sig-1].sa.sa_handler == SIG_IGN) {
> -		t->sighand->action[sig-1].sa.sa_handler = SIG_DFL;
> -	}
> -	if (sigismember(&t->blocked, sig)) {
> -		sigdelset(&t->blocked, sig);
> +	action = &t->sighand->action[sig-1];
> +	ignored = action->sa.sa_handler == SIG_IGN;

	alwaysfatal = sig == SIGKILL || sig == SIGSTOP;

> +	blocked = sigismember(&t->blocked, sig);
> +	if (blocked || ignored) {

	if (blocked || ignored || alwaysfatal) {

> +		action->sa.sa_handler = SIG_DFL;
> +		if (blocked) {

		if (blocked || alwaysfatal) {

> +			sigdelset(&t->blocked, sig);
> +			recalc_sigpending_tsk(t);
> +		}
>  	}
> -	recalc_sigpending_tsk(t);
>  	ret = specific_send_sig_info(sig, info, t);
>  	spin_unlock_irqrestore(&t->sighand->siglock, flags);
>  

Re: synchronous signal in the blocked signal context

[Siddha, Suresh B]

On Tue, Aug 01, 2006 at 11:13:32AM -0700, Paul E. McKenney wrote:
> On Tue, Aug 01, 2006 at 08:25:12AM -0700, Linus Torvalds wrote:
> > 
> > 
> > On Tue, 1 Aug 2006, Paul E. McKenney wrote:
> > > > 
> > > > Paul? Should I just revert, or did you have some deeper reason for it?
> > > 
> > > I cannot claim any deep thought on this one, so please do revert it.
> > 
> > Well, I do have to say that I like the notion of trying to have the _same_ 
> > semantics for "force_sig_info()" and "force_sig_specific()", so in that 
> > way your patch is fine - I just missed the fact that it changed it back to 
> > the old broken ones (that results in endless SIGSEGV's if the SIGSEGV 
> > happens when setting up the handler for the SIGSEGV and other 
> > "interesting" issues, where a bug can result in the user process hanging 
> > instead of just killing it outright).
> 
> I guess I am glad I was not -totally- insane when submitting the
> original patch.  ;-)
> 
> > However, I wonder if the _proper_ fix is to just either remove 
> > "force_sig_specific()" entirely, or just make that one match the semantics 
> > of "force_sig_info()" instead (rather than doing it the other way - change 
> > for_sig_specific() to match force_sig_info()).
> 
> One question -- the original (2.6.14 or thereabouts) version of
> force_sig_info() would do the sigdelset() and recalc_sig_pending()
> even if the signal was not blocked, while your patch below would
> do sigdelset()/recalc_sig_pending() only if the signal was blocked,
> even if it was not ignored.  Not sure this matters, but thought I
> should ask.
> 
> > force_sig_info() has only two uses, and both should be ok with the 
> 
> s/force_sig_info/force_sig_specific/?  I see >100 uses of force_sig_info().
> 
> > force_sig_specific() semantics, since they are for SIGSTOP and SIGKILL 
> > respectively, and those should not be blockable unless you're a kernel 
> > thread (and I don't think either of them could validly ever be used with 
> > kernel threads anyway), so doing it the other way around _should_ be ok.
> 
> OK, SIGSTOP and SIGKILL cannot be ignored or blocked.  So wouldn't
> they end up skipping the recalc_sig_pending() in the new code,
> where they would have ended up executing it in the 2.6.14 version
> of force_sig_specific()?

I don't think it matters.
signal_wake_up() in the path of specific_send_sig_info() should anyhow
do that.

thanks,
suresh

Re: synchronous signal in the blocked signal context

[Paul E. McKenney]

On Tue, Aug 01, 2006 at 11:13:04AM -0700, Siddha, Suresh B wrote:
> On Tue, Aug 01, 2006 at 11:13:32AM -0700, Paul E. McKenney wrote:
> > On Tue, Aug 01, 2006 at 08:25:12AM -0700, Linus Torvalds wrote:
> > > 
> > > 
> > > On Tue, 1 Aug 2006, Paul E. McKenney wrote:
> > > > > 
> > > > > Paul? Should I just revert, or did you have some deeper reason for it?
> > > > 
> > > > I cannot claim any deep thought on this one, so please do revert it.
> > > 
> > > Well, I do have to say that I like the notion of trying to have the _same_ 
> > > semantics for "force_sig_info()" and "force_sig_specific()", so in that 
> > > way your patch is fine - I just missed the fact that it changed it back to 
> > > the old broken ones (that results in endless SIGSEGV's if the SIGSEGV 
> > > happens when setting up the handler for the SIGSEGV and other 
> > > "interesting" issues, where a bug can result in the user process hanging 
> > > instead of just killing it outright).
> > 
> > I guess I am glad I was not -totally- insane when submitting the
> > original patch.  ;-)
> > 
> > > However, I wonder if the _proper_ fix is to just either remove 
> > > "force_sig_specific()" entirely, or just make that one match the semantics 
> > > of "force_sig_info()" instead (rather than doing it the other way - change 
> > > for_sig_specific() to match force_sig_info()).
> > 
> > One question -- the original (2.6.14 or thereabouts) version of
> > force_sig_info() would do the sigdelset() and recalc_sig_pending()
> > even if the signal was not blocked, while your patch below would
> > do sigdelset()/recalc_sig_pending() only if the signal was blocked,
> > even if it was not ignored.  Not sure this matters, but thought I
> > should ask.
> > 
> > > force_sig_info() has only two uses, and both should be ok with the 
> > 
> > s/force_sig_info/force_sig_specific/?  I see >100 uses of force_sig_info().
> > 
> > > force_sig_specific() semantics, since they are for SIGSTOP and SIGKILL 
> > > respectively, and those should not be blockable unless you're a kernel 
> > > thread (and I don't think either of them could validly ever be used with 
> > > kernel threads anyway), so doing it the other way around _should_ be ok.
> > 
> > OK, SIGSTOP and SIGKILL cannot be ignored or blocked.  So wouldn't
> > they end up skipping the recalc_sig_pending() in the new code,
> > where they would have ended up executing it in the 2.6.14 version
> > of force_sig_specific()?
> 
> I don't think it matters.
> signal_wake_up() in the path of specific_send_sig_info() should anyhow
> do that.

OK, looks plausible upon reviewing the code paths.

							Thanx, Paul