Skip to contentSkip to conversation

signal: sigprocmask fixes

RFC PATCH Superseded Oleg Nesterov 4 participants 20 messages Apr 11, 2011 – Apr 14, 2011 x86

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Oleg Nesterov

author
#0 lore raw

cover signal: sigprocmask fixes

Hello,

I am not sure this is really wrong and should be fixed. Please comment.

1-2: probably make sense anyway, cleanup + optimization.

4: the main change. From the changelog,

	In short, almost every changing of current->blocked is wrong,
	or at least can lead to the unexpected results.

5-6: If this should be fixed, then we need a lot more trivial changes
like these ones.

What do you all think?

In theory this can explain the hang reported by Nikita, but this is not clear.

Oleg.

author
#1 lore raw

PATCH 1/6 signal: introduce retarget_shared_pending()

No functional changes. Move the notyfy-other-threads code from exit_signals() to the new helper, retarget_shared_pending().

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

---

 kernel/signal.c |   26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)
kernel/signal.c renamed from kernel/signal.c~1_add_retarget_helper +17 -9
@@ -2017,10 +2017,25 @@ relock:
 	return signr;
 }
 
+/*
+ * It could be that complete_signal() picked us to notify about the
+ * group-wide signal. Another thread should be notified now to take
+ * the signal since we will not.
+ */
+static void retarget_shared_pending(struct task_struct *tsk)
+{
+	struct task_struct *t;
+
+	t = tsk;
+	while_each_thread(tsk, t) {
+		if (!signal_pending(t) && !(t->flags & PF_EXITING))
+			recalc_sigpending_and_wake(t);
+	}
+}
+
 void exit_signals(struct task_struct *tsk)
 {
 	int group_stop = 0;
-	struct task_struct *t;
 
 	if (thread_group_empty(tsk) || signal_group_exit(tsk->signal)) {
 		tsk->flags |= PF_EXITING;

@@ -2036,14 +2051,7 @@ void exit_signals(struct task_struct *ts
 	if (!signal_pending(tsk))
 		goto out;
 
-	/*
-	 * It could be that __group_complete_signal() choose us to
-	 * notify about group-wide signal. Another thread should be
-	 * woken now to take the signal since we will not.
-	 */
-	for (t = tsk; (t = next_thread(t)) != tsk; )
-		if (!signal_pending(t) && !(t->flags & PF_EXITING))
-			recalc_sigpending_and_wake(t);
+	retarget_shared_pending(tsk);
 
 	if (unlikely(tsk->signal->group_stop_count) &&
 			!--tsk->signal->group_stop_count) {
author
#3 lore raw

PATCH 2/6 signal: retarget_shared_pending: consider shared/unblocked signals only

In reply to Oleg Nesterov

exit_signals() checks signal_pending() before retarget_shared_pending() but this is suboptimal. We can avoid the while_each_thread() loop in case when there are no shared signals visible to us.

Add the has_pending_signals(shared_pending, blocked) check. We don not use tsk->blocked directly but pass this sigset_t as an argument, this is needed for the next patch.

Note: we can optimize this more. while_each_thread(t) can check t->blocked into account and stop after every pending signal has the new target.

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

---

 kernel/signal.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
kernel/signal.c renamed from kernel/signal.c~2_optimize_retarget +5 -2
@@ -2022,10 +2022,13 @@ relock:
  * group-wide signal. Another thread should be notified now to take
  * the signal since we will not.
  */
-static void retarget_shared_pending(struct task_struct *tsk)
+static void retarget_shared_pending(struct task_struct *tsk, sigset_t *blocked)
 {
 	struct task_struct *t;
 
+	if (!has_pending_signals(&tsk->signal->shared_pending.signal, blocked))
+		return;
+
 	t = tsk;
 	while_each_thread(tsk, t) {
 		if (!signal_pending(t) && !(t->flags & PF_EXITING))

@@ -2051,7 +2054,7 @@ void exit_signals(struct task_struct *ts
 	if (!signal_pending(tsk))
 		goto out;
 
-	retarget_shared_pending(tsk);
+	retarget_shared_pending(tsk, &tsk->blocked);
 
 	if (unlikely(tsk->signal->group_stop_count) &&
 			!--tsk->signal->group_stop_count) {
Reviewed-by on v1 2/6
#4 lore raw

Re: [PATCH 2/6] signal: retarget_shared_pending: consider shared/unblocked signals only

Quoted 9 lines · On Mon, 11 Apr 2011 19:20:45 +0200 Oleg Nesterov <[email protected]> wrote:

exit_signals() checks signal_pending() before retarget_shared_pending() but this is suboptimal. We can avoid the while_each_thread() loop in case when there are no shared signals visible to us.

Add the has_pending_signals(shared_pending, blocked) check. We don not use tsk->blocked directly but pass this sigset_t as an argument, this is needed for the next patch.

Quoted 5 lines

Note: we can optimize this more. while_each_thread(t) can check t->blocked into account and stop after every pending signal has the new target.

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

Nice. Do you have plans to implement that further optimisation?

Reviewed-by Matt Fleming <[email protected]>
author
#5 lore raw

Re: [PATCH 2/6] signal: retarget_shared_pending: consider shared/unblocked signals only

Quoted 13 lines · Matt Fleming
Quoted 9 lines · On Mon, 11 Apr 2011 19:20:45 +0200 Oleg Nesterov <[email protected]> wrote:

exit_signals() checks signal_pending() before retarget_shared_pending() but this is suboptimal. We can avoid the while_each_thread() loop in case when there are no shared signals visible to us.

Add the has_pending_signals(shared_pending, blocked) check. We don not use tsk->blocked directly but pass this sigset_t as an argument, this is needed for the next patch.

Quoted 7 lines
Quoted 5 lines

Note: we can optimize this more. while_each_thread(t) can check t->blocked into account and stop after every pending signal has the new target.

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

Nice. Do you have plans to implement that further optimisation?

Probably yes, this is straightforward...

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

Thanks Matt, thanks all.

I'll reply to other emails tomorrow. I was distracted by other things, sorry. Oh, as always :/

Oleg.

author
#6 lore raw

PATCH 3/6 signal: sigprocmask: narrow the scope of ->sigloc

In reply to Oleg Nesterov

No functional changes, preparation to simplify the review of the next change.

1. We can read current->block lockless, nobody else can ever change this mask.

2. Calculate the resulting sigset_t outside of ->siglock into the temporary variable, then take ->siglock and change ->blocked.

Also, kill the stale comment about BKL.

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

---

 kernel/signal.c |   28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)
kernel/signal.c renamed from kernel/signal.c~3_cleanup_sigprocmask +12 -16
@@ -2101,12 +2101,6 @@ long do_no_restart_syscall(struct restar
 }
 
 /*
- * We don't need to get the kernel lock - this is all local to this
- * particular thread.. (and that's good, because this is _heavily_
- * used by various programs)
- */
-
-/*
  * This is also useful for kernel threads that want to temporarily
  * (or permanently) block certain signals.
  *

@@ -2116,30 +2110,32 @@ long do_no_restart_syscall(struct restar
  */
 int sigprocmask(int how, sigset_t *set, sigset_t *oldset)
 {
-	int error;
+	struct task_struct *tsk = current;
+	sigset_t newset;
 
-	spin_lock_irq(&current->sighand->siglock);
 	if (oldset)
-		*oldset = current->blocked;
+		*oldset = tsk->blocked;
 
-	error = 0;
 	switch (how) {
 	case SIG_BLOCK:
-		sigorsets(&current->blocked, &current->blocked, set);
+		sigorsets(&newset, &tsk->blocked, set);
 		break;
 	case SIG_UNBLOCK:
-		signandsets(&current->blocked, &current->blocked, set);
+		signandsets(&newset, &tsk->blocked, set);
 		break;
 	case SIG_SETMASK:
-		current->blocked = *set;
+		newset = *set;
 		break;
 	default:
-		error = -EINVAL;
+		return -EINVAL;
 	}
+
+	spin_lock_irq(&tsk->sighand->siglock);
+	tsk->blocked = newset;
 	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	spin_unlock_irq(&tsk->sighand->siglock);
 
-	return error;
+	return 0;
 }
 
 /**
#7 lore raw

Re: [PATCH 3/6] signal: sigprocmask: narrow the scope of ->sigloc

Quoted 5 lines · On Mon, 11 Apr 2011 19:21:09 +0200 Oleg Nesterov <[email protected]> wrote:

No functional changes, preparation to simplify the review of the next change.

1. We can read current->block lockless, nobody else can ever change this mask.

Is it worth mentioning this in a comment? What is the reason that it's OK? Is it because we only ever modify current->blocked and never another task's blocked signals and that we never modify current->blocked from within any form of interrupt context?

Some of the code in fs/{autofs4,coda} seems to do interesting things with current->blocked, particularly the daemon stuff in autofs4.

Signature · 1 line
Matt Fleming, Intel Open Source Technology Center
author
#8 lore raw

PATCH 4/6 signal: sigprocmask() should do retarget_shared_pending()

In reply to Oleg Nesterov

In short, almost every changing of current->blocked is wrong, or at least can lead to the unexpected results.

For example. Two threads T1 and T2, T1 sleeps in sigtimedwait/pause/etc. kill(tgid, SIG) can pick T2 for TIF_SIGPENDING. If T2 calls sigprocmask() and blocks SIG before it notices the pending signal, nobody else can handle this pending shared signal.

I am not sure this is bug, but at least this looks strange imho. T1 should not sleep forever, there is a signal which should wake it up.

This patch changes sigprocmask() to call retarget_shared_pending() as exit_signals() does. To calculate the "blocked" argument we add the new helper, signorsets(), although we could do ~(newset & ~current->blocked) instead. According to grep, nobody in arch/ defines __HAVE_ARCH_SIG_SETOPS, we only need to change linux/signal.h.

Note: for this particular case we could simply change sigprocmask() to return -EINTR if signal_pending(), but then we should change other callers and, more importantly, if we need this fix then sigprocmask() will have more callers and some of them can't restart. See the next patch as a random example.

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

---

 include/linux/signal.h |    3 +++
 kernel/signal.c        |    5 +++++
 2 files changed, 8 insertions(+)
include/linux/signal.h renamed from include/linux/signal.h~4_sigprocmask_retarget +4 -0
@@ -126,10 +126,14 @@ _SIG_SET_BINOP(sigandsets, _sig_and)
 #define _sig_nand(x,y)	((x) & ~(y))
 _SIG_SET_BINOP(signandsets, _sig_nand)
 
+#define _sig_nor(x,y)	((x) | ~(y))
+_SIG_SET_BINOP(signorsets, _sig_nor)
+
 #undef _SIG_SET_BINOP
 #undef _sig_or
 #undef _sig_and
 #undef _sig_nand
+#undef _sig_nor
 
 #define _SIG_SET_OP(name, op)						\
 static inline void name(sigset_t *set)					\
kernel/signal.c renamed from kernel/signal.c~4_sigprocmask_retarget +5 -0
@@ -2131,6 +2131,11 @@ int sigprocmask(int how, sigset_t *set, 
 	}
 
 	spin_lock_irq(&tsk->sighand->siglock);
+	if (signal_pending(tsk) && !thread_group_empty(tsk)) {
+		sigset_t not_newblocked;
+		signorsets(&not_newblocked, &current->blocked, &newset);
+		retarget_shared_pending(tsk, &not_newblocked);
+	}
 	tsk->blocked = newset;
 	recalc_sigpending();
 	spin_unlock_irq(&tsk->sighand->siglock);
#9 lore raw

Re: [PATCH 4/6] signal: sigprocmask() should do retarget_shared_pending()

Quoted 10 lines · On Mon, 11 Apr 2011 19:21:37 +0200 Oleg Nesterov <[email protected]> wrote:

In short, almost every changing of current->blocked is wrong, or at least can lead to the unexpected results.

For example. Two threads T1 and T2, T1 sleeps in sigtimedwait/pause/etc. kill(tgid, SIG) can pick T2 for TIF_SIGPENDING. If T2 calls sigprocmask() and blocks SIG before it notices the pending signal, nobody else can handle this pending shared signal.

I am not sure this is bug, but at least this looks strange imho. T1 should not sleep forever, there is a signal which should wake it up.

Agreed.

kernel/signal.c int sigprocmask(int how, sigset_t *set, · line 2141

Quoted 12 lines
kernel/signal.c renamed from kernel/signal.c~4_sigprocmask_retarget +5 -0
@@ -2131,6 +2131,11 @@ int sigprocmask(int how, sigset_t *set, 
 	}
 
 	spin_lock_irq(&tsk->sighand->siglock);
+	if (signal_pending(tsk) && !thread_group_empty(tsk)) {
+		sigset_t not_newblocked;
+		signorsets(&not_newblocked, &current->blocked, &newset);
+		retarget_shared_pending(tsk, &not_newblocked);
+	}
 	tsk->blocked = newset;
 	recalc_sigpending();
 	spin_unlock_irq(&tsk->sighand->siglock);

Oh man, that took me a while to understand.

So we're only retargetting the signals that we just blocked? That makes sense but would you mind adding a comment?

Signature · 1 line
Matt Fleming, Intel Open Source Technology Center
#10 lore raw

Re: [PATCH 4/6] signal: sigprocmask() should do retarget_shared_pending()

In reply to Oleg Nesterov
Quoted 3 lines · Oleg Nesterov

I am not sure this is bug, but at least this looks strange imho. T1 should not sleep forever, there is a signal which should wake it up.

Hmm. I worry about the overhead of this, and I'm not 100% convinced we need it.

But my _biggest_ objection to the series is a purely technical one:

Quoted 4 lines
include/linux/signal.h  2011-04-11 18:16:51.000000000 +0200 renamed from include/linux/signal.h~4_sigprocmask_retarget   2011-04-06 21:33:50.000000000 +0200 +0 -0
@@ -2131,6 +2131,11 @@ int sigprocmask(int how, sigset_t *set,
        }

include/linux/signal.h  2011-04-11 18:16:51.000000000 +0200 int sigprocmask(int how, sigset_t *set, · line 2141

Quoted 10 lines
include/linux/signal.h  2011-04-11 18:16:51.000000000 +0200 renamed from include/linux/signal.h~4_sigprocmask_retarget   2011-04-06 21:33:50.000000000 +0200 +5 -0
 
        spin_lock_irq(&tsk->sighand->siglock);
+       if (signal_pending(tsk) && !thread_group_empty(tsk)) {
+               sigset_t not_newblocked;
+               signorsets(&not_newblocked, &current->blocked, &newset);
+               retarget_shared_pending(tsk, &not_newblocked);
+       }
        tsk->blocked = newset;
        recalc_sigpending();
        spin_unlock_irq(&tsk->sighand->siglock);

I absolutely detest how you made "sigprocmask()" the main interface to do this all, and then add new callers.

It's a horrid interface with that crazy "how" argument, and comes out of the user-space system call interface. If we make kernel users do this, especially critical ones like the signal handling code, please just extract out just the actual "set new signal mask" part.

So please just introduce a "sig_set_blocked()" or something, without the crazy "switch (how)" crud, and make sigprocmask() and everybody else use _that_ instead.

That would make me much happier about the patch series, I suspect.

Linus

author
#11 lore raw

Re: [PATCH 4/6] signal: sigprocmask() should do retarget_shared_pending()

Quoted 7 lines · Linus Torvalds
Quoted 3 lines · Oleg Nesterov

I am not sure this is bug, but at least this looks strange imho. T1 should not sleep forever, there is a signal which should wake it up.

Hmm. I worry about the overhead of this, and I'm not 100% convinced we need it.

Indeed. That is why RFC.

I simply do not know if this is buggy or not. I reported this oddity a long ago, but I can't recall the result of discussion (or it was ignored ?).

And I do not like the fact we need a lot of changes, albeit trivial. We should convert almost every code which changes current->blocked. Otoh, perhaps this makes sense by itself...

Quoted 18 lines
Quoted 4 lines
include/linux/signal.h  2011-04-11 18:16:51.000000000 +0200 renamed from include/linux/signal.h~4_sigprocmask_retarget   2011-04-06 21:33:50.000000000 +0200 +0 -0
@@ -2131,6 +2131,11 @@ int sigprocmask(int how, sigset_t *set,
        }

include/linux/signal.h  2011-04-11 18:16:51.000000000 +0200 int sigprocmask(int how, sigset_t *set, · line 2141

Quoted 10 lines
include/linux/signal.h  2011-04-11 18:16:51.000000000 +0200 renamed from include/linux/signal.h~4_sigprocmask_retarget   2011-04-06 21:33:50.000000000 +0200 +5 -0
 
        spin_lock_irq(&tsk->sighand->siglock);
+       if (signal_pending(tsk) && !thread_group_empty(tsk)) {
+               sigset_t not_newblocked;
+               signorsets(&not_newblocked, &current->blocked, &newset);
+               retarget_shared_pending(tsk, &not_newblocked);
+       }
        tsk->blocked = newset;
        recalc_sigpending();
        spin_unlock_irq(&tsk->sighand->siglock);

I absolutely detest how you made "sigprocmask()" the main interface to do this all, and then add new callers.

Quoted 8 lines

It's a horrid interface with that crazy "how" argument, and comes out of the user-space system call interface. If we make kernel users do this, especially critical ones like the signal handling code, please just extract out just the actual "set new signal mask" part.

So please just introduce a "sig_set_blocked()" or something, without the crazy "switch (how)" crud, and make sigprocmask() and everybody else use _that_ instead.

You know, initially I did exactly this. set_current_blocked() was its name. But then I noticed that handle_signal() can naturally use SIG_BLOCK, sigtimedwait() could use SIG_UNBLOCK...

Nevermind,

Quoted 1 line

That would make me much happier about the patch series, I suspect.

OK. I'll redo and resend.

Oleg.

#12 lore raw

Re: [PATCH 4/6] signal: sigprocmask() should do retarget_shared_pending()

In reply to Oleg Nesterov

Hello, Oleg.

Quoted 4 lines · Oleg Nesterov
include/linux/signal.h renamed from include/linux/signal.h~4_sigprocmask_retarget +0 -0
@@ -126,10 +126,14 @@ _SIG_SET_BINOP(sigandsets, _sig_and)
 #define _sig_nand(x,y)	((x) & ~(y))

include/linux/signal.h _SIG_SET_BINOP(sigandsets, _sig_and) · line 136

Quoted 10 lines
include/linux/signal.h renamed from include/linux/signal.h~4_sigprocmask_retarget +4 -0
 _SIG_SET_BINOP(signandsets, _sig_nand)
 
+#define _sig_nor(x,y)	((x) | ~(y))
+_SIG_SET_BINOP(signorsets, _sig_nor)
+
 #undef _SIG_SET_BINOP
 #undef _sig_or
 #undef _sig_and
 #undef _sig_nand
+#undef _sig_nor

I'm confused. Isn't nand ^(A&B) and nor ^(A|B)?

kernel/signal.c int sigprocmask(int how, sigset_t *set, · line 2137

Quoted 12 lines
include/linux/signal.h renamed from include/linux/signal.h~4_sigprocmask_retarget +0 -0
 #define _SIG_SET_OP(name, op)						\
 static inline void name(sigset_t *set)					\
kernel/signal.c renamed from kernel/signal.c~4_sigprocmask_retarget +4 -0
@@ -2131,6 +2131,11 @@ int sigprocmask(int how, sigset_t *set, 
 	}
 
 	spin_lock_irq(&tsk->sighand->siglock);
+	if (signal_pending(tsk) && !thread_group_empty(tsk)) {
+		sigset_t not_newblocked;
+		signorsets(&not_newblocked, &current->blocked, &newset);
+		retarget_shared_pending(tsk, &not_newblocked);

I think it would be much easier to follow the logic if retarget_shared_pending() took target mask instead of blocked (ie. negation of blocked) and there were more comments. Combined with the confusing definition of nor, I had to spend quite some time trying to wrap my head around it but the logic here isn't all that complex and it should have been easier.

Other than that, I agree with the proposed changes, but I think we really need to do retargeting (and the initial targeting too) more efficiently as you noted in the earlier commit message. Altering signal mask is far hotter path than actual signal delivery. Although the thread walking code wouldn't get activated unless signal is already pending, I still think we better avoid looping threads unnecessarily.

Thanks.

Signature · 1 line
tejun
author
#13 lore raw

Re: [PATCH 4/6] signal: sigprocmask() should do retarget_shared_pending()

Quoted 10 lines · Tejun Heo

Hello, Oleg.

Quoted 6 lines · Oleg Nesterov
include/linux/signal.h renamed from include/linux/signal.h~4_sigprocmask_retarget +0 -0
@@ -126,10 +126,14 @@ _SIG_SET_BINOP(sigandsets, _sig_and)
 #define _sig_nand(x,y)	((x) & ~(y))
 _SIG_SET_BINOP(signandsets, _sig_nand)
Quoted 10 lines

include/linux/signal.h _SIG_SET_BINOP(sigandsets, _sig_and) · line 136

Quoted 8 lines
include/linux/signal.h renamed from include/linux/signal.h~4_sigprocmask_retarget +4 -0
+#define _sig_nor(x,y)	((x) | ~(y))
+_SIG_SET_BINOP(signorsets, _sig_nor)
+
 #undef _SIG_SET_BINOP
 #undef _sig_or
 #undef _sig_and
 #undef _sig_nand
+#undef _sig_nor

I'm confused. Isn't nand ^(A&B) and nor ^(A|B)?

Well, I don't know the common definition... But please note that signandsets() does ((x) & ~(y)), so I defined nor as (x | ~y) by analogy.

Quoted 7 lines
Quoted 7 lines
include/linux/signal.h renamed from include/linux/signal.h~4_sigprocmask_retarget +0 -0
 #define _SIG_SET_OP(name, op)						\
 static inline void name(sigset_t *set)					\
kernel/signal.c renamed from kernel/signal.c~4_sigprocmask_retarget +0 -0
@@ -2131,6 +2131,11 @@ int sigprocmask(int how, sigset_t *set, 
 	}
Quoted 8 lines

kernel/signal.c int sigprocmask(int how, sigset_t *set, · line 2137

Quoted 5 lines
kernel/signal.c renamed from kernel/signal.c~4_sigprocmask_retarget +4 -0
 	spin_lock_irq(&tsk->sighand->siglock);
+	if (signal_pending(tsk) && !thread_group_empty(tsk)) {
+		sigset_t not_newblocked;
+		signorsets(&not_newblocked, &current->blocked, &newset);
+		retarget_shared_pending(tsk, &not_newblocked);

I think it would be much easier to follow the logic if retarget_shared_pending() took target mask instead of blocked

Indeed. But currently we only have has_pending_signals() which needs blocked, not mask.

Quoted 3 lines

but I think we
really need to do retargeting (and the initial targeting too) more efficiently as you noted in the earlier commit message.

Yes, will do. And to do this we need the mask, not blocked.

But I'd prefer to do this after this series to make the first patches simpler.

The main optimization is the first has_pending_signals() check which can likely avoid while_each_thread() altogether. Once we start looping we already lost. But anyway I agree, we should do this. Perhaps we can add more optimizations later... say, perhaps we can add something like TIF_SIGPENDING_SHARED, I am not sure.

Oleg.

#14 lore raw

Re: [PATCH 4/6] signal: sigprocmask() should do retarget_shared_pending()

Quoted 3 lines · Oleg Nesterov

Well, I don't know the common definition... But please note that signandsets() does ((x) & ~(y)), so I defined nor as (x | ~y) by analogy.

Yeah, "nand" is really "not and" (ie "~((x)&(y))").

The expression ((x) & ~(y)) should typically be called "andn" (see for example the x86 instruction set "pandn" and "andnpd" or whatever they are called for sse instructions).

And "nor" really should be "~(x|y)". You want the "orn" combination (and the famously hard-to-google "porn" instruction for the vectorized version)

Linus

author
#15 lore raw

PATCH 5/6 x86: signal: handle_signal() should use sigprocmask()

In reply to Oleg Nesterov

This is ugly, but if sigprocmask() needs retarget_shared_pending() then handle signal should follow this logic. In theory it is newer correct to add the new signals to current->blocked, the signal handler can sleep/etc so we should notify other threads in case we block the pending signal and nobody else has TIF_SIGPENDING.

Of course, this change doesn't make signals faster :/

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

---

 arch/x86/kernel/signal.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
arch/x86/kernel/signal.c renamed from arch/x86/kernel/signal.c~5_handle_signal +4 -5
@@ -682,6 +682,7 @@ static int
 handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka,
 	      sigset_t *oldset, struct pt_regs *regs)
 {
+	sigset_t blocked;
 	int ret;
 
 	/* Are we from a system call? */

@@ -741,12 +742,10 @@ handle_signal(unsigned long sig, siginfo
 	 */
 	regs->flags &= ~X86_EFLAGS_TF;
 
-	spin_lock_irq(&current->sighand->siglock);
-	sigorsets(&current->blocked, &current->blocked, &ka->sa.sa_mask);
+	blocked = ka->sa.sa_mask;
 	if (!(ka->sa.sa_flags & SA_NODEFER))
-		sigaddset(&current->blocked, sig);
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+		sigaddset(&blocked, sig);
+	sigprocmask(SIG_BLOCK, &blocked, NULL);
 
 	tracehook_signal_handler(sig, info, ka, regs,
 				 test_thread_flag(TIF_SINGLESTEP));
Reviewed-by on v1 5/6
#16 lore raw

Re: [PATCH 5/6] x86: signal: handle_signal() should use sigprocmask()

Quoted 2 lines · On Mon, 11 Apr 2011 19:22:01 +0200 Oleg Nesterov <[email protected]> wrote:

This is ugly, but if sigprocmask() needs retarget_shared_pending() then handle signal should follow this logic. In theory it is newer correct to

							   ^^^ never
Quoted 7 lines

add the new signals to current->blocked, the signal handler can sleep/etc so we should notify other threads in case we block the pending signal and nobody else has TIF_SIGPENDING.

Of course, this change doesn't make signals faster :/

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

I guess all assigments to current->blocked need auditing now? Forcing everything through sigprocmask() seems worthwhile to me.

Signature · 1 line
Matt Fleming, Intel Open Source Technology Center
author
#17 lore raw

PATCH 6/6 x86: signal: sys_rt_sigreturn() should use sigprocmask()

In reply to Oleg Nesterov

Normally sys_rt_sigreturn() restores the old current->blocked which was changed by handle_signal(), and unblocking is always fine.

But the debugger or application itself can change frame->uc_sigmask and thus we need sigprocmask()->retarget_shared_pending().

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

---

 arch/x86/kernel/signal.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
arch/x86/kernel/signal.c renamed from arch/x86/kernel/signal.c~6_sigreturn +1 -4
@@ -601,10 +601,7 @@ long sys_rt_sigreturn(struct pt_regs *re
 		goto badframe;
 
 	sigdelsetmask(&set, ~_BLOCKABLE);
-	spin_lock_irq(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	sigprocmask(SIG_SETMASK, &set, NULL);
 
 	if (restore_sigcontext(regs, &frame->uc.uc_mcontext, &ax))
 		goto badframe;
Reviewed-by on v1 6/6
#18 lore raw

Re: [PATCH 6/6] x86: signal: sys_rt_sigreturn() should use sigprocmask()

Quoted 7 lines · On Mon, 11 Apr 2011 19:22:29 +0200 Oleg Nesterov <[email protected]> wrote:

Normally sys_rt_sigreturn() restores the old current->blocked which was changed by handle_signal(), and unblocking is always fine.

But the debugger or application itself can change frame->uc_sigmask and thus we need sigprocmask()->retarget_shared_pending().

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

I'm just curious, have you actually seen this bug?

author
#19 lore raw

Re: [PATCH 6/6] x86: signal: sys_rt_sigreturn() should use sigprocmask()

Quoted 7 lines · Matt Fleming
Quoted 3 lines · On Mon, 11 Apr 2011 19:22:29 +0200 Oleg Nesterov <[email protected]> wrote:

Normally sys_rt_sigreturn() restores the old current->blocked which was changed by handle_signal(), and unblocking is always fine.

Quoted 6 lines
Quoted 4 lines

But the debugger or application itself can change frame->uc_sigmask and thus we need sigprocmask()->retarget_shared_pending().

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

Thanks Matt.

Quoted 1 line

I'm just curious, have you actually seen this bug?

No, never.

But recently Nikita reported the problem which _might_ be explained by this. see http://marc.info/?t=129732355500007 . Although it is quite possible there is something else.

Oleg.