[RFC PATCH v2 0/7] signal: sigprocmask fixes

[RFC PATCH v2 0/7] signal: sigprocmask fixes

[Oleg Nesterov]

Andrew, please drop V1:

	signal-introduce-retarget_shared_pending.patch
	signal-retarget_shared_pending-consider-shared-unblocked-signals-only.patch
	signal-sigprocmask-narrow-the-scope-of-sigloc.patch
	signal-sigprocmask-should-do-retarget_shared_pending.patch
	x86-signal-handle_signal-should-use-sigprocmask.patch
	x86-signal-sys_rt_sigreturn-should-use-sigprocmask.patch

Changes in V2:

	- 2/7 change retarget_shared_pending() to accept mask, not ~mask

	- 3/7 is new, it adds the optimization promised in 2/7

	- 4/7 add the small comment about current->blocked as Matt
	      suggested

	- 5/7 add the new helper, set_current_blocked(), suggested
	      by Linus

	- 8/7 is the new and a bit off-topic cleanup, but sys_rt_sigprocmask()
	      looks so annoying

Matt, I didn't dare to keep your reviewed-by tags because of the
changes above, hopefully you can re-ack.

Once again: if we need this, then we need a lot more (trivial) changes
like 6/7 and 7/7. Basically every change of ->blocked should be converted
to use set_current_blocked(). OTOH, perhaps this makes sense by itself.

Oleg.

[PATCH v2 1/7] signal: introduce retarget_shared_pending()

[Oleg Nesterov]

No functional changes. Move the notify-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-17 20:00:13.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-04-17 21:02:59.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) {

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

[Matt Fleming]

On Mon, 18 Apr 2011 15:44:43 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> No functional changes. Move the notify-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 v2 1/7] signal: introduce retarget_shared_pending()

[Tejun Heo]

On Mon, Apr 18, 2011 at 03:44:43PM +0200, Oleg Nesterov wrote:
> No functional changes. Move the notify-other-threads code from exit_signals()
> to the new helper, retarget_shared_pending().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

-- 
tejun

[PATCH v2 2/7] 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 "shared_pending.signal & ~blocked" check. We don't use tsk->blocked
directly but pass ~blocked 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, see the
next patch.

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

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

--- sigprocmask/kernel/signal.c~2_optimize_retarget	2011-04-17 21:02:59.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-04-17 21:08:05.000000000 +0200
@@ -2022,10 +2022,15 @@ 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 *set)
 {
+	sigset_t shared_pending;
 	struct task_struct *t;
 
+	sigandsets(&shared_pending, &tsk->signal->shared_pending.signal, set);
+	if (sigisemptyset(&shared_pending))
+		return;
+
 	t = tsk;
 	while_each_thread(tsk, t) {
 		if (!signal_pending(t) && !(t->flags & PF_EXITING))
@@ -2036,6 +2041,7 @@ static void retarget_shared_pending(stru
 void exit_signals(struct task_struct *tsk)
 {
 	int group_stop = 0;
+	sigset_t set;
 
 	if (thread_group_empty(tsk) || signal_group_exit(tsk->signal)) {
 		tsk->flags |= PF_EXITING;
@@ -2051,7 +2057,9 @@ void exit_signals(struct task_struct *ts
 	if (!signal_pending(tsk))
 		goto out;
 
-	retarget_shared_pending(tsk);
+	set = tsk->blocked;
+	signotset(&set);
+	retarget_shared_pending(tsk, &set);
 
 	if (unlikely(tsk->signal->group_stop_count) &&
 			!--tsk->signal->group_stop_count) {

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

[Matt Fleming]

On Mon, 18 Apr 2011 15:45:01 +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 "shared_pending.signal & ~blocked" check. We don't use tsk->blocked
> directly but pass ~blocked 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, see the
> next patch.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

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

[Tejun Heo]

On Mon, Apr 18, 2011 at 03:45:01PM +0200, Oleg Nesterov wrote:
> -static void retarget_shared_pending(struct task_struct *tsk)
> +static void retarget_shared_pending(struct task_struct *tsk, sigset_t *set)

Hmm... @set?  Maybe a name which refelcts its purpose, say @target or
@consider would be better?  Also, it would be really great to have
docbook function comment describing the parameter.

Other than that,

 Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

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

[Oleg Nesterov]

On 04/25, Tejun Heo wrote:
>
> On Mon, Apr 18, 2011 at 03:45:01PM +0200, Oleg Nesterov wrote:
> > -static void retarget_shared_pending(struct task_struct *tsk)
> > +static void retarget_shared_pending(struct task_struct *tsk, sigset_t *set)
>
> Hmm... @set?  Maybe a name which refelcts its purpose, say @target or
> @consider would be better?  Also, it would be really great to have
> docbook function comment describing the parameter.

OK... May be @which ? but I agree with any naming.


This series is already in -mm, I'd like to avoid another resend. So I'll
send another patch which addresses your comments on top of this series.

OK?

>  Acked-by: Tejun Heo <tj@kernel.org>

Thanks!

Oleg.

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

[Tejun Heo]

Hello,

On Mon, Apr 25, 2011 at 05:20:40PM +0200, Oleg Nesterov wrote:
> > Hmm... @set?  Maybe a name which refelcts its purpose, say @target or
> > @consider would be better?  Also, it would be really great to have
> > docbook function comment describing the parameter.
> 
> OK... May be @which ? but I agree with any naming.

Yeah, @which sounds fine to me.

> This series is already in -mm, I'd like to avoid another resend. So I'll
> send another patch which addresses your comments on top of this series.
> 
> OK?

Why not route through the signal/ptrace tree?  Also, is the tree
included in linux-next now?

Thanks.

-- 
tejun

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

[Oleg Nesterov]

On 04/25, Tejun Heo wrote:
>
> On Mon, Apr 25, 2011 at 05:20:40PM +0200, Oleg Nesterov wrote:
> > This series is already in -mm, I'd like to avoid another resend. So I'll
> > send another patch which addresses your comments on top of this series.
> >
> > OK?
>
> Why not route through the signal/ptrace tree?

I did these changes against the Linus's tree to simplify the review, and
because there are completely orthogonal to ptrace changes. Also, I like
very much the fact -mm has users/testers.

In fact, there are trivial conflicts with the ptrace branch. I think
ptrace should be flushed first, so I'll rebase this "sigprocmask" branch
when I address all comments.

Or do you think I should merge these changes into ptrace branch? I'd like
to keep them separate, but I am not sure if I should...

Oleg.

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

[Tejun Heo]

Hey,

On Mon, Apr 25, 2011 at 7:02 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> I did these changes against the Linus's tree to simplify the review, and
> because there are completely orthogonal to ptrace changes. Also, I like
> very much the fact -mm has users/testers.
>
> In fact, there are trivial conflicts with the ptrace branch. I think
> ptrace should be flushed first, so I'll rebase this "sigprocmask" branch
> when I address all comments.
>
> Or do you think I should merge these changes into ptrace branch? I'd like
> to keep them separate, but I am not sure if I should...

I don't know.  Signal/ptrace is closely coupled and you would be
reviewing/acking anyway, and linux-next has some test coverage (I
don't know how much but...), so I think it would be least painful to
route these together.  You can create separate topic branches for
signal and ptrace but I don't think that's required.  Anyways, yeah,
if there's no objection, I think it would be best to route these
together with the ptrace changes.  The conflicts wouldn't be trivial
and for a reason.

Thanks.

-- 
tejun

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

[Oleg Nesterov]

Hi,

On 04/25, Tejun Heo wrote:
> Hey,
>
> On Mon, Apr 25, 2011 at 7:02 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > I did these changes against the Linus's tree to simplify the review, and
> > because there are completely orthogonal to ptrace changes. Also, I like
> > very much the fact -mm has users/testers.
> >
> > In fact, there are trivial conflicts with the ptrace branch. I think
> > ptrace should be flushed first, so I'll rebase this "sigprocmask" branch
> > when I address all comments.
> >
> > Or do you think I should merge these changes into ptrace branch? I'd like
> > to keep them separate, but I am not sure if I should...
>
> I don't know.  Signal/ptrace is closely coupled and you would be
> reviewing/acking anyway, and linux-next has some test coverage (I
> don't know how much but...), so I think it would be least painful to
> route these together.  You can create separate topic branches for
> signal and ptrace but I don't think that's required.  Anyways, yeah,
> if there's no objection, I think it would be best to route these
> together with the ptrace changes.  The conflicts wouldn't be trivial
> and for a reason.

OK. I tried to update my branch to address the comments from you and
Matt, but I got lost inside the git-learning-curve. Will do tomorrow.

Until then, could you review the updated version of the new changes
we discussed yesterday? (will send in a minute).

Oleg.

[PATCHSET] signals-review branch

[Oleg Nesterov]

Hello.

I collected the patches which were acked by Tejun and Matt and (I hope)
Linus agrees with,

	git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc.git signals-review

	[PATCH 01/13] signal: introduce retarget_shared_pending()
	[PATCH 02/13] signal: retarget_shared_pending: consider shared/unblocked signals only
	[PATCH 03/13] signal: retarget_shared_pending: optimize while_each_thread() loop
	[PATCH 04/13] signal: sigprocmask: narrow the scope of ->siglock
	[PATCH 05/13] signal: sigprocmask() should do retarget_shared_pending()
	[PATCH 06/13] x86: signal: handle_signal() should use set_current_blocked()
	[PATCH 07/13] x86: signal: sys_rt_sigreturn() should use set_current_blocked()
	[PATCH 08/13] signal: cleanup sys_rt_sigprocmask()
	[PATCH 09/13] signal: sys_rt_sigtimedwait: simplify the timeout logic
	[PATCH 10/13] signal: introduce do_sigtimedwait() to factor out compat/native code
	[PATCH 11/13] signal: do_sigtimedwait() needs retarget_shared_pending()
	[PATCH 12/13] signal: rename signandsets() to sigandnsets()
	[PATCH 13/13] signal: cleanup sys_sigprocmask()

I do not want to spam lkml again, all changes are purely cosmetic and hopefully
address the comments from you and Matt:

	all: rediff against ptrace branch

	[PATCH 02/13] signal: retarget_shared_pending: consider shared/unblocked signals only
	commit f646e227b88a164a841d6b6dd969d8a45272dd83

		retarget_shared_pending:
		
			s/set/which/, s/shared_pending/retarget

		exit_signals:

			s/set/unblocked

	[PATCH 03/13] signal: retarget_shared_pending: optimize while_each_thread() loop
	commit fec9993db093acfc3999a364e31f8adae41fcb28

		retarget_shared_pending:

			remove the unnecessary () in "if ((t->flags & PF_EXITING))"

			add the small comment

	[PATCH 05/13] signal: sigprocmask() should do retarget_shared_pending()
	commit e6fa16ab9c1e9b344428e6fea4d29e3cc4b28fb0

		set_current_blocked:

			add some comments

	[PATCH 07/13] x86: signal: sys_rt_sigreturn() should use set_current_blocked()
	commit e9bd3f0faa90084f188830d77723bafe422e486b

		no changes. I was asked to add the comment, but I have no
		idea what should be documented. The patch and the code are
		trivial.

	[PATCH 08/13] signal: cleanup sys_rt_sigprocmask()
	commit bb7efee2ca63b08795ffb3cda96fc89d2e641b79

		do not touch the "XXX" comment which I misunderstood

	[PATCH 10/13] signal: introduce do_sigtimedwait() to factor out compat/native code
	commit 943df1485a8ff0e600729e082e568ece04d4de9e

		Well. As it was suggested, I added the docbook coment to the
		new helper. Trivial copy-and-paste from sys_rt_sigtimedwait(),
		not sure we need these extra 6 lines.

Unless you have other comments, I'll merge this into ptrace branch.

Oleg.

Re: [PATCHSET] signals-review branch

[Tejun Heo]

On Thu, Apr 28, 2011 at 05:26:13PM +0200, Oleg Nesterov wrote:
> I collected the patches which were acked by Tejun and Matt and (I hope)
> Linus agrees with,
> 
> 	git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc.git signals-review
> 
> 	[PATCH 01/13] signal: introduce retarget_shared_pending()
> 	[PATCH 02/13] signal: retarget_shared_pending: consider shared/unblocked signals only
> 	[PATCH 03/13] signal: retarget_shared_pending: optimize while_each_thread() loop
> 	[PATCH 04/13] signal: sigprocmask: narrow the scope of ->siglock
> 	[PATCH 05/13] signal: sigprocmask() should do retarget_shared_pending()
> 	[PATCH 06/13] x86: signal: handle_signal() should use set_current_blocked()
> 	[PATCH 07/13] x86: signal: sys_rt_sigreturn() should use set_current_blocked()
> 	[PATCH 08/13] signal: cleanup sys_rt_sigprocmask()
> 	[PATCH 09/13] signal: sys_rt_sigtimedwait: simplify the timeout logic
> 	[PATCH 10/13] signal: introduce do_sigtimedwait() to factor out compat/native code
> 	[PATCH 11/13] signal: do_sigtimedwait() needs retarget_shared_pending()
> 	[PATCH 12/13] signal: rename signandsets() to sigandnsets()
> 	[PATCH 13/13] signal: cleanup sys_sigprocmask()

Looks all fine to me.  I'll base further patches on top of these.

Thanks.

-- 
tejun

[PATCH v2 3/7] signal: retarget_shared_pending: optimize while_each_thread() loop

[Oleg Nesterov]

retarget_shared_pending() blindly does recalc_sigpending_and_wake() for
every sub-thread, this is suboptimal. We can check t->blocked and stop
looping once every bit in shared_pending has the new target.

Note: we do not take task_is_stopped_or_traced(t) into account, we are
not trying to speed up the signal delivery or to avoid the unnecessary
(but harmless) signal_wake_up(0) in this unlikely case.

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

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

--- sigprocmask/kernel/signal.c~3_optimize_retarget_loop	2011-04-17 21:08:05.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-04-17 21:28:21.000000000 +0200
@@ -2033,8 +2033,18 @@ static void retarget_shared_pending(stru
 
 	t = tsk;
 	while_each_thread(tsk, t) {
-		if (!signal_pending(t) && !(t->flags & PF_EXITING))
-			recalc_sigpending_and_wake(t);
+		if ((t->flags & PF_EXITING))
+			continue;
+		if (!has_pending_signals(&shared_pending, &t->blocked))
+			continue;
+
+		sigandsets(&shared_pending, &shared_pending, &t->blocked);
+
+		if (!signal_pending(t))
+			signal_wake_up(t, 0);
+
+		if (sigisemptyset(&shared_pending))
+			break;
 	}
 }
 

Re: [PATCH v2 3/7] signal: retarget_shared_pending: optimize while_each_thread() loop

[Matt Fleming]

On Mon, 18 Apr 2011 15:45:18 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> retarget_shared_pending() blindly does recalc_sigpending_and_wake() for
> every sub-thread, this is suboptimal. We can check t->blocked and stop
> looping once every bit in shared_pending has the new target.
> 
> Note: we do not take task_is_stopped_or_traced(t) into account, we are
> not trying to speed up the signal delivery or to avoid the unnecessary
> (but harmless) signal_wake_up(0) in this unlikely case.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Nice.

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

Re: [PATCH v2 3/7] signal: retarget_shared_pending: optimize while_each_thread() loop

[Tejun Heo]

Hello,

More nitpicks.

On Mon, Apr 18, 2011 at 03:45:18PM +0200, Oleg Nesterov wrote:
> --- sigprocmask/kernel/signal.c~3_optimize_retarget_loop	2011-04-17 21:08:05.000000000 +0200
> +++ sigprocmask/kernel/signal.c	2011-04-17 21:28:21.000000000 +0200
> @@ -2033,8 +2033,18 @@ static void retarget_shared_pending(stru
>  
>  	t = tsk;
>  	while_each_thread(tsk, t) {
> -		if (!signal_pending(t) && !(t->flags & PF_EXITING))
> -			recalc_sigpending_and_wake(t);
> +		if ((t->flags & PF_EXITING))

Inner () unnecessary.

> +			continue;
> +		if (!has_pending_signals(&shared_pending, &t->blocked))
> +			continue;
> +
> +		sigandsets(&shared_pending, &shared_pending, &t->blocked);
> +
> +		if (!signal_pending(t))
> +			signal_wake_up(t, 0);
> +
> +		if (sigisemptyset(&shared_pending))
> +			break;

More comments, please.  Other than that,

 Acked-by: Tejun Heo <tj@kernel.org>

-- 
tejun

[PATCH v2 4/7] signal: sigprocmask: narrow the scope of ->siglock

[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 |   29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

--- sigprocmask/kernel/signal.c~4_cleanup_sigprocmask	2011-04-17 21:28:21.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-04-17 22:16:44.000000000 +0200
@@ -2116,12 +2116,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.
  *
@@ -2131,30 +2125,33 @@ 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);
+	/* Lockless, only current can change ->blocked, never from irq */
 	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;
 }
 
 /**

Re: [PATCH v2 4/7] signal: sigprocmask: narrow the scope of ->siglock

[Matt Fleming]

On Mon, 18 Apr 2011 15:45:38 +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.
> 
> 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>

Thanks for that updated comment about not being able to change
->blocked from irq context!

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

Re: [PATCH v2 4/7] signal: sigprocmask: narrow the scope of ->siglock

[Tejun Heo]

On Mon, Apr 18, 2011 at 03:45:38PM +0200, Oleg Nesterov 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.

                 current->blocked

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

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

[PATCH v2 5/7] 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 moves the code which actually changes ->blocked into the new
helper, set_current_blocked() and changes this code to call
retarget_shared_pending() as exit_signals() does. We should only care about
the signals we just blocked, we use "newset & ~current->blocked" as a mask.

We do not check !sigisemptyset(newblocked), retarget_shared_pending() is
cheap unless mask & shared_pending.

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 set_current_blocked() 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 |    1 +
 kernel/signal.c        |   21 ++++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

--- sigprocmask/include/linux/signal.h~5_sigprocmask_retarget	2011-04-17 22:23:29.000000000 +0200
+++ sigprocmask/include/linux/signal.h	2011-04-17 22:36:41.000000000 +0200
@@ -243,6 +243,7 @@ extern long do_rt_tgsigqueueinfo(pid_t t
 				 siginfo_t *info);
 extern long do_sigpending(void __user *, unsigned long);
 extern int sigprocmask(int, sigset_t *, sigset_t *);
+extern void set_current_blocked(const sigset_t *);
 extern int show_unhandled_signals;
 
 struct pt_regs;
--- sigprocmask/kernel/signal.c~5_sigprocmask_retarget	2011-04-17 22:16:44.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-04-17 22:32:58.000000000 +0200
@@ -2115,6 +2115,21 @@ long do_no_restart_syscall(struct restar
 	return -EINTR;
 }
 
+void set_current_blocked(const sigset_t *newset)
+{
+	struct task_struct *tsk = current;
+
+	spin_lock_irq(&tsk->sighand->siglock);
+	if (signal_pending(tsk) && !thread_group_empty(tsk)) {
+		sigset_t newblocked;
+		signandsets(&newblocked, newset, &current->blocked);
+		retarget_shared_pending(tsk, &newblocked);
+	}
+	tsk->blocked = *newset;
+	recalc_sigpending();
+	spin_unlock_irq(&tsk->sighand->siglock);
+}
+
 /*
  * This is also useful for kernel threads that want to temporarily
  * (or permanently) block certain signals.
@@ -2146,11 +2161,7 @@ int sigprocmask(int how, sigset_t *set, 
 		return -EINVAL;
 	}
 
-	spin_lock_irq(&tsk->sighand->siglock);
-	tsk->blocked = newset;
-	recalc_sigpending();
-	spin_unlock_irq(&tsk->sighand->siglock);
-
+	set_current_blocked(&newset);
 	return 0;
 }
 

Re: [PATCH v2 5/7] signal: sigprocmask() should do retarget_shared_pending()

[Matt Fleming]

On Mon, 18 Apr 2011 15:45:57 +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.
> 
> This patch moves the code which actually changes ->blocked into the new
> helper, set_current_blocked() and changes this code to call
> retarget_shared_pending() as exit_signals() does. We should only care about
> the signals we just blocked, we use "newset & ~current->blocked" as a mask.
> 
> We do not check !sigisemptyset(newblocked), retarget_shared_pending() is
> cheap unless mask & shared_pending.
> 
> 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 set_current_blocked() 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>

This looks much simpler to me.

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

Re: [PATCH v2 5/7] signal: sigprocmask() should do retarget_shared_pending()

[Tejun Heo]

Hey, again.

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

Acked-by: Tejun Heo <tj@kernel.org>

But, I really think we can use some comments around here.  Things
might look obvious now but it isn't very intuitive piece of code.

> +void set_current_blocked(const sigset_t *newset)
> +{
> +	struct task_struct *tsk = current;
> +
> +	spin_lock_irq(&tsk->sighand->siglock);
> +	if (signal_pending(tsk) && !thread_group_empty(tsk)) {
> +		sigset_t newblocked;
> +		signandsets(&newblocked, newset, &current->blocked);

While you're touching code around here, can you please rename
signandsets() to sigandnsets()?  It ain't nand!!!

Thanks.

-- 
tejun

[PATCH v2 6/7] x86: signal: handle_signal() should use set_current_blocked()

[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~6_handle_signal	2011-04-15 19:21:30.000000000 +0200
+++ sigprocmask/arch/x86/kernel/signal.c	2011-04-17 23:07:14.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);
+	sigorsets(&blocked, &current->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);
+	set_current_blocked(&blocked);
 
 	tracehook_signal_handler(sig, info, ka, regs,
 				 test_thread_flag(TIF_SINGLESTEP));

Re: [PATCH v2 6/7] x86: signal: handle_signal() should use set_current_blocked()

[Matt Fleming]

On Mon, 18 Apr 2011 15:46:15 +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>

Re: [PATCH v2 6/7] x86: signal: handle_signal() should use set_current_blocked()

[Tejun Heo]

On Mon, Apr 18, 2011 at 03:46:15PM +0200, Oleg Nesterov 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 :/

I don't think it's gonna make things go much slower either except for
pathological cases.

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

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

[PATCH v2 7/7] x86: signal: sys_rt_sigreturn() should use set_current_blocked()

[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 set_current_blocked()->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~7_sigreturn	2011-04-17 23:07:14.000000000 +0200
+++ sigprocmask/arch/x86/kernel/signal.c	2011-04-17 23:19:13.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);
+	set_current_blocked(&set);
 
 	if (restore_sigcontext(regs, &frame->uc.uc_mcontext, &ax))
 		goto badframe;

Re: [PATCH v2 7/7] x86: signal: sys_rt_sigreturn() should use set_current_blocked()

[Matt Fleming]

On Mon, 18 Apr 2011 15:46:41 +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 set_current_blocked()->retarget_shared_pending().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

But does sys_sigreturn() also need this change?

Re: [PATCH v2 7/7] x86: signal: sys_rt_sigreturn() should use set_current_blocked()

[Oleg Nesterov]

On 04/22, Matt Fleming wrote:
>
> On Mon, 18 Apr 2011 15:46:41 +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 set_current_blocked()->retarget_shared_pending().
> >
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
>
> Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com>

Thanks Matt.

> But does sys_sigreturn() also need this change?

Of course, it needs. From 0/7:

	Once again: if we need this, then we need a lot more (trivial) changes
	like 6/7 and 7/7. Basically every change of ->blocked should be converted
	to use set_current_blocked().

6 and 7 are simple examples, most of the other changes will look similary.

Except sys_rt_sigtimedwait(), it changes both ->real_blocked and blocked,
see the patches I sent. sys_sigprocmask() is a bit annoying, but the
necessary changes are simple.

Oleg.

Re: [PATCH v2 7/7] x86: signal: sys_rt_sigreturn() should use set_current_blocked()

[Tejun Heo]

On Mon, Apr 18, 2011 at 03:46:41PM +0200, Oleg Nesterov 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 set_current_blocked()->retarget_shared_pending().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

>  arch/x86/kernel/signal.c |    5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> --- sigprocmask/arch/x86/kernel/signal.c~7_sigreturn	2011-04-17 23:07:14.000000000 +0200
> +++ sigprocmask/arch/x86/kernel/signal.c	2011-04-17 23:19:13.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);
> +	set_current_blocked(&set);

Comment!

Thanks.

-- 
tejun

[PATCH v2 8/7] signal: cleanup sys_rt_sigprocmask()

[Oleg Nesterov]

sys_rt_sigprocmask() looks unnecessarily complicated, simplify it.
We can just read current->blocked lockless unconditionally before
anything else and then copy-to-user it if needed.  At worst we
copy 4 words on mips.

We could copy-to-user the old mask first and simplify the code even
more, but the patch tries to keep the current behaviour: we change
current->block even if copy_to_user(oset) fails.

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

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

--- sigprocmask/kernel/signal.c~8_sys_sigprocmask	2011-04-17 22:32:58.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-04-18 14:45:57.000000000 +0200
@@ -2172,40 +2172,34 @@ int sigprocmask(int how, sigset_t *set, 
  *  @oset: previous value of signal mask if non-null
  *  @sigsetsize: size of sigset_t type
  */
-SYSCALL_DEFINE4(rt_sigprocmask, int, how, sigset_t __user *, set,
+SYSCALL_DEFINE4(rt_sigprocmask, int, how, sigset_t __user *, nset,
 		sigset_t __user *, oset, size_t, sigsetsize)
 {
-	int error = -EINVAL;
 	sigset_t old_set, new_set;
+	int error;
 
-	/* XXX: Don't preclude handling different sized sigset_t's.  */
+	/* Don't preclude handling different sized sigset_t's. */
 	if (sigsetsize != sizeof(sigset_t))
-		goto out;
+		return -EINVAL;
 
-	if (set) {
-		error = -EFAULT;
-		if (copy_from_user(&new_set, set, sizeof(*set)))
-			goto out;
+	old_set = current->blocked;
+
+	if (nset) {
+		if (copy_from_user(&new_set, nset, sizeof(sigset_t)))
+			return -EFAULT;
 		sigdelsetmask(&new_set, sigmask(SIGKILL)|sigmask(SIGSTOP));
 
-		error = sigprocmask(how, &new_set, &old_set);
+		error = sigprocmask(how, &new_set, NULL);
 		if (error)
-			goto out;
-		if (oset)
-			goto set_old;
-	} else if (oset) {
-		spin_lock_irq(&current->sighand->siglock);
-		old_set = current->blocked;
-		spin_unlock_irq(&current->sighand->siglock);
+			return error;
+	}
 
-	set_old:
-		error = -EFAULT;
-		if (copy_to_user(oset, &old_set, sizeof(*oset)))
-			goto out;
+	if (oset) {
+		if (copy_to_user(oset, &old_set, sizeof(sigset_t)))
+			return -EFAULT;
 	}
-	error = 0;
-out:
-	return error;
+
+	return 0;
 }
 
 long do_sigpending(void __user *set, unsigned long sigsetsize)

Re: [PATCH v2 8/7] signal: cleanup sys_rt_sigprocmask()

[Matt Fleming]

On Mon, 18 Apr 2011 15:47:00 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> sys_rt_sigprocmask() looks unnecessarily complicated, simplify it.
> We can just read current->blocked lockless unconditionally before
> anything else and then copy-to-user it if needed.  At worst we
> copy 4 words on mips.
> 
> We could copy-to-user the old mask first and simplify the code even
> more, but the patch tries to keep the current behaviour: we change
> current->block even if copy_to_user(oset) fails.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
> 
>  kernel/signal.c |   40 +++++++++++++++++-----------------------
>  1 file changed, 17 insertions(+), 23 deletions(-)
> 
> --- sigprocmask/kernel/signal.c~8_sys_sigprocmask	2011-04-17 22:32:58.000000000 +0200
> +++ sigprocmask/kernel/signal.c	2011-04-18 14:45:57.000000000 +0200
> @@ -2172,40 +2172,34 @@ int sigprocmask(int how, sigset_t *set, 
>   *  @oset: previous value of signal mask if non-null
>   *  @sigsetsize: size of sigset_t type
>   */
> -SYSCALL_DEFINE4(rt_sigprocmask, int, how, sigset_t __user *, set,
> +SYSCALL_DEFINE4(rt_sigprocmask, int, how, sigset_t __user *, nset,
>  		sigset_t __user *, oset, size_t, sigsetsize)
>  {
> -	int error = -EINVAL;
>  	sigset_t old_set, new_set;
> +	int error;
>  
> -	/* XXX: Don't preclude handling different sized sigset_t's.  */
> +	/* Don't preclude handling different sized sigset_t's. */
>  	if (sigsetsize != sizeof(sigset_t))
> -		goto out;
> +		return -EINVAL;

I don't think it's correct to remove the 'XXX'. The comment is
currently saying "We don't handle different sized sigset_t's, but we
should", whereas removing the 'XXX' says to me that we _DO_ handle
different sized sigset_t's. If you don't like the 'XXX' you could
always swap it for a 'TODO'?

Otherwise looks like a very nice cleanup.

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

Re: [PATCH v2 8/7] signal: cleanup sys_rt_sigprocmask()

[Oleg Nesterov]

On 04/22, Matt Fleming wrote:
>
> On Mon, 18 Apr 2011 15:47:00 +0200
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > -	/* XXX: Don't preclude handling different sized sigset_t's.  */
> > +	/* Don't preclude handling different sized sigset_t's. */
> >  	if (sigsetsize != sizeof(sigset_t))
> > -		goto out;
> > +		return -EINVAL;
>
> I don't think it's correct to remove the 'XXX'. The comment is
> currently saying "We don't handle different sized sigset_t's, but we
> should", whereas removing the 'XXX' says to me that we _DO_ handle
> different sized sigset_t's.

Hmm. I think you are right. I simply didn't know what "preclude" means
and thus misunderstood the comment.

> If you don't like the 'XXX' you could
> always swap it for a 'TODO'?

I think we should just remove this comment. It is confusing. This check
is trivial and does not need any explanation. The comment (and the code)
is very old, it predates the git history. I do not think this API will be
changed, unlikely we need to handle the different sized sigset_t's.

What do you think?

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

Thanks!

Oleg.

Re: [PATCH v2 8/7] signal: cleanup sys_rt_sigprocmask()

[Matt Fleming]

On Sat, 23 Apr 2011 20:20:31 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> I think we should just remove this comment. It is confusing. This check
> is trivial and does not need any explanation. The comment (and the code)
> is very old, it predates the git history. I do not think this API will be
> changed, unlikely we need to handle the different sized sigset_t's.
> 
> What do you think?

Sure, that is OK with me!

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v2 8/7] signal: cleanup sys_rt_sigprocmask()

[Tejun Heo]

On Mon, Apr 18, 2011 at 03:47:00PM +0200, Oleg Nesterov wrote:
> sys_rt_sigprocmask() looks unnecessarily complicated, simplify it.
> We can just read current->blocked lockless unconditionally before
> anything else and then copy-to-user it if needed.  At worst we
> copy 4 words on mips.
> 
> We could copy-to-user the old mask first and simplify the code even
> more, but the patch tries to keep the current behaviour: we change
> current->block even if copy_to_user(oset) fails.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [RFC PATCH v2 0/7] signal: sigprocmask fixes

[Linus Torvalds]

On Mon, Apr 18, 2011 at 6:44 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> Once again: if we need this, then we need a lot more (trivial) changes
> like 6/7 and 7/7. Basically every change of ->blocked should be converted
> to use set_current_blocked(). OTOH, perhaps this makes sense by itself.

Hmm. The more I think about this, the less I like it.

What if the pending thread signal was thread-specific to begin with?

For example, if we have a SIGFPE and a SIGKILL that happen at the same
time, a dying task may have a SIGFPE pending when it dies, and that
SIGFPE should _not_ be just distributed out to the other threads in
the thread group.

Am I missing something that protects against this?

                  Linus

Re: [RFC PATCH v2 0/7] signal: sigprocmask fixes

[Oleg Nesterov]

On 04/18, Linus Torvalds wrote:
>
> On Mon, Apr 18, 2011 at 6:44 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Once again: if we need this, then we need a lot more (trivial) changes
> > like 6/7 and 7/7. Basically every change of ->blocked should be converted
> > to use set_current_blocked(). OTOH, perhaps this makes sense by itself.
>
> Hmm. The more I think about this, the less I like it.
>
> What if the pending thread signal was thread-specific to begin with?

These patches should not change the current behaviour in this case.
We never try to re-target the thread-specific signals. Note that
retarget_shared_pending() checks ->signal->shared_pending only.

> For example, if we have a SIGFPE and a SIGKILL that happen at the same
> time, a dying task may have a SIGFPE pending when it dies, and that
> SIGFPE should _not_ be just distributed out to the other threads in
> the thread group.

Yes, and it won't be.

Btw, we do not need to distribute SIGKILL too, we can change
retarget_shared_pending() to remove SIGKILL from shared_pending.
But this only matters when the caller is exit_signals(), and in
this case it should likely notice signal_group_exit() unless
SIGKILL (in unlikely case) it comes in between.

Or I misunderstood?

Oleg.

Re: [RFC PATCH v2 0/7] signal: sigprocmask fixes

[Linus Torvalds]

On Mon, Apr 18, 2011 at 10:32 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> These patches should not change the current behaviour in this case.
> We never try to re-target the thread-specific signals. Note that
> retarget_shared_pending() checks ->signal->shared_pending only.

Ok, I clearly didn't follow the details closely enough.

In that case, this series looks fairly ok to me. I'm still unsure
whether it's _required_, but it looks sane and reasonable.

                   Linus

[PATCH 0/3] do_sigtimedwait() needs retarget_shared_pending()

[Oleg Nesterov]

On 04/18, Linus Torvalds wrote:
>
> In that case, this series looks fairly ok to me. I'm still unsure
> whether it's _required_,

Indeed! It is a bit annoying, I am not sure too. This "problem" is
very, very, old. And nobody complained so far. Except, this _might_
explain the hang reported by Nikita.

OTOH. Probably set_current_blocked() makes sense anyway, it would be
nice to require that nobody can play with ->blocked directly. We can
reconsider retarget_shared_pendind() or simply kill it if it adds the
noticeable overhead in practice.

So, I'll assume we need this changes. Correctness is always good. All
further changes should be simple and straightforward, I am going to
trim CC. Except sys_rt_sigtimedwait() is nasty, we need another helper
for simplicity.

If nobody objects, I'll send at lot of other "needs set_current_blocked"
simple patches to Andrew.

Oleg.

[PATCH 1/3] signal: sys_rt_sigtimedwait: simplify the timeout logic

[Oleg Nesterov]

No functional changes, cleanup compat_sys_rt_sigtimedwait() and
sys_rt_sigtimedwait().

Calculate the timeout before we take ->siglock, this simplifies and
lessens the code. Use timespec_valid() to check the timespec.

I don't understand why we are adding (ts.tv_sec || ts.tv_nsec) to
timespec_to_jiffies(&ts). Perhaps to ensure we will sleep at least
one jiffy if ts != 0? But in this case we should only check tv_nsec,
I don't think timespec_to_jiffies() can return zero if tv_sec != 0.
In fact I suspect timespec_to_jiffies() can only return zero if
tv_sec == tv_nsec == 0 because we add "TICK_NSEC - 1", but I am not
sure I understand correctly this math.

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

 kernel/signal.c |   46 ++++++++++++++++++++--------------------------
 kernel/compat.c |   38 ++++++++++++++++----------------------
 2 files changed, 36 insertions(+), 48 deletions(-)

--- sigprocmask/kernel/signal.c~1_sigtimedwait_to	2011-04-22 15:48:33.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-04-22 19:05:51.000000000 +0200
@@ -2327,7 +2327,7 @@ SYSCALL_DEFINE4(rt_sigtimedwait, const s
 	sigset_t these;
 	struct timespec ts;
 	siginfo_t info;
-	long timeout = 0;
+	long timeout;
 
 	/* XXX: Don't preclude handling different sized sigset_t's.  */
 	if (sigsetsize != sizeof(sigset_t))
@@ -2343,41 +2343,35 @@ SYSCALL_DEFINE4(rt_sigtimedwait, const s
 	sigdelsetmask(&these, sigmask(SIGKILL)|sigmask(SIGSTOP));
 	signotset(&these);
 
+	timeout = MAX_SCHEDULE_TIMEOUT;
 	if (uts) {
 		if (copy_from_user(&ts, uts, sizeof(ts)))
 			return -EFAULT;
-		if (ts.tv_nsec >= 1000000000L || ts.tv_nsec < 0
-		    || ts.tv_sec < 0)
+		if (!timespec_valid(&ts))
 			return -EINVAL;
+		timeout = timespec_to_jiffies(&ts) + (ts.tv_sec || ts.tv_nsec);
 	}
 
 	spin_lock_irq(&current->sighand->siglock);
 	sig = dequeue_signal(current, &these, &info);
-	if (!sig) {
-		timeout = MAX_SCHEDULE_TIMEOUT;
-		if (uts)
-			timeout = (timespec_to_jiffies(&ts)
-				   + (ts.tv_sec || ts.tv_nsec));
-
-		if (timeout) {
-			/*
-			 * None ready -- temporarily unblock those we're
-			 * interested while we are sleeping in so that we'll
-			 * be awakened when they arrive.
-			 */
-			current->real_blocked = current->blocked;
-			sigandsets(&current->blocked, &current->blocked, &these);
-			recalc_sigpending();
-			spin_unlock_irq(&current->sighand->siglock);
+	if (!sig && timeout) {
+		/*
+		 * None ready -- temporarily unblock those we're
+		 * interested while we are sleeping in so that we'll
+		 * be awakened when they arrive.
+		 */
+		current->real_blocked = current->blocked;
+		sigandsets(&current->blocked, &current->blocked, &these);
+		recalc_sigpending();
+		spin_unlock_irq(&current->sighand->siglock);
 
-			timeout = schedule_timeout_interruptible(timeout);
+		timeout = schedule_timeout_interruptible(timeout);
 
-			spin_lock_irq(&current->sighand->siglock);
-			sig = dequeue_signal(current, &these, &info);
-			current->blocked = current->real_blocked;
-			siginitset(&current->real_blocked, 0);
-			recalc_sigpending();
-		}
+		spin_lock_irq(&current->sighand->siglock);
+		sig = dequeue_signal(current, &these, &info);
+		current->blocked = current->real_blocked;
+		siginitset(&current->real_blocked, 0);
+		recalc_sigpending();
 	}
 	spin_unlock_irq(&current->sighand->siglock);
 
--- sigprocmask/kernel/compat.c~1_sigtimedwait_to	2011-04-14 21:23:22.000000000 +0200
+++ sigprocmask/kernel/compat.c	2011-04-22 19:19:39.000000000 +0200
@@ -893,7 +893,7 @@ compat_sys_rt_sigtimedwait (compat_sigse
 	int sig;
 	struct timespec t;
 	siginfo_t info;
-	long ret, timeout = 0;
+	long ret, timeout;
 
 	if (sigsetsize != sizeof(sigset_t))
 		return -EINVAL;
@@ -904,36 +904,30 @@ compat_sys_rt_sigtimedwait (compat_sigse
 	sigdelsetmask(&s,sigmask(SIGKILL)|sigmask(SIGSTOP));
 	signotset(&s);
 
+	timeout = MAX_SCHEDULE_TIMEOUT;
 	if (uts) {
 		if (get_compat_timespec (&t, uts))
 			return -EFAULT;
-		if (t.tv_nsec >= 1000000000L || t.tv_nsec < 0
-				|| t.tv_sec < 0)
+		if (!timespec_valid(&t))
 			return -EINVAL;
+		timeout = timespec_to_jiffies(&t) + (t.tv_sec || t.tv_nsec);
 	}
 
 	spin_lock_irq(&current->sighand->siglock);
 	sig = dequeue_signal(current, &s, &info);
-	if (!sig) {
-		timeout = MAX_SCHEDULE_TIMEOUT;
-		if (uts)
-			timeout = timespec_to_jiffies(&t)
-				+(t.tv_sec || t.tv_nsec);
-		if (timeout) {
-			current->real_blocked = current->blocked;
-			sigandsets(&current->blocked, &current->blocked, &s);
-
-			recalc_sigpending();
-			spin_unlock_irq(&current->sighand->siglock);
+	if (!sig && timeout) {
+		current->real_blocked = current->blocked;
+		sigandsets(&current->blocked, &current->blocked, &s);
+		recalc_sigpending();
+		spin_unlock_irq(&current->sighand->siglock);
 
-			timeout = schedule_timeout_interruptible(timeout);
+		timeout = schedule_timeout_interruptible(timeout);
 
-			spin_lock_irq(&current->sighand->siglock);
-			sig = dequeue_signal(current, &s, &info);
-			current->blocked = current->real_blocked;
-			siginitset(&current->real_blocked, 0);
-			recalc_sigpending();
-		}
+		spin_lock_irq(&current->sighand->siglock);
+		sig = dequeue_signal(current, &s, &info);
+		current->blocked = current->real_blocked;
+		siginitset(&current->real_blocked, 0);
+		recalc_sigpending();
 	}
 	spin_unlock_irq(&current->sighand->siglock);
 
@@ -943,7 +937,7 @@ compat_sys_rt_sigtimedwait (compat_sigse
 			if (copy_siginfo_to_user32(uinfo, &info))
 				ret = -EFAULT;
 		}
-	}else {
+	} else {
 		ret = timeout?-EINTR:-EAGAIN;
 	}
 	return ret;

Re: [PATCH 1/3] signal: sys_rt_sigtimedwait: simplify the timeout logic

[Tejun Heo]

On Sat, Apr 23, 2011 at 07:59:22PM +0200, Oleg Nesterov wrote:
> No functional changes, cleanup compat_sys_rt_sigtimedwait() and
> sys_rt_sigtimedwait().
> 
> Calculate the timeout before we take ->siglock, this simplifies and
> lessens the code. Use timespec_valid() to check the timespec.
> 
> I don't understand why we are adding (ts.tv_sec || ts.tv_nsec) to
> timespec_to_jiffies(&ts). Perhaps to ensure we will sleep at least
> one jiffy if ts != 0? But in this case we should only check tv_nsec,
> I don't think timespec_to_jiffies() can return zero if tv_sec != 0.
> In fact I suspect timespec_to_jiffies() can only return zero if
> tv_sec == tv_nsec == 0 because we add "TICK_NSEC - 1", but I am not
> sure I understand correctly this math.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

It might be a good idea to note the weird jiffies calculation with a
comment?

Thanks.

-- 
tejun

Re: [PATCH 1/3] signal: sys_rt_sigtimedwait: simplify the timeout logic

[Oleg Nesterov]

On 04/25, Tejun Heo wrote:
>
> On Sat, Apr 23, 2011 at 07:59:22PM +0200, Oleg Nesterov wrote:
> >
> > I don't understand why we are adding (ts.tv_sec || ts.tv_nsec) to
> > timespec_to_jiffies(&ts). Perhaps to ensure we will sleep at least
> > one jiffy if ts != 0? But in this case we should only check tv_nsec,
> > I don't think timespec_to_jiffies() can return zero if tv_sec != 0.
> > In fact I suspect timespec_to_jiffies() can only return zero if
> > tv_sec == tv_nsec == 0 because we add "TICK_NSEC - 1", but I am not
> > sure I understand correctly this math.
> >
> It might be a good idea to note the weird jiffies calculation with a
> comment?

If only I knew what this comment could say except

	/* Why do we add (tv_sec || tv_nsec) ?  */

I'd better send 4/3 which simply removes this (I hope) unneeded code.

Oleg.

Re: [PATCH 1/3] signal: sys_rt_sigtimedwait: simplify the timeout logic

[Linus Torvalds]

On Mon, Apr 25, 2011 at 10:26 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> If only I knew what this comment could say except
>
>        /* Why do we add (tv_sec || tv_nsec) ?  */
>
> I'd better send 4/3 which simply removes this (I hope) unneeded code.

It's to guarantee that timeout is at least one tick more than asked
for, because the rule is that you really have to wait for AT LEAST the
time asked for. With the "zero timeout" being special, since that is
"immediate".

Imagine that you ask for one timer tick - but that you're in the
_middle_ of the current one. Waiting for the next timer is going to be
too short - you'll only get half a timer tick. So we need to ask for
"ceiling(nanoseconds / nanosecondspertick) + 1" to make sure that we
really wait _longer_ than asked for.

So "+ (tv_sec || tv_nsec)" is just the "+1" for the "not zero timeout" case.

                     Linus

Re: [PATCH 1/3] signal: sys_rt_sigtimedwait: simplify the timeout logic

[Oleg Nesterov]

On 04/25, Linus Torvalds wrote:
>
> On Mon, Apr 25, 2011 at 10:26 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > If only I knew what this comment could say except
> >
> >        /* Why do we add (tv_sec || tv_nsec) ?  */
> >
> > I'd better send 4/3 which simply removes this (I hope) unneeded code.
>
> It's to guarantee that timeout is at least one tick more than asked
> for, because the rule is that you really have to wait for AT LEAST the
> time asked for.

Aah, thanks. This makes sense. I'll add the comment.

> So "+ (tv_sec || tv_nsec)" is just the "+1" for the "not zero timeout" case.

So, we have

	timeout = timespec_to_jiffies(ts) + (tv_sec || tv_nsec);
	...

	if (timeout)
		timeout = schedule_timeout_interruptible(timeout);

Perhaps it makes sense to turn this code into

	timeout = timespec_to_jiffies(ts);

	if (timeout)
		// make sure we sleep at least the time we asked for
		timeout = schedule_timeout_interruptible(timeout + 1);

Assuming that timespec_to_jiffies() always returns nonzero if ts is
"not zero timeout". I think it does.

Oleg.

Re: [PATCH 1/3] signal: sys_rt_sigtimedwait: simplify the timeout logic

[Linus Torvalds]

On Mon, Apr 25, 2011 at 10:56 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> Assuming that timespec_to_jiffies() always returns nonzero if ts is
> "not zero timeout". I think it does.

You really need to double-check that it does. We've not always done
that right, and while I think "round up" is the right thing to do
there, it had better be verified and then a comment added to make sure
it doesn't change.

But yes, with verification and comment, your "if (timeout)  ..
timeout+1" would work fine.

                  Linus

Re: [PATCH 1/3] signal: sys_rt_sigtimedwait: simplify the timeout logic

[Matt Fleming]

On Sat, 23 Apr 2011 19:59:22 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> No functional changes, cleanup compat_sys_rt_sigtimedwait() and
> sys_rt_sigtimedwait().
> 
> Calculate the timeout before we take ->siglock, this simplifies and
> lessens the code. Use timespec_valid() to check the timespec.
> 
> I don't understand why we are adding (ts.tv_sec || ts.tv_nsec) to
> timespec_to_jiffies(&ts). Perhaps to ensure we will sleep at least
> one jiffy if ts != 0? But in this case we should only check tv_nsec,
> I don't think timespec_to_jiffies() can return zero if tv_sec != 0.
> In fact I suspect timespec_to_jiffies() can only return zero if
> tv_sec == tv_nsec == 0 because we add "TICK_NSEC - 1", but I am not
> sure I understand correctly this math.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH 2/3] signal: introduce do_sigtimedwait() to factor out compat/native code

[Oleg Nesterov]

Factor out the common code in sys_rt_sigtimedwait/compat_sys_rt_sigtimedwait
to the new helper, do_sigtimedwait().

Perhaps it would be better to move compat_sys_rt_sigtimedwait() into
signal.c under CONFIG_COMPAT, then we can make do_sigtimedwait() static.

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

 include/linux/signal.h |    1 
 kernel/signal.c        |   71 ++++++++++++++++++++++++++-----------------------
 kernel/compat.c        |   29 ++------------------
 3 files changed, 43 insertions(+), 58 deletions(-)

--- sigprocmask/include/linux/signal.h~2_do_sigtimedwait	2011-04-22 15:48:33.000000000 +0200
+++ sigprocmask/include/linux/signal.h	2011-04-22 21:05:48.000000000 +0200
@@ -242,6 +242,7 @@ extern int __group_send_sig_info(int, st
 extern long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig,
 				 siginfo_t *info);
 extern long do_sigpending(void __user *, unsigned long);
+extern int do_sigtimedwait(sigset_t *, siginfo_t *, long);
 extern int sigprocmask(int, sigset_t *, sigset_t *);
 extern void set_current_blocked(const sigset_t *);
 extern int show_unhandled_signals;
--- sigprocmask/kernel/signal.c~2_do_sigtimedwait	2011-04-22 19:05:51.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-04-22 20:57:06.000000000 +0200
@@ -2311,6 +2311,39 @@ int copy_siginfo_to_user(siginfo_t __use
 
 #endif
 
+int do_sigtimedwait(sigset_t *these, siginfo_t *info, long timeout)
+{
+	struct task_struct *tsk = current;
+	int sig;
+
+	spin_lock_irq(&tsk->sighand->siglock);
+	sig = dequeue_signal(tsk, these, info);
+	if (!sig && timeout) {
+		/*
+		 * None ready -- temporarily unblock those we're
+		 * interested while we are sleeping in so that we'll
+		 * be awakened when they arrive.
+		 */
+		tsk->real_blocked = tsk->blocked;
+		sigandsets(&tsk->blocked, &tsk->blocked, these);
+		recalc_sigpending();
+		spin_unlock_irq(&tsk->sighand->siglock);
+
+		timeout = schedule_timeout_interruptible(timeout);
+
+		spin_lock_irq(&tsk->sighand->siglock);
+		sig = dequeue_signal(tsk, these, info);
+		tsk->blocked = tsk->real_blocked;
+		siginitset(&tsk->real_blocked, 0);
+		recalc_sigpending();
+	}
+	spin_unlock_irq(&tsk->sighand->siglock);
+
+	if (sig)
+		return sig;
+	return timeout ? -EINTR : -EAGAIN;
+}
+
 /**
  *  sys_rt_sigtimedwait - synchronously wait for queued signals specified
  *			in @uthese
@@ -2323,11 +2356,11 @@ SYSCALL_DEFINE4(rt_sigtimedwait, const s
 		siginfo_t __user *, uinfo, const struct timespec __user *, uts,
 		size_t, sigsetsize)
 {
-	int ret, sig;
 	sigset_t these;
 	struct timespec ts;
 	siginfo_t info;
 	long timeout;
+	int ret;
 
 	/* XXX: Don't preclude handling different sized sigset_t's.  */
 	if (sigsetsize != sizeof(sigset_t))
@@ -2352,39 +2385,11 @@ SYSCALL_DEFINE4(rt_sigtimedwait, const s
 		timeout = timespec_to_jiffies(&ts) + (ts.tv_sec || ts.tv_nsec);
 	}
 
-	spin_lock_irq(&current->sighand->siglock);
-	sig = dequeue_signal(current, &these, &info);
-	if (!sig && timeout) {
-		/*
-		 * None ready -- temporarily unblock those we're
-		 * interested while we are sleeping in so that we'll
-		 * be awakened when they arrive.
-		 */
-		current->real_blocked = current->blocked;
-		sigandsets(&current->blocked, &current->blocked, &these);
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
-
-		timeout = schedule_timeout_interruptible(timeout);
-
-		spin_lock_irq(&current->sighand->siglock);
-		sig = dequeue_signal(current, &these, &info);
-		current->blocked = current->real_blocked;
-		siginitset(&current->real_blocked, 0);
-		recalc_sigpending();
-	}
-	spin_unlock_irq(&current->sighand->siglock);
+	ret = do_sigtimedwait(&these, &info, timeout);
 
-	if (sig) {
-		ret = sig;
-		if (uinfo) {
-			if (copy_siginfo_to_user(uinfo, &info))
-				ret = -EFAULT;
-		}
-	} else {
-		ret = -EAGAIN;
-		if (timeout)
-			ret = -EINTR;
+	if (ret > 0 && uinfo) {
+		if (copy_siginfo_to_user(uinfo, &info))
+			ret = -EFAULT;
 	}
 
 	return ret;
--- sigprocmask/kernel/compat.c~2_do_sigtimedwait	2011-04-22 19:19:39.000000000 +0200
+++ sigprocmask/kernel/compat.c	2011-04-22 21:20:33.000000000 +0200
@@ -890,7 +890,6 @@ compat_sys_rt_sigtimedwait (compat_sigse
 {
 	compat_sigset_t s32;
 	sigset_t s;
-	int sig;
 	struct timespec t;
 	siginfo_t info;
 	long ret, timeout;
@@ -913,33 +912,13 @@ compat_sys_rt_sigtimedwait (compat_sigse
 		timeout = timespec_to_jiffies(&t) + (t.tv_sec || t.tv_nsec);
 	}
 
-	spin_lock_irq(&current->sighand->siglock);
-	sig = dequeue_signal(current, &s, &info);
-	if (!sig && timeout) {
-		current->real_blocked = current->blocked;
-		sigandsets(&current->blocked, &current->blocked, &s);
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
-
-		timeout = schedule_timeout_interruptible(timeout);
+	ret = do_sigtimedwait(&s, &info, timeout);
 
-		spin_lock_irq(&current->sighand->siglock);
-		sig = dequeue_signal(current, &s, &info);
-		current->blocked = current->real_blocked;
-		siginitset(&current->real_blocked, 0);
-		recalc_sigpending();
+	if (ret > 0 && uinfo) {
+		if (copy_siginfo_to_user32(uinfo, &info))
+			ret = -EFAULT;
 	}
-	spin_unlock_irq(&current->sighand->siglock);
 
-	if (sig) {
-		ret = sig;
-		if (uinfo) {
-			if (copy_siginfo_to_user32(uinfo, &info))
-				ret = -EFAULT;
-		}
-	} else {
-		ret = timeout?-EINTR:-EAGAIN;
-	}
 	return ret;
 
 }

Re: [PATCH 2/3] signal: introduce do_sigtimedwait() to factor out compat/native code

[Tejun Heo]

On Sat, Apr 23, 2011 at 07:59:40PM +0200, Oleg Nesterov wrote:
> Factor out the common code in sys_rt_sigtimedwait/compat_sys_rt_sigtimedwait
> to the new helper, do_sigtimedwait().
> 
> Perhaps it would be better to move compat_sys_rt_sigtimedwait() into
> signal.c under CONFIG_COMPAT, then we can make do_sigtimedwait() static.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

-- 
tejun

Re: [PATCH 2/3] signal: introduce do_sigtimedwait() to factor out compat/native code

[Tejun Heo]

Just one more thing.

On Sat, Apr 23, 2011 at 07:59:40PM +0200, Oleg Nesterov wrote:
> +int do_sigtimedwait(sigset_t *these, siginfo_t *info, long timeout)

Maybe @these isn't the base name here?  It implies that these are the
signals the function is interested in but in reality it is the
negation of that.  The original function should be blamed for using
the same name while negating its meaning but separating out the
function makes the inconsitency stand out.

Thanks.

-- 
tejun

Re: [PATCH 2/3] signal: introduce do_sigtimedwait() to factor out compat/native code

[Oleg Nesterov]

On 04/25, Tejun Heo wrote:
>
> Just one more thing.
>
> On Sat, Apr 23, 2011 at 07:59:40PM +0200, Oleg Nesterov wrote:
> > +int do_sigtimedwait(sigset_t *these, siginfo_t *info, long timeout)
>
> Maybe @these isn't the base name here?  It implies that these are the
> signals the function is interested in but in reality it is the
> negation of that.

Yees, I simply copied the old name. And yes, I tried to invent the
better name but failed.

Hmm. I think this patch can do a bit more. We can factor out
sigdelsetmask(SIGKILL | SIGSTOP) + signotset() as well, I'll resend.

Still, what should be the name? @set? @mask? @which?

Oleg.

Re: [PATCH 2/3] signal: introduce do_sigtimedwait() to factor out compat/native code

[Tejun Heo]

Hey,

On Mon, Apr 25, 2011 at 05:33:16PM +0200, Oleg Nesterov wrote:
> On 04/25, Tejun Heo wrote:
> >
> > Just one more thing.
> >
> > On Sat, Apr 23, 2011 at 07:59:40PM +0200, Oleg Nesterov wrote:
> > > +int do_sigtimedwait(sigset_t *these, siginfo_t *info, long timeout)
> >
> > Maybe @these isn't the base name here?  It implies that these are the
                           ^^^^
                     oops, best

> > signals the function is interested in but in reality it is the
> > negation of that.
> 
> Yees, I simply copied the old name. And yes, I tried to invent the
> better name but failed.
> 
> Hmm. I think this patch can do a bit more. We can factor out
> sigdelsetmask(SIGKILL | SIGSTOP) + signotset() as well, I'll resend.
> 
> Still, what should be the name? @set? @mask? @which?

Given that next_signal() and friends already use @mask, probably
@mask?  As long as positive selection and negative masking are clearly
distinguishible...

Thanks.

-- 
tejun

Re: [PATCH 2/3] signal: introduce do_sigtimedwait() to factor out compat/native code

[Matt Fleming]

On Sat, 23 Apr 2011 19:59:40 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> Factor out the common code in sys_rt_sigtimedwait/compat_sys_rt_sigtimedwait
> to the new helper, do_sigtimedwait().
> 
> Perhaps it would be better to move compat_sys_rt_sigtimedwait() into
> signal.c under CONFIG_COMPAT, then we can make do_sigtimedwait() static.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

The changes Tejun suggested seem like a good idea. I also think that
moving compat_sys_rt_sigtimedwait() into signal.c makes sense.

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending()

[Oleg Nesterov]

do_sigtimedwait() changes current->blocked and thus it needs
set_current_bloked()->retarget_shared_pending().

We could use set_current_bloked() directly. It is fine to change
->real_blocked from all-zeroes to ->blocked and vice versa lockless,
but this is not immediately clear, looks racy, and needs a huge
comment to explain why this is correct.

To keep the things simple this patch adds the new static helper,
__set_task_blocked() which should be called with ->siglock held. This
way we can change both ->real_blocked and ->blocked atomically under
->siglock as the current code does. This is more understandable.

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

 kernel/signal.c |   21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

--- sigprocmask/kernel/signal.c~3_sigtimedwait_retarget	2011-04-22 20:57:06.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-04-22 21:49:11.000000000 +0200
@@ -2115,11 +2115,8 @@ long do_no_restart_syscall(struct restar
 	return -EINTR;
 }
 
-void set_current_blocked(const sigset_t *newset)
+static void __set_task_blocked(struct task_struct *tsk, const sigset_t *newset)
 {
-	struct task_struct *tsk = current;
-
-	spin_lock_irq(&tsk->sighand->siglock);
 	if (signal_pending(tsk) && !thread_group_empty(tsk)) {
 		sigset_t newblocked;
 		signandsets(&newblocked, newset, &current->blocked);
@@ -2127,6 +2124,14 @@ void set_current_blocked(const sigset_t 
 	}
 	tsk->blocked = *newset;
 	recalc_sigpending();
+}
+
+void set_current_blocked(const sigset_t *newset)
+{
+	struct task_struct *tsk = current;
+
+	spin_lock_irq(&tsk->sighand->siglock);
+	__set_task_blocked(tsk, newset);
 	spin_unlock_irq(&tsk->sighand->siglock);
 }
 
@@ -2322,7 +2327,8 @@ int do_sigtimedwait(sigset_t *these, sig
 		/*
 		 * None ready -- temporarily unblock those we're
 		 * interested while we are sleeping in so that we'll
-		 * be awakened when they arrive.
+		 * be awakened when they arrive. Unblocking is always
+		 * fine, we can avoid set_current_blocked().
 		 */
 		tsk->real_blocked = tsk->blocked;
 		sigandsets(&tsk->blocked, &tsk->blocked, these);
@@ -2332,10 +2338,9 @@ int do_sigtimedwait(sigset_t *these, sig
 		timeout = schedule_timeout_interruptible(timeout);
 
 		spin_lock_irq(&tsk->sighand->siglock);
-		sig = dequeue_signal(tsk, these, info);
-		tsk->blocked = tsk->real_blocked;
+		__set_task_blocked(tsk, &tsk->real_blocked);
 		siginitset(&tsk->real_blocked, 0);
-		recalc_sigpending();
+		sig = dequeue_signal(tsk, these, info);
 	}
 	spin_unlock_irq(&tsk->sighand->siglock);
 

Re: [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending()

[Tejun Heo]

Hello,

On Sat, Apr 23, 2011 at 08:00:00PM +0200, Oleg Nesterov wrote:
> do_sigtimedwait() changes current->blocked and thus it needs
> set_current_bloked()->retarget_shared_pending().
> 
> We could use set_current_bloked() directly. It is fine to change
> ->real_blocked from all-zeroes to ->blocked and vice versa lockless,
> but this is not immediately clear, looks racy, and needs a huge
> comment to explain why this is correct.
> 
> To keep the things simple this patch adds the new static helper,
> __set_task_blocked() which should be called with ->siglock held. This
> way we can change both ->real_blocked and ->blocked atomically under
> ->siglock as the current code does. This is more understandable.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

> @@ -2322,7 +2327,8 @@ int do_sigtimedwait(sigset_t *these, sig
>  		/*
>  		 * None ready -- temporarily unblock those we're
>  		 * interested while we are sleeping in so that we'll
> -		 * be awakened when they arrive.
> +		 * be awakened when they arrive. Unblocking is always
> +		 * fine, we can avoid set_current_blocked().
>  		 */
>  		tsk->real_blocked = tsk->blocked;
>  		sigandsets(&tsk->blocked, &tsk->blocked, these);

Maybe it would be a good idea to introduce a new helper which checks /
enforces that the operation indeed is only unblocking?  Also, it can
be a pure preference but I think _locked suffix is better / more
common for APIs which expect the caller to be responsible for locking.

Thanks.

-- 
tejun

Re: [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending()

[Oleg Nesterov]

On 04/25, Tejun Heo wrote:
>
> > @@ -2322,7 +2327,8 @@ int do_sigtimedwait(sigset_t *these, sig
> >  		/*
> >  		 * None ready -- temporarily unblock those we're
> >  		 * interested while we are sleeping in so that we'll
> > -		 * be awakened when they arrive.
> > +		 * be awakened when they arrive. Unblocking is always
> > +		 * fine, we can avoid set_current_blocked().
> >  		 */
> >  		tsk->real_blocked = tsk->blocked;
> >  		sigandsets(&tsk->blocked, &tsk->blocked, these);
>
> Maybe it would be a good idea to introduce a new helper which checks /
> enforces that the operation indeed is only unblocking?

I hope nobody will change ->blocked directly, except this function
and force_sig_info(). And daemonize/allow_signal/disallow_signal, but
there are special and probably we can already kill this deprecated
block/unblock code and forbid kernel_thread(CLONE_SIGHAND) + daemonize().
In fact I think daemonize() should go away.

So, I don't really think we need another helper to unblock something.

> Also, it can
> be a pure preference but I think _locked suffix is better / more
> common for APIs which expect the caller to be responsible for locking.

Again, I can rename... Cough, but in this case please simply suggest
another name. set_tsk_blocked_locked?

Oleg.

Re: [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending()

[Tejun Heo]

Hello,

On Mon, Apr 25, 2011 at 06:01:15PM +0200, Oleg Nesterov wrote:
> > Maybe it would be a good idea to introduce a new helper which checks /
> > enforces that the operation indeed is only unblocking?
> 
> I hope nobody will change ->blocked directly, except this function
> and force_sig_info(). And daemonize/allow_signal/disallow_signal, but
> there are special and probably we can already kill this deprecated
> block/unblock code and forbid kernel_thread(CLONE_SIGHAND) + daemonize().
> In fact I think daemonize() should go away.
> 
> So, I don't really think we need another helper to unblock something.

Oh I see.  I thought there would be quite a number of places
unblocking directly.  If that's not the case, it's fine with me.

> > Also, it can
> > be a pure preference but I think _locked suffix is better / more
> > common for APIs which expect the caller to be responsible for locking.
> 
> Again, I can rename... Cough, but in this case please simply suggest
> another name. set_tsk_blocked_locked?

Oooh, blocked_locked, didn't see that one coming.  Maybe
set_tsk_sigmask() and set_tsk_sigmask_locked()?  I prefer sigmask to
blocked anyway, so...

Thanks.

-- 
tejun

Re: [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending()

[Oleg Nesterov]

On 04/25, Tejun Heo wrote:
>
> On Mon, Apr 25, 2011 at 06:01:15PM +0200, Oleg Nesterov wrote:
> >
> > Again, I can rename... Cough, but in this case please simply suggest
> > another name. set_tsk_blocked_locked?
>
> Oooh, blocked_locked, didn't see that one coming.  Maybe
> set_tsk_sigmask()

but it is not _tsk, it is specially named set_current_blocked() to
show that it only applies to current. And _blocked clearly shows what
it should change, like set_current_state().

OK, this is purely cosmetic, and __set_tsk_blocked() is static and has
a single caller. Can we keep this naming for now? it would be trivial
to rename later.

Oleg.

Re: [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending()

[Tejun Heo]

On Mon, Apr 25, 2011 at 7:07 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>> set_tsk_sigmask()
>
> but it is not _tsk, it is specially named set_current_blocked() to
> show that it only applies to current. And _blocked clearly shows what
> it should change, like set_current_state().

Ooh, I meant set_current_sigmask().

> OK, this is purely cosmetic, and __set_tsk_blocked() is static and has
> a single caller. Can we keep this naming for now? it would be trivial
> to rename later.

Sure, no biggie.

-- 
tejun

Re: [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending()

[Matt Fleming]

On Mon, 25 Apr 2011 19:07:38 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> On 04/25, Tejun Heo wrote:
> >
> > On Mon, Apr 25, 2011 at 06:01:15PM +0200, Oleg Nesterov wrote:
> > >
> > > Again, I can rename... Cough, but in this case please simply suggest
> > > another name. set_tsk_blocked_locked?
> >
> > Oooh, blocked_locked, didn't see that one coming.  Maybe
> > set_tsk_sigmask()
> 
> but it is not _tsk, it is specially named set_current_blocked() to
> show that it only applies to current. And _blocked clearly shows what
> it should change, like set_current_state().
> 
> OK, this is purely cosmetic, and __set_tsk_blocked() is static and has
> a single caller. Can we keep this naming for now? it would be trivial
> to rename later.

It might be worth adding a comment to __set_tsk_blocked() saying that
it expects to be called with ->siglock held.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending()

[Matt Fleming]

On Sat, 23 Apr 2011 20:00:00 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> do_sigtimedwait() changes current->blocked and thus it needs
> set_current_bloked()->retarget_shared_pending().

If you do another version of this patch could you fix up the function
names in the commit log, s/set_current_bloked/set_current_blocked/ ? Or
maybe Andrew can fix it up if he pulls them into -mm.

> We could use set_current_bloked() directly. It is fine to change
> ->real_blocked from all-zeroes to ->blocked and vice versa lockless,
> but this is not immediately clear, looks racy, and needs a huge
> comment to explain why this is correct.
> 
> To keep the things simple this patch adds the new static helper,
> __set_task_blocked() which should be called with ->siglock held. This
> way we can change both ->real_blocked and ->blocked atomically under
> ->siglock as the current code does. This is more understandable.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH v2 0/6] sigtimedwait/sigprocmask need retarget_shared_pending()

[Oleg Nesterov]

Changes:

	1/6: remove the evidence of my ignorance from the changelog,
	     += (tv_sec || tv_nsec) is correct

	2/6: more factorization, rename the argument, add the comment
	     from Linus.

	     please re-add the acks.

	3/6: fix the typos in the changelog, thanks Matt

	4-6: new.	

I really hope that this is the last "nontrivial" changes in this area
which need any discussion, everything else looks simple.

Oleg.

[PATCH v2 1/6] signal: sys_rt_sigtimedwait: simplify the timeout logic

[Oleg Nesterov]

No functional changes, cleanup compat_sys_rt_sigtimedwait() and
sys_rt_sigtimedwait().

Calculate the timeout before we take ->siglock, this simplifies and
lessens the code. Use timespec_valid() to check the timespec.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Tejun Heo <tj@kernel.org>
Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com>
---

 kernel/signal.c |   46 ++++++++++++++++++++--------------------------
 kernel/compat.c |   38 ++++++++++++++++----------------------
 2 files changed, 36 insertions(+), 48 deletions(-)

--- sigprocmask/kernel/signal.c~1_sigtimedwait_to	2011-04-26 19:52:30.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-04-26 19:53:19.000000000 +0200
@@ -2327,7 +2327,7 @@ SYSCALL_DEFINE4(rt_sigtimedwait, const s
 	sigset_t these;
 	struct timespec ts;
 	siginfo_t info;
-	long timeout = 0;
+	long timeout;
 
 	/* XXX: Don't preclude handling different sized sigset_t's.  */
 	if (sigsetsize != sizeof(sigset_t))
@@ -2343,41 +2343,35 @@ SYSCALL_DEFINE4(rt_sigtimedwait, const s
 	sigdelsetmask(&these, sigmask(SIGKILL)|sigmask(SIGSTOP));
 	signotset(&these);
 
+	timeout = MAX_SCHEDULE_TIMEOUT;
 	if (uts) {
 		if (copy_from_user(&ts, uts, sizeof(ts)))
 			return -EFAULT;
-		if (ts.tv_nsec >= 1000000000L || ts.tv_nsec < 0
-		    || ts.tv_sec < 0)
+		if (!timespec_valid(&ts))
 			return -EINVAL;
+		timeout = timespec_to_jiffies(&ts) + (ts.tv_sec || ts.tv_nsec);
 	}
 
 	spin_lock_irq(&current->sighand->siglock);
 	sig = dequeue_signal(current, &these, &info);
-	if (!sig) {
-		timeout = MAX_SCHEDULE_TIMEOUT;
-		if (uts)
-			timeout = (timespec_to_jiffies(&ts)
-				   + (ts.tv_sec || ts.tv_nsec));
-
-		if (timeout) {
-			/*
-			 * None ready -- temporarily unblock those we're
-			 * interested while we are sleeping in so that we'll
-			 * be awakened when they arrive.
-			 */
-			current->real_blocked = current->blocked;
-			sigandsets(&current->blocked, &current->blocked, &these);
-			recalc_sigpending();
-			spin_unlock_irq(&current->sighand->siglock);
+	if (!sig && timeout) {
+		/*
+		 * None ready -- temporarily unblock those we're
+		 * interested while we are sleeping in so that we'll
+		 * be awakened when they arrive.
+		 */
+		current->real_blocked = current->blocked;
+		sigandsets(&current->blocked, &current->blocked, &these);
+		recalc_sigpending();
+		spin_unlock_irq(&current->sighand->siglock);
 
-			timeout = schedule_timeout_interruptible(timeout);
+		timeout = schedule_timeout_interruptible(timeout);
 
-			spin_lock_irq(&current->sighand->siglock);
-			sig = dequeue_signal(current, &these, &info);
-			current->blocked = current->real_blocked;
-			siginitset(&current->real_blocked, 0);
-			recalc_sigpending();
-		}
+		spin_lock_irq(&current->sighand->siglock);
+		sig = dequeue_signal(current, &these, &info);
+		current->blocked = current->real_blocked;
+		siginitset(&current->real_blocked, 0);
+		recalc_sigpending();
 	}
 	spin_unlock_irq(&current->sighand->siglock);
 
--- sigprocmask/kernel/compat.c~1_sigtimedwait_to	2011-04-26 19:52:30.000000000 +0200
+++ sigprocmask/kernel/compat.c	2011-04-26 19:53:19.000000000 +0200
@@ -893,7 +893,7 @@ compat_sys_rt_sigtimedwait (compat_sigse
 	int sig;
 	struct timespec t;
 	siginfo_t info;
-	long ret, timeout = 0;
+	long ret, timeout;
 
 	if (sigsetsize != sizeof(sigset_t))
 		return -EINVAL;
@@ -904,36 +904,30 @@ compat_sys_rt_sigtimedwait (compat_sigse
 	sigdelsetmask(&s,sigmask(SIGKILL)|sigmask(SIGSTOP));
 	signotset(&s);
 
+	timeout = MAX_SCHEDULE_TIMEOUT;
 	if (uts) {
 		if (get_compat_timespec (&t, uts))
 			return -EFAULT;
-		if (t.tv_nsec >= 1000000000L || t.tv_nsec < 0
-				|| t.tv_sec < 0)
+		if (!timespec_valid(&t))
 			return -EINVAL;
+		timeout = timespec_to_jiffies(&t) + (t.tv_sec || t.tv_nsec);
 	}
 
 	spin_lock_irq(&current->sighand->siglock);
 	sig = dequeue_signal(current, &s, &info);
-	if (!sig) {
-		timeout = MAX_SCHEDULE_TIMEOUT;
-		if (uts)
-			timeout = timespec_to_jiffies(&t)
-				+(t.tv_sec || t.tv_nsec);
-		if (timeout) {
-			current->real_blocked = current->blocked;
-			sigandsets(&current->blocked, &current->blocked, &s);
-
-			recalc_sigpending();
-			spin_unlock_irq(&current->sighand->siglock);
+	if (!sig && timeout) {
+		current->real_blocked = current->blocked;
+		sigandsets(&current->blocked, &current->blocked, &s);
+		recalc_sigpending();
+		spin_unlock_irq(&current->sighand->siglock);
 
-			timeout = schedule_timeout_interruptible(timeout);
+		timeout = schedule_timeout_interruptible(timeout);
 
-			spin_lock_irq(&current->sighand->siglock);
-			sig = dequeue_signal(current, &s, &info);
-			current->blocked = current->real_blocked;
-			siginitset(&current->real_blocked, 0);
-			recalc_sigpending();
-		}
+		spin_lock_irq(&current->sighand->siglock);
+		sig = dequeue_signal(current, &s, &info);
+		current->blocked = current->real_blocked;
+		siginitset(&current->real_blocked, 0);
+		recalc_sigpending();
 	}
 	spin_unlock_irq(&current->sighand->siglock);
 
@@ -943,7 +937,7 @@ compat_sys_rt_sigtimedwait (compat_sigse
 			if (copy_siginfo_to_user32(uinfo, &info))
 				ret = -EFAULT;
 		}
-	}else {
+	} else {
 		ret = timeout?-EINTR:-EAGAIN;
 	}
 	return ret;

[PATCH v2 2/6] signal: introduce do_sigtimedwait() to factor out compat/native code

[Oleg Nesterov]

Factor out the common code in sys_rt_sigtimedwait/compat_sys_rt_sigtimedwait
to the new helper, do_sigtimedwait().

Add the comment to document the extra tick we add to timespec_to_jiffies(ts),
thanks to Linus who explained this to me.

Perhaps it would be better to move compat_sys_rt_sigtimedwait() into
signal.c under CONFIG_COMPAT, then we can make do_sigtimedwait() static.

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

 include/linux/signal.h |    2 
 kernel/signal.c        |  104 +++++++++++++++++++++++++++----------------------
 kernel/compat.c        |   39 ++----------------
 3 files changed, 67 insertions(+), 78 deletions(-)

--- sigprocmask/include/linux/signal.h~2_do_sigtimedwait	2011-04-26 19:52:30.000000000 +0200
+++ sigprocmask/include/linux/signal.h	2011-04-26 19:53:42.000000000 +0200
@@ -242,6 +242,8 @@ extern int __group_send_sig_info(int, st
 extern long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig,
 				 siginfo_t *info);
 extern long do_sigpending(void __user *, unsigned long);
+extern int do_sigtimedwait(const sigset_t *, siginfo_t *,
+				const struct timespec *);
 extern int sigprocmask(int, sigset_t *, sigset_t *);
 extern void set_current_blocked(const sigset_t *);
 extern int show_unhandled_signals;
--- sigprocmask/kernel/signal.c~2_do_sigtimedwait	2011-04-26 19:53:19.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-04-26 19:53:42.000000000 +0200
@@ -2311,6 +2311,60 @@ int copy_siginfo_to_user(siginfo_t __use
 
 #endif
 
+int do_sigtimedwait(const sigset_t *which, siginfo_t *info,
+			const struct timespec *ts)
+{
+	struct task_struct *tsk = current;
+	long timeout = MAX_SCHEDULE_TIMEOUT;
+	sigset_t mask = *which;
+	int sig;
+
+	if (ts) {
+		if (!timespec_valid(ts))
+			return -EINVAL;
+		timeout = timespec_to_jiffies(ts);
+		/*
+		 * We can be close to the next tick, add another one
+		 * to ensure we will wait at least the time asked for.
+		 */
+		if (ts->tv_sec || ts->tv_nsec)
+			timeout++;
+	}
+
+	/*
+	 * Invert the set of allowed signals to get those we want to block.
+	 */
+	sigdelsetmask(&mask, sigmask(SIGKILL) | sigmask(SIGSTOP));
+	signotset(&mask);
+
+	spin_lock_irq(&tsk->sighand->siglock);
+	sig = dequeue_signal(tsk, &mask, info);
+	if (!sig && timeout) {
+		/*
+		 * None ready, temporarily unblock those we're interested
+		 * while we are sleeping in so that we'll be awakened when
+		 * they arrive.
+		 */
+		tsk->real_blocked = tsk->blocked;
+		sigandsets(&tsk->blocked, &tsk->blocked, &mask);
+		recalc_sigpending();
+		spin_unlock_irq(&tsk->sighand->siglock);
+
+		timeout = schedule_timeout_interruptible(timeout);
+
+		spin_lock_irq(&tsk->sighand->siglock);
+		sig = dequeue_signal(tsk, &mask, info);
+		tsk->blocked = tsk->real_blocked;
+		siginitset(&tsk->real_blocked, 0);
+		recalc_sigpending();
+	}
+	spin_unlock_irq(&tsk->sighand->siglock);
+
+	if (sig)
+		return sig;
+	return timeout ? -EINTR : -EAGAIN;
+}
+
 /**
  *  sys_rt_sigtimedwait - synchronously wait for queued signals specified
  *			in @uthese
@@ -2323,11 +2377,10 @@ SYSCALL_DEFINE4(rt_sigtimedwait, const s
 		siginfo_t __user *, uinfo, const struct timespec __user *, uts,
 		size_t, sigsetsize)
 {
-	int ret, sig;
 	sigset_t these;
 	struct timespec ts;
 	siginfo_t info;
-	long timeout;
+	int ret;
 
 	/* XXX: Don't preclude handling different sized sigset_t's.  */
 	if (sigsetsize != sizeof(sigset_t))
@@ -2336,55 +2389,16 @@ SYSCALL_DEFINE4(rt_sigtimedwait, const s
 	if (copy_from_user(&these, uthese, sizeof(these)))
 		return -EFAULT;
 
-	/*
-	 * Invert the set of allowed signals to get those we
-	 * want to block.
-	 */
-	sigdelsetmask(&these, sigmask(SIGKILL)|sigmask(SIGSTOP));
-	signotset(&these);
-
-	timeout = MAX_SCHEDULE_TIMEOUT;
 	if (uts) {
 		if (copy_from_user(&ts, uts, sizeof(ts)))
 			return -EFAULT;
-		if (!timespec_valid(&ts))
-			return -EINVAL;
-		timeout = timespec_to_jiffies(&ts) + (ts.tv_sec || ts.tv_nsec);
 	}
 
-	spin_lock_irq(&current->sighand->siglock);
-	sig = dequeue_signal(current, &these, &info);
-	if (!sig && timeout) {
-		/*
-		 * None ready -- temporarily unblock those we're
-		 * interested while we are sleeping in so that we'll
-		 * be awakened when they arrive.
-		 */
-		current->real_blocked = current->blocked;
-		sigandsets(&current->blocked, &current->blocked, &these);
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
-
-		timeout = schedule_timeout_interruptible(timeout);
-
-		spin_lock_irq(&current->sighand->siglock);
-		sig = dequeue_signal(current, &these, &info);
-		current->blocked = current->real_blocked;
-		siginitset(&current->real_blocked, 0);
-		recalc_sigpending();
-	}
-	spin_unlock_irq(&current->sighand->siglock);
+	ret = do_sigtimedwait(&these, &info, uts ? &ts : NULL);
 
-	if (sig) {
-		ret = sig;
-		if (uinfo) {
-			if (copy_siginfo_to_user(uinfo, &info))
-				ret = -EFAULT;
-		}
-	} else {
-		ret = -EAGAIN;
-		if (timeout)
-			ret = -EINTR;
+	if (ret > 0 && uinfo) {
+		if (copy_siginfo_to_user(uinfo, &info))
+			ret = -EFAULT;
 	}
 
 	return ret;
--- sigprocmask/kernel/compat.c~2_do_sigtimedwait	2011-04-26 19:53:19.000000000 +0200
+++ sigprocmask/kernel/compat.c	2011-04-26 19:53:42.000000000 +0200
@@ -890,10 +890,9 @@ compat_sys_rt_sigtimedwait (compat_sigse
 {
 	compat_sigset_t s32;
 	sigset_t s;
-	int sig;
 	struct timespec t;
 	siginfo_t info;
-	long ret, timeout;
+	long ret;
 
 	if (sigsetsize != sizeof(sigset_t))
 		return -EINVAL;
@@ -901,45 +900,19 @@ compat_sys_rt_sigtimedwait (compat_sigse
 	if (copy_from_user(&s32, uthese, sizeof(compat_sigset_t)))
 		return -EFAULT;
 	sigset_from_compat(&s, &s32);
-	sigdelsetmask(&s,sigmask(SIGKILL)|sigmask(SIGSTOP));
-	signotset(&s);
 
-	timeout = MAX_SCHEDULE_TIMEOUT;
 	if (uts) {
-		if (get_compat_timespec (&t, uts))
+		if (get_compat_timespec(&t, uts))
 			return -EFAULT;
-		if (!timespec_valid(&t))
-			return -EINVAL;
-		timeout = timespec_to_jiffies(&t) + (t.tv_sec || t.tv_nsec);
 	}
 
-	spin_lock_irq(&current->sighand->siglock);
-	sig = dequeue_signal(current, &s, &info);
-	if (!sig && timeout) {
-		current->real_blocked = current->blocked;
-		sigandsets(&current->blocked, &current->blocked, &s);
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
-
-		timeout = schedule_timeout_interruptible(timeout);
+	ret = do_sigtimedwait(&s, &info, uts ? &t : NULL);
 
-		spin_lock_irq(&current->sighand->siglock);
-		sig = dequeue_signal(current, &s, &info);
-		current->blocked = current->real_blocked;
-		siginitset(&current->real_blocked, 0);
-		recalc_sigpending();
+	if (ret > 0 && uinfo) {
+		if (copy_siginfo_to_user32(uinfo, &info))
+			ret = -EFAULT;
 	}
-	spin_unlock_irq(&current->sighand->siglock);
 
-	if (sig) {
-		ret = sig;
-		if (uinfo) {
-			if (copy_siginfo_to_user32(uinfo, &info))
-				ret = -EFAULT;
-		}
-	} else {
-		ret = timeout?-EINTR:-EAGAIN;
-	}
 	return ret;
 
 }

Re: [PATCH v2 2/6] signal: introduce do_sigtimedwait() to factor out compat/native code

[Tejun Heo]

On Tue, Apr 26, 2011 at 09:49:04PM +0200, Oleg Nesterov wrote:
> Factor out the common code in sys_rt_sigtimedwait/compat_sys_rt_sigtimedwait
> to the new helper, do_sigtimedwait().
> 
> Add the comment to document the extra tick we add to timespec_to_jiffies(ts),
> thanks to Linus who explained this to me.
> 
> Perhaps it would be better to move compat_sys_rt_sigtimedwait() into
> signal.c under CONFIG_COMPAT, then we can make do_sigtimedwait() static.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

But please do consider adding docbook function comment to the new
helper function.

Thanks.

-- 
tejun

Re: [PATCH v2 2/6] signal: introduce do_sigtimedwait() to factor out compat/native code

[Matt Fleming]

On Tue, 26 Apr 2011 21:49:04 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> Factor out the common code in sys_rt_sigtimedwait/compat_sys_rt_sigtimedwait
> to the new helper, do_sigtimedwait().
> 
> Add the comment to document the extra tick we add to timespec_to_jiffies(ts),
> thanks to Linus who explained this to me.
> 
> Perhaps it would be better to move compat_sys_rt_sigtimedwait() into
> signal.c under CONFIG_COMPAT, then we can make do_sigtimedwait() static.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v2 2/6] signal: introduce do_sigtimedwait() to factor out compat/native code

[Mike Frysinger]

On Tue, Apr 26, 2011 at 15:49, Oleg Nesterov wrote:
> --- sigprocmask/include/linux/signal.h~2_do_sigtimedwait        2011-04-26 19:52:30.000000000 +0200
> +++ sigprocmask/include/linux/signal.h  2011-04-26 19:53:42.000000000 +0200
> @@ -242,6 +242,8 @@ extern int __group_send_sig_info(int, st
>  extern long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig,
>                                 siginfo_t *info);
>  extern long do_sigpending(void __user *, unsigned long);
> +extern int do_sigtimedwait(const sigset_t *, siginfo_t *,
> +                               const struct timespec *);
>  extern int sigprocmask(int, sigset_t *, sigset_t *);
>  extern void set_current_blocked(const sigset_t *);
>  extern int show_unhandled_signals;

this causes a build warning:
In file included from arch/blackfin/kernel/signal.c:8:
include/linux/signal.h:246: warning: 'struct timespec' declared inside
parameter list
include/linux/signal.h:246: warning: its scope is only this definition
or declaration, which is probably not what you want
-mike

Re: [PATCH v2 2/6] signal: introduce do_sigtimedwait() to factor out compat/native code

[Oleg Nesterov]

On 05/11, Mike Frysinger wrote:
>
> On Tue, Apr 26, 2011 at 15:49, Oleg Nesterov wrote:
> > --- sigprocmask/include/linux/signal.h~2_do_sigtimedwait        2011-04-26 19:52:30.000000000 +0200
> > +++ sigprocmask/include/linux/signal.h  2011-04-26 19:53:42.000000000 +0200
> > @@ -242,6 +242,8 @@ extern int __group_send_sig_info(int, st
> >  extern long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig,
> >                                 siginfo_t *info);
> >  extern long do_sigpending(void __user *, unsigned long);
> > +extern int do_sigtimedwait(const sigset_t *, siginfo_t *,
> > +                               const struct timespec *);
> >  extern int sigprocmask(int, sigset_t *, sigset_t *);
> >  extern void set_current_blocked(const sigset_t *);
> >  extern int show_unhandled_signals;
>
> this causes a build warning:
> In file included from arch/blackfin/kernel/signal.c:8:
> include/linux/signal.h:246: warning: 'struct timespec' declared inside
> parameter list

Oh, thanks Mike. I'll send the simple fix tomorrow.

Oleg.

[PATCH] signal: trivial, fix the "timespec declared inside parameter list" warning

[Oleg Nesterov]

Fix the compile warning, do_sigtimedwait(struct timespec *) in signal.h
needs the forward declaration of timespec.

Reported-by: Mike Frysinger <vapier.adi@gmail.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 include/linux/signal.h |    1 +
 1 file changed, 1 insertion(+)

--- sigprocmask/include/linux/signal.h~15_stw_warning	2011-05-12 20:44:43.000000000 +0200
+++ sigprocmask/include/linux/signal.h	2011-05-13 18:10:40.000000000 +0200
@@ -242,6 +242,7 @@ extern int __group_send_sig_info(int, st
 extern long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig,
 				 siginfo_t *info);
 extern long do_sigpending(void __user *, unsigned long);
+struct timespec;
 extern int do_sigtimedwait(const sigset_t *, siginfo_t *,
 				const struct timespec *);
 extern int sigprocmask(int, sigset_t *, sigset_t *);

Re: [PATCH] signal: trivial, fix the "timespec declared inside parameter list" warning

[Mike Frysinger]

On Fri, May 13, 2011 at 12:44, Oleg Nesterov wrote:
> --- sigprocmask/include/linux/signal.h~15_stw_warning   2011-05-12 20:44:43.000000000 +0200
> +++ sigprocmask/include/linux/signal.h  2011-05-13 18:10:40.000000000 +0200
> @@ -242,6 +242,7 @@ extern int __group_send_sig_info(int, st
>  extern long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig,
>                                 siginfo_t *info);
>  extern long do_sigpending(void __user *, unsigned long);
> +struct timespec;
>  extern int do_sigtimedwait(const sigset_t *, siginfo_t *,
>                                const struct timespec *);
>  extern int sigprocmask(int, sigset_t *, sigset_t *);

seems like you'd want to stick this higher up in the file (like after
the includes or at the top of the prototype block) so that in the
future, you dont have to move it if someone adds a new func before it.
-mike

Re: [PATCH] signal: trivial, fix the "timespec declared inside parameter list" warning

[Oleg Nesterov]

On 05/13, Mike Frysinger wrote:
>
> On Fri, May 13, 2011 at 12:44, Oleg Nesterov wrote:
> > --- sigprocmask/include/linux/signal.h~15_stw_warning   2011-05-12 20:44:43.000000000 +0200
> > +++ sigprocmask/include/linux/signal.h  2011-05-13 18:10:40.000000000 +0200
> > @@ -242,6 +242,7 @@ extern int __group_send_sig_info(int, st
> >  extern long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig,
> >                                 siginfo_t *info);
> >  extern long do_sigpending(void __user *, unsigned long);
> > +struct timespec;
> >  extern int do_sigtimedwait(const sigset_t *, siginfo_t *,
> >                                const struct timespec *);
> >  extern int sigprocmask(int, sigset_t *, sigset_t *);
>
> seems like you'd want to stick this higher up in the file (like after
> the includes or at the top of the prototype block)

OK, lets move it at the top of the prototype block. I'd like to keep
it close to the user. If we do this, then we should probably move
pt_regs as well.

Oleg.

[PATCH v2] signal: trivial, fix the "timespec declared inside parameter list" warning

[Oleg Nesterov]

Fix the compile warning, do_sigtimedwait(struct timespec *) in signal.h
needs the forward declaration of timespec.

Reported-by: Mike Frysinger <vapier.adi@gmail.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 include/linux/signal.h |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--- sigprocmask/include/linux/signal.h~15_stw_warning	2011-05-12 20:44:43.000000000 +0200
+++ sigprocmask/include/linux/signal.h	2011-05-16 14:53:08.000000000 +0200
@@ -234,6 +234,9 @@ static inline int valid_signal(unsigned 
 	return sig <= _NSIG ? 1 : 0;
 }
 
+struct timespec;
+struct pt_regs;
+
 extern int next_signal(struct sigpending *pending, sigset_t *mask);
 extern int do_send_sig_info(int sig, struct siginfo *info,
 				struct task_struct *p, bool group);
@@ -248,7 +251,6 @@ extern int sigprocmask(int, sigset_t *, 
 extern void set_current_blocked(const sigset_t *);
 extern int show_unhandled_signals;
 
-struct pt_regs;
 extern int get_signal_to_deliver(siginfo_t *info, struct k_sigaction *return_ka, struct pt_regs *regs, void *cookie);
 extern void exit_signals(struct task_struct *tsk);
 

Re: [PATCH v2] signal: trivial, fix the "timespec declared inside parameter list" warning

[Mike Frysinger]

On Mon, May 16, 2011 at 08:57, Oleg Nesterov wrote:
> Fix the compile warning, do_sigtimedwait(struct timespec *) in signal.h
> needs the forward declaration of timespec.

looks good to me, thanks !
Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike

Re: [PATCH v2] signal: trivial, fix the "timespec declared inside parameter list" warning

[Andrew Morton]

On Mon, 16 May 2011 14:57:29 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> Fix the compile warning, do_sigtimedwait(struct timespec *) in signal.h
> needs the forward declaration of timespec.
> 

The offending patch is in your tree, so you may as well put this patch
in there too.

> --- sigprocmask/include/linux/signal.h~15_stw_warning	2011-05-12 20:44:43.000000000 +0200
> +++ sigprocmask/include/linux/signal.h	2011-05-16 14:53:08.000000000 +0200
> @@ -234,6 +234,9 @@ static inline int valid_signal(unsigned 
>  	return sig <= _NSIG ? 1 : 0;
>  }
>  
> +struct timespec;
> +struct pt_regs;
> +
>  extern int next_signal(struct sigpending *pending, sigset_t *mask);
>  extern int do_send_sig_info(int sig, struct siginfo *info,
>  				struct task_struct *p, bool group);

Please put the forward declarations at top-of-file.  In this case,
inside #ifdef __KERNEL__.  This reduces the risk of accumulating
duplicated forward declarations, as has happened in the past.

Re: [PATCH v2] signal: trivial, fix the "timespec declared inside parameter list" warning

[Oleg Nesterov]

On 05/18, Andrew Morton wrote:
>
> On Mon, 16 May 2011 14:57:29 +0200
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > Fix the compile warning, do_sigtimedwait(struct timespec *) in signal.h
> > needs the forward declaration of timespec.
> >
>
> The offending patch is in your tree, so you may as well put this patch
> in there too.

Yes, it is already in my tree, sorry for confusion.

> > --- sigprocmask/include/linux/signal.h~15_stw_warning	2011-05-12 20:44:43.000000000 +0200
> > +++ sigprocmask/include/linux/signal.h	2011-05-16 14:53:08.000000000 +0200
> > @@ -234,6 +234,9 @@ static inline int valid_signal(unsigned 
> >  	return sig <= _NSIG ? 1 : 0;
> >  }
> >
> > +struct timespec;
> > +struct pt_regs;
> > +
> >  extern int next_signal(struct sigpending *pending, sigset_t *mask);
> >  extern int do_send_sig_info(int sig, struct siginfo *info,
> >  				struct task_struct *p, bool group);
>
> Please put the forward declarations at top-of-file.  In this case,
> inside #ifdef __KERNEL__.  This reduces the risk of accumulating
> duplicated forward declarations, as has happened in the past.

This is what Mike suggested from the very beginnig. Perhaps this
would be better but since I already applied this patch... I'd prefer
to not test my git skills, unless you or Mike object.

Oleg.

Re: [PATCH v2] signal: trivial, fix the "timespec declared inside parameter list" warning

[Mike Frysinger]

On Thu, May 19, 2011 at 14:19, Oleg Nesterov wrote:
> On 05/18, Andrew Morton wrote:
>> On Mon, 16 May 2011 14:57:29 +0200 Oleg Nesterov wrote:
>> > --- sigprocmask/include/linux/signal.h~15_stw_warning       2011-05-12 20:44:43.000000000 +0200
>> > +++ sigprocmask/include/linux/signal.h      2011-05-16 14:53:08.000000000 +0200
>> > @@ -234,6 +234,9 @@ static inline int valid_signal(unsigned
>> >     return sig <= _NSIG ? 1 : 0;
>> >  }
>> >
>> > +struct timespec;
>> > +struct pt_regs;
>> > +
>> >  extern int next_signal(struct sigpending *pending, sigset_t *mask);
>> >  extern int do_send_sig_info(int sig, struct siginfo *info,
>> >                             struct task_struct *p, bool group);
>>
>> Please put the forward declarations at top-of-file.  In this case,
>> inside #ifdef __KERNEL__.  This reduces the risk of accumulating
>> duplicated forward declarations, as has happened in the past.
>
> This is what Mike suggested from the very beginnig. Perhaps this
> would be better but since I already applied this patch... I'd prefer
> to not test my git skills, unless you or Mike object.

i'm ok with this version as it's at the top of the existing prototype
block.  Andrew's version also would work of course.
-mike

[PATCH v2 3/6] signal: do_sigtimedwait() needs retarget_shared_pending()

[Oleg Nesterov]

do_sigtimedwait() changes current->blocked and thus it needs
set_current_blocked()->retarget_shared_pending().

We could use set_current_blocked() directly. It is fine to change
->real_blocked from all-zeroes to ->blocked and vice versa lockless,
but this is not immediately clear, looks racy, and needs a huge
comment to explain why this is correct.

To keep the things simple this patch adds the new static helper,
__set_task_blocked() which should be called with ->siglock held. This
way we can change both ->real_blocked and ->blocked atomically under
->siglock as the current code does. This is more understandable.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Tejun Heo <tj@kernel.org>
Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com>
---

 kernel/signal.c |   21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

--- sigprocmask/kernel/signal.c~3_sigtimedwait_retarget	2011-04-26 19:53:42.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-04-26 19:53:55.000000000 +0200
@@ -2115,11 +2115,8 @@ long do_no_restart_syscall(struct restar
 	return -EINTR;
 }
 
-void set_current_blocked(const sigset_t *newset)
+static void __set_task_blocked(struct task_struct *tsk, const sigset_t *newset)
 {
-	struct task_struct *tsk = current;
-
-	spin_lock_irq(&tsk->sighand->siglock);
 	if (signal_pending(tsk) && !thread_group_empty(tsk)) {
 		sigset_t newblocked;
 		signandsets(&newblocked, newset, &current->blocked);
@@ -2127,6 +2124,14 @@ void set_current_blocked(const sigset_t 
 	}
 	tsk->blocked = *newset;
 	recalc_sigpending();
+}
+
+void set_current_blocked(const sigset_t *newset)
+{
+	struct task_struct *tsk = current;
+
+	spin_lock_irq(&tsk->sighand->siglock);
+	__set_task_blocked(tsk, newset);
 	spin_unlock_irq(&tsk->sighand->siglock);
 }
 
@@ -2343,7 +2348,8 @@ int do_sigtimedwait(const sigset_t *whic
 		/*
 		 * None ready, temporarily unblock those we're interested
 		 * while we are sleeping in so that we'll be awakened when
-		 * they arrive.
+		 * they arrive. Unblocking is always fine, we can avoid
+		 * set_current_blocked().
 		 */
 		tsk->real_blocked = tsk->blocked;
 		sigandsets(&tsk->blocked, &tsk->blocked, &mask);
@@ -2353,10 +2359,9 @@ int do_sigtimedwait(const sigset_t *whic
 		timeout = schedule_timeout_interruptible(timeout);
 
 		spin_lock_irq(&tsk->sighand->siglock);
-		sig = dequeue_signal(tsk, &mask, info);
-		tsk->blocked = tsk->real_blocked;
+		__set_task_blocked(tsk, &tsk->real_blocked);
 		siginitset(&tsk->real_blocked, 0);
-		recalc_sigpending();
+		sig = dequeue_signal(tsk, &mask, info);
 	}
 	spin_unlock_irq(&tsk->sighand->siglock);
 

[PATCH v2 4/6] signal: cleanup sys_sigprocmask()

[Oleg Nesterov]

Cleanup. Remove the unneeded goto's, we can simply read blocked.sig[0]
unconditionally and then copy-to-user it if oset != NULL.

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

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

--- sigprocmask/kernel/signal.c~4_cleanup_old_sigprocmask	2011-04-26 19:53:55.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-04-26 19:54:09.000000000 +0200
@@ -2691,29 +2691,28 @@ SYSCALL_DEFINE1(sigpending, old_sigset_t
 /**
  *  sys_sigprocmask - examine and change blocked signals
  *  @how: whether to add, remove, or set signals
- *  @set: signals to add or remove (if non-null)
+ *  @nset: signals to add or remove (if non-null)
  *  @oset: previous value of signal mask if non-null
  *
  * Some platforms have their own version with special arguments;
  * others support only sys_rt_sigprocmask.
  */
 
-SYSCALL_DEFINE3(sigprocmask, int, how, old_sigset_t __user *, set,
+SYSCALL_DEFINE3(sigprocmask, int, how, old_sigset_t __user *, nset,
 		old_sigset_t __user *, oset)
 {
-	int error;
 	old_sigset_t old_set, new_set;
+	int error;
 
-	if (set) {
-		error = -EFAULT;
-		if (copy_from_user(&new_set, set, sizeof(*set)))
-			goto out;
-		new_set &= ~(sigmask(SIGKILL) | sigmask(SIGSTOP));
+	old_set = current->blocked.sig[0];
 
-		spin_lock_irq(&current->sighand->siglock);
-		old_set = current->blocked.sig[0];
+	if (nset) {
+		if (copy_from_user(&new_set, nset, sizeof(*nset)))
+			return -EFAULT;
+		new_set &= ~(sigmask(SIGKILL) | sigmask(SIGSTOP));
 
 		error = 0;
+		spin_lock_irq(&current->sighand->siglock);
 		switch (how) {
 		default:
 			error = -EINVAL;
@@ -2732,19 +2731,15 @@ SYSCALL_DEFINE3(sigprocmask, int, how, o
 		recalc_sigpending();
 		spin_unlock_irq(&current->sighand->siglock);
 		if (error)
-			goto out;
-		if (oset)
-			goto set_old;
-	} else if (oset) {
-		old_set = current->blocked.sig[0];
-	set_old:
-		error = -EFAULT;
+			return error;
+	}
+
+	if (oset) {
 		if (copy_to_user(oset, &old_set, sizeof(*oset)))
-			goto out;
+			return -EFAULT;
 	}
-	error = 0;
-out:
-	return error;
+
+	return 0;
 }
 #endif /* __ARCH_WANT_SYS_SIGPROCMASK */
 

Re: [PATCH v2 4/6] signal: cleanup sys_sigprocmask()

[Tejun Heo]

On Tue, Apr 26, 2011 at 09:49:43PM +0200, Oleg Nesterov wrote:
> Cleanup. Remove the unneeded goto's, we can simply read blocked.sig[0]
> unconditionally and then copy-to-user it if oset != NULL.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH v2 4/6] signal: cleanup sys_sigprocmask()

[Matt Fleming]

On Tue, 26 Apr 2011 21:49:43 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> Cleanup. Remove the unneeded goto's, we can simply read blocked.sig[0]
> unconditionally and then copy-to-user it if oset != NULL.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending()

[Oleg Nesterov]

sys_sigprocmask() changes current->blocked by hand. Convert this code to
use sigprocmask() which implies the necessary retarget_shared_pending().

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

 kernel/signal.c |   24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

--- sigprocmask/kernel/signal.c~5_old_sigprocmask_retarget	2011-04-26 19:54:09.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-04-26 19:54:20.000000000 +0200
@@ -2702,6 +2702,7 @@ SYSCALL_DEFINE3(sigprocmask, int, how, o
 		old_sigset_t __user *, oset)
 {
 	old_sigset_t old_set, new_set;
+	sigset_t new_full_set;
 	int error;
 
 	old_set = current->blocked.sig[0];
@@ -2711,25 +2712,12 @@ SYSCALL_DEFINE3(sigprocmask, int, how, o
 			return -EFAULT;
 		new_set &= ~(sigmask(SIGKILL) | sigmask(SIGSTOP));
 
-		error = 0;
-		spin_lock_irq(&current->sighand->siglock);
-		switch (how) {
-		default:
-			error = -EINVAL;
-			break;
-		case SIG_BLOCK:
-			sigaddsetmask(&current->blocked, new_set);
-			break;
-		case SIG_UNBLOCK:
-			sigdelsetmask(&current->blocked, new_set);
-			break;
-		case SIG_SETMASK:
-			current->blocked.sig[0] = new_set;
-			break;
-		}
+		sigemptyset(&new_full_set);
+		if (how == SIG_SETMASK)
+			new_full_set = current->blocked;
+		new_full_set.sig[0] = new_set;
 
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
+		error = sigprocmask(how, &new_full_set, NULL);
 		if (error)
 			return error;
 	}

Re: [PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending()

[Linus Torvalds]

> +               sigemptyset(&new_full_set);
> +               if (how == SIG_SETMASK)
> +                       new_full_set = current->blocked;
> +               new_full_set.sig[0] = new_set;

Ugh. This is just ugly.

Could we not instead turn the whole thing into a "clear these bits"
and "set these bits", and get rid of the "how" entirely for the helper
function?

IOW, we'd have

  switch (how) {
  case SIG_BLOCK:
      clear_bits = 0;
      set_bits = new_set;
      break;
  case SIG_UNBLOCK:
      clear_bits = new_set;
      set_bits = 0;
      break
  case SIG_SET:
     clear_bits = low_bits;
     set_bits = new_set;
     break;
   default:
     return -EINVAL;
  }

and notice how you now can do that helper function *WITHOUT* any
conditionals, and just make it do

    sigprocmask(&clear, &set, NULL);

which handles all cases correctly (just "andn clear" + "or set") with
no if's or switch'es.

This is why I _detest_ that idiotic "sigprocmask()" interface. That
"how" parameter is the invention of somebody who didn't understand
sets. It's stupid. There is no reason to have multiple different set
operations, when in practice all anybody ever wants is the "clear
these bits and set those other bits" - an operation that is not only
more generic than the idiotic "how", but is _faster_ too, because it
involves no conditionals.

So I realize that we cannot get away from the broken user interface,
but I do not believe that that means that our _internal_ helper
functions should use that idiotic and broken interface!

I had basically this same comment earlier when you did something
similarly mindless for another case.

So basic rule should be: if you ever pass "how" to any helper
functions, it's broken.

                                     Linus

Re: [PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending()

[Oleg Nesterov]

On 04/26, Linus Torvalds wrote:
>
> > +               sigemptyset(&new_full_set);
> > +               if (how == SIG_SETMASK)
> > +                       new_full_set = current->blocked;
> > +               new_full_set.sig[0] = new_set;
>
> Ugh. This is just ugly.

Agreed.

> Could we not instead turn the whole thing into a "clear these bits"
> and "set these bits", and get rid of the "how" entirely for the helper
> function?
>
> IOW, we'd have
>
>   switch (how) {
>   case SIG_BLOCK:
>       clear_bits = 0;
>       set_bits = new_set;
>       break;
>   case SIG_UNBLOCK:
>       clear_bits = new_set;
>       set_bits = 0;
>       break
>   case SIG_SET:
>      clear_bits = low_bits;
>      set_bits = new_set;
>      break;
>    default:
>      return -EINVAL;
>   }
>
> and notice how you now can do that helper function *WITHOUT* any
> conditionals, and just make it do
>
>     sigprocmask(&clear, &set, NULL);
>
> which handles all cases correctly (just "andn clear" + "or set") with
> no if's or switch'es.
>
> This is why I _detest_ that idiotic "sigprocmask()" interface.

Agreed, but...

Yes, sigprocmask(how) is ugly, but there are sys_rt_sigprocmask() and
sys_sigprocmask() which have to handle these SIG_* operations anyway.
So, I think we should do:

	1. Almost all callers of sigprocmask() use SIG_SETMASK, we can
	   simply change them to use set_current_blocked().

	2. Add the new helper (probably like you suggested) and convert
	   other 9 callers.

	3. Unexport sigprocmask() and remove the last "*oldset" argument,
	   it should be only used by sys_*sigprocmask() syscalls.

But firstly I'd like to finish this "don't change ->blocked directly"
conversion. And this patch changes sys_sigprocmask() so that it looks
similar to sys_rt_sigprocmask().

What do you think?

If you can't accept sys_sigprocmask()->sigprocmask(), will you agree
with

	SYSCALL_DEFINE3(sigprocmask, int, how, old_sigset_t __user *, nset,
			old_sigset_t __user *, oset)
	{
		old_sigset_t old_set, new_set;
		sigset_t new_blocked;

		old_set = current->blocked.sig[0];

		if (nset) {
			if (copy_from_user(&new_set, nset, sizeof(*nset)))
				return -EFAULT;
			new_set &= ~(sigmask(SIGKILL) | sigmask(SIGSTOP));

			new_blocked = current->blocked;

			switch (how) {
			case SIG_BLOCK:
				sigaddsetmask(&new_blocked, new_set);
				break;
			case SIG_UNBLOCK:
				sigdelsetmask(&new_blocked, new_set);
				break;
			case SIG_SETMASK:
				new_blocked.sig[0] = new_set;
				break;
			default:
				return -EINVAL;
			}

			set_current_blocked(&new_blocked);
		}

		if (oset) {
			if (copy_to_user(oset, &old_set, sizeof(*oset)))
				return -EFAULT;
		}

		return 0;
	}
?

Oleg.

Re: [PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending()

[Tejun Heo]

Hello,

Just my 5 cents.

On Wed, Apr 27, 2011 at 02:57:10PM +0200, Oleg Nesterov wrote:
> Yes, sigprocmask(how) is ugly, but there are sys_rt_sigprocmask() and
> sys_sigprocmask() which have to handle these SIG_* operations anyway.
> So, I think we should do:
> 
> 	1. Almost all callers of sigprocmask() use SIG_SETMASK, we can
> 	   simply change them to use set_current_blocked().

I agree.  We don't need to worry about atomicity here, so there's no
reason to encode bitops (be it and/or or andn/xor) when the
determination of the new value can be simply done in the caller.

Thanks.

-- 
tejun

[PATCH v2 0/1] signal: sys_sigprocmask() needs retarget_shared_pending()

[Oleg Nesterov]

On 04/26, Linus Torvalds wrote:
>
> > +               sigemptyset(&new_full_set);
> > +               if (how == SIG_SETMASK)
> > +                       new_full_set = current->blocked;
> > +               new_full_set.sig[0] = new_set;
>
> Ugh. This is just ugly.

OK, please find v2. It doesn't use sigprocmask() helper().

Hopefully this is the last "nontrivial" conversion, everything else
looks simple.

Oleg.

[PATCH v2 1/1] signal: sys_sigprocmask() needs retarget_shared_pending()

[Oleg Nesterov]

sys_sigprocmask() changes current->blocked by hand. Convert this code
to use set_current_blocked().

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

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

--- sigprocmask/kernel/signal.c~14_old_sigprocmask_retarget	2011-04-30 19:07:11.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-05-01 21:34:51.000000000 +0200
@@ -2900,7 +2900,7 @@ SYSCALL_DEFINE3(sigprocmask, int, how, o
 		old_sigset_t __user *, oset)
 {
 	old_sigset_t old_set, new_set;
-	int error;
+	sigset_t new_blocked;
 
 	old_set = current->blocked.sig[0];
 
@@ -2909,27 +2909,23 @@ SYSCALL_DEFINE3(sigprocmask, int, how, o
 			return -EFAULT;
 		new_set &= ~(sigmask(SIGKILL) | sigmask(SIGSTOP));
 
-		error = 0;
-		spin_lock_irq(&current->sighand->siglock);
+		new_blocked = current->blocked;
+
 		switch (how) {
-		default:
-			error = -EINVAL;
-			break;
 		case SIG_BLOCK:
-			sigaddsetmask(&current->blocked, new_set);
+			sigaddsetmask(&new_blocked, new_set);
 			break;
 		case SIG_UNBLOCK:
-			sigdelsetmask(&current->blocked, new_set);
+			sigdelsetmask(&new_blocked, new_set);
 			break;
 		case SIG_SETMASK:
-			current->blocked.sig[0] = new_set;
+			new_blocked.sig[0] = new_set;
 			break;
+		default:
+			return -EINVAL;
 		}
 
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
-		if (error)
-			return error;
+		set_current_blocked(&new_blocked);
 	}
 
 	if (oset) {

[PATCH v2 6/6] signal: rename signandsets() to sigandnsets()

[Oleg Nesterov]

As Tejun and Linus pointed out, "nand" is the wrong name for "x & ~y",
it should be "andn". Rename signandsets() as suggested.

Suggested-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 include/linux/signal.h |    6 +++---
 kernel/signal.c        |    6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

--- sigprocmask/include/linux/signal.h~6_rename_nand	2011-04-26 19:53:42.000000000 +0200
+++ sigprocmask/include/linux/signal.h	2011-04-26 19:58:01.000000000 +0200
@@ -123,13 +123,13 @@ _SIG_SET_BINOP(sigorsets, _sig_or)
 #define _sig_and(x,y)	((x) & (y))
 _SIG_SET_BINOP(sigandsets, _sig_and)
 
-#define _sig_nand(x,y)	((x) & ~(y))
-_SIG_SET_BINOP(signandsets, _sig_nand)
+#define _sig_andn(x,y)	((x) & ~(y))
+_SIG_SET_BINOP(sigandnsets, _sig_andn)
 
 #undef _SIG_SET_BINOP
 #undef _sig_or
 #undef _sig_and
-#undef _sig_nand
+#undef _sig_andn
 
 #define _SIG_SET_OP(name, op)						\
 static inline void name(sigset_t *set)					\
--- sigprocmask/kernel/signal.c~6_rename_nand	2011-04-26 19:54:20.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-04-26 19:59:43.000000000 +0200
@@ -592,7 +592,7 @@ static int rm_from_queue_full(sigset_t *
 	if (sigisemptyset(&m))
 		return 0;
 
-	signandsets(&s->signal, &s->signal, mask);
+	sigandnsets(&s->signal, &s->signal, mask);
 	list_for_each_entry_safe(q, n, &s->list, list) {
 		if (sigismember(mask, q->info.si_signo)) {
 			list_del_init(&q->list);
@@ -2119,7 +2119,7 @@ static void __set_task_blocked(struct ta
 {
 	if (signal_pending(tsk) && !thread_group_empty(tsk)) {
 		sigset_t newblocked;
-		signandsets(&newblocked, newset, &current->blocked);
+		sigandnsets(&newblocked, newset, &current->blocked);
 		retarget_shared_pending(tsk, &newblocked);
 	}
 	tsk->blocked = *newset;
@@ -2157,7 +2157,7 @@ int sigprocmask(int how, sigset_t *set, 
 		sigorsets(&newset, &tsk->blocked, set);
 		break;
 	case SIG_UNBLOCK:
-		signandsets(&newset, &tsk->blocked, set);
+		sigandnsets(&newset, &tsk->blocked, set);
 		break;
 	case SIG_SETMASK:
 		newset = *set;

Re: [PATCH v2 6/6] signal: rename signandsets() to sigandnsets()

[Tejun Heo]

On Tue, Apr 26, 2011 at 09:50:18PM +0200, Oleg Nesterov wrote:
> As Tejun and Linus pointed out, "nand" is the wrong name for "x & ~y",
> it should be "andn". Rename signandsets() as suggested.
> 
> Suggested-by: Tejun Heo <tj@kernel.org>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH v2 6/6] signal: rename signandsets() to sigandnsets()

[Matt Fleming]

On Tue, 26 Apr 2011 21:50:18 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> As Tejun and Linus pointed out, "nand" is the wrong name for "x & ~y",
> it should be "andn". Rename signandsets() as suggested.
> 
> Suggested-by: Tejun Heo <tj@kernel.org>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com> 

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

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending()

[George Spelvin]

> and notice how you now can do that helper function *WITHOUT* any
> conditionals, and just make it do
> 
>     sigprocmask(&clear, &set, NULL);
> 
> which handles all cases correctly (just "andn clear" + "or set") with
> no if's or switch'es.

Gripe: Arrgh, apostrophe disease!  Linus, youre picking up nasty habits.

Suggestion: Actually, the usual, more flexible implementation, is
"andn mask" + "xor set".  That gives all 4 bit operations (nop, set,
clear, and toggle) unique bit combinations.

Not that it's of any use in many cases, but it has better hack value.

That would convert to:
  switch (how) {
  case SIG_BLOCK:
      mask_bits = new_set;
      set_bits = new_set;
      break;
  case SIG_UNBLOCK:
      mask_bits = new_set;
      set_bits = 0;
      break
  case SIG_SET:
     mask_bits = low_bits | new_set;
     set_bits = new_set;
     break;
   default:
     return -EINVAL;
  }

If you prefer separate set & clear fields, with both set meaning "toggle",
which admittedly is a more elegant representation, then it's
"andn (set|clear)" + "xor set".

Re: [PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending()

[Linus Torvalds]

On Tue, Apr 26, 2011 at 6:35 PM, George Spelvin <linux@horizon.com> wrote:
>>
>> which handles all cases correctly (just "andn clear" + "or set") with
>> no if's or switch'es.
>
> Gripe: Arrgh, apostrophe disease!  Linus, youre picking up nasty habits.

I didn't want to make C keywords into English words. So if it's about
English words it's "no ifs or buts", but when talking about C keywords
it's "if's or switch'es".

That's my stand, and I'll stick to it. At least until next time, when
I might decide on yet another arbitrary rule ;)

> Suggestion: Actually, the usual, more flexible implementation, is
> "andn mask" + "xor set".  That gives all 4 bit operations (nop, set,
> clear, and toggle) unique bit combinations.

Yeah, we can do that too. I agree it is more flexible, although I
don't think we'd ever need it.

And I'm not sure it's more "usual" - most drivers I've seen (where
this kind of "clear/set mask" issue comes up pretty often) tend to use
the simpler clear/set model, because it tends to make the usage more
sensible. Most users simply don't tend to need the toggle operation.

But my grep-fu to actually show examples is weak. One example I found would be

 - drivers/media/video/tw9910.c: tw9910_mask_set()

which uses that "andnot + or" model I suggested.

But I'm sure there are "andnot + xor" uses out there too.

And I don't actually mind either way. I'd certainly be perfectly ok
with the andn/xor model. It's just the "how" model I detest.

                            Linus