[RFC PATCH 0/6] signal: sigprocmask fixes

[RFC PATCH 0/6] signal: sigprocmask fixes

[Oleg Nesterov]

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.

[PATCH 1/6] signal: introduce retarget_shared_pending()

[Oleg Nesterov]

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

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

 kernel/signal.c |   26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

--- sigprocmask/kernel/signal.c~1_add_retarget_helper	2011-04-10 18:24:18.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-04-10 19:33:45.000000000 +0200
@@ -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) {

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

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

 kernel/signal.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

--- sigprocmask/kernel/signal.c~2_optimize_retarget	2011-04-10 19:33:45.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-04-10 21:54:24.000000000 +0200
@@ -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) {

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

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

 kernel/signal.c |   28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

--- sigprocmask/kernel/signal.c~3_cleanup_sigprocmask	2011-04-10 21:54:24.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-04-10 21:57:42.000000000 +0200
@@ -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;
 }
 
 /**

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

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

 include/linux/signal.h |    3 +++
 kernel/signal.c        |    5 +++++
 2 files changed, 8 insertions(+)

--- sigprocmask/include/linux/signal.h~4_sigprocmask_retarget	2011-04-06 21:33:50.000000000 +0200
+++ sigprocmask/include/linux/signal.h	2011-04-11 18:16:51.000000000 +0200
@@ -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)					\
--- sigprocmask/kernel/signal.c~4_sigprocmask_retarget	2011-04-10 21:57:42.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-04-11 18:02:22.000000000 +0200
@@ -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);

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

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

 arch/x86/kernel/signal.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

--- sigprocmask/arch/x86/kernel/signal.c~5_handle_signal	2011-04-06 21:33:43.000000000 +0200
+++ sigprocmask/arch/x86/kernel/signal.c	2011-04-11 18:33:17.000000000 +0200
@@ -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));

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

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

 arch/x86/kernel/signal.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

--- sigprocmask/arch/x86/kernel/signal.c~6_sigreturn	2011-04-11 18:33:17.000000000 +0200
+++ sigprocmask/arch/x86/kernel/signal.c	2011-04-11 18:57:27.000000000 +0200
@@ -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;

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

[Matt Fleming]

On Mon, 11 Apr 2011 19:21:09 +0200
Oleg Nesterov <oleg@redhat.com> 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.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 1/6] signal: introduce retarget_shared_pending()

[Matt Fleming]

On Mon, 11 Apr 2011 19:20:23 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> No functional changes. Move the notyfy-other-threads code from
> exit_signals() to the new helper, retarget_shared_pending().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

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

[Matt Fleming]

On Mon, 11 Apr 2011 19:20:45 +0200
Oleg Nesterov <oleg@redhat.com> 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.
> 
> 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 <oleg@redhat.com>

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

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

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

[Matt Fleming]

On Mon, 11 Apr 2011 19:21:37 +0200
Oleg Nesterov <oleg@redhat.com> 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.

> @@ -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?

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Matt Fleming]

On Mon, 11 Apr 2011 19:22:01 +0200
Oleg Nesterov <oleg@redhat.com> 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

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

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

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

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Matt Fleming]

On Mon, 11 Apr 2011 19:22:29 +0200
Oleg Nesterov <oleg@redhat.com> 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 <oleg@redhat.com>

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

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

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

[Linus Torvalds]

On Mon, Apr 11, 2011 at 10:21 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> 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:

> --- sigprocmask/include/linux/signal.h~4_sigprocmask_retarget   2011-04-06 21:33:50.000000000 +0200
> +++ sigprocmask/include/linux/signal.h  2011-04-11 18:16:51.000000000 +0200
> @@ -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);

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

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

[Tejun Heo]

Hello, Oleg.

On Mon, Apr 11, 2011 at 07:21:37PM +0200, Oleg Nesterov wrote:
> --- sigprocmask/include/linux/signal.h~4_sigprocmask_retarget	2011-04-06 21:33:50.000000000 +0200
> +++ sigprocmask/include/linux/signal.h	2011-04-11 18:16:51.000000000 +0200
> @@ -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

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

>  #define _SIG_SET_OP(name, op)						\
>  static inline void name(sigset_t *set)					\
> --- sigprocmask/kernel/signal.c~4_sigprocmask_retarget	2011-04-10 21:57:42.000000000 +0200
> +++ sigprocmask/kernel/signal.c	2011-04-11 18:02:22.000000000 +0200
> @@ -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.

-- 
tejun

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

[Oleg Nesterov]

On 04/12, Matt Fleming wrote:
>
> On Mon, 11 Apr 2011 19:20:45 +0200
> Oleg Nesterov <oleg@redhat.com> 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.
> >
> > 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 <oleg@redhat.com>
>
> Nice. Do you have plans to implement that further optimisation?

Probably yes, this is straightforward...

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

Thanks Matt, thanks all.

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

Oleg.

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

[Oleg Nesterov]

On 04/12, Linus Torvalds wrote:
>
> On Mon, Apr 11, 2011 at 10:21 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > 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...

> > --- sigprocmask/include/linux/signal.h~4_sigprocmask_retarget   2011-04-06 21:33:50.000000000 +0200
> > +++ sigprocmask/include/linux/signal.h  2011-04-11 18:16:51.000000000 +0200
> > @@ -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);
>
> 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.

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,

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

OK. I'll redo and resend.

Oleg.

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

[Oleg Nesterov]

On 04/13, Tejun Heo wrote:
>
> Hello, Oleg.
>
> On Mon, Apr 11, 2011 at 07:21:37PM +0200, Oleg Nesterov wrote:
> > --- sigprocmask/include/linux/signal.h~4_sigprocmask_retarget	2011-04-06 21:33:50.000000000 +0200
> > +++ sigprocmask/include/linux/signal.h	2011-04-11 18:16:51.000000000 +0200
> > @@ -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
>
> 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.

> >  #define _SIG_SET_OP(name, op)						\
> >  static inline void name(sigset_t *set)					\
> > --- sigprocmask/kernel/signal.c~4_sigprocmask_retarget	2011-04-10 21:57:42.000000000 +0200
> > +++ sigprocmask/kernel/signal.c	2011-04-11 18:02:22.000000000 +0200
> > @@ -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

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

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

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

[Oleg Nesterov]

On 04/12, Matt Fleming wrote:
>
> On Mon, 11 Apr 2011 19:22:29 +0200
> Oleg Nesterov <oleg@redhat.com> 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 <oleg@redhat.com>
>
> Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com>

Thanks Matt.

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

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

[Linus Torvalds]

On Thu, Apr 14, 2011 at 1:10 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> 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