All of lore.kernel.org
 help / color / mirror / Atom feed
* + signal-sigprocmask-should-do-retarget_shared_pending.patch added to -mm tree
@ 2011-04-11 20:38 akpm
  0 siblings, 0 replies; 2+ messages in thread
From: akpm @ 2011-04-11 20:38 UTC (permalink / raw)
  To: mm-commits; +Cc: oleg, hpa, matt, mingo, nyoushchenko, tglx, tj


The patch titled
     signal: sigprocmask() should do retarget_shared_pending()
has been added to the -mm tree.  Its filename is
     signal-sigprocmask-should-do-retarget_shared_pending.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
out what to do about this

The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/

------------------------------------------------------
Subject: signal: sigprocmask() should do retarget_shared_pending()
From: Oleg Nesterov <oleg@redhat.com>

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

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

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

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

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

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: "Nikita V. Youshchenko" <nyoushchenko@mvista.com>
Cc: Matt Fleming <matt@console-pimps.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

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

diff -puN include/linux/signal.h~signal-sigprocmask-should-do-retarget_shared_pending include/linux/signal.h
--- a/include/linux/signal.h~signal-sigprocmask-should-do-retarget_shared_pending
+++ a/include/linux/signal.h
@@ -126,10 +126,14 @@ _SIG_SET_BINOP(sigandsets, _sig_and)
 #define _sig_nand(x,y)	((x) & ~(y))
 _SIG_SET_BINOP(signandsets, _sig_nand)
 
+#define _sig_nor(x,y)	((x) | ~(y))
+_SIG_SET_BINOP(signorsets, _sig_nor)
+
 #undef _SIG_SET_BINOP
 #undef _sig_or
 #undef _sig_and
 #undef _sig_nand
+#undef _sig_nor
 
 #define _SIG_SET_OP(name, op)						\
 static inline void name(sigset_t *set)					\
diff -puN kernel/signal.c~signal-sigprocmask-should-do-retarget_shared_pending kernel/signal.c
--- a/kernel/signal.c~signal-sigprocmask-should-do-retarget_shared_pending
+++ a/kernel/signal.c
@@ -2314,6 +2314,11 @@ int sigprocmask(int how, sigset_t *set, 
 	}
 
 	spin_lock_irq(&tsk->sighand->siglock);
+	if (signal_pending(tsk) && !thread_group_empty(tsk)) {
+		sigset_t not_newblocked;
+		signorsets(&not_newblocked, &current->blocked, &newset);
+		retarget_shared_pending(tsk, &not_newblocked);
+	}
 	tsk->blocked = newset;
 	recalc_sigpending();
 	spin_unlock_irq(&tsk->sighand->siglock);
_

Patches currently in -mm which might be from oleg@redhat.com are

linux-next.patch
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
fs-execc-provide-the-correct-process-pid-to-the-pipe-helper.patch


^ permalink raw reply	[flat|nested] 2+ messages in thread

* + signal-sigprocmask-should-do-retarget_shared_pending.patch added to -mm tree
@ 2011-04-20 21:19 akpm
  0 siblings, 0 replies; 2+ messages in thread
From: akpm @ 2011-04-20 21:19 UTC (permalink / raw)
  To: mm-commits; +Cc: oleg, hpa, matt, mingo, nyoushchenko, tglx, tj


The patch titled
     signal: sigprocmask() should do retarget_shared_pending()
has been added to the -mm tree.  Its filename is
     signal-sigprocmask-should-do-retarget_shared_pending.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
out what to do about this

The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/

------------------------------------------------------
Subject: signal: sigprocmask() should do retarget_shared_pending()
From: Oleg Nesterov <oleg@redhat.com>

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>
Cc: Tejun Heo <tj@kernel.org>
Cc: "Nikita V. Youshchenko" <nyoushchenko@mvista.com>
Cc: Matt Fleming <matt@console-pimps.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/signal.h |    1 +
 kernel/signal.c        |   21 ++++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff -puN include/linux/signal.h~signal-sigprocmask-should-do-retarget_shared_pending include/linux/signal.h
--- a/include/linux/signal.h~signal-sigprocmask-should-do-retarget_shared_pending
+++ a/include/linux/signal.h
@@ -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;
diff -puN kernel/signal.c~signal-sigprocmask-should-do-retarget_shared_pending kernel/signal.c
--- a/kernel/signal.c~signal-sigprocmask-should-do-retarget_shared_pending
+++ a/kernel/signal.c
@@ -2298,6 +2298,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.
@@ -2329,11 +2344,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;
 }
 
_

Patches currently in -mm which might be from oleg@redhat.com are

linux-next.patch
cgroups-read-write-lock-clone_thread-forking-per-threadgroup.patch
cgroups-add-per-thread-subsystem-callbacks.patch
cgroups-make-procs-file-writable.patch
cgroups-use-flex_array-in-attach_proc.patch
asm-generic-ptraceh-start-a-common-low-level-ptrace-helper.patch
blackfin-convert-to-asm-generic-ptraceh.patch
x86-convert-to-asm-generic-ptraceh.patch
sh-convert-to-asm-generic-ptraceh.patch
kgdbts-unify-generalize-gdb-breakpoint-adjustment.patch
signal-introduce-retarget_shared_pending.patch
signal-retarget_shared_pending-consider-shared-unblocked-signals-only.patch
signal-retarget_shared_pending-optimize-while_each_thread-loop.patch
signal-sigprocmask-narrow-the-scope-of-siglock.patch
signal-sigprocmask-should-do-retarget_shared_pending.patch
x86-signal-handle_signal-should-use-set_current_blocked.patch
x86-signal-sys_rt_sigreturn-should-use-set_current_blocked.patch
signal-cleanup-sys_rt_sigprocmask.patch
fs-execc-provide-the-correct-process-pid-to-the-pipe-helper.patch


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2011-04-20 21:19 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-04-11 20:38 + signal-sigprocmask-should-do-retarget_shared_pending.patch added to -mm tree akpm
2011-04-20 21:19 akpm

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