* [PATCH 01/41] alpha: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:56 ` [PATCH 02/41] arm: " Matt Fleming
` (41 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov
Cc: linux-kernel, Richard Henderson, Ivan Kokshaysky, Matt Turner, Al Viro
From: Matt Fleming <matt.fleming@linux.intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check for shared signals we're about to block.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Matt Turner <mattst88@gmail.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Matt Fleming <matt.fleming@linux.intel.com>
---
arch/alpha/kernel/signal.c | 31 +++++++++++++------------------
1 files changed, 13 insertions(+), 18 deletions(-)
diff --git a/arch/alpha/kernel/signal.c b/arch/alpha/kernel/signal.c
index 6f7feb5..ec6fe13 100644
--- a/arch/alpha/kernel/signal.c
+++ b/arch/alpha/kernel/signal.c
@@ -120,12 +120,13 @@ SYSCALL_DEFINE5(rt_sigaction, int, sig, const struct sigaction __user *, act,
*/
SYSCALL_DEFINE1(sigsuspend, old_sigset_t, mask)
{
- mask &= _BLOCKABLE;
- spin_lock_irq(¤t->sighand->siglock);
+ sigset_t blocked;
+
current->saved_sigmask = current->blocked;
- siginitset(¤t->blocked, mask);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+
+ mask &= _BLOCKABLE;
+ siginitset(&blocked, mask);
+ set_current_blocked(&blocked);
current->state = TASK_INTERRUPTIBLE;
schedule();
@@ -238,10 +239,7 @@ do_sigreturn(struct sigcontext __user *sc, struct pt_regs *regs,
goto give_sigsegv;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(sc, regs, sw))
goto give_sigsegv;
@@ -276,10 +274,7 @@ do_rt_sigreturn(struct rt_sigframe __user *frame, struct pt_regs *regs,
goto give_sigsegv;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_block(&set);
if (restore_sigcontext(&frame->uc.uc_mcontext, regs, sw))
goto give_sigsegv;
@@ -502,12 +497,12 @@ handle_signal(int sig, struct k_sigaction *ka, siginfo_t *info,
ret = setup_frame(sig, ka, oldset, regs, sw);
if (ret == 0) {
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked,¤t->blocked,&ka->sa.sa_mask);
+ sigset_t blocked;
+
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked,sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
}
return ret;
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 02/41] arm: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
2011-08-11 13:56 ` [PATCH 01/41] alpha: " Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 17:04 ` Will Deacon
2011-08-11 13:56 ` [PATCH 03/41] avr32: Don't mask signals in the error path Matt Fleming
` (40 subsequent siblings)
42 siblings, 1 reply; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov
Cc: linux-kernel, Russell King, Arnd Bergmann, Dave Martin,
Nicolas Pitre, Will Deacon
From: Matt Fleming <matt.fleming@linux.intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check for shared signals we're about to block.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Arnd Bergmann <arnd.bergmann@linaro.org>
Cc: Dave Martin <dave.martin@linaro.org>
Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
CC: Will Deacon <will.deacon@arm.com>
Signed-off-by: Matt Fleming <matt.fleming@linux.intel.com>
---
arch/arm/kernel/signal.c | 25 +++++++++++--------------
1 files changed, 11 insertions(+), 14 deletions(-)
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index 0340224..b97668a 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -66,12 +66,13 @@ const unsigned long syscall_restart_code[2] = {
*/
asmlinkage int sys_sigsuspend(int restart, unsigned long oldmask, old_sigset_t mask)
{
- mask &= _BLOCKABLE;
- spin_lock_irq(¤t->sighand->siglock);
+ sigset_t blocked;
+
current->saved_sigmask = current->blocked;
- siginitset(¤t->blocked, mask);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+
+ mask &= _BLOCKABLE;
+ siginitset(&blocked, mask);
+ set_current_block(&blocked);
current->state = TASK_INTERRUPTIBLE;
schedule();
@@ -281,10 +282,7 @@ static int restore_sigframe(struct pt_regs *regs, struct sigframe __user *sf)
err = __copy_from_user(&set, &sf->uc.uc_sigmask, sizeof(set));
if (err == 0) {
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
}
__get_user_error(regs->ARM_r0, &sf->uc.uc_mcontext.arm_r0, err);
@@ -607,6 +605,7 @@ handle_signal(unsigned long sig, struct k_sigaction *ka,
{
struct thread_info *thread = current_thread_info();
struct task_struct *tsk = current;
+ sigset_t blocked;
int usig = sig;
int ret;
@@ -637,13 +636,11 @@ handle_signal(unsigned long sig, struct k_sigaction *ka,
/*
* Block the signal if we were successful.
*/
- spin_lock_irq(&tsk->sighand->siglock);
- sigorsets(&tsk->blocked, &tsk->blocked,
+ sigorsets(&blocked, &tsk->blocked,
&ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(&tsk->blocked, sig);
- recalc_sigpending();
- spin_unlock_irq(&tsk->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
return 0;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* Re: [PATCH 02/41] arm: Use set_current_blocked()
2011-08-11 13:56 ` [PATCH 02/41] arm: " Matt Fleming
@ 2011-08-11 17:04 ` Will Deacon
0 siblings, 0 replies; 87+ messages in thread
From: Will Deacon @ 2011-08-11 17:04 UTC (permalink / raw)
To: Matt Fleming
Cc: Oleg Nesterov, linux-kernel, Russell King, Arnd Bergmann,
Dave Martin, Nicolas Pitre
Hi Matt,
On Thu, Aug 11, 2011 at 02:56:36PM +0100, Matt Fleming wrote:
> From: Matt Fleming <matt.fleming@linux.intel.com>
>
> As described in e6fa16ab ("signal: sigprocmask() should do
> retarget_shared_pending()") the modification of current->blocked is
> incorrect as we need to check for shared signals we're about to block.
>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Arnd Bergmann <arnd.bergmann@linaro.org>
> Cc: Dave Martin <dave.martin@linaro.org>
> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> CC: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Matt Fleming <matt.fleming@linux.intel.com>
> ---
> arch/arm/kernel/signal.c | 25 +++++++++++--------------
> 1 files changed, 11 insertions(+), 14 deletions(-)
Looks good to me.
Acked-by: Will Deacon <will.deacon@arm.com>
Cheers,
Will
^ permalink raw reply [flat|nested] 87+ messages in thread
* [PATCH 03/41] avr32: Don't mask signals in the error path
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
2011-08-11 13:56 ` [PATCH 01/41] alpha: " Matt Fleming
2011-08-11 13:56 ` [PATCH 02/41] arm: " Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:56 ` [PATCH 04/41] avr32: use set_current_blocked() in handle_signal/sys_rt_sigreturn Matt Fleming
` (39 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Haavard Skinnemoen, Hans-Christian Egtvedt
From: Matt Fleming <matt.fleming@linux.intel.com>
The current handle_signal() implementation is broken - it will mask
signals if we fail to setup the signal stack frame, which isn't the
desired behaviour, we should only be masking signals if we succeed in
setting up the stack frame. It looks like this code was copied from
the old (broken) arm implementation but wasn't updated when the arm
code was fixed in commit a6c61e9dfdd0 ("[ARM] 3168/1: Update ARM
signal delivery and masking").
Cc: Haavard Skinnemoen <hskinnemoen@gmail.com>
Cc: Hans-Christian Egtvedt <egtvedt@samfundet.no>
Signed-off-by: Matt Fleming <matt.fleming@linux.intel.com>
---
arch/avr32/kernel/signal.c | 25 ++++++++++++-------------
1 files changed, 12 insertions(+), 13 deletions(-)
diff --git a/arch/avr32/kernel/signal.c b/arch/avr32/kernel/signal.c
index 64f886f..9c075e1 100644
--- a/arch/avr32/kernel/signal.c
+++ b/arch/avr32/kernel/signal.c
@@ -238,22 +238,21 @@ handle_signal(unsigned long sig, struct k_sigaction *ka, siginfo_t *info,
*/
ret |= !valid_user_regs(regs);
+ if (ret != 0) {
+ force_sigsegv(sig, current);
+ return;
+ }
+
/*
- * Block the signal if we were unsuccessful.
+ * Block the signal if we were successful.
*/
- if (ret != 0 || !(ka->sa.sa_flags & SA_NODEFER)) {
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked, ¤t->blocked,
- &ka->sa.sa_mask);
+ spin_lock_irq(¤t->sighand->siglock);
+ sigorsets(¤t->blocked, ¤t->blocked,
+ &ka->sa.sa_mask);
+ if (!(ka->sa.sa_flags & SA_NODEFER))
sigaddset(¤t->blocked, sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
- }
-
- if (ret == 0)
- return;
-
- force_sigsegv(sig, current);
+ recalc_sigpending();
+ spin_unlock_irq(¤t->sighand->siglock);
}
/*
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 04/41] avr32: use set_current_blocked() in handle_signal/sys_rt_sigreturn
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (2 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 03/41] avr32: Don't mask signals in the error path Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:56 ` [PATCH 05/41] blackfin: Use set_current_blocked() Matt Fleming
` (38 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Haavard Skinnemoen, Hans-Christian Egtvedt
From: Oleg Nesterov <oleg@redhat.com>
It is wrong to change ->blocked directly, see e6fa16ab.
Change handle_signal() and sys_rt_sigreturn() to use
the right helper, set_current_blocked().
Cc: Haavard Skinnemoen <hskinnemoen@gmail.com>
Cc: Hans-Christian Egtvedt <egtvedt@samfundet.no>
Reviewed-by: Matt Fleming <matt.fleming@intel.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
arch/avr32/kernel/signal.c | 15 +++++----------
1 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/arch/avr32/kernel/signal.c b/arch/avr32/kernel/signal.c
index 9c075e1..06f4293 100644
--- a/arch/avr32/kernel/signal.c
+++ b/arch/avr32/kernel/signal.c
@@ -87,10 +87,7 @@ asmlinkage int sys_rt_sigreturn(struct pt_regs *regs)
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(regs, &frame->uc.uc_mcontext))
goto badframe;
@@ -226,6 +223,7 @@ static inline void
handle_signal(unsigned long sig, struct k_sigaction *ka, siginfo_t *info,
sigset_t *oldset, struct pt_regs *regs, int syscall)
{
+ sigset_t blocked;
int ret;
/*
@@ -246,13 +244,10 @@ handle_signal(unsigned long sig, struct k_sigaction *ka, siginfo_t *info,
/*
* Block the signal if we were successful.
*/
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked, ¤t->blocked,
- &ka->sa.sa_mask);
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked, sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
}
/*
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 05/41] blackfin: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (3 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 04/41] avr32: use set_current_blocked() in handle_signal/sys_rt_sigreturn Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:56 ` [PATCH 06/41] cris: " Matt Fleming
` (37 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Mike Frysinger
From: Matt Fleming <matt.fleming@linux.intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Matt Fleming <matt.fleming@linux.intel.com>
---
arch/blackfin/kernel/signal.c | 16 ++++++----------
1 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/arch/blackfin/kernel/signal.c b/arch/blackfin/kernel/signal.c
index d536f35..96b8666 100644
--- a/arch/blackfin/kernel/signal.c
+++ b/arch/blackfin/kernel/signal.c
@@ -99,10 +99,7 @@ asmlinkage int do_rt_sigreturn(unsigned long __unused)
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_block(&set);
if (rt_restore_sigcontext(regs, &frame->uc.uc_mcontext, &r0))
goto badframe;
@@ -267,13 +264,12 @@ handle_signal(int sig, siginfo_t *info, struct k_sigaction *ka,
ret = setup_rt_frame(sig, ka, info, oldset, regs);
if (ret == 0) {
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked, ¤t->blocked,
- &ka->sa.sa_mask);
+ sigset_t blocked;
+
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked, sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
}
return ret;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 06/41] cris: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (4 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 05/41] blackfin: Use set_current_blocked() Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:56 ` [PATCH 07/41] frv: " Matt Fleming
` (36 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Mikael Starvik, Jesper Nilsson
From: Matt Fleming <matt.fleming@linux.intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Mikael Starvik <starvik@axis.com>
Cc: Jesper Nilsson <jesper.nilsson@axis.com>
Signed-off-by: Matt Fleming <matt.fleming@linux.intel.com>
---
arch/cris/arch-v10/kernel/signal.c | 33 +++++++++++++------------------
arch/cris/arch-v32/kernel/signal.c | 37 +++++++++++++----------------------
2 files changed, 28 insertions(+), 42 deletions(-)
diff --git a/arch/cris/arch-v10/kernel/signal.c b/arch/cris/arch-v10/kernel/signal.c
index e78fe49..9f8c29c 100644
--- a/arch/cris/arch-v10/kernel/signal.c
+++ b/arch/cris/arch-v10/kernel/signal.c
@@ -50,12 +50,14 @@ void do_signal(int canrestart, struct pt_regs *regs);
int sys_sigsuspend(old_sigset_t mask, long r11, long r12, long r13, long mof,
long srp, struct pt_regs *regs)
{
- mask &= _BLOCKABLE;
- spin_lock_irq(¤t->sighand->siglock);
+ sigset_t blocked;
+
current->saved_sigmask = current->blocked;
- siginitset(¤t->blocked, mask);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+
+ mask &= _BLOCKABLE;
+ siginitset(&blocked, mask);
+ set_current_blocked(&blocked);
+
current->state = TASK_INTERRUPTIBLE;
schedule();
set_thread_flag(TIF_RESTORE_SIGMASK);
@@ -184,10 +186,7 @@ asmlinkage int sys_sigreturn(long r10, long r11, long r12, long r13, long mof,
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(regs, &frame->sc))
goto badframe;
@@ -223,10 +222,7 @@ asmlinkage int sys_rt_sigreturn(long r10, long r11, long r12, long r13,
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(regs, &frame->uc.uc_mcontext))
goto badframe;
@@ -469,13 +465,12 @@ static inline int handle_signal(int canrestart, unsigned long sig,
ret = setup_frame(sig, ka, oldset, regs);
if (ret == 0) {
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked, ¤t->blocked,
- &ka->sa.sa_mask);
+ sigset_t blocked;
+
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked, sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
}
return ret;
}
diff --git a/arch/cris/arch-v32/kernel/signal.c b/arch/cris/arch-v32/kernel/signal.c
index ce4ab1a..e493656 100644
--- a/arch/cris/arch-v32/kernel/signal.c
+++ b/arch/cris/arch-v32/kernel/signal.c
@@ -62,12 +62,14 @@ int
sys_sigsuspend(old_sigset_t mask, long r11, long r12, long r13, long mof,
long srp, struct pt_regs *regs)
{
- mask &= _BLOCKABLE;
- spin_lock_irq(¤t->sighand->siglock);
+ sigset_t blocked;
+
current->saved_sigmask = current->blocked;
- siginitset(¤t->blocked, mask);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+
+ mask &= _BLOCKABLE;
+ siginitset(&blocked, mask);
+ set_current_blocked(&blocked);
+
current->state = TASK_INTERRUPTIBLE;
schedule();
set_thread_flag(TIF_RESTORE_SIGMASK);
@@ -176,12 +178,7 @@ sys_sigreturn(long r10, long r11, long r12, long r13, long mof, long srp,
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
-
- current->blocked = set;
-
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(regs, &frame->sc))
goto badframe;
@@ -222,12 +219,7 @@ sys_rt_sigreturn(long r10, long r11, long r12, long r13, long mof, long srp,
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
-
- current->blocked = set;
-
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(regs, &frame->uc.uc_mcontext))
goto badframe;
@@ -516,13 +508,12 @@ handle_signal(int canrestart, unsigned long sig,
ka->sa.sa_handler = SIG_DFL;
if (ret == 0) {
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked, ¤t->blocked,
- &ka->sa.sa_mask);
+ sigset_t blocked;
+
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked, sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
}
return ret;
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 07/41] frv: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (5 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 06/41] cris: " Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:56 ` [PATCH 08/41] h8300: " Matt Fleming
` (35 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, David Howells, Al Viro
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/frv/kernel/signal.c | 32 +++++++++++++-------------------
1 files changed, 13 insertions(+), 19 deletions(-)
diff --git a/arch/frv/kernel/signal.c b/arch/frv/kernel/signal.c
index bab0129..7f624e4 100644
--- a/arch/frv/kernel/signal.c
+++ b/arch/frv/kernel/signal.c
@@ -40,12 +40,13 @@ struct fdpic_func_descriptor {
*/
asmlinkage int sys_sigsuspend(int history0, int history1, old_sigset_t mask)
{
- mask &= _BLOCKABLE;
- spin_lock_irq(¤t->sighand->siglock);
+ sigset_t blocked;
+
current->saved_sigmask = current->blocked;
- siginitset(¤t->blocked, mask);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+
+ mask &= _BLOCKABLE;
+ siginitset(&blocked, mask);
+ set_current_blocked(&blocked);
current->state = TASK_INTERRUPTIBLE;
schedule();
@@ -158,10 +159,7 @@ asmlinkage int sys_sigreturn(void)
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(&frame->sc, &gr8))
goto badframe;
@@ -184,10 +182,7 @@ asmlinkage int sys_rt_sigreturn(void)
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(&frame->uc.uc_mcontext, &gr8))
goto badframe;
@@ -475,13 +470,12 @@ static int handle_signal(unsigned long sig, siginfo_t *info,
ret = setup_frame(sig, ka, oldset);
if (ret == 0) {
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked, ¤t->blocked,
- &ka->sa.sa_mask);
+ sigset_t blocked;
+
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked, sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
}
return ret;
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 08/41] h8300: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (6 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 07/41] frv: " Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:56 ` [PATCH 09/41] ia64: " Matt Fleming
` (34 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Yoshinori Sato
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/h8300/kernel/signal.c | 36 +++++++++++++-----------------------
1 files changed, 13 insertions(+), 23 deletions(-)
diff --git a/arch/h8300/kernel/signal.c b/arch/h8300/kernel/signal.c
index af842c3..4c06f98 100644
--- a/arch/h8300/kernel/signal.c
+++ b/arch/h8300/kernel/signal.c
@@ -57,14 +57,13 @@ asmlinkage int do_signal(struct pt_regs *regs, sigset_t *oldset);
asmlinkage int do_sigsuspend(struct pt_regs *regs)
{
old_sigset_t mask = regs->er3;
- sigset_t saveset;
+ sigset_t saveset, blocked;
- mask &= _BLOCKABLE;
- spin_lock_irq(¤t->sighand->siglock);
saveset = current->blocked;
- siginitset(¤t->blocked, mask);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+
+ mask &= _BLOCKABLE;
+ siginitset(&blocked, mask);
+ set_current_blocked(&blocked);
regs->er0 = -EINTR;
while (1) {
@@ -90,11 +89,8 @@ do_rt_sigsuspend(struct pt_regs *regs)
return -EFAULT;
sigdelsetmask(&newset, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
saveset = current->blocked;
- current->blocked = newset;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&newset);
regs->er0 = -EINTR;
while (1) {
@@ -232,10 +228,7 @@ asmlinkage int do_sigreturn(unsigned long __unused,...)
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(regs, &frame->sc, &er0))
goto badframe;
@@ -260,10 +253,7 @@ asmlinkage int do_rt_sigreturn(unsigned long __unused,...)
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_unlock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_lock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(regs, &frame->uc.uc_mcontext, &er0))
goto badframe;
@@ -463,6 +453,8 @@ static void
handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka,
sigset_t *oldset, struct pt_regs * regs)
{
+ sigset_t blocked;
+
/* are we from a system call? */
if (regs->orig_er0 >= 0) {
switch (regs->er0) {
@@ -489,12 +481,10 @@ handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka,
else
setup_frame(sig, ka, oldset, regs);
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked,¤t->blocked,&ka->sa.sa_mask);
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked,sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
}
/*
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 09/41] ia64: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (7 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 08/41] h8300: " Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:56 ` [PATCH 10/41] m32r: " Matt Fleming
` (33 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Tony Luck, Fenghua Yu
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/ia64/kernel/signal.c | 18 ++++++------------
1 files changed, 6 insertions(+), 12 deletions(-)
diff --git a/arch/ia64/kernel/signal.c b/arch/ia64/kernel/signal.c
index 7bdafc8..1261b2f 100644
--- a/arch/ia64/kernel/signal.c
+++ b/arch/ia64/kernel/signal.c
@@ -201,13 +201,7 @@ ia64_rt_sigreturn (struct sigscratch *scr)
goto give_sigsegv;
sigdelsetmask(&set, ~_BLOCKABLE);
-
- spin_lock_irq(¤t->sighand->siglock);
- {
- current->blocked = set;
- recalc_sigpending();
- }
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(sc, scr))
goto give_sigsegv;
@@ -424,15 +418,15 @@ static long
handle_signal (unsigned long sig, struct k_sigaction *ka, siginfo_t *info, sigset_t *oldset,
struct sigscratch *scr)
{
+ sigset_t blocked;
+
if (!setup_frame(sig, ka, info, oldset, scr))
return 0;
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked, ¤t->blocked, &ka->sa.sa_mask);
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked, sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
/*
* Let tracing know that we've done the handler setup.
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 10/41] m32r: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (8 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 09/41] ia64: " Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:56 ` [PATCH 11/41] m68k: " Matt Fleming
` (32 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Hirokazu Takata, Al Viro, Kyle McMartin
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Hirokazu Takata <takata@linux-m32r.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kyle McMartin <kyle@redhat.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/m32r/kernel/signal.c | 16 +++++++---------
1 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/arch/m32r/kernel/signal.c b/arch/m32r/kernel/signal.c
index a08697f..98f016a 100644
--- a/arch/m32r/kernel/signal.c
+++ b/arch/m32r/kernel/signal.c
@@ -112,10 +112,7 @@ sys_rt_sigreturn(unsigned long r0, unsigned long r1,
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(regs, &frame->uc.uc_mcontext, &result))
goto badframe;
@@ -274,6 +271,8 @@ static int
handle_signal(unsigned long sig, struct k_sigaction *ka, siginfo_t *info,
sigset_t *oldset, struct pt_regs *regs)
{
+ sigset_t blocked;
+
/* Are we from a system call? */
if (regs->syscall_nr >= 0) {
/* If so, check system call restarting.. */
@@ -300,12 +299,11 @@ handle_signal(unsigned long sig, struct k_sigaction *ka, siginfo_t *info,
if (setup_rt_frame(sig, ka, info, oldset, regs))
return -EFAULT;
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked,¤t->blocked,&ka->sa.sa_mask);
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked,sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
+
return 0;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 11/41] m68k: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (9 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 10/41] m32r: " Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:56 ` [PATCH 12/41] microblaze: Don't reimplement force_sigsegv() Matt Fleming
` (31 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Geert Uytterhoeven, Greg Ungerer
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Greg Ungerer <gerg@uclinux.org>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/m68k/kernel/signal_mm.c | 24 ++++++++++++------------
arch/m68k/kernel/signal_no.c | 30 ++++++++++++------------------
2 files changed, 24 insertions(+), 30 deletions(-)
diff --git a/arch/m68k/kernel/signal_mm.c b/arch/m68k/kernel/signal_mm.c
index a0afc23..eeeaf3c 100644
--- a/arch/m68k/kernel/signal_mm.c
+++ b/arch/m68k/kernel/signal_mm.c
@@ -97,12 +97,13 @@ int handle_kernel_fault(struct pt_regs *regs)
asmlinkage int
sys_sigsuspend(int unused0, int unused1, old_sigset_t mask)
{
- mask &= _BLOCKABLE;
- spin_lock_irq(¤t->sighand->siglock);
+ sigset_t blocked;
+
current->saved_sigmask = current->blocked;
- siginitset(¤t->blocked, mask);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+
+ mask &= _BLOCKABLE;
+ siginitset(&blocked, mask);
+ set_current_blocked(&blocked);
current->state = TASK_INTERRUPTIBLE;
schedule();
@@ -465,8 +466,7 @@ asmlinkage int do_sigreturn(unsigned long __unused)
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- current->blocked = set;
- recalc_sigpending();
+ set_current_blocked(&set);
if (restore_sigcontext(regs, &frame->sc, frame + 1))
goto badframe;
@@ -491,8 +491,7 @@ asmlinkage int do_rt_sigreturn(unsigned long __unused)
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- current->blocked = set;
- recalc_sigpending();
+ set_current_blocked(&set);
if (rt_restore_ucontext(regs, sw, &frame->uc))
goto badframe;
@@ -950,6 +949,7 @@ static void
handle_signal(int sig, struct k_sigaction *ka, siginfo_t *info,
sigset_t *oldset, struct pt_regs *regs)
{
+ sigset_t blocked;
int err;
/* are we from a system call? */
if (regs->orig_d0 >= 0)
@@ -965,10 +965,10 @@ handle_signal(int sig, struct k_sigaction *ka, siginfo_t *info,
if (err)
return;
- sigorsets(¤t->blocked,¤t->blocked,&ka->sa.sa_mask);
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked,sig);
- recalc_sigpending();
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
if (test_thread_flag(TIF_DELAYED_TRACE)) {
regs->sr &= ~0x8000;
diff --git a/arch/m68k/kernel/signal_no.c b/arch/m68k/kernel/signal_no.c
index 36a81bb..97f3c39 100644
--- a/arch/m68k/kernel/signal_no.c
+++ b/arch/m68k/kernel/signal_no.c
@@ -60,12 +60,13 @@ void ret_from_user_rt_signal(void);
asmlinkage int
sys_sigsuspend(int unused0, int unused1, old_sigset_t mask)
{
- mask &= _BLOCKABLE;
- spin_lock_irq(¤t->sighand->siglock);
+ sigset_t blocked;
+
current->saved_sigmask = current->blocked;
- siginitset(¤t->blocked, mask);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+
+ mask &= _BLOCKABLE;
+ siginitset(&blocked, mask);
+ set_current_blocked(&blocked);
current->state = TASK_INTERRUPTIBLE;
schedule();
@@ -343,10 +344,7 @@ asmlinkage int do_sigreturn(unsigned long __unused)
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(regs, &frame->sc, frame + 1, &d0))
goto badframe;
@@ -372,10 +370,7 @@ asmlinkage int do_rt_sigreturn(unsigned long __unused)
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (rt_restore_ucontext(regs, sw, &frame->uc, &d0))
goto badframe;
@@ -693,6 +688,7 @@ static void
handle_signal(int sig, struct k_sigaction *ka, siginfo_t *info,
sigset_t *oldset, struct pt_regs *regs)
{
+ sigset_t blocked;
int err;
/* are we from a system call? */
if (regs->orig_d0 >= 0)
@@ -708,12 +704,10 @@ handle_signal(int sig, struct k_sigaction *ka, siginfo_t *info,
if (err)
return;
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked,¤t->blocked,&ka->sa.sa_mask);
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked,sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
clear_thread_flag(TIF_RESTORE_SIGMASK);
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 12/41] microblaze: Don't reimplement force_sigsegv()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (10 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 11/41] m68k: " Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-16 17:20 ` Oleg Nesterov
2011-08-11 13:56 ` [PATCH 13/41] microblaze: No need to reset handler if SA_ONESHOT Matt Fleming
` (30 subsequent siblings)
42 siblings, 1 reply; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Michal Simek
From: Matt Fleming <matt.fleming@intel.com>
Instead of open coding the sequence from force_sigsegv() just call
it. This also fixes a race because sa_handler was being modified
without holding ->sighand->siglock.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Michal Simek <monstr@monstr.eu>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/microblaze/kernel/signal.c | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/arch/microblaze/kernel/signal.c b/arch/microblaze/kernel/signal.c
index 5996711..90de06d 100644
--- a/arch/microblaze/kernel/signal.c
+++ b/arch/microblaze/kernel/signal.c
@@ -270,9 +270,7 @@ static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
return;
give_sigsegv:
- if (sig == SIGSEGV)
- ka->sa.sa_handler = SIG_DFL;
- force_sig(SIGSEGV, current);
+ force_sigsegv(sig, current);
}
/* Handle restarting system calls */
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* Re: [PATCH 12/41] microblaze: Don't reimplement force_sigsegv()
2011-08-11 13:56 ` [PATCH 12/41] microblaze: Don't reimplement force_sigsegv() Matt Fleming
@ 2011-08-16 17:20 ` Oleg Nesterov
2011-08-16 17:20 ` Oleg Nesterov
0 siblings, 1 reply; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-16 17:20 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-kernel, Michal Simek
On 08/11, Matt Fleming wrote:
>
> Instead of open coding the sequence from force_sigsegv() just call
> it. This also fixes a race because sa_handler was being modified
> without holding ->sighand->siglock.
>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Michal Simek <monstr@monstr.eu>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
> arch/microblaze/kernel/signal.c | 4 +---
> 1 files changed, 1 insertions(+), 3 deletions(-)
>
> diff --git a/arch/microblaze/kernel/signal.c b/arch/microblaze/kernel/signal.c
> index 5996711..90de06d 100644
> --- a/arch/microblaze/kernel/signal.c
> +++ b/arch/microblaze/kernel/signal.c
> @@ -270,9 +270,7 @@ static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
> return;
>
> give_sigsegv:
> - if (sig == SIGSEGV)
> - ka->sa.sa_handler = SIG_DFL;
> - force_sig(SIGSEGV, current);
> + force_sigsegv(sig, current);
> }
This and the next patch seem to have the same problems with the
changelogs as 18 and 19.
13-14 looks correct to me...
Oleg.
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 12/41] microblaze: Don't reimplement force_sigsegv()
2011-08-16 17:20 ` Oleg Nesterov
@ 2011-08-16 17:20 ` Oleg Nesterov
2011-08-17 13:01 ` Michal Simek
0 siblings, 1 reply; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-16 17:20 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-kernel, Michal Simek
On 08/16, Oleg Nesterov wrote:
>
> This and the next patch seem to have the same problems with the
> changelogs as 18 and 19.
>
> 13-14 looks correct to me...
I meant 14-15, sorry for noise
Oleg.
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 12/41] microblaze: Don't reimplement force_sigsegv()
2011-08-16 17:20 ` Oleg Nesterov
@ 2011-08-17 13:01 ` Michal Simek
2011-08-17 13:14 ` Oleg Nesterov
2011-08-17 13:17 ` Matt Fleming
0 siblings, 2 replies; 87+ messages in thread
From: Michal Simek @ 2011-08-17 13:01 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Matt Fleming, linux-kernel
Oleg Nesterov wrote:
> On 08/16, Oleg Nesterov wrote:
>> This and the next patch seem to have the same problems with the
>> changelogs as 18 and 19.
>>
>> 13-14 looks correct to me...
>
> I meant 14-15, sorry for noise
Do you mean that there is
From: Matt Fleming <matt.fleming@intel.com> ?
Anyway: I have looked at patches 12-15 and they look good to me.
Fix that "From:" line and you can add my
Acked-by: Michal Simek <monstr@monstr.eu>
If you want to propose 12-15 through my Microblaze tree, please let me know.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 12/41] microblaze: Don't reimplement force_sigsegv()
2011-08-17 13:01 ` Michal Simek
@ 2011-08-17 13:14 ` Oleg Nesterov
2011-08-17 13:25 ` Matt Fleming
2011-08-17 13:17 ` Matt Fleming
1 sibling, 1 reply; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-17 13:14 UTC (permalink / raw)
To: Michal Simek; +Cc: Matt Fleming, linux-kernel
On 08/17, Michal Simek wrote:
>
> Oleg Nesterov wrote:
>> On 08/16, Oleg Nesterov wrote:
>>> This and the next patch seem to have the same problems with the
>>> changelogs as 18 and 19.
>>>
>>> 13-14 looks correct to me...
>>
>> I meant 14-15, sorry for noise
>
> Do you mean that there is
> From: Matt Fleming <matt.fleming@intel.com> ?
Confused... "From" looks correct to me.
The changelog is wrong and should be updated (Matt seems to agree).
This patch fixes the bug, this should be explained. The problem is,
ka points to the copy of sighand->action, the current code is pointless.
> Anyway: I have looked at patches 12-15 and they look good to me.
>
> Acked-by: Michal Simek <monstr@monstr.eu>
Assuming the changelog is fixed,
Acked-by: Oleg Nesterov <oleg@redhat.com>
> If you want to propose 12-15 through my Microblaze tree, please let me know.
Probably this is the best option. Matt, what do you think?
Oleg.
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 12/41] microblaze: Don't reimplement force_sigsegv()
2011-08-17 13:14 ` Oleg Nesterov
@ 2011-08-17 13:25 ` Matt Fleming
0 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-17 13:25 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Michal Simek, linux-kernel
On Wed, 2011-08-17 at 15:14 +0200, Oleg Nesterov wrote:
>
> Assuming the changelog is fixed,
>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
Thanks!
> > If you want to propose 12-15 through my Microblaze tree, please let me know.
>
> Probably this is the best option. Matt, what do you think?
Yeah, that seems like the best solution. All of the patches are
independent so it's easy for the respective maintainers to apply them
when they're ready.
I'll be sending out updated versions today.
--
Matt Fleming, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 12/41] microblaze: Don't reimplement force_sigsegv()
2011-08-17 13:01 ` Michal Simek
2011-08-17 13:14 ` Oleg Nesterov
@ 2011-08-17 13:17 ` Matt Fleming
2011-08-17 13:22 ` Michal Simek
1 sibling, 1 reply; 87+ messages in thread
From: Matt Fleming @ 2011-08-17 13:17 UTC (permalink / raw)
To: monstr; +Cc: Oleg Nesterov, linux-kernel
On Wed, 2011-08-17 at 15:01 +0200, Michal Simek wrote:
> Oleg Nesterov wrote:
> > On 08/16, Oleg Nesterov wrote:
> >> This and the next patch seem to have the same problems with the
> >> changelogs as 18 and 19.
> >>
> >> 13-14 looks correct to me...
> >
> > I meant 14-15, sorry for noise
>
> Do you mean that there is
> From: Matt Fleming <matt.fleming@intel.com> ?
>
> Anyway: I have looked at patches 12-15 and they look good to me.
>
> Fix that "From:" line and you can add my
What's wrong with the From: line? That should be fine. Oleg was saying
that the description in the changelog needed updating.
> Acked-by: Michal Simek <monstr@monstr.eu>
Thanks.
--
Matt Fleming, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 12/41] microblaze: Don't reimplement force_sigsegv()
2011-08-17 13:17 ` Matt Fleming
@ 2011-08-17 13:22 ` Michal Simek
0 siblings, 0 replies; 87+ messages in thread
From: Michal Simek @ 2011-08-17 13:22 UTC (permalink / raw)
To: Matt Fleming; +Cc: Oleg Nesterov, linux-kernel
Matt Fleming wrote:
> On Wed, 2011-08-17 at 15:01 +0200, Michal Simek wrote:
>> Oleg Nesterov wrote:
>>> On 08/16, Oleg Nesterov wrote:
>>>> This and the next patch seem to have the same problems with the
>>>> changelogs as 18 and 19.
>>>>
>>>> 13-14 looks correct to me...
>>> I meant 14-15, sorry for noise
>> Do you mean that there is
>> From: Matt Fleming <matt.fleming@intel.com> ?
>>
>> Anyway: I have looked at patches 12-15 and they look good to me.
>>
>> Fix that "From:" line and you can add my
>
> What's wrong with the From: line? That should be fine. Oleg was saying
> that the description in the changelog needed updating.
It is just additional information but I see when you apply it with git-am it is not there.
That's fine.
Fix what Oleg described and let me know if you want to add to Microblaze tree.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
^ permalink raw reply [flat|nested] 87+ messages in thread
* [PATCH 13/41] microblaze: No need to reset handler if SA_ONESHOT
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (11 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 12/41] microblaze: Don't reimplement force_sigsegv() Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:56 ` [PATCH 14/41] microblaze: Fix signal masking Matt Fleming
` (29 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Michal Simek
From: Matt Fleming <matt.fleming@intel.com>
get_signal_to_deliver() already resets the signal handler if
SA_ONESHOT is set in ka->sa.sa_flags, there's no need to do it again
in handle_signal(). Furthermore, because we were modifying
ka->sa.sa_handler without holding ->sighand->siglock there's the
potential we can race with another thread if it modifies the signal
handlers.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Michal Simek <monstr@monstr.eu>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/microblaze/kernel/signal.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/arch/microblaze/kernel/signal.c b/arch/microblaze/kernel/signal.c
index 90de06d..9e749c0 100644
--- a/arch/microblaze/kernel/signal.c
+++ b/arch/microblaze/kernel/signal.c
@@ -320,9 +320,6 @@ handle_signal(unsigned long sig, struct k_sigaction *ka,
else
setup_rt_frame(sig, ka, NULL, oldset, regs);
- if (ka->sa.sa_flags & SA_ONESHOT)
- ka->sa.sa_handler = SIG_DFL;
-
if (!(ka->sa.sa_flags & SA_NODEFER)) {
spin_lock_irq(¤t->sighand->siglock);
sigorsets(¤t->blocked,
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 14/41] microblaze: Fix signal masking
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (12 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 13/41] microblaze: No need to reset handler if SA_ONESHOT Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:56 ` [PATCH 15/41] microblaze: Use set_current_blocked() Matt Fleming
` (28 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Michal Simek
From: Matt Fleming <matt.fleming@intel.com>
There are a couple of problems with the current signal code,
1. If we failed to setup the signal stack frame then we
should not be masking any signals.
2. ka->sa.sa_mask is only added to the current blocked signals list if
SA_NODEFER is set in ka->sa.sa_flags. If we successfully setup the
signal frame and are going to run the handler then we must honour
sa_mask.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Michal Simek <monstr@monstr.eu>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/microblaze/kernel/signal.c | 31 ++++++++++++++++++-------------
1 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/arch/microblaze/kernel/signal.c b/arch/microblaze/kernel/signal.c
index 9e749c0..f2c13d5 100644
--- a/arch/microblaze/kernel/signal.c
+++ b/arch/microblaze/kernel/signal.c
@@ -169,7 +169,7 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size)
return (void __user *)((sp - frame_size) & -8UL);
}
-static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
+static int setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
sigset_t *set, struct pt_regs *regs)
{
struct rt_sigframe __user *frame;
@@ -267,10 +267,11 @@ static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
current->comm, current->pid, frame, regs->pc);
#endif
- return;
+ return 0;
give_sigsegv:
force_sigsegv(sig, current);
+ return -EFAULT;
}
/* Handle restarting system calls */
@@ -314,21 +315,25 @@ static int
handle_signal(unsigned long sig, struct k_sigaction *ka,
siginfo_t *info, sigset_t *oldset, struct pt_regs *regs)
{
+ int ret;
+
/* Set up the stack frame */
if (ka->sa.sa_flags & SA_SIGINFO)
- setup_rt_frame(sig, ka, info, oldset, regs);
+ ret = setup_rt_frame(sig, ka, info, oldset, regs);
else
- setup_rt_frame(sig, ka, NULL, oldset, regs);
+ ret = setup_rt_frame(sig, ka, NULL, oldset, regs);
+
+ if (ret)
+ return ret;
- if (!(ka->sa.sa_flags & SA_NODEFER)) {
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked,
- ¤t->blocked, &ka->sa.sa_mask);
+ spin_lock_irq(¤t->sighand->siglock);
+ sigorsets(¤t->blocked, ¤t->blocked, &ka->sa.sa_mask);
+ if (!(ka->sa.sa_flags & SA_NODEFER))
sigaddset(¤t->blocked, sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
- }
- return 1;
+ recalc_sigpending();
+ spin_unlock_irq(¤t->sighand->siglock);
+
+ return 0;
}
/*
@@ -369,7 +374,7 @@ int do_signal(struct pt_regs *regs, sigset_t *oldset, int in_syscall)
/* Whee! Actually deliver the signal. */
if (in_syscall)
handle_restart(regs, &ka, 1);
- if (handle_signal(signr, &ka, &info, oldset, regs)) {
+ if (!handle_signal(signr, &ka, &info, oldset, regs)) {
/*
* A signal was successfully delivered; the saved
* sigmask will have been stored in the signal frame,
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 15/41] microblaze: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (13 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 14/41] microblaze: Fix signal masking Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:56 ` [PATCH 16/41] MIPS: " Matt Fleming
` (27 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Michal Simek
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Michal Simek <monstr@monstr.eu>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/microblaze/kernel/signal.c | 14 +++++---------
1 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/arch/microblaze/kernel/signal.c b/arch/microblaze/kernel/signal.c
index f2c13d5..aaf7c27 100644
--- a/arch/microblaze/kernel/signal.c
+++ b/arch/microblaze/kernel/signal.c
@@ -105,10 +105,7 @@ asmlinkage long sys_rt_sigreturn(struct pt_regs *regs)
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(regs, &frame->uc.uc_mcontext, &rval))
goto badframe;
@@ -315,6 +312,7 @@ static int
handle_signal(unsigned long sig, struct k_sigaction *ka,
siginfo_t *info, sigset_t *oldset, struct pt_regs *regs)
{
+ sigset_t blocked;
int ret;
/* Set up the stack frame */
@@ -326,12 +324,10 @@ handle_signal(unsigned long sig, struct k_sigaction *ka,
if (ret)
return ret;
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked, ¤t->blocked, &ka->sa.sa_mask);
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked, sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
return 0;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 16/41] MIPS: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (14 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 15/41] microblaze: Use set_current_blocked() Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:56 ` [PATCH 17/41] mn10300: " Matt Fleming
` (26 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Ralf Baechle, Al Viro, David Daney
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: David Daney <ddaney@caviumnetworks.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/mips/kernel/signal.c | 29 ++++++++---------------------
arch/mips/kernel/signal32.c | 20 ++++----------------
arch/mips/kernel/signal_n32.c | 10 ++--------
3 files changed, 14 insertions(+), 45 deletions(-)
diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index dbbe0ce..bd3f75b 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -255,11 +255,8 @@ asmlinkage int sys_sigsuspend(nabi_no_regargs struct pt_regs regs)
return -EFAULT;
sigdelsetmask(&newset, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
current->saved_sigmask = current->blocked;
- current->blocked = newset;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&newset);
current->state = TASK_INTERRUPTIBLE;
schedule();
@@ -284,11 +281,8 @@ asmlinkage int sys_rt_sigsuspend(nabi_no_regargs struct pt_regs regs)
return -EFAULT;
sigdelsetmask(&newset, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
current->saved_sigmask = current->blocked;
- current->blocked = newset;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&newset);
current->state = TASK_INTERRUPTIBLE;
schedule();
@@ -360,10 +354,7 @@ asmlinkage void sys_sigreturn(nabi_no_regargs struct pt_regs regs)
goto badframe;
sigdelsetmask(&blocked, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = blocked;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&blocked);
sig = restore_sigcontext(®s, &frame->sf_sc);
if (sig < 0)
@@ -399,10 +390,7 @@ asmlinkage void sys_rt_sigreturn(nabi_no_regargs struct pt_regs regs)
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
sig = restore_sigcontext(®s, &frame->rs_uc.uc_mcontext);
if (sig < 0)
@@ -546,6 +534,7 @@ static int handle_signal(unsigned long sig, siginfo_t *info,
int ret;
struct mips_abi *abi = current->thread.abi;
void *vdso = current->mm->context.vdso;
+ sigset_t blocked;
if (regs->regs[0]) {
switch(regs->regs[2]) {
@@ -578,12 +567,10 @@ static int handle_signal(unsigned long sig, siginfo_t *info,
if (ret)
return ret;
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked, ¤t->blocked, &ka->sa.sa_mask);
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked, sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
return ret;
}
diff --git a/arch/mips/kernel/signal32.c b/arch/mips/kernel/signal32.c
index aae9866..902a889 100644
--- a/arch/mips/kernel/signal32.c
+++ b/arch/mips/kernel/signal32.c
@@ -290,11 +290,8 @@ asmlinkage int sys32_sigsuspend(nabi_no_regargs struct pt_regs regs)
return -EFAULT;
sigdelsetmask(&newset, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
current->saved_sigmask = current->blocked;
- current->blocked = newset;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&newset);
current->state = TASK_INTERRUPTIBLE;
schedule();
@@ -318,11 +315,8 @@ asmlinkage int sys32_rt_sigsuspend(nabi_no_regargs struct pt_regs regs)
return -EFAULT;
sigdelsetmask(&newset, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
current->saved_sigmask = current->blocked;
- current->blocked = newset;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&newset);
current->state = TASK_INTERRUPTIBLE;
schedule();
@@ -488,10 +482,7 @@ asmlinkage void sys32_sigreturn(nabi_no_regargs struct pt_regs regs)
goto badframe;
sigdelsetmask(&blocked, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = blocked;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&blocked);
sig = restore_sigcontext32(®s, &frame->sf_sc);
if (sig < 0)
@@ -529,10 +520,7 @@ asmlinkage void sys32_rt_sigreturn(nabi_no_regargs struct pt_regs regs)
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
sig = restore_sigcontext32(®s, &frame->rs_uc.uc_mcontext);
if (sig < 0)
diff --git a/arch/mips/kernel/signal_n32.c b/arch/mips/kernel/signal_n32.c
index ee24d81..30fc7ff 100644
--- a/arch/mips/kernel/signal_n32.c
+++ b/arch/mips/kernel/signal_n32.c
@@ -94,11 +94,8 @@ asmlinkage int sysn32_rt_sigsuspend(nabi_no_regargs struct pt_regs regs)
sigset_from_compat(&newset, &uset);
sigdelsetmask(&newset, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
current->saved_sigmask = current->blocked;
- current->blocked = newset;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&newset);
current->state = TASK_INTERRUPTIBLE;
schedule();
@@ -122,10 +119,7 @@ asmlinkage void sysn32_rt_sigreturn(nabi_no_regargs struct pt_regs regs)
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
sig = restore_sigcontext(®s, &frame->rs_uc.uc_mcontext);
if (sig < 0)
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 17/41] mn10300: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (15 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 16/41] MIPS: " Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:56 ` [PATCH 18/41] OpenRISC: Don't reimplement force_sigsegv() Matt Fleming
` (25 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, David Howells, Koichi Yasutake, Al Viro
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Koichi Yasutake <yasutake.koichi@jp.panasonic.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/mn10300/kernel/signal.c | 32 +++++++++++++-------------------
1 files changed, 13 insertions(+), 19 deletions(-)
diff --git a/arch/mn10300/kernel/signal.c b/arch/mn10300/kernel/signal.c
index 690f4e9..107fb4e 100644
--- a/arch/mn10300/kernel/signal.c
+++ b/arch/mn10300/kernel/signal.c
@@ -38,12 +38,13 @@
*/
asmlinkage long sys_sigsuspend(int history0, int history1, old_sigset_t mask)
{
- mask &= _BLOCKABLE;
- spin_lock_irq(¤t->sighand->siglock);
+ sigset_t blocked;
+
current->saved_sigmask = current->blocked;
- siginitset(¤t->blocked, mask);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+
+ mask &= _BLOCKABLE;
+ siginitset(&blocked, mask);
+ set_current_blocked(&blocked);
current->state = TASK_INTERRUPTIBLE;
schedule();
@@ -172,10 +173,7 @@ asmlinkage long sys_sigreturn(void)
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(current_frame(), &frame->sc, &d0))
goto badframe;
@@ -203,10 +201,7 @@ asmlinkage long sys_rt_sigreturn(void)
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(current_frame(), &frame->uc.uc_mcontext, &d0))
goto badframe;
@@ -477,13 +472,12 @@ static int handle_signal(int sig,
ret = setup_frame(sig, ka, oldset, regs);
if (ret == 0) {
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked, ¤t->blocked,
- &ka->sa.sa_mask);
+ sigset_t blocked;
+
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked, sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
}
return ret;
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 18/41] OpenRISC: Don't reimplement force_sigsegv()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (16 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 17/41] mn10300: " Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-16 16:49 ` Oleg Nesterov
2011-08-11 13:56 ` [PATCH 19/41] OpenRISC: No need to reset handler if SA_ONESHOT Matt Fleming
` (24 subsequent siblings)
42 siblings, 1 reply; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Jonas Bonn, Arnd Bergmann
From: Matt Fleming <matt.fleming@intel.com>
Instead of open coding the sequence from force_sigsegv() just call
it. This also fixes a race because sa_handler was being modified
without holding ->sighand->siglock.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Jonas Bonn <jonas@southpole.se>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/openrisc/kernel/signal.c | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/arch/openrisc/kernel/signal.c b/arch/openrisc/kernel/signal.c
index 5f759c7..c023db9 100644
--- a/arch/openrisc/kernel/signal.c
+++ b/arch/openrisc/kernel/signal.c
@@ -257,9 +257,7 @@ static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
return;
give_sigsegv:
- if (sig == SIGSEGV)
- ka->sa.sa_handler = SIG_DFL;
- force_sig(SIGSEGV, current);
+ force_sigsegv(sig, current);
}
static inline void
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* Re: [PATCH 18/41] OpenRISC: Don't reimplement force_sigsegv()
2011-08-11 13:56 ` [PATCH 18/41] OpenRISC: Don't reimplement force_sigsegv() Matt Fleming
@ 2011-08-16 16:49 ` Oleg Nesterov
2011-08-16 19:33 ` Matt Fleming
0 siblings, 1 reply; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-16 16:49 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-kernel, Jonas Bonn, Arnd Bergmann
On 08/11, Matt Fleming wrote:
>
> Instead of open coding the sequence from force_sigsegv() just call
> it. This also fixes a race because sa_handler was being modified
> without holding ->sighand->siglock.
>
> --- a/arch/openrisc/kernel/signal.c
> +++ b/arch/openrisc/kernel/signal.c
> @@ -257,9 +257,7 @@ static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
> return;
>
> give_sigsegv:
> - if (sig == SIGSEGV)
> - ka->sa.sa_handler = SIG_DFL;
> - force_sig(SIGSEGV, current);
> + force_sigsegv(sig, current);
> }
Agreed, but...
I don't really understand the changelog, which race this patch fix?
Yes, we shouldn't change sa_handler lockless, this "breaks the rules"
but I do not see any immediate problem. And since force_sigsegv() drops
the lock after setting SIG_DFL we can "race" with the sub-thread anyway.
Hmm. Looking more, I think that this patch is not the cleanup, but the
bugfix. The current code is simply wrong, it plays with ka, and it points
to the _copy_ of sighand->action[], so this code is simply pointless.
Unless I missed something, could you fix the changelog and resend?
Oleg.
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 18/41] OpenRISC: Don't reimplement force_sigsegv()
2011-08-16 16:49 ` Oleg Nesterov
@ 2011-08-16 19:33 ` Matt Fleming
2011-08-18 17:47 ` Oleg Nesterov
0 siblings, 1 reply; 87+ messages in thread
From: Matt Fleming @ 2011-08-16 19:33 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Jonas Bonn, Arnd Bergmann
On Tue, 2011-08-16 at 18:49 +0200, Oleg Nesterov wrote:
> On 08/11, Matt Fleming wrote:
> >
> > Instead of open coding the sequence from force_sigsegv() just call
> > it. This also fixes a race because sa_handler was being modified
> > without holding ->sighand->siglock.
> >
> > --- a/arch/openrisc/kernel/signal.c
> > +++ b/arch/openrisc/kernel/signal.c
> > @@ -257,9 +257,7 @@ static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
> > return;
> >
> > give_sigsegv:
> > - if (sig == SIGSEGV)
> > - ka->sa.sa_handler = SIG_DFL;
> > - force_sig(SIGSEGV, current);
> > + force_sigsegv(sig, current);
> > }
>
> Agreed, but...
>
> I don't really understand the changelog, which race this patch fix?
>
> Yes, we shouldn't change sa_handler lockless, this "breaks the rules"
> but I do not see any immediate problem. And since force_sigsegv() drops
> the lock after setting SIG_DFL we can "race" with the sub-thread anyway.
Argh, yeah this and the rest of patches that try to fixup 'ka' are
wrong, because like you say, we're operating on a copy on the stack, so
there's no race. I missed that :-(
I did notice that race in force_sigsegv() too, is it a real problem? It
certainly looked to me like it could be a real problem.
> Hmm. Looking more, I think that this patch is not the cleanup, but the
> bugfix. The current code is simply wrong, it plays with ka, and it points
> to the _copy_ of sighand->action[], so this code is simply pointless.
Yeah, I will make that more explicit in the changelog.
> Unless I missed something, could you fix the changelog and resend?
Nope you haven't missed anything, you're spot on. I will resend.
--
Matt Fleming, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 18/41] OpenRISC: Don't reimplement force_sigsegv()
2011-08-16 19:33 ` Matt Fleming
@ 2011-08-18 17:47 ` Oleg Nesterov
0 siblings, 0 replies; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-18 17:47 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-kernel, Jonas Bonn, Arnd Bergmann
On 08/16, Matt Fleming wrote:
>
> On Tue, 2011-08-16 at 18:49 +0200, Oleg Nesterov wrote:
> >
> > And since force_sigsegv() drops
> > the lock after setting SIG_DFL we can "race" with the sub-thread anyway.
>
> I did notice that race in force_sigsegv() too, is it a real problem?
Oh, I don't really know. I mean, I do not know if this really needs
the fix.
OK, suppose that another thread does signal(SIGSEGV, SIG_IGN) in
between. This probably means it asks for the problems anyway. and
we can pretend this was done before this SIGSEGV was dequeued.
If it does signal(SIGSEGV, my_handler), then most probably
force_sigsegv() will be called again soon, after dequeueing SIGSEGV.
Oleg.
^ permalink raw reply [flat|nested] 87+ messages in thread
* [PATCH 19/41] OpenRISC: No need to reset handler if SA_ONESHOT
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (17 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 18/41] OpenRISC: Don't reimplement force_sigsegv() Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-16 16:53 ` Oleg Nesterov
2011-08-11 13:56 ` [PATCH 20/41] OpenRISC: Don't mask signals if we fail to setup signal stack Matt Fleming
` (23 subsequent siblings)
42 siblings, 1 reply; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Jonas Bonn, Arnd Bergmann
From: Matt Fleming <matt.fleming@intel.com>
get_signal_to_deliver() already resets the signal handler if
SA_ONESHOT is set in ka->sa.sa_flags, there's no need to do it again
in handle_signal(). Furthermore, because we were modifying
ka->sa.sa_handler without holding ->sighand->siglock there's the
potential we can race with another thread if it modifies the signal
handlers.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Jonas Bonn <jonas@southpole.se>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/openrisc/kernel/signal.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/arch/openrisc/kernel/signal.c b/arch/openrisc/kernel/signal.c
index c023db9..2f75e74 100644
--- a/arch/openrisc/kernel/signal.c
+++ b/arch/openrisc/kernel/signal.c
@@ -267,9 +267,6 @@ handle_signal(unsigned long sig,
{
setup_rt_frame(sig, ka, info, oldset, regs);
- if (ka->sa.sa_flags & SA_ONESHOT)
- ka->sa.sa_handler = SIG_DFL;
-
spin_lock_irq(¤t->sighand->siglock);
sigorsets(¤t->blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* Re: [PATCH 19/41] OpenRISC: No need to reset handler if SA_ONESHOT
2011-08-11 13:56 ` [PATCH 19/41] OpenRISC: No need to reset handler if SA_ONESHOT Matt Fleming
@ 2011-08-16 16:53 ` Oleg Nesterov
0 siblings, 0 replies; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-16 16:53 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-kernel, Jonas Bonn, Arnd Bergmann
On 08/11, Matt Fleming wrote:
>
> get_signal_to_deliver() already resets the signal handler if
> SA_ONESHOT is set in ka->sa.sa_flags,
Yes.
> there's no need to do it again
> in handle_signal().
Again, this code in fact does nothing, it plays with the copy.
> Furthermore, because we were modifying
> ka->sa.sa_handler without holding ->sighand->siglock there's the
> potential we can race with another thread if it modifies the signal
> handlers.
and thus this is not true.
Afaics, the changelog should be fixed but the patch is fine ;)
Oleg.
^ permalink raw reply [flat|nested] 87+ messages in thread
* [PATCH 20/41] OpenRISC: Don't mask signals if we fail to setup signal stack
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (18 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 19/41] OpenRISC: No need to reset handler if SA_ONESHOT Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:56 ` [PATCH 21/41] OpenRISC: Use set_current_blocked() Matt Fleming
` (22 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Jonas Bonn, Arnd Bergmann
From: Matt Fleming <matt.fleming@intel.com>
setup_rt_frame() needs to return an indication of whether it succeeded
or failed in setting up the signal stack frame. If setup_rt_frame()
fails then we must not modify current->blocked.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Jonas Bonn <jonas@southpole.se>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/openrisc/kernel/signal.c | 29 ++++++++++++++++++-----------
1 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/arch/openrisc/kernel/signal.c b/arch/openrisc/kernel/signal.c
index 2f75e74..b0bdfda 100644
--- a/arch/openrisc/kernel/signal.c
+++ b/arch/openrisc/kernel/signal.c
@@ -196,8 +196,8 @@ static inline void __user *get_sigframe(struct k_sigaction *ka,
* trampoline which performs the syscall sigreturn, or a provided
* user-mode trampoline.
*/
-static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
- sigset_t *set, struct pt_regs *regs)
+static int setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
+ sigset_t *set, struct pt_regs *regs)
{
struct rt_sigframe *frame;
unsigned long return_ip;
@@ -254,18 +254,23 @@ static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
/* actually move the usp to reflect the stacked frame */
regs->sp = (unsigned long)frame;
- return;
+ return 0;
give_sigsegv:
force_sigsegv(sig, current);
+ return -EFAULT;
}
-static inline void
+static inline int
handle_signal(unsigned long sig,
siginfo_t *info, struct k_sigaction *ka,
sigset_t *oldset, struct pt_regs *regs)
{
- setup_rt_frame(sig, ka, info, oldset, regs);
+ int ret;
+
+ ret = setup_rt_frame(sig, ka, info, oldset, regs);
+ if (ret)
+ return ret;
spin_lock_irq(¤t->sighand->siglock);
sigorsets(¤t->blocked, ¤t->blocked, &ka->sa.sa_mask);
@@ -274,6 +279,8 @@ handle_signal(unsigned long sig,
recalc_sigpending();
spin_unlock_irq(¤t->sighand->siglock);
+
+ return 0;
}
/*
@@ -362,13 +369,13 @@ void do_signal(struct pt_regs *regs)
oldset = ¤t->blocked;
/* Whee! Actually deliver the signal. */
- handle_signal(signr, &info, &ka, oldset, regs);
- /* a signal was successfully delivered; the saved
- * sigmask will have been stored in the signal frame,
- * and will be restored by sigreturn, so we can simply
- * clear the TIF_RESTORE_SIGMASK flag */
- if (test_thread_flag(TIF_RESTORE_SIGMASK))
+ if (!handle_signal(signr, &info, &ka, oldset, regs)) {
+ /* a signal was successfully delivered; the saved
+ * sigmask will have been stored in the signal frame,
+ * and will be restored by sigreturn, so we can simply
+ * clear the TIF_RESTORE_SIGMASK flag */
clear_thread_flag(TIF_RESTORE_SIGMASK);
+ }
tracehook_signal_handler(signr, &info, &ka, regs,
test_thread_flag(TIF_SINGLESTEP));
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 21/41] OpenRISC: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (19 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 20/41] OpenRISC: Don't mask signals if we fail to setup signal stack Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:56 ` [PATCH 22/41] parisc: " Matt Fleming
` (21 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Jonas Bonn, Arnd Bergmann
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Jonas Bonn <jonas@southpole.se>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/openrisc/kernel/signal.c | 15 +++++----------
1 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/arch/openrisc/kernel/signal.c b/arch/openrisc/kernel/signal.c
index b0bdfda..50a5156 100644
--- a/arch/openrisc/kernel/signal.c
+++ b/arch/openrisc/kernel/signal.c
@@ -108,10 +108,7 @@ asmlinkage long _sys_rt_sigreturn(struct pt_regs *regs)
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(regs, &frame->uc.uc_mcontext))
goto badframe;
@@ -266,19 +263,17 @@ handle_signal(unsigned long sig,
siginfo_t *info, struct k_sigaction *ka,
sigset_t *oldset, struct pt_regs *regs)
{
+ sigset_t blocked;
int ret;
ret = setup_rt_frame(sig, ka, info, oldset, regs);
if (ret)
return ret;
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked, ¤t->blocked, &ka->sa.sa_mask);
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked, sig);
- recalc_sigpending();
-
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
return 0;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 22/41] parisc: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (20 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 21/41] OpenRISC: Use set_current_blocked() Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:56 ` [PATCH 23/41] powerpc: " Matt Fleming
` (20 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov
Cc: linux-kernel, Kyle McMartin, Helge Deller, James E.J. Bottomley
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Kyle McMartin <kyle@mcmartin.ca>
Cc: Helge Deller <deller@gmx.de>
Cc: "James E.J. Bottomley" <jejb@parisc-linux.org>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/parisc/kernel/signal.c | 15 ++++++---------
1 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/arch/parisc/kernel/signal.c b/arch/parisc/kernel/signal.c
index 12c1ed3..4d64f72 100644
--- a/arch/parisc/kernel/signal.c
+++ b/arch/parisc/kernel/signal.c
@@ -131,10 +131,7 @@ sys_rt_sigreturn(struct pt_regs *regs, int in_syscall)
}
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
/* Good thing we saved the old gr[30], eh? */
#ifdef CONFIG_64BIT
@@ -447,6 +444,8 @@ static long
handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka,
sigset_t *oldset, struct pt_regs *regs, int in_syscall)
{
+ sigset_t blocked;
+
DBG(1,"handle_signal: sig=%ld, ka=%p, info=%p, oldset=%p, regs=%p\n",
sig, ka, info, oldset, regs);
@@ -454,12 +453,10 @@ handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka,
if (!setup_rt_frame(sig, ka, info, oldset, regs, in_syscall))
return 0;
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked,¤t->blocked,&ka->sa.sa_mask);
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked,sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
tracehook_signal_handler(sig, info, ka, regs,
test_thread_flag(TIF_SINGLESTEP) ||
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 23/41] powerpc: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (21 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 22/41] parisc: " Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:56 ` [PATCH 24/41] score: Don't mask signals if we fail to setup signal stack Matt Fleming
` (19 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Benjamin Herrenschmidt, Paul Mackerras
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/powerpc/kernel/signal.c | 16 ++++++----------
arch/powerpc/kernel/signal_32.c | 11 ++++++-----
2 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index 2300426..8f6731f 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -56,10 +56,7 @@ void __user * get_sigframe(struct k_sigaction *ka, struct pt_regs *regs,
void restore_sigmask(sigset_t *set)
{
sigdelsetmask(set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = *set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(set);
}
static void check_syscall_restart(struct pt_regs *regs, struct k_sigaction *ka,
@@ -167,13 +164,12 @@ static int do_signal_pending(sigset_t *oldset, struct pt_regs *regs)
regs->trap = 0;
if (ret) {
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked, ¤t->blocked,
- &ka.sa.sa_mask);
+ sigset_t blocked;
+
+ sigorsets(&blocked, ¤t->blocked, &ka.sa.sa_mask);
if (!(ka.sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked, signr);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, signr);
+ set_current_blocked(&blocked);
/*
* A signal was successfully delivered; the saved sigmask is in
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 78b76dc..5fc9d19 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -242,12 +242,13 @@ static inline int restore_general_regs(struct pt_regs *regs,
*/
long sys_sigsuspend(old_sigset_t mask)
{
- mask &= _BLOCKABLE;
- spin_lock_irq(¤t->sighand->siglock);
+ sigset_t blocked;
+
current->saved_sigmask = current->blocked;
- siginitset(¤t->blocked, mask);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+
+ mask &= _BLOCKABLE;
+ siginitset(&blocked, mask);
+ set_current_blocked(&blocked);
current->state = TASK_INTERRUPTIBLE;
schedule();
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 24/41] score: Don't mask signals if we fail to setup signal stack
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (22 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 23/41] powerpc: " Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:56 ` [PATCH 25/41] score: Use set_current_blocked() Matt Fleming
` (18 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Chen Liqin, Lennox Wu
From: Matt Fleming <matt.fleming@intel.com>
If setup_rt_frame() returns -EFAULT then we must not block any signals
in the current process.
Cc: Chen Liqin <liqin.chen@sunplusct.com>
Cc: Lennox Wu <lennox.wu@gmail.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/score/kernel/signal.c | 14 ++++++++------
1 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/arch/score/kernel/signal.c b/arch/score/kernel/signal.c
index aa57440..bf9e33e 100644
--- a/arch/score/kernel/signal.c
+++ b/arch/score/kernel/signal.c
@@ -272,12 +272,14 @@ static int handle_signal(unsigned long sig, siginfo_t *info,
*/
ret = setup_rt_frame(ka, regs, sig, oldset, info);
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked, ¤t->blocked, &ka->sa.sa_mask);
- if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked, sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ if (ret == 0) {
+ spin_lock_irq(¤t->sighand->siglock);
+ sigorsets(¤t->blocked, ¤t->blocked, &ka->sa.sa_mask);
+ if (!(ka->sa.sa_flags & SA_NODEFER))
+ sigaddset(¤t->blocked, sig);
+ recalc_sigpending();
+ spin_unlock_irq(¤t->sighand->siglock);
+ }
return ret;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 25/41] score: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (23 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 24/41] score: Don't mask signals if we fail to setup signal stack Matt Fleming
@ 2011-08-11 13:56 ` Matt Fleming
2011-08-11 13:57 ` [PATCH 26/41] sh: No need to reset handler if SA_ONESHOT Matt Fleming
` (17 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Chen Liqin, Lennox Wu
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Chen Liqin <liqin.chen@sunplusct.com>
Cc: Lennox Wu <lennox.wu@gmail.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/score/kernel/signal.c | 15 ++++++---------
1 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/arch/score/kernel/signal.c b/arch/score/kernel/signal.c
index bf9e33e..3ba0298 100644
--- a/arch/score/kernel/signal.c
+++ b/arch/score/kernel/signal.c
@@ -159,10 +159,7 @@ score_rt_sigreturn(struct pt_regs *regs)
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
sig = restore_sigcontext(regs, &frame->rs_uc.uc_mcontext);
if (sig < 0)
@@ -273,12 +270,12 @@ static int handle_signal(unsigned long sig, siginfo_t *info,
ret = setup_rt_frame(ka, regs, sig, oldset, info);
if (ret == 0) {
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked, ¤t->blocked, &ka->sa.sa_mask);
+ sigset_t blocked;
+
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked, sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
}
return ret;
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 26/41] sh: No need to reset handler if SA_ONESHOT
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (24 preceding siblings ...)
2011-08-11 13:56 ` [PATCH 25/41] score: Use set_current_blocked() Matt Fleming
@ 2011-08-11 13:57 ` Matt Fleming
2011-08-16 17:25 ` Oleg Nesterov
2011-08-11 13:57 ` [PATCH 27/41] sh: Use set_current_blocked() Matt Fleming
` (16 subsequent siblings)
42 siblings, 1 reply; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:57 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Paul Mundt
From: Matt Fleming <matt.fleming@intel.com>
get_signal_to_deliver() already resets the signal handler if
SA_ONESHOT is set in ka->sa.sa_flags, there's no need to do it again
in handle_signal(). Furthermore, because we were modifying
ka->sa.sa_handler without holding ->sighand->siglock there's the
potential we can race with another thread if it modifies the signal
handlers.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Paul Mundt <lethal@linux-sh.org>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/sh/kernel/signal_32.c | 3 ---
arch/sh/kernel/signal_64.c | 3 ---
2 files changed, 0 insertions(+), 6 deletions(-)
diff --git a/arch/sh/kernel/signal_32.c b/arch/sh/kernel/signal_32.c
index 579cd2c..7a00304 100644
--- a/arch/sh/kernel/signal_32.c
+++ b/arch/sh/kernel/signal_32.c
@@ -548,9 +548,6 @@ handle_signal(unsigned long sig, struct k_sigaction *ka, siginfo_t *info,
else
ret = setup_frame(sig, ka, oldset, regs);
- if (ka->sa.sa_flags & SA_ONESHOT)
- ka->sa.sa_handler = SIG_DFL;
-
if (ret == 0) {
spin_lock_irq(¤t->sighand->siglock);
sigorsets(¤t->blocked,¤t->blocked,&ka->sa.sa_mask);
diff --git a/arch/sh/kernel/signal_64.c b/arch/sh/kernel/signal_64.c
index 5a9f1f1..c0e7d10 100644
--- a/arch/sh/kernel/signal_64.c
+++ b/arch/sh/kernel/signal_64.c
@@ -738,9 +738,6 @@ handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka,
else
ret = setup_frame(sig, ka, oldset, regs);
- if (ka->sa.sa_flags & SA_ONESHOT)
- ka->sa.sa_handler = SIG_DFL;
-
if (ret == 0) {
spin_lock_irq(¤t->sighand->siglock);
sigorsets(¤t->blocked,¤t->blocked,&ka->sa.sa_mask);
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* Re: [PATCH 26/41] sh: No need to reset handler if SA_ONESHOT
2011-08-11 13:57 ` [PATCH 26/41] sh: No need to reset handler if SA_ONESHOT Matt Fleming
@ 2011-08-16 17:25 ` Oleg Nesterov
0 siblings, 0 replies; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-16 17:25 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-kernel, Paul Mundt
On 08/11, Matt Fleming wrote:
>
> get_signal_to_deliver() already resets the signal handler if
> SA_ONESHOT is set in ka->sa.sa_flags, there's no need to do it again
> in handle_signal(). Furthermore, because we were modifying
> ka->sa.sa_handler without holding ->sighand->siglock there's the
> potential we can race with another thread if it modifies the signal
> handlers.
Again ;) the patch is fine, the changelog is not right.
Oleg.
^ permalink raw reply [flat|nested] 87+ messages in thread
* [PATCH 27/41] sh: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (25 preceding siblings ...)
2011-08-11 13:57 ` [PATCH 26/41] sh: No need to reset handler if SA_ONESHOT Matt Fleming
@ 2011-08-11 13:57 ` Matt Fleming
2011-08-11 13:57 ` [PATCH 28/41] sparc: " Matt Fleming
` (15 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:57 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Paul Mundt
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Paul Mundt <lethal@linux-sh.org>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/sh/kernel/signal_32.c | 32 +++++++++++++-------------------
arch/sh/kernel/signal_64.c | 37 +++++++++++++------------------------
2 files changed, 26 insertions(+), 43 deletions(-)
diff --git a/arch/sh/kernel/signal_32.c b/arch/sh/kernel/signal_32.c
index 7a00304..c97705f 100644
--- a/arch/sh/kernel/signal_32.c
+++ b/arch/sh/kernel/signal_32.c
@@ -58,12 +58,13 @@ sys_sigsuspend(old_sigset_t mask,
unsigned long r5, unsigned long r6, unsigned long r7,
struct pt_regs __regs)
{
- mask &= _BLOCKABLE;
- spin_lock_irq(¤t->sighand->siglock);
+ sigset_t blocked;
+
current->saved_sigmask = current->blocked;
- siginitset(¤t->blocked, mask);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+
+ mask &= _BLOCKABLE;
+ siginitset(&blocked, mask);
+ set_current_blocked(&blocked);
current->state = TASK_INTERRUPTIBLE;
schedule();
@@ -240,11 +241,7 @@ asmlinkage int sys_sigreturn(unsigned long r4, unsigned long r5,
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
-
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(regs, &frame->sc, &r0))
goto badframe;
@@ -274,10 +271,7 @@ asmlinkage int sys_rt_sigreturn(unsigned long r4, unsigned long r5,
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(regs, &frame->uc.uc_mcontext, &r0))
goto badframe;
@@ -549,12 +543,12 @@ handle_signal(unsigned long sig, struct k_sigaction *ka, siginfo_t *info,
ret = setup_frame(sig, ka, oldset, regs);
if (ret == 0) {
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked,¤t->blocked,&ka->sa.sa_mask);
+ sigset_t blocked;
+
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked,sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
}
return ret;
diff --git a/arch/sh/kernel/signal_64.c b/arch/sh/kernel/signal_64.c
index c0e7d10..14d5ed4 100644
--- a/arch/sh/kernel/signal_64.c
+++ b/arch/sh/kernel/signal_64.c
@@ -163,14 +163,13 @@ sys_sigsuspend(old_sigset_t mask,
unsigned long r6, unsigned long r7,
struct pt_regs * regs)
{
- sigset_t saveset;
+ sigset_t saveset, blocked;
- mask &= _BLOCKABLE;
- spin_lock_irq(¤t->sighand->siglock);
saveset = current->blocked;
- siginitset(¤t->blocked, mask);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+
+ mask &= _BLOCKABLE;
+ siginitset(&blocked, mask);
+ set_current_blocked(&blocked);
REF_REG_RET = -EINTR;
while (1) {
@@ -202,11 +201,8 @@ sys_rt_sigsuspend(sigset_t *unewset, size_t sigsetsize,
if (copy_from_user(&newset, unewset, sizeof(newset)))
return -EFAULT;
sigdelsetmask(&newset, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
saveset = current->blocked;
- current->blocked = newset;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&newset);
REF_REG_RET = -EINTR;
while (1) {
@@ -412,11 +408,7 @@ asmlinkage int sys_sigreturn(unsigned long r2, unsigned long r3,
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
-
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(regs, &frame->sc, &ret))
goto badframe;
@@ -449,10 +441,7 @@ asmlinkage int sys_rt_sigreturn(unsigned long r2, unsigned long r3,
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(regs, &frame->uc.uc_mcontext, &ret))
goto badframe;
@@ -739,12 +728,12 @@ handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka,
ret = setup_frame(sig, ka, oldset, regs);
if (ret == 0) {
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked,¤t->blocked,&ka->sa.sa_mask);
+ sigset_t blocked;
+
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked,sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
}
return ret;
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 28/41] sparc: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (26 preceding siblings ...)
2011-08-11 13:57 ` [PATCH 27/41] sh: Use set_current_blocked() Matt Fleming
@ 2011-08-11 13:57 ` Matt Fleming
2011-08-11 13:57 ` [PATCH 29/41] tile: " Matt Fleming
` (14 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:57 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, David S. Miller
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/sparc/kernel/signal32.c | 19 ++++++-------------
arch/sparc/kernel/signal_32.c | 30 ++++++++++++------------------
arch/sparc/kernel/signal_64.c | 30 ++++++++++++------------------
3 files changed, 30 insertions(+), 49 deletions(-)
diff --git a/arch/sparc/kernel/signal32.c b/arch/sparc/kernel/signal32.c
index 75fad42..4f381ce 100644
--- a/arch/sparc/kernel/signal32.c
+++ b/arch/sparc/kernel/signal32.c
@@ -287,10 +287,7 @@ void do_sigreturn32(struct pt_regs *regs)
case 1: set.sig[0] = seta[0] + (((long)seta[1]) << 32);
}
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
return;
segv:
@@ -383,10 +380,7 @@ asmlinkage void do_rt_sigreturn32(struct pt_regs *regs)
case 1: set.sig[0] = seta.sig[0] + (((long)seta.sig[1]) << 32);
}
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
return;
segv:
force_sig(SIGSEGV, current);
@@ -756,6 +750,7 @@ static inline int handle_signal32(unsigned long signr, struct k_sigaction *ka,
siginfo_t *info,
sigset_t *oldset, struct pt_regs *regs)
{
+ sigset_t blocked;
int err;
if (ka->sa.sa_flags & SA_SIGINFO)
@@ -766,12 +761,10 @@ static inline int handle_signal32(unsigned long signr, struct k_sigaction *ka,
if (err)
return err;
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked,¤t->blocked,&ka->sa.sa_mask);
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NOMASK))
- sigaddset(¤t->blocked,signr);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, signr);
+ set_current_blocked(&blocked);
tracehook_signal_handler(signr, info, ka, regs, 0);
diff --git a/arch/sparc/kernel/signal_32.c b/arch/sparc/kernel/signal_32.c
index 5e5c5fd..d65c31d 100644
--- a/arch/sparc/kernel/signal_32.c
+++ b/arch/sparc/kernel/signal_32.c
@@ -60,12 +60,13 @@ struct rt_signal_frame {
static int _sigpause_common(old_sigset_t set)
{
- set &= _BLOCKABLE;
- spin_lock_irq(¤t->sighand->siglock);
+ sigset_t blocked;
+
current->saved_sigmask = current->blocked;
- siginitset(¤t->blocked, set);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+
+ set &= _BLOCKABLE;
+ siginitset(&blocked, set);
+ set_current_blocked(&blocked);
current->state = TASK_INTERRUPTIBLE;
schedule();
@@ -165,10 +166,7 @@ asmlinkage void do_sigreturn(struct pt_regs *regs)
goto segv_and_exit;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
return;
segv_and_exit:
@@ -229,10 +227,7 @@ asmlinkage void do_rt_sigreturn(struct pt_regs *regs)
set_fs(old_fs);
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
return;
segv:
force_sig(SIGSEGV, current);
@@ -484,6 +479,7 @@ static inline int
handle_signal(unsigned long signr, struct k_sigaction *ka,
siginfo_t *info, sigset_t *oldset, struct pt_regs *regs)
{
+ sigset_t blocked;
int err;
if (ka->sa.sa_flags & SA_SIGINFO)
@@ -494,12 +490,10 @@ handle_signal(unsigned long signr, struct k_sigaction *ka,
if (err)
return err;
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked,¤t->blocked,&ka->sa.sa_mask);
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NOMASK))
- sigaddset(¤t->blocked, signr);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, signr);
+ set_current_blocked(&blocked);
tracehook_signal_handler(signr, info, ka, regs, 0);
diff --git a/arch/sparc/kernel/signal_64.c b/arch/sparc/kernel/signal_64.c
index 006fe45..bb153d1 100644
--- a/arch/sparc/kernel/signal_64.c
+++ b/arch/sparc/kernel/signal_64.c
@@ -69,10 +69,7 @@ asmlinkage void sparc64_set_context(struct pt_regs *regs)
goto do_sigsegv;
}
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
}
if (test_thread_flag(TIF_32BIT)) {
pc &= 0xffffffff;
@@ -241,12 +238,13 @@ struct rt_signal_frame {
static long _sigpause_common(old_sigset_t set)
{
- set &= _BLOCKABLE;
- spin_lock_irq(¤t->sighand->siglock);
+ sigset_t blocked;
+
current->saved_sigmask = current->blocked;
- siginitset(¤t->blocked, set);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+
+ set &= _BLOCKABLE;
+ siginitset(&blocked, set);
+ set_current_blocked(&set);
current->state = TASK_INTERRUPTIBLE;
schedule();
@@ -341,10 +339,7 @@ void do_rt_sigreturn(struct pt_regs *regs)
pt_regs_clear_syscall(regs);
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
return;
segv:
force_sig(SIGSEGV, current);
@@ -498,18 +493,17 @@ static inline int handle_signal(unsigned long signr, struct k_sigaction *ka,
siginfo_t *info,
sigset_t *oldset, struct pt_regs *regs)
{
+ sigset_t blocked;
int err;
err = setup_rt_frame(ka, regs, signr, oldset,
(ka->sa.sa_flags & SA_SIGINFO) ? info : NULL);
if (err)
return err;
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked,¤t->blocked,&ka->sa.sa_mask);
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NOMASK))
- sigaddset(¤t->blocked,signr);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, signr);
+ set_current_blocked(&blocked);
tracehook_signal_handler(signr, info, ka, regs, 0);
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 29/41] tile: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (27 preceding siblings ...)
2011-08-11 13:57 ` [PATCH 28/41] sparc: " Matt Fleming
@ 2011-08-11 13:57 ` Matt Fleming
2011-08-11 17:10 ` Chris Metcalf
2011-08-11 13:57 ` [PATCH 30/41] um: " Matt Fleming
` (13 subsequent siblings)
42 siblings, 1 reply; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:57 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Chris Metcalf, Arnd Bergmann
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Chris Metcalf <cmetcalf@tilera.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/tile/kernel/compat_signal.c | 5 +----
arch/tile/kernel/signal.c | 16 ++++++----------
2 files changed, 7 insertions(+), 14 deletions(-)
diff --git a/arch/tile/kernel/compat_signal.c b/arch/tile/kernel/compat_signal.c
index a7869ad..77763cc 100644
--- a/arch/tile/kernel/compat_signal.c
+++ b/arch/tile/kernel/compat_signal.c
@@ -303,10 +303,7 @@ long compat_sys_rt_sigreturn(struct pt_regs *regs)
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(regs, &frame->uc.uc_mcontext))
goto badframe;
diff --git a/arch/tile/kernel/signal.c b/arch/tile/kernel/signal.c
index bedaf4e..6ddceec 100644
--- a/arch/tile/kernel/signal.c
+++ b/arch/tile/kernel/signal.c
@@ -97,10 +97,7 @@ SYSCALL_DEFINE1(rt_sigreturn, struct pt_regs *, regs)
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(regs, &frame->uc.uc_mcontext))
goto badframe;
@@ -286,13 +283,12 @@ static int handle_signal(unsigned long sig, siginfo_t *info,
* the work_pending path in the return-to-user code, and
* either way we can re-enable interrupts unconditionally.
*/
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked,
- ¤t->blocked, &ka->sa.sa_mask);
+ sigset_t blocked;
+
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked, sig);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
}
return ret;
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* Re: [PATCH 29/41] tile: Use set_current_blocked()
2011-08-11 13:57 ` [PATCH 29/41] tile: " Matt Fleming
@ 2011-08-11 17:10 ` Chris Metcalf
0 siblings, 0 replies; 87+ messages in thread
From: Chris Metcalf @ 2011-08-11 17:10 UTC (permalink / raw)
To: Matt Fleming; +Cc: Oleg Nesterov, linux-kernel, Arnd Bergmann
On 8/11/2011 9:57 AM, Matt Fleming wrote:
> From: Matt Fleming <matt.fleming@intel.com>
>
> As described in e6fa16ab ("signal: sigprocmask() should do
> retarget_shared_pending()") the modification of current->blocked is
> incorrect as we need to check whether the signal we're about to block
> is pending in the shared queue.
>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Chris Metcalf <cmetcalf@tilera.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
> arch/tile/kernel/compat_signal.c | 5 +----
> arch/tile/kernel/signal.c | 16 ++++++----------
> 2 files changed, 7 insertions(+), 14 deletions(-)
Acked-by: Chris Metcalf <cmetcalf@tilera.com>
--
Chris Metcalf, Tilera Corp.
http://www.tilera.com
^ permalink raw reply [flat|nested] 87+ messages in thread
* [PATCH 30/41] um: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (28 preceding siblings ...)
2011-08-11 13:57 ` [PATCH 29/41] tile: " Matt Fleming
@ 2011-08-11 13:57 ` Matt Fleming
2011-08-11 13:57 ` [PATCH 31/41] um: Don't restore current->blocked on error Matt Fleming
` (12 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:57 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Richard Weinberger
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Richard Weinberger <richard@nod.at>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/um/kernel/signal.c | 22 +++++++++++-----------
arch/um/sys-i386/signal.c | 12 ++----------
arch/um/sys-x86_64/signal.c | 6 +-----
3 files changed, 14 insertions(+), 26 deletions(-)
diff --git a/arch/um/kernel/signal.c b/arch/um/kernel/signal.c
index b5c094c..1812524 100644
--- a/arch/um/kernel/signal.c
+++ b/arch/um/kernel/signal.c
@@ -73,13 +73,12 @@ static int handle_signal(struct pt_regs *regs, unsigned long signr,
spin_unlock_irq(¤t->sighand->siglock);
force_sigsegv(signr, current);
} else {
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked, ¤t->blocked,
- &ka->sa.sa_mask);
+ sigset_t blocked:
+
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked, signr);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, signr);
+ set_current_blocked(&blocked);
}
return err;
@@ -163,12 +162,13 @@ int do_signal(void)
*/
long sys_sigsuspend(int history0, int history1, old_sigset_t mask)
{
- mask &= _BLOCKABLE;
- spin_lock_irq(¤t->sighand->siglock);
+ sigset_t blocked;
+
current->saved_sigmask = current->blocked;
- siginitset(¤t->blocked, mask);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+
+ mask &= _BLOCKABLE;
+ siginitset(&blocked, mask);
+ set_current_blocked(&blocked);
current->state = TASK_INTERRUPTIBLE;
schedule();
diff --git a/arch/um/sys-i386/signal.c b/arch/um/sys-i386/signal.c
index 89a4662..4403b48 100644
--- a/arch/um/sys-i386/signal.c
+++ b/arch/um/sys-i386/signal.c
@@ -458,11 +458,7 @@ long sys_sigreturn(struct pt_regs regs)
goto segfault;
sigdelsetmask(&set, ~_BLOCKABLE);
-
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (copy_sc_from_user(¤t->thread.regs, sc))
goto segfault;
@@ -489,11 +485,7 @@ long sys_rt_sigreturn(struct pt_regs regs)
goto segfault;
sigdelsetmask(&set, ~_BLOCKABLE);
-
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (copy_sc_from_user(¤t->thread.regs, &uc->uc_mcontext))
goto segfault;
diff --git a/arch/um/sys-x86_64/signal.c b/arch/um/sys-x86_64/signal.c
index b6b65c7..581b64d 100644
--- a/arch/um/sys-x86_64/signal.c
+++ b/arch/um/sys-x86_64/signal.c
@@ -270,11 +270,7 @@ long sys_rt_sigreturn(struct pt_regs *regs)
goto segfault;
sigdelsetmask(&set, ~_BLOCKABLE);
-
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (copy_sc_from_user(¤t->thread.regs, &uc->uc_mcontext,
&frame->fpstate))
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 31/41] um: Don't restore current->blocked on error
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (29 preceding siblings ...)
2011-08-11 13:57 ` [PATCH 30/41] um: " Matt Fleming
@ 2011-08-11 13:57 ` Matt Fleming
2011-08-16 17:38 ` Oleg Nesterov
2011-08-11 13:57 ` [PATCH 32/41] unicore32: Use set_current_blocked() Matt Fleming
` (11 subsequent siblings)
42 siblings, 1 reply; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:57 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Richard Weinberger
From: Matt Fleming <matt.fleming@intel.com>
If we fail to setup the signal stack frame then we don't need to
restore current->blocked because it is not modified by
setup_signal_stack_*.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Richard Weinberger <richard@nod.at>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/um/kernel/signal.c | 8 ++------
1 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/arch/um/kernel/signal.c b/arch/um/kernel/signal.c
index 1812524..d177685 100644
--- a/arch/um/kernel/signal.c
+++ b/arch/um/kernel/signal.c
@@ -66,13 +66,9 @@ static int handle_signal(struct pt_regs *regs, unsigned long signr,
#endif
err = setup_signal_stack_si(sp, signr, ka, regs, info, oldset);
- if (err) {
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = *oldset;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ if (err)
force_sigsegv(signr, current);
- } else {
+ else {
sigset_t blocked:
sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* Re: [PATCH 31/41] um: Don't restore current->blocked on error
2011-08-11 13:57 ` [PATCH 31/41] um: Don't restore current->blocked on error Matt Fleming
@ 2011-08-16 17:38 ` Oleg Nesterov
0 siblings, 0 replies; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-16 17:38 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-kernel, Richard Weinberger
On 08/11, Matt Fleming wrote:
>
> If we fail to setup the signal stack frame then we don't need to
> restore current->blocked because it is not modified by
> setup_signal_stack_*.
Yes. And this is simply wrong if TIF_RESTORE_SIGMASK. We can block
SIGSEGV which we need to dequeue.
ACK
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Richard Weinberger <richard@nod.at>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
> arch/um/kernel/signal.c | 8 ++------
> 1 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/arch/um/kernel/signal.c b/arch/um/kernel/signal.c
> index 1812524..d177685 100644
> --- a/arch/um/kernel/signal.c
> +++ b/arch/um/kernel/signal.c
> @@ -66,13 +66,9 @@ static int handle_signal(struct pt_regs *regs, unsigned long signr,
> #endif
> err = setup_signal_stack_si(sp, signr, ka, regs, info, oldset);
>
> - if (err) {
> - spin_lock_irq(¤t->sighand->siglock);
> - current->blocked = *oldset;
> - recalc_sigpending();
> - spin_unlock_irq(¤t->sighand->siglock);
> + if (err)
> force_sigsegv(signr, current);
> - } else {
> + else {
> sigset_t blocked:
>
> sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
> --
> 1.7.4.4
>
^ permalink raw reply [flat|nested] 87+ messages in thread
* [PATCH 32/41] unicore32: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (30 preceding siblings ...)
2011-08-11 13:57 ` [PATCH 31/41] um: Don't restore current->blocked on error Matt Fleming
@ 2011-08-11 13:57 ` Matt Fleming
2011-08-18 8:34 ` Guan Xuetao
2011-08-11 13:57 ` [PATCH 33/41] xtensa: Don't reimplement force_sigsegv() Matt Fleming
` (10 subsequent siblings)
42 siblings, 1 reply; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:57 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Guan Xuetao
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/unicore32/kernel/signal.c | 15 +++++----------
1 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/arch/unicore32/kernel/signal.c b/arch/unicore32/kernel/signal.c
index b163fca..911b549 100644
--- a/arch/unicore32/kernel/signal.c
+++ b/arch/unicore32/kernel/signal.c
@@ -63,10 +63,7 @@ static int restore_sigframe(struct pt_regs *regs, struct sigframe __user *sf)
err = __copy_from_user(&set, &sf->uc.uc_sigmask, sizeof(set));
if (err == 0) {
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
}
err |= __get_user(regs->UCreg_00, &sf->uc.uc_mcontext.regs.UCreg_00);
@@ -321,6 +318,7 @@ static int handle_signal(unsigned long sig, struct k_sigaction *ka,
{
struct thread_info *thread = current_thread_info();
struct task_struct *tsk = current;
+ sigset_t blocked;
int usig = sig;
int ret;
@@ -372,13 +370,10 @@ static int handle_signal(unsigned long sig, struct k_sigaction *ka,
/*
* Block the signal if we were successful.
*/
- spin_lock_irq(&tsk->sighand->siglock);
- sigorsets(&tsk->blocked, &tsk->blocked,
- &ka->sa.sa_mask);
+ sigorsets(&blocked, &tsk->blocked, &ka->sa.sa_mask);
if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(&tsk->blocked, sig);
- recalc_sigpending();
- spin_unlock_irq(&tsk->sighand->siglock);
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
return 0;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* Re: [PATCH 32/41] unicore32: Use set_current_blocked()
2011-08-11 13:57 ` [PATCH 32/41] unicore32: Use set_current_blocked() Matt Fleming
@ 2011-08-18 8:34 ` Guan Xuetao
0 siblings, 0 replies; 87+ messages in thread
From: Guan Xuetao @ 2011-08-18 8:34 UTC (permalink / raw)
To: Matt Fleming; +Cc: Oleg Nesterov, linux-kernel, arnd
It looks good to me.
And already tested by compilation.
Cc: Arnd Bergmann <arnd@arndb.de>
Acked-by: Guan Xuetao <gxt@mprc.pku.edu.cn>
Thanks & Regards.
Guan Xuetao
On Thu, 2011-08-11 at 14:57 +0100, Matt Fleming wrote:
> From: Matt Fleming <matt.fleming@intel.com>
>
> As described in e6fa16ab ("signal: sigprocmask() should do
> retarget_shared_pending()") the modification of current->blocked is
> incorrect as we need to check whether the signal we're about to block
> is pending in the shared queue.
>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
> arch/unicore32/kernel/signal.c | 15 +++++----------
> 1 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/arch/unicore32/kernel/signal.c b/arch/unicore32/kernel/signal.c
> index b163fca..911b549 100644
> --- a/arch/unicore32/kernel/signal.c
> +++ b/arch/unicore32/kernel/signal.c
> @@ -63,10 +63,7 @@ static int restore_sigframe(struct pt_regs *regs, struct sigframe __user *sf)
> err = __copy_from_user(&set, &sf->uc.uc_sigmask, sizeof(set));
> if (err == 0) {
> sigdelsetmask(&set, ~_BLOCKABLE);
> - spin_lock_irq(¤t->sighand->siglock);
> - current->blocked = set;
> - recalc_sigpending();
> - spin_unlock_irq(¤t->sighand->siglock);
> + set_current_blocked(&set);
> }
>
> err |= __get_user(regs->UCreg_00, &sf->uc.uc_mcontext.regs.UCreg_00);
> @@ -321,6 +318,7 @@ static int handle_signal(unsigned long sig, struct k_sigaction *ka,
> {
> struct thread_info *thread = current_thread_info();
> struct task_struct *tsk = current;
> + sigset_t blocked;
> int usig = sig;
> int ret;
>
> @@ -372,13 +370,10 @@ static int handle_signal(unsigned long sig, struct k_sigaction *ka,
> /*
> * Block the signal if we were successful.
> */
> - spin_lock_irq(&tsk->sighand->siglock);
> - sigorsets(&tsk->blocked, &tsk->blocked,
> - &ka->sa.sa_mask);
> + sigorsets(&blocked, &tsk->blocked, &ka->sa.sa_mask);
> if (!(ka->sa.sa_flags & SA_NODEFER))
> - sigaddset(&tsk->blocked, sig);
> - recalc_sigpending();
> - spin_unlock_irq(&tsk->sighand->siglock);
> + sigaddset(&blocked, sig);
> + set_current_blocked(&blocked);
>
> return 0;
> }
^ permalink raw reply [flat|nested] 87+ messages in thread
* [PATCH 33/41] xtensa: Don't reimplement force_sigsegv()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (31 preceding siblings ...)
2011-08-11 13:57 ` [PATCH 32/41] unicore32: Use set_current_blocked() Matt Fleming
@ 2011-08-11 13:57 ` Matt Fleming
2011-08-16 17:40 ` Oleg Nesterov
2011-08-11 13:57 ` [PATCH 34/41] xtensa: No need to reset handler if SA_ONESHOT Matt Fleming
` (9 subsequent siblings)
42 siblings, 1 reply; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:57 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Chris Zankel
From: Matt Fleming <matt.fleming@intel.com>
Instead of open coding the sequence from force_sigsegv() just call
it. This also fixes a race because sa_handler was being modified
without holding ->sighand->siglock.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Chris Zankel <chris@zankel.net>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/xtensa/kernel/signal.c | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/arch/xtensa/kernel/signal.c b/arch/xtensa/kernel/signal.c
index f2220b5..4f53770 100644
--- a/arch/xtensa/kernel/signal.c
+++ b/arch/xtensa/kernel/signal.c
@@ -425,9 +425,7 @@ static void setup_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
return;
give_sigsegv:
- if (sig == SIGSEGV)
- ka->sa.sa_handler = SIG_DFL;
- force_sig(SIGSEGV, current);
+ force_sigsegv(sig, current);
}
/*
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* Re: [PATCH 33/41] xtensa: Don't reimplement force_sigsegv()
2011-08-11 13:57 ` [PATCH 33/41] xtensa: Don't reimplement force_sigsegv() Matt Fleming
@ 2011-08-16 17:40 ` Oleg Nesterov
0 siblings, 0 replies; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-16 17:40 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-kernel, Chris Zankel
On 08/11, Matt Fleming wrote:
>
> Instead of open coding the sequence from force_sigsegv() just call
> it. This also fixes a race because sa_handler was being modified
> without holding ->sighand->siglock.
The same problem with the changelog in this and the next patch.
The patch itself looks nice.
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Chris Zankel <chris@zankel.net>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
> arch/xtensa/kernel/signal.c | 4 +---
> 1 files changed, 1 insertions(+), 3 deletions(-)
>
> diff --git a/arch/xtensa/kernel/signal.c b/arch/xtensa/kernel/signal.c
> index f2220b5..4f53770 100644
> --- a/arch/xtensa/kernel/signal.c
> +++ b/arch/xtensa/kernel/signal.c
> @@ -425,9 +425,7 @@ static void setup_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
> return;
>
> give_sigsegv:
> - if (sig == SIGSEGV)
> - ka->sa.sa_handler = SIG_DFL;
> - force_sig(SIGSEGV, current);
> + force_sigsegv(sig, current);
> }
>
> /*
> --
> 1.7.4.4
>
^ permalink raw reply [flat|nested] 87+ messages in thread
* [PATCH 34/41] xtensa: No need to reset handler if SA_ONESHOT
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (32 preceding siblings ...)
2011-08-11 13:57 ` [PATCH 33/41] xtensa: Don't reimplement force_sigsegv() Matt Fleming
@ 2011-08-11 13:57 ` Matt Fleming
2011-08-11 13:57 ` [PATCH 35/41] xtensa: Don't mask signals if we fail to setup signal stack Matt Fleming
` (8 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:57 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Chris Zankel
From: Matt Fleming <matt.fleming@intel.com>
get_signal_to_deliver() already resets the signal handler if
SA_ONESHOT is set in ka->sa.sa_flags, there's no need to do it again
in handle_signal(). Furthermore, because we were modifying
ka->sa.sa_handler without holding ->sighand->siglock there's the
potential we can race with another thread if it modifies the signal
handlers.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Chris Zankel <chris@zankel.net>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/xtensa/kernel/signal.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/arch/xtensa/kernel/signal.c b/arch/xtensa/kernel/signal.c
index 4f53770..24655e3 100644
--- a/arch/xtensa/kernel/signal.c
+++ b/arch/xtensa/kernel/signal.c
@@ -536,9 +536,6 @@ int do_signal(struct pt_regs *regs, sigset_t *oldset)
/* Set up the stack frame */
setup_frame(signr, &ka, &info, oldset, regs);
- if (ka.sa.sa_flags & SA_ONESHOT)
- ka.sa.sa_handler = SIG_DFL;
-
spin_lock_irq(¤t->sighand->siglock);
sigorsets(¤t->blocked, ¤t->blocked, &ka.sa.sa_mask);
if (!(ka.sa.sa_flags & SA_NODEFER))
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 35/41] xtensa: Don't mask signals if we fail to setup signal stack
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (33 preceding siblings ...)
2011-08-11 13:57 ` [PATCH 34/41] xtensa: No need to reset handler if SA_ONESHOT Matt Fleming
@ 2011-08-11 13:57 ` Matt Fleming
2011-08-11 13:57 ` [PATCH 36/41] xtensa: Use set_current_blocked() Matt Fleming
` (7 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:57 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Chris Zankel
From: Matt Fleming <matt.fleming@intel.com>
setup_frame() needs to return an indication of whether it succeeded or
failed in setting up the signal stack frame. If setup_frame() fails
then we must not modify current->blocked.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Chris Zankel <chris@zankel.net>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/xtensa/kernel/signal.c | 11 +++++++----
1 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/arch/xtensa/kernel/signal.c b/arch/xtensa/kernel/signal.c
index 24655e3..17ceab8 100644
--- a/arch/xtensa/kernel/signal.c
+++ b/arch/xtensa/kernel/signal.c
@@ -336,8 +336,8 @@ gen_return_code(unsigned char *codemem)
}
-static void setup_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
- sigset_t *set, struct pt_regs *regs)
+static int setup_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
+ sigset_t *set, struct pt_regs *regs)
{
struct rt_sigframe *frame;
int err = 0;
@@ -422,10 +422,11 @@ static void setup_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
current->comm, current->pid, signal, frame, regs->pc);
#endif
- return;
+ return 0;
give_sigsegv:
force_sigsegv(sig, current);
+ return -EFAULT;
}
/*
@@ -534,7 +535,9 @@ int do_signal(struct pt_regs *regs, sigset_t *oldset)
/* Whee! Actually deliver the signal. */
/* Set up the stack frame */
- setup_frame(signr, &ka, &info, oldset, regs);
+ ret = setup_frame(signr, &ka, &info, oldset, regs);
+ if (ret)
+ return ret;
spin_lock_irq(¤t->sighand->siglock);
sigorsets(¤t->blocked, ¤t->blocked, &ka.sa.sa_mask);
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 36/41] xtensa: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (34 preceding siblings ...)
2011-08-11 13:57 ` [PATCH 35/41] xtensa: Don't mask signals if we fail to setup signal stack Matt Fleming
@ 2011-08-11 13:57 ` Matt Fleming
2011-08-11 13:57 ` [PATCH 37/41] autofs4: " Matt Fleming
` (6 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:57 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Chris Zankel
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Chris Zankel <chris@zankel.net>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/xtensa/kernel/signal.c | 19 ++++++-------------
1 files changed, 6 insertions(+), 13 deletions(-)
diff --git a/arch/xtensa/kernel/signal.c b/arch/xtensa/kernel/signal.c
index 17ceab8..4df5da5 100644
--- a/arch/xtensa/kernel/signal.c
+++ b/arch/xtensa/kernel/signal.c
@@ -260,10 +260,7 @@ asmlinkage long xtensa_rt_sigreturn(long a0, long a1, long a2, long a3,
goto badframe;
sigdelsetmask(&set, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = set;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&set);
if (restore_sigcontext(regs, frame))
goto badframe;
@@ -448,11 +445,8 @@ asmlinkage long xtensa_rt_sigsuspend(sigset_t __user *unewset,
return -EFAULT;
sigdelsetmask(&newset, ~_BLOCKABLE);
- spin_lock_irq(¤t->sighand->siglock);
saveset = current->blocked;
- current->blocked = newset;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&newset);
regs->areg[2] = -EINTR;
while (1) {
@@ -487,6 +481,7 @@ int do_signal(struct pt_regs *regs, sigset_t *oldset)
siginfo_t info;
int signr;
struct k_sigaction ka;
+ sigset_t blocked;
if (!user_mode(regs))
return 0;
@@ -539,12 +534,10 @@ int do_signal(struct pt_regs *regs, sigset_t *oldset)
if (ret)
return ret;
- spin_lock_irq(¤t->sighand->siglock);
- sigorsets(¤t->blocked, ¤t->blocked, &ka.sa.sa_mask);
+ sigorsets(&blocked, ¤t->blocked, &ka.sa.sa_mask);
if (!(ka.sa.sa_flags & SA_NODEFER))
- sigaddset(¤t->blocked, signr);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ sigaddset(&blocked, signr);
+ set_current_blocked(&blocked);
if (current->ptrace & PT_SINGLESTEP)
task_pt_regs(current)->icountlevel = 1;
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 37/41] autofs4: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (35 preceding siblings ...)
2011-08-11 13:57 ` [PATCH 36/41] xtensa: Use set_current_blocked() Matt Fleming
@ 2011-08-11 13:57 ` Matt Fleming
2011-08-16 17:47 ` Oleg Nesterov
2011-08-11 13:57 ` [PATCH 38/41] coda: " Matt Fleming
` (5 subsequent siblings)
42 siblings, 1 reply; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:57 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Ian Kent
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Ian Kent <raven@themaw.net>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
fs/autofs4/waitq.c | 15 +++++----------
1 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index e1fbdee..64ff15a 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -458,21 +458,16 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
*/
if (wq->name.name) {
/* Block all but "shutdown" signals while waiting */
- sigset_t oldset;
+ sigset_t oldset, blocked;
unsigned long irqflags;
- spin_lock_irqsave(¤t->sighand->siglock, irqflags);
oldset = current->blocked;
- siginitsetinv(¤t->blocked, SHUTDOWN_SIGS & ~oldset.sig[0]);
- recalc_sigpending();
- spin_unlock_irqrestore(¤t->sighand->siglock, irqflags);
+ siginitsetinv(&blocked, SHUTDOWN_SIGS & ~oldset.sig[0]);
+ set_current_blocked(&blocked);
wait_event_interruptible(wq->queue, wq->name.name == NULL);
-
- spin_lock_irqsave(¤t->sighand->siglock, irqflags);
- current->blocked = oldset;
- recalc_sigpending();
- spin_unlock_irqrestore(¤t->sighand->siglock, irqflags);
+
+ set_current_blocked(&oldset);
} else {
DPRINTK("skipped sleeping");
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* Re: [PATCH 37/41] autofs4: Use set_current_blocked()
2011-08-11 13:57 ` [PATCH 37/41] autofs4: " Matt Fleming
@ 2011-08-16 17:47 ` Oleg Nesterov
2011-08-16 20:29 ` Matt Fleming
0 siblings, 1 reply; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-16 17:47 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-kernel, Ian Kent
On 08/11, Matt Fleming wrote:
>
> As described in e6fa16ab ("signal: sigprocmask() should do
> retarget_shared_pending()") the modification of current->blocked is
> incorrect as we need to check whether the signal we're about to block
> is pending in the shared queue.
ACK.
cough, can't resist...
> @@ -458,21 +458,16 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
> */
> if (wq->name.name) {
> /* Block all but "shutdown" signals while waiting */
> - sigset_t oldset;
> + sigset_t oldset, blocked;
> unsigned long irqflags;
>
> - spin_lock_irqsave(¤t->sighand->siglock, irqflags);
> oldset = current->blocked;
> - siginitsetinv(¤t->blocked, SHUTDOWN_SIGS & ~oldset.sig[0]);
> - recalc_sigpending();
> - spin_unlock_irqrestore(¤t->sighand->siglock, irqflags);
> + siginitsetinv(&blocked, SHUTDOWN_SIGS & ~oldset.sig[0]);
> + set_current_blocked(&blocked);
>
> wait_event_interruptible(wq->queue, wq->name.name == NULL);
> -
> - spin_lock_irqsave(¤t->sighand->siglock, irqflags);
> - current->blocked = oldset;
> - recalc_sigpending();
> - spin_unlock_irqrestore(¤t->sighand->siglock, irqflags);
> +
this adds the trailing whitespaces ;)
Oleg.
^ permalink raw reply [flat|nested] 87+ messages in thread* Re: [PATCH 37/41] autofs4: Use set_current_blocked()
2011-08-16 17:47 ` Oleg Nesterov
@ 2011-08-16 20:29 ` Matt Fleming
0 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-16 20:29 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Ian Kent
On Tue, 2011-08-16 at 19:47 +0200, Oleg Nesterov wrote:
> On 08/11, Matt Fleming wrote:
> >
> > As described in e6fa16ab ("signal: sigprocmask() should do
> > retarget_shared_pending()") the modification of current->blocked is
> > incorrect as we need to check whether the signal we're about to block
> > is pending in the shared queue.
>
> ACK.
>
>
> cough, can't resist...
>
> > @@ -458,21 +458,16 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
> > */
> > if (wq->name.name) {
> > /* Block all but "shutdown" signals while waiting */
> > - sigset_t oldset;
> > + sigset_t oldset, blocked;
> > unsigned long irqflags;
> >
> > - spin_lock_irqsave(¤t->sighand->siglock, irqflags);
> > oldset = current->blocked;
> > - siginitsetinv(¤t->blocked, SHUTDOWN_SIGS & ~oldset.sig[0]);
> > - recalc_sigpending();
> > - spin_unlock_irqrestore(¤t->sighand->siglock, irqflags);
> > + siginitsetinv(&blocked, SHUTDOWN_SIGS & ~oldset.sig[0]);
> > + set_current_blocked(&blocked);
> >
> > wait_event_interruptible(wq->queue, wq->name.name == NULL);
> > -
> > - spin_lock_irqsave(¤t->sighand->siglock, irqflags);
> > - current->blocked = oldset;
> > - recalc_sigpending();
> > - spin_unlock_irqrestore(¤t->sighand->siglock, irqflags);
> > +
>
> this adds the trailing whitespaces ;)
Whoops! I fixed that and added your Acked-by. Thanks!
--
Matt Fleming, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 87+ messages in thread
* [PATCH 38/41] coda: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (36 preceding siblings ...)
2011-08-11 13:57 ` [PATCH 37/41] autofs4: " Matt Fleming
@ 2011-08-11 13:57 ` Matt Fleming
2011-08-11 13:57 ` [PATCH 39/41] dlm: Remove another superfluous call to recalc_sigpending() Matt Fleming
` (4 subsequent siblings)
42 siblings, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:57 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Jan Harkes
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Jan Harkes <jaharkes@cs.cmu.edu>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
fs/coda/upcall.c | 19 ++++++++-----------
1 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/fs/coda/upcall.c b/fs/coda/upcall.c
index 9727e0c..ce5925d 100644
--- a/fs/coda/upcall.c
+++ b/fs/coda/upcall.c
@@ -575,24 +575,21 @@ int venus_statfs(struct dentry *dentry, struct kstatfs *sfs)
*/
static void coda_block_signals(sigset_t *old)
{
- spin_lock_irq(¤t->sighand->siglock);
+ sigset_t blocked;
+
*old = current->blocked;
- sigfillset(¤t->blocked);
- sigdelset(¤t->blocked, SIGKILL);
- sigdelset(¤t->blocked, SIGSTOP);
- sigdelset(¤t->blocked, SIGINT);
+ sigfillset(&blocked);
+ sigdelset(&blocked, SIGKILL);
+ sigdelset(&blocked, SIGSTOP);
+ sigdelset(&blocked, SIGINT);
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(&blocked);
}
static void coda_unblock_signals(sigset_t *old)
{
- spin_lock_irq(¤t->sighand->siglock);
- current->blocked = *old;
- recalc_sigpending();
- spin_unlock_irq(¤t->sighand->siglock);
+ set_current_blocked(old);
}
/* Don't allow signals to interrupt the following upcalls before venus
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* [PATCH 39/41] dlm: Remove another superfluous call to recalc_sigpending()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (37 preceding siblings ...)
2011-08-11 13:57 ` [PATCH 38/41] coda: " Matt Fleming
@ 2011-08-11 13:57 ` Matt Fleming
2011-08-11 15:39 ` David Teigland
2011-08-16 19:36 ` Oleg Nesterov
2011-08-11 13:57 ` [PATCH 40/41] ncpfs: Use set_current_blocked() Matt Fleming
` (3 subsequent siblings)
42 siblings, 2 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:57 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Christine Caulfield, David Teigland
From: Matt Fleming <matt.fleming@intel.com>
recalc_sigpending() is called within sigprocmask(), so there is no
need call it again after sigprocmask() has returned. I must have
missed this call when removing the other recalc_sigpending() in commit
4bcad6c1ef53 ("dlm: Remove superfluous call to recalc_sigpending())".
Cc: Christine Caulfield <ccaulfie@redhat.com>
Cc: David Teigland <teigland@redhat.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
fs/dlm/user.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/fs/dlm/user.c b/fs/dlm/user.c
index d8ea607..b38b122 100644
--- a/fs/dlm/user.c
+++ b/fs/dlm/user.c
@@ -678,7 +678,6 @@ static int device_close(struct inode *inode, struct file *file)
device_remove_lockspace() */
sigprocmask(SIG_SETMASK, &tmpsig, NULL);
- recalc_sigpending();
return 0;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* Re: [PATCH 39/41] dlm: Remove another superfluous call to recalc_sigpending()
2011-08-11 13:57 ` [PATCH 39/41] dlm: Remove another superfluous call to recalc_sigpending() Matt Fleming
@ 2011-08-11 15:39 ` David Teigland
2011-08-16 19:36 ` Oleg Nesterov
1 sibling, 0 replies; 87+ messages in thread
From: David Teigland @ 2011-08-11 15:39 UTC (permalink / raw)
To: Matt Fleming; +Cc: Oleg Nesterov, linux-kernel, Christine Caulfield
On Thu, Aug 11, 2011 at 02:57:13PM +0100, Matt Fleming wrote:
> From: Matt Fleming <matt.fleming@intel.com>
>
> recalc_sigpending() is called within sigprocmask(), so there is no
> need call it again after sigprocmask() has returned. I must have
> missed this call when removing the other recalc_sigpending() in commit
> 4bcad6c1ef53 ("dlm: Remove superfluous call to recalc_sigpending())".
ack
^ permalink raw reply [flat|nested] 87+ messages in thread* Re: [PATCH 39/41] dlm: Remove another superfluous call to recalc_sigpending()
2011-08-11 13:57 ` [PATCH 39/41] dlm: Remove another superfluous call to recalc_sigpending() Matt Fleming
2011-08-11 15:39 ` David Teigland
@ 2011-08-16 19:36 ` Oleg Nesterov
1 sibling, 0 replies; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-16 19:36 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-kernel, Christine Caulfield, David Teigland
On 08/11, Matt Fleming wrote:
>
> recalc_sigpending() is called within sigprocmask(), so there is no
> need call it again after sigprocmask() has returned.
And more, this is simply wrong. This can race with signal_wake_up().
ACK. but may me you can update the changelog although I won't insist.
Oleg.
^ permalink raw reply [flat|nested] 87+ messages in thread
* [PATCH 40/41] ncpfs: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (38 preceding siblings ...)
2011-08-11 13:57 ` [PATCH 39/41] dlm: Remove another superfluous call to recalc_sigpending() Matt Fleming
@ 2011-08-11 13:57 ` Matt Fleming
2011-08-16 17:56 ` Oleg Nesterov
2011-08-11 13:57 ` [PATCH 41/41] exit: Use __set_task_blocked() Matt Fleming
` (2 subsequent siblings)
42 siblings, 1 reply; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:57 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Petr Vandrovec, Al Viro, Arnd Bergmann
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Petr Vandrovec <petr@vandrovec.name>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
fs/ncpfs/sock.c | 12 +++++-------
1 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/fs/ncpfs/sock.c b/fs/ncpfs/sock.c
index 3a15872..883fdc3 100644
--- a/fs/ncpfs/sock.c
+++ b/fs/ncpfs/sock.c
@@ -749,7 +749,7 @@ static int ncp_do_request(struct ncp_server *server, int size,
return -EIO;
}
{
- sigset_t old_set;
+ sigset_t old_set, blocked;
unsigned long mask, flags;
spin_lock_irqsave(¤t->sighand->siglock, flags);
@@ -769,16 +769,14 @@ static int ncp_do_request(struct ncp_server *server, int size,
if (current->sighand->action[SIGQUIT - 1].sa.sa_handler == SIG_DFL)
mask |= sigmask(SIGQUIT);
}
- siginitsetinv(¤t->blocked, mask);
- recalc_sigpending();
+
+ siginitsetinv(&blocked, mask);
+ __set_task_blocked(current, &blocked);
spin_unlock_irqrestore(¤t->sighand->siglock, flags);
result = do_ncp_rpc_call(server, size, reply, max_reply_size);
- spin_lock_irqsave(¤t->sighand->siglock, flags);
- current->blocked = old_set;
- recalc_sigpending();
- spin_unlock_irqrestore(¤t->sighand->siglock, flags);
+ set_current_blocked(&old_set);
}
DDPRINTK("do_ncp_rpc_call returned %d\n", result);
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* Re: [PATCH 40/41] ncpfs: Use set_current_blocked()
2011-08-11 13:57 ` [PATCH 40/41] ncpfs: Use set_current_blocked() Matt Fleming
@ 2011-08-16 17:56 ` Oleg Nesterov
2011-08-16 20:56 ` Matt Fleming
0 siblings, 1 reply; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-16 17:56 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-kernel, Petr Vandrovec, Al Viro, Arnd Bergmann
On 08/11, Matt Fleming wrote:
>
> As described in e6fa16ab ("signal: sigprocmask() should do
> retarget_shared_pending()") the modification of current->blocked is
> incorrect as we need to check whether the signal we're about to block
> is pending in the shared queue.
I'd wish I could understand this code but this seems impossible ;)
IOW, "This doesn't seem right at all." looks reasonable, and the
PF_EXITING adds even more confusion.
As for this patch, it looks (almost) fine anyway. But,
> @@ -749,7 +749,7 @@ static int ncp_do_request(struct ncp_server *server, int size,
> return -EIO;
> }
> {
> - sigset_t old_set;
> + sigset_t old_set, blocked;
> unsigned long mask, flags;
>
> spin_lock_irqsave(¤t->sighand->siglock, flags);
> @@ -769,16 +769,14 @@ static int ncp_do_request(struct ncp_server *server, int size,
> if (current->sighand->action[SIGQUIT - 1].sa.sa_handler == SIG_DFL)
> mask |= sigmask(SIGQUIT);
> }
> - siginitsetinv(¤t->blocked, mask);
> - recalc_sigpending();
> +
> + siginitsetinv(&blocked, mask);
> + __set_task_blocked(current, &blocked);
> spin_unlock_irqrestore(¤t->sighand->siglock, flags);
Why do we take ->siglock in the first place?
I think it is not needed. We can calculate mask/blocked lockless and
use set_task_blocked(). This also makes sense because __set_task_blocked
is not exported ;)
the sighand->action[] checks are racy anyway in the mt case, siglock
can't help.
Oleg.
^ permalink raw reply [flat|nested] 87+ messages in thread* Re: [PATCH 40/41] ncpfs: Use set_current_blocked()
2011-08-16 17:56 ` Oleg Nesterov
@ 2011-08-16 20:56 ` Matt Fleming
2011-08-17 12:04 ` Oleg Nesterov
0 siblings, 1 reply; 87+ messages in thread
From: Matt Fleming @ 2011-08-16 20:56 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Petr Vandrovec, Al Viro, Arnd Bergmann
On Tue, 2011-08-16 at 19:56 +0200, Oleg Nesterov wrote:
> On 08/11, Matt Fleming wrote:
> >
> > As described in e6fa16ab ("signal: sigprocmask() should do
> > retarget_shared_pending()") the modification of current->blocked is
> > incorrect as we need to check whether the signal we're about to block
> > is pending in the shared queue.
>
> I'd wish I could understand this code but this seems impossible ;)
Yeah, I gave up after staring at it for about twenty minutes. I couldn't
fathom the logic behind it.
> IOW, "This doesn't seem right at all." looks reasonable, and the
> PF_EXITING adds even more confusion.
Definitely. If I was more confident in this area of the kernel I would
have just deleted it ;-)
In fact, the more I stare at it, the more I think it needs removing.
Because the thread doesn't hold ->siglock over do_ncp_rpc_call() another
thread could change the signal handler for SIGINT or SIGQUIT mid-call.
Which makes the code under "if (server->m.flags & NCP_MOUNT_INTR)"
pointless.
Petr, Al, Arnd? Could one of you hit me with a clue bat?
> As for this patch, it looks (almost) fine anyway. But,
>
> > @@ -749,7 +749,7 @@ static int ncp_do_request(struct ncp_server *server, int size,
> > return -EIO;
> > }
> > {
> > - sigset_t old_set;
> > + sigset_t old_set, blocked;
> > unsigned long mask, flags;
> >
> > spin_lock_irqsave(¤t->sighand->siglock, flags);
> > @@ -769,16 +769,14 @@ static int ncp_do_request(struct ncp_server *server, int size,
> > if (current->sighand->action[SIGQUIT - 1].sa.sa_handler == SIG_DFL)
> > mask |= sigmask(SIGQUIT);
> > }
> > - siginitsetinv(¤t->blocked, mask);
> > - recalc_sigpending();
> > +
> > + siginitsetinv(&blocked, mask);
> > + __set_task_blocked(current, &blocked);
> > spin_unlock_irqrestore(¤t->sighand->siglock, flags);
>
> Why do we take ->siglock in the first place?
>
> I think it is not needed. We can calculate mask/blocked lockless and
> use set_task_blocked(). This also makes sense because __set_task_blocked
> is not exported ;)
Eek! Sorry, I didn't realise this didn't compile.
> the sighand->action[] checks are racy anyway in the mt case, siglock
> can't help.
Hmm.. really? I thought that ->siglock serialised modifications to
sighand->action[] even in the mt case, no? This was the only reason that
I left the sighand locking around in this function.
--
Matt Fleming, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 87+ messages in thread* Re: [PATCH 40/41] ncpfs: Use set_current_blocked()
2011-08-16 20:56 ` Matt Fleming
@ 2011-08-17 12:04 ` Oleg Nesterov
2011-08-17 13:58 ` Matt Fleming
0 siblings, 1 reply; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-17 12:04 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-kernel, Petr Vandrovec, Al Viro, Arnd Bergmann
On 08/16, Matt Fleming wrote:
>
> On Tue, 2011-08-16 at 19:56 +0200, Oleg Nesterov wrote:
> > On 08/11, Matt Fleming wrote:
> > >
> > > As described in e6fa16ab ("signal: sigprocmask() should do
> > > retarget_shared_pending()") the modification of current->blocked is
> > > incorrect as we need to check whether the signal we're about to block
> > > is pending in the shared queue.
> >
> > I'd wish I could understand this code but this seems impossible ;)
>
> Yeah, I gave up after staring at it for about twenty minutes. I couldn't
> fathom the logic behind it.
>
> > IOW, "This doesn't seem right at all." looks reasonable, and the
> > PF_EXITING adds even more confusion.
>
> Definitely. If I was more confident in this area of the kernel I would
> have just deleted it ;-)
Same here ;)
> Because the thread doesn't hold ->siglock over do_ncp_rpc_call() another
> thread could change the signal handler for SIGINT or SIGQUIT mid-call.
> Which makes the code under "if (server->m.flags & NCP_MOUNT_INTR)"
> pointless.
(indeed, and see below)
> > Why do we take ->siglock in the first place?
> >
> > I think it is not needed. We can calculate mask/blocked lockless and
> > use set_task_blocked(). This also makes sense because __set_task_blocked
> > is not exported ;)
>
> Eek! Sorry, I didn't realise this didn't compile.
>
> > the sighand->action[] checks are racy anyway in the mt case, siglock
> > can't help.
>
> Hmm.. really? I thought that ->siglock serialised modifications to
> sighand->action[] even in the mt case, no?
Sure. But another thread can change sighand->action[] right after we
drop ->siglock. So how can this lock help? We simply read the word,
this is atomic and doesn't need the locking.
Oleg.
^ permalink raw reply [flat|nested] 87+ messages in thread* Re: [PATCH 40/41] ncpfs: Use set_current_blocked()
2011-08-17 12:04 ` Oleg Nesterov
@ 2011-08-17 13:58 ` Matt Fleming
2011-08-17 14:41 ` Oleg Nesterov
0 siblings, 1 reply; 87+ messages in thread
From: Matt Fleming @ 2011-08-17 13:58 UTC (permalink / raw)
To: Oleg Nesterov
Cc: linux-kernel, Petr Vandrovec, Al Viro, Arnd Bergmann,
linux-fsdevel, Andrew Morton
On Wed, 2011-08-17 at 14:04 +0200, Oleg Nesterov wrote:
> On 08/16, Matt Fleming wrote:
> >
> > > the sighand->action[] checks are racy anyway in the mt case, siglock
> > > can't help.
> >
> > Hmm.. really? I thought that ->siglock serialised modifications to
> > sighand->action[] even in the mt case, no?
>
> Sure. But another thread can change sighand->action[] right after we
> drop ->siglock. So how can this lock help? We simply read the word,
> this is atomic and doesn't need the locking.
Oh right, in the scenario in ncp_do_request(), sure I understand that. I
thought you were saying that in the general case ->siglock doesn't
protect sighand->action[]! That's why I was confused ;-)
OK, how about this patch (instead of 40/41) which gets rid of all the
nasties? I've Cc'd linux-fsdevel so people can hopefully OK this from a
file system perspective.
Some Tested-by's would be good too.
--------8<--------
>From bb1650295054bdfa96f8f4ff61507d314be8296a Mon Sep 17 00:00:00 2001
From: Matt Fleming <matt.fleming@intel.com>
Date: Wed, 17 Aug 2011 13:59:12 +0100
Subject: [PATCH] ncpfs: Don't attempt to mask signals during
do_ncp_rpc_call()
Delete the code in ncp_do_request() that attempts to mask signals
across the call to do_ncp_rpc_call(). This code was racy because it
dropped ->siglock across do_ncp_rpc_call() so it was possible for
another thread to modify the signal handlers, which made the code
pointless.
Instead of fixing the code to hold the lock across the call let's just
delete it because, as the FIXME comment (which has been around since
the beginning of git history) says, trying to block signals doesn't
seem right at all.
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
fs/ncpfs/sock.c | 32 +-------------------------------
1 files changed, 1 insertions(+), 31 deletions(-)
diff --git a/fs/ncpfs/sock.c b/fs/ncpfs/sock.c
index 3a15872..6618402 100644
--- a/fs/ncpfs/sock.c
+++ b/fs/ncpfs/sock.c
@@ -748,38 +748,8 @@ static int ncp_do_request(struct ncp_server *server, int size,
if (!ncp_conn_valid(server)) {
return -EIO;
}
- {
- sigset_t old_set;
- unsigned long mask, flags;
-
- spin_lock_irqsave(¤t->sighand->siglock, flags);
- old_set = current->blocked;
- if (current->flags & PF_EXITING)
- mask = 0;
- else
- mask = sigmask(SIGKILL);
- if (server->m.flags & NCP_MOUNT_INTR) {
- /* FIXME: This doesn't seem right at all. So, like,
- we can't handle SIGINT and get whatever to stop?
- What if we've blocked it ourselves? What about
- alarms? Why, in fact, are we mucking with the
- sigmask at all? -- r~ */
- if (current->sighand->action[SIGINT - 1].sa.sa_handler == SIG_DFL)
- mask |= sigmask(SIGINT);
- if (current->sighand->action[SIGQUIT - 1].sa.sa_handler == SIG_DFL)
- mask |= sigmask(SIGQUIT);
- }
- siginitsetinv(¤t->blocked, mask);
- recalc_sigpending();
- spin_unlock_irqrestore(¤t->sighand->siglock, flags);
-
- result = do_ncp_rpc_call(server, size, reply, max_reply_size);
- spin_lock_irqsave(¤t->sighand->siglock, flags);
- current->blocked = old_set;
- recalc_sigpending();
- spin_unlock_irqrestore(¤t->sighand->siglock, flags);
- }
+ result = do_ncp_rpc_call(server, size, reply, max_reply_size);
DDPRINTK("do_ncp_rpc_call returned %d\n", result);
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* Re: [PATCH 40/41] ncpfs: Use set_current_blocked()
2011-08-17 13:58 ` Matt Fleming
@ 2011-08-17 14:41 ` Oleg Nesterov
[not found] ` <CA+i2_De=mKMHj++b5=ZPdXxp7pm2KzY+PzCaG++GWSud20a_qQ@mail.gmail.com>
0 siblings, 1 reply; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-17 14:41 UTC (permalink / raw)
To: Matt Fleming
Cc: linux-kernel, Petr Vandrovec, Al Viro, Arnd Bergmann,
linux-fsdevel, Andrew Morton
On 08/17, Matt Fleming wrote:
>
> On Wed, 2011-08-17 at 14:04 +0200, Oleg Nesterov wrote:
> > On 08/16, Matt Fleming wrote:
> > >
> > > > the sighand->action[] checks are racy anyway in the mt case, siglock
> > > > can't help.
> > >
> > > Hmm.. really? I thought that ->siglock serialised modifications to
> > > sighand->action[] even in the mt case, no?
> >
> > Sure. But another thread can change sighand->action[] right after we
> > drop ->siglock. So how can this lock help? We simply read the word,
> > this is atomic and doesn't need the locking.
>
> Oh right, in the scenario in ncp_do_request(), sure I understand that. I
> thought you were saying that in the general case ->siglock doesn't
> protect sighand->action[]! That's why I was confused ;-)
>
> OK, how about this patch (instead of 40/41) which gets rid of all the
> nasties? I've Cc'd linux-fsdevel so people can hopefully OK this from a
> file system perspective.
Well, of course I am in no position to ack this change ;)
But obviously I like the idea to kill the obviously wrong code.
In particular, the PF_EXITING/SIGKILL logic looks as "must die
in any case" to me.
If maintainers object, you can remove ->siglock and convert the code
to use set_current_blocked(). IOW, simplify your original patch.
Oleg.
> From bb1650295054bdfa96f8f4ff61507d314be8296a Mon Sep 17 00:00:00 2001
> From: Matt Fleming <matt.fleming@intel.com>
> Date: Wed, 17 Aug 2011 13:59:12 +0100
> Subject: [PATCH] ncpfs: Don't attempt to mask signals during
> do_ncp_rpc_call()
>
> Delete the code in ncp_do_request() that attempts to mask signals
> across the call to do_ncp_rpc_call(). This code was racy because it
> dropped ->siglock across do_ncp_rpc_call() so it was possible for
> another thread to modify the signal handlers, which made the code
> pointless.
>
> Instead of fixing the code to hold the lock across the call let's just
> delete it because, as the FIXME comment (which has been around since
> the beginning of git history) says, trying to block signals doesn't
> seem right at all.
>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
> fs/ncpfs/sock.c | 32 +-------------------------------
> 1 files changed, 1 insertions(+), 31 deletions(-)
>
> diff --git a/fs/ncpfs/sock.c b/fs/ncpfs/sock.c
> index 3a15872..6618402 100644
> --- a/fs/ncpfs/sock.c
> +++ b/fs/ncpfs/sock.c
> @@ -748,38 +748,8 @@ static int ncp_do_request(struct ncp_server *server, int size,
> if (!ncp_conn_valid(server)) {
> return -EIO;
> }
> - {
> - sigset_t old_set;
> - unsigned long mask, flags;
> -
> - spin_lock_irqsave(¤t->sighand->siglock, flags);
> - old_set = current->blocked;
> - if (current->flags & PF_EXITING)
> - mask = 0;
> - else
> - mask = sigmask(SIGKILL);
> - if (server->m.flags & NCP_MOUNT_INTR) {
> - /* FIXME: This doesn't seem right at all. So, like,
> - we can't handle SIGINT and get whatever to stop?
> - What if we've blocked it ourselves? What about
> - alarms? Why, in fact, are we mucking with the
> - sigmask at all? -- r~ */
> - if (current->sighand->action[SIGINT - 1].sa.sa_handler == SIG_DFL)
> - mask |= sigmask(SIGINT);
> - if (current->sighand->action[SIGQUIT - 1].sa.sa_handler == SIG_DFL)
> - mask |= sigmask(SIGQUIT);
> - }
> - siginitsetinv(¤t->blocked, mask);
> - recalc_sigpending();
> - spin_unlock_irqrestore(¤t->sighand->siglock, flags);
> -
> - result = do_ncp_rpc_call(server, size, reply, max_reply_size);
>
> - spin_lock_irqsave(¤t->sighand->siglock, flags);
> - current->blocked = old_set;
> - recalc_sigpending();
> - spin_unlock_irqrestore(¤t->sighand->siglock, flags);
> - }
> + result = do_ncp_rpc_call(server, size, reply, max_reply_size);
>
> DDPRINTK("do_ncp_rpc_call returned %d\n", result);
>
> --
> 1.7.4.4
>
>
^ permalink raw reply [flat|nested] 87+ messages in thread
* [PATCH 41/41] exit: Use __set_task_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (39 preceding siblings ...)
2011-08-11 13:57 ` [PATCH 40/41] ncpfs: Use set_current_blocked() Matt Fleming
@ 2011-08-11 13:57 ` Matt Fleming
2011-08-16 18:06 ` Oleg Nesterov
2011-08-11 16:03 ` [PATCH 00/41] signal: Use set_current_blocked() Oleg Nesterov
2011-08-16 19:58 ` Oleg Nesterov
42 siblings, 1 reply; 87+ messages in thread
From: Matt Fleming @ 2011-08-11 13:57 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Tejun Heo, Andrew Morton
From: Matt Fleming <matt.fleming@intel.com>
As described in e6fa16ab ("signal: sigprocmask() should do
retarget_shared_pending()") the modification of current->blocked is
incorrect as we need to check whether the signal we're about to block
is pending in the shared queue.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
kernel/exit.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/kernel/exit.c b/kernel/exit.c
index 2913b35..a47c0f3 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -369,19 +369,22 @@ static void set_special_pids(struct pid *pid)
*/
int allow_signal(int sig)
{
+ sigset_t blocked;
+
if (!valid_signal(sig) || sig < 1)
return -EINVAL;
spin_lock_irq(¤t->sighand->siglock);
/* This is only needed for daemonize()'ed kthreads */
- sigdelset(¤t->blocked, sig);
+ siginitset(&blocked, current->blocked);
+ sigdelset(&blocked, sig);
/*
* Kernel threads handle their own signals. Let the signal code
* know it'll be handled, so that they don't get converted to
* SIGKILL or just silently dropped.
*/
current->sighand->action[(sig)-1].sa.sa_handler = (void __user *)2;
- recalc_sigpending();
+ __set_task_blocked(current, &blocked);
spin_unlock_irq(¤t->sighand->siglock);
return 0;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 87+ messages in thread* Re: [PATCH 41/41] exit: Use __set_task_blocked()
2011-08-11 13:57 ` [PATCH 41/41] exit: Use __set_task_blocked() Matt Fleming
@ 2011-08-16 18:06 ` Oleg Nesterov
2011-08-16 19:44 ` [PATCH 0/1] kthreads: allow_signal: don't play with ->blocked Oleg Nesterov
2011-08-16 21:51 ` [PATCH 41/41] exit: Use __set_task_blocked() Matt Fleming
0 siblings, 2 replies; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-16 18:06 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-kernel, Tejun Heo, Andrew Morton
On 08/11, Matt Fleming wrote:
>
> As described in e6fa16ab ("signal: sigprocmask() should do
> retarget_shared_pending()") the modification of current->blocked is
> incorrect as we need to check whether the signal we're about to block
> is pending in the shared queue.
Yes. but in this case the code is correct. First of all, unblocking
is always fine. Also, this should be used by kthreads only, and they
are single-threaded.
But there are other reasons why I don't like this change, even if we
ignore the fact the patched exit.c can't be compiled (__set_task_blocked
is not exported ;).
> int allow_signal(int sig)
> {
> + sigset_t blocked;
> +
> if (!valid_signal(sig) || sig < 1)
> return -EINVAL;
>
> spin_lock_irq(¤t->sighand->siglock);
> /* This is only needed for daemonize()'ed kthreads */
Exactly. And nowadays the daemonize()'ed kthreads should not play
with allow_signal().
And more, I think it is the time to kill daemonize(). Contrary to
what /bin/grep thinks, it has only one user.
In short: this code in allow_signal() should die. I'll try to send
the patches soon...
Oleg.
^ permalink raw reply [flat|nested] 87+ messages in thread* [PATCH 0/1] kthreads: allow_signal: don't play with ->blocked
2011-08-16 18:06 ` Oleg Nesterov
@ 2011-08-16 19:44 ` Oleg Nesterov
2011-08-16 19:44 ` [PATCH 1/1] " Oleg Nesterov
2011-08-16 21:51 ` [PATCH 41/41] exit: Use __set_task_blocked() Matt Fleming
1 sibling, 1 reply; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-16 19:44 UTC (permalink / raw)
To: Andrew Morton, Matt Fleming; +Cc: linux-kernel, Tejun Heo
On 08/16, Oleg Nesterov wrote:
>
> > int allow_signal(int sig)
> > {
> > + sigset_t blocked;
> > +
> > if (!valid_signal(sig) || sig < 1)
> > return -EINVAL;
> >
> > spin_lock_irq(¤t->sighand->siglock);
> > /* This is only needed for daemonize()'ed kthreads */
>
> Exactly. And nowadays the daemonize()'ed kthreads should not play
> with allow_signal().
>
> And more, I think it is the time to kill daemonize(). Contrary to
> what /bin/grep thinks, it has only one user.
No, daemonize() has no users which actually need it.
Imho we should deprecate it, it has no users after the patches I sent.
> In short: this code in allow_signal() should die.
Yes.
To clarify: this change breaks drivers/staging/rtl8712/, but I belive
it is wrong and should be fixed. I have already sent the patch.
Oleg.
^ permalink raw reply [flat|nested] 87+ messages in thread* [PATCH 1/1] kthreads: allow_signal: don't play with ->blocked
2011-08-16 19:44 ` [PATCH 0/1] kthreads: allow_signal: don't play with ->blocked Oleg Nesterov
@ 2011-08-16 19:44 ` Oleg Nesterov
2011-08-16 19:51 ` Tejun Heo
0 siblings, 1 reply; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-16 19:44 UTC (permalink / raw)
To: Andrew Morton, Matt Fleming; +Cc: linux-kernel, Tejun Heo
allow_signal(sig) unblocks the signal. This was only needed because
we had the daemonize()'ed kthreads playing with signals. And daemonize()
can't use ignore_signals() but does sigprocmask(SIG_BLOCK) because it
was used after kernel_thread(CLONE_SIGHAND).
Nobody does this any longer, we can remove this hack. And hopefully
we can deprecate daemonize() soon, all current users do not actually
need it.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
kernel/exit.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
--- 3.1/kernel/exit.c~4_allow_signal_dont_unblock 2011-08-16 20:14:20.000000000 +0200
+++ 3.1/kernel/exit.c 2011-08-16 21:20:11.000000000 +0200
@@ -365,7 +365,8 @@ static void set_special_pids(struct pid
/*
* Let kernel threads use this to say that they allow a certain signal.
- * Must not be used if kthread was cloned with CLONE_SIGHAND.
+ * Must not be used if kthread was cloned with CLONE_SIGHAND or daemonize()
+ * was called.
*/
int allow_signal(int sig)
{
@@ -373,8 +374,6 @@ int allow_signal(int sig)
return -EINVAL;
spin_lock_irq(¤t->sighand->siglock);
- /* This is only needed for daemonize()'ed kthreads */
- sigdelset(¤t->blocked, sig);
/*
* Kernel threads handle their own signals. Let the signal code
* know it'll be handled, so that they don't get converted to
^ permalink raw reply [flat|nested] 87+ messages in thread* Re: [PATCH 1/1] kthreads: allow_signal: don't play with ->blocked
2011-08-16 19:44 ` [PATCH 1/1] " Oleg Nesterov
@ 2011-08-16 19:51 ` Tejun Heo
2011-08-16 20:07 ` Oleg Nesterov
2011-08-16 21:50 ` Matt Fleming
0 siblings, 2 replies; 87+ messages in thread
From: Tejun Heo @ 2011-08-16 19:51 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Andrew Morton, Matt Fleming, linux-kernel
Hello, Oleg.
On Tue, Aug 16, 2011 at 09:44:50PM +0200, Oleg Nesterov wrote:
> allow_signal(sig) unblocks the signal. This was only needed because
> we had the daemonize()'ed kthreads playing with signals. And daemonize()
> can't use ignore_signals() but does sigprocmask(SIG_BLOCK) because it
> was used after kernel_thread(CLONE_SIGHAND).
>
> Nobody does this any longer, we can remove this hack. And hopefully
> we can deprecate daemonize() soon, all current users do not actually
> need it.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
I agree with the patchset but given that daemonize() isn't all that
popular and you already posted most (or was it all?) conversions,
wouldn't it be better to do this in a single patchset? ie. Convert
all daemonize() users, kill daemonize(), and drop the hack from
allow_signal().
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 1/1] kthreads: allow_signal: don't play with ->blocked
2011-08-16 19:51 ` Tejun Heo
@ 2011-08-16 20:07 ` Oleg Nesterov
2011-08-16 21:50 ` Matt Fleming
1 sibling, 0 replies; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-16 20:07 UTC (permalink / raw)
To: Tejun Heo; +Cc: Andrew Morton, Matt Fleming, linux-kernel
On 08/16, Tejun Heo wrote:
>
> Hello, Oleg.
>
> On Tue, Aug 16, 2011 at 09:44:50PM +0200, Oleg Nesterov wrote:
> > allow_signal(sig) unblocks the signal. This was only needed because
> > we had the daemonize()'ed kthreads playing with signals. And daemonize()
> > can't use ignore_signals() but does sigprocmask(SIG_BLOCK) because it
> > was used after kernel_thread(CLONE_SIGHAND).
> >
> > Nobody does this any longer, we can remove this hack. And hopefully
> > we can deprecate daemonize() soon, all current users do not actually
> > need it.
> >
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
>
> I agree with the patchset but given that daemonize() isn't all that
> popular and you already posted most (or was it all?) conversions,
> wouldn't it be better to do this in a single patchset? ie. Convert
> all daemonize() users, kill daemonize(), and drop the hack from
> allow_signal().
May be... But please note that there are too different things.
daemonize() should be deprecated (imho), but this is a bit off-topic.
I think that a daemonize()'ed kthread should not play with allow_signal()
anyway. And nobody does, except rtl8712 which should be fixed afaics.
So this code is already unneeded, but looks confusing.
Tejun, Matt, I am sorry. I have to run away, I'll reply to other
emails tomorrow.
Thanks for review!
Oleg.
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 1/1] kthreads: allow_signal: don't play with ->blocked
2011-08-16 19:51 ` Tejun Heo
2011-08-16 20:07 ` Oleg Nesterov
@ 2011-08-16 21:50 ` Matt Fleming
2011-08-17 7:27 ` Tejun Heo
1 sibling, 1 reply; 87+ messages in thread
From: Matt Fleming @ 2011-08-16 21:50 UTC (permalink / raw)
To: Tejun Heo; +Cc: Oleg Nesterov, Andrew Morton, linux-kernel
On Tue, 2011-08-16 at 21:51 +0200, Tejun Heo wrote:
>
> I agree with the patchset but given that daemonize() isn't all that
> popular and you already posted most (or was it all?) conversions,
> wouldn't it be better to do this in a single patchset? ie. Convert
> all daemonize() users, kill daemonize(), and drop the hack from
> allow_signal().
But because daemonize() is exported by the kernel should it go through
the Documentation/feature-removal-schedule.txt procedure? And if so, can
the allow_signal() patch still go in before daemonize() is removed?
--
Matt Fleming, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 1/1] kthreads: allow_signal: don't play with ->blocked
2011-08-16 21:50 ` Matt Fleming
@ 2011-08-17 7:27 ` Tejun Heo
2011-08-17 9:56 ` Matt Fleming
2011-08-17 18:26 ` Oleg Nesterov
0 siblings, 2 replies; 87+ messages in thread
From: Tejun Heo @ 2011-08-17 7:27 UTC (permalink / raw)
To: Matt Fleming; +Cc: Oleg Nesterov, Andrew Morton, linux-kernel
Hello,
On Tue, Aug 16, 2011 at 10:50:22PM +0100, Matt Fleming wrote:
> On Tue, 2011-08-16 at 21:51 +0200, Tejun Heo wrote:
> > I agree with the patchset but given that daemonize() isn't all that
> > popular and you already posted most (or was it all?) conversions,
> > wouldn't it be better to do this in a single patchset? ie. Convert
> > all daemonize() users, kill daemonize(), and drop the hack from
> > allow_signal().
>
> But because daemonize() is exported by the kernel should it go through
> the Documentation/feature-removal-schedule.txt procedure? And if so, can
> the allow_signal() patch still go in before daemonize() is removed?
IMHO, not really. APIs get modified and dropped all the time and only
small fraction goes through feature-removal-schedule. For APIs which
are widely used and/or difficult to migrate from, it sure makes sense
to do the staged removal but in this case it's an interface which is
quite unpopular and with relatively easy workaround (just use
kthread).
The worst thing we can do regarding API change is silently changing
semantics while not changing the interface. For this patchset I don't
think it would matter all that much but is going that route.
ie. allow_signal() behavior is proposed to be changed because
in-kernel daemonize() users don't depend on it while leaving
daemonize() alone. This is much worse than simply removing
daemonize() with sufficient explanation in the commit message.
Out-of-kernel user which depended on the combination working would now
be left with code which compiles fine but behaves differently, which
sucks big time.
These changes _are_ related and interdependent, and routing these
small changes through different trees often end up delaying things
unnecessarily. One subsystem maintainer forgets to apply a patch or
send pull request and it can get easily drawn out half a year and
people forget what the original change was about after a while often
leading to half done conversions. So, let's please collect all the
related patches into one series, drop all in-kernel daemonize() users,
kill daemonize() and then change allow_signal() behavior.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 1/1] kthreads: allow_signal: don't play with ->blocked
2011-08-17 7:27 ` Tejun Heo
@ 2011-08-17 9:56 ` Matt Fleming
2011-08-17 18:26 ` Oleg Nesterov
1 sibling, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-17 9:56 UTC (permalink / raw)
To: Tejun Heo; +Cc: Oleg Nesterov, Andrew Morton, linux-kernel
On Wed, 2011-08-17 at 09:27 +0200, Tejun Heo wrote:
> Hello,
>
> On Tue, Aug 16, 2011 at 10:50:22PM +0100, Matt Fleming wrote:
> >
> > But because daemonize() is exported by the kernel should it go through
> > the Documentation/feature-removal-schedule.txt procedure? And if so, can
> > the allow_signal() patch still go in before daemonize() is removed?
>
> IMHO, not really. APIs get modified and dropped all the time and only
> small fraction goes through feature-removal-schedule. For APIs which
> are widely used and/or difficult to migrate from, it sure makes sense
> to do the staged removal but in this case it's an interface which is
> quite unpopular and with relatively easy workaround (just use
> kthread).
>
> The worst thing we can do regarding API change is silently changing
> semantics while not changing the interface. For this patchset I don't
> think it would matter all that much but is going that route.
> ie. allow_signal() behavior is proposed to be changed because
> in-kernel daemonize() users don't depend on it while leaving
> daemonize() alone. This is much worse than simply removing
> daemonize() with sufficient explanation in the commit message.
> Out-of-kernel user which depended on the combination working would now
> be left with code which compiles fine but behaves differently, which
> sucks big time.
>
> These changes _are_ related and interdependent, and routing these
> small changes through different trees often end up delaying things
> unnecessarily. One subsystem maintainer forgets to apply a patch or
> send pull request and it can get easily drawn out half a year and
> people forget what the original change was about after a while often
> leading to half done conversions. So, let's please collect all the
> related patches into one series, drop all in-kernel daemonize() users,
> kill daemonize() and then change allow_signal() behavior.
OK, that makes a lot of sense to me. Thanks for the rationale.
Oleg, feel to carry over my Acked-by's for the patches you've already
posted if you decide to go ahead with Tejun's plan.
--
Matt Fleming, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 1/1] kthreads: allow_signal: don't play with ->blocked
2011-08-17 7:27 ` Tejun Heo
2011-08-17 9:56 ` Matt Fleming
@ 2011-08-17 18:26 ` Oleg Nesterov
1 sibling, 0 replies; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-17 18:26 UTC (permalink / raw)
To: Tejun Heo; +Cc: Matt Fleming, Andrew Morton, linux-kernel
On 08/17, Tejun Heo wrote:
>
> On Tue, Aug 16, 2011 at 10:50:22PM +0100, Matt Fleming wrote:
> > On Tue, 2011-08-16 at 21:51 +0200, Tejun Heo wrote:
> > > I agree with the patchset but given that daemonize() isn't all that
> > > popular and you already posted most (or was it all?) conversions,
Yes, with the patches I sent daemonize() has no callers.
> but in this case it's an interface which is
> quite unpopular and with relatively easy workaround (just use
> kthread).
Agreed.
> The worst thing we can do regarding API change is silently changing
> semantics while not changing the interface.
> ...
> Out-of-kernel user which depended on the combination working would now
> be left with code which compiles fine but behaves differently, which
> sucks big time.
Yes, this is of course possible.
> So, let's please collect all the
> related patches into one series,
This is what I can't understand ;) This connects to your
"How do you wanna route these" question in another thread.
> drop all in-kernel daemonize() users,
> kill daemonize() and then change allow_signal() behavior.
OK, if we kill daemonize() without the deprecation stage, this is fine.
Initially I assumed it won't go away soon, and this sigdelset()
is really nasty although minor.
Oleg.
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 41/41] exit: Use __set_task_blocked()
2011-08-16 18:06 ` Oleg Nesterov
2011-08-16 19:44 ` [PATCH 0/1] kthreads: allow_signal: don't play with ->blocked Oleg Nesterov
@ 2011-08-16 21:51 ` Matt Fleming
1 sibling, 0 replies; 87+ messages in thread
From: Matt Fleming @ 2011-08-16 21:51 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, Tejun Heo, Andrew Morton
On Tue, 2011-08-16 at 20:06 +0200, Oleg Nesterov wrote:
> On 08/11, Matt Fleming wrote:
> >
> > As described in e6fa16ab ("signal: sigprocmask() should do
> > retarget_shared_pending()") the modification of current->blocked is
> > incorrect as we need to check whether the signal we're about to block
> > is pending in the shared queue.
>
> Yes. but in this case the code is correct. First of all, unblocking
> is always fine. Also, this should be used by kthreads only, and they
> are single-threaded.
>
> But there are other reasons why I don't like this change, even if we
> ignore the fact the patched exit.c can't be compiled (__set_task_blocked
> is not exported ;).
OK, I've dropped this patch from the series.
--
Matt Fleming, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 00/41] signal: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (40 preceding siblings ...)
2011-08-11 13:57 ` [PATCH 41/41] exit: Use __set_task_blocked() Matt Fleming
@ 2011-08-11 16:03 ` Oleg Nesterov
2011-08-16 19:40 ` Matt Fleming
2011-08-16 19:58 ` Oleg Nesterov
42 siblings, 1 reply; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-11 16:03 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-kernel
Hi Matt,
On 08/11, Matt Fleming wrote:
>
> I did a tree wide scan of all the code that modifies current->blocked
> without using the set_current_blocked() accessor. This patch series is
> the result of the conversion to the new accessor functions.
Yes, this is what I was going to do, but didn't find time ;)
Thanks, everything looks "obviously fine" at first glance. except
ncpfs does something mysterious (and afaics wrong) with or without
the patch.
I'll try to read this carefully on weekend though. I do not expect I'll
find any problem, just may be a couple of nits about the changelogs.
Say, 41/41... doesn't hurt, but in fact the current code is fine as is.
Oleg.
^ permalink raw reply [flat|nested] 87+ messages in thread* Re: [PATCH 00/41] signal: Use set_current_blocked()
2011-08-11 16:03 ` [PATCH 00/41] signal: Use set_current_blocked() Oleg Nesterov
@ 2011-08-16 19:40 ` Matt Fleming
2011-08-17 17:01 ` Oleg Nesterov
0 siblings, 1 reply; 87+ messages in thread
From: Matt Fleming @ 2011-08-16 19:40 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel
On Thu, 2011-08-11 at 18:03 +0200, Oleg Nesterov wrote:
>
> I'll try to read this carefully on weekend though. I do not expect I'll
> find any problem, just may be a couple of nits about the changelogs.
> Say, 41/41... doesn't hurt, but in fact the current code is fine as is.
Thanks for the review Oleg! Did you want me to send just the updated
versions of patches or the entire series again? It would seem over the
top to send the entire series.
Also, are you going to pull these patches into your tree and send them
to Linus or would it be better to route them through -mm?
--
Matt Fleming, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 00/41] signal: Use set_current_blocked()
2011-08-16 19:40 ` Matt Fleming
@ 2011-08-17 17:01 ` Oleg Nesterov
2011-08-17 22:17 ` Matt Fleming
0 siblings, 1 reply; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-17 17:01 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-kernel
On 08/16, Matt Fleming wrote:
>
> Did you want me to send just the updated
> versions of patches or the entire series again? It would seem over the
> top to send the entire series.
This is up to you. Just in case, feel free to add my acked-by or
reviewed-by to any patch I didn't comment explicitly. Or may be
> Also, are you going to pull these patches into your tree and send them
> to Linus or would it be better to route them through -mm?
This is the question ;)
I simply do not know what should we do. I think this should be
routed via maintainers.
Or I can push this into the poor ptrace branch... but I am
thinking with horror about the possible conflicts in arch/ code...
What do you think?
Damn. And I can't relax. This is very minor, but I still think
it would be nice to have a common helper which plays sa_mask /
SA_NODEFER to set ->blocked. But in this case, you probably need
to resend the whole series ;)
I mean something like the patch below. OK, nevermind. Probably
this factorization doesn't buy too much, but it will complicate
the merging.
Oleg.
--- x/include/linux/signal.h
+++ x/include/linux/signal.h
@@ -254,6 +254,7 @@ extern void set_current_blocked(const si
extern int show_unhandled_signals;
extern int get_signal_to_deliver(siginfo_t *info, struct k_sigaction *return_ka, struct pt_regs *regs, void *cookie);
+extern void xxx(struct k_sigaction *ka);
extern void exit_signals(struct task_struct *tsk);
extern struct kmem_cache *sighand_cachep;
--- x/kernel/signal.c
+++ x/kernel/signal.c
@@ -2314,6 +2314,16 @@ relock:
return signr;
}
+void xxx(struct k_sigaction *ka)
+{
+ sigset_t blocked;
+
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
+ if (!(ka->sa.sa_flags & SA_NODEFER))
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
+}
+
/*
* It could be that complete_signal() picked us to notify about the
* group-wide signal. Other threads should be notified now to take
--- x/arch/x86/kernel/signal.c
+++ x/arch/x86/kernel/signal.c
@@ -682,7 +682,6 @@ static int
handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka,
struct pt_regs *regs)
{
- sigset_t blocked;
int ret;
/* Are we from a system call? */
@@ -733,14 +732,10 @@ handle_signal(unsigned long sig, siginfo
*/
regs->flags &= ~X86_EFLAGS_TF;
- sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
- if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(&blocked, sig);
- set_current_blocked(&blocked);
+ xxx(ka);
tracehook_signal_handler(sig, info, ka, regs,
test_thread_flag(TIF_SINGLESTEP));
-
return 0;
}
^ permalink raw reply [flat|nested] 87+ messages in thread* Re: [PATCH 00/41] signal: Use set_current_blocked()
2011-08-17 17:01 ` Oleg Nesterov
@ 2011-08-17 22:17 ` Matt Fleming
2011-08-18 11:09 ` Matt Fleming
0 siblings, 1 reply; 87+ messages in thread
From: Matt Fleming @ 2011-08-17 22:17 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel
On Wed, 2011-08-17 at 19:01 +0200, Oleg Nesterov wrote:
> On 08/16, Matt Fleming wrote:
> >
> > Did you want me to send just the updated
> > versions of patches or the entire series again? It would seem over the
> > top to send the entire series.
>
> This is up to you. Just in case, feel free to add my acked-by or
> reviewed-by to any patch I didn't comment explicitly. Or may be
>
> > Also, are you going to pull these patches into your tree and send them
> > to Linus or would it be better to route them through -mm?
>
> This is the question ;)
>
> I simply do not know what should we do. I think this should be
> routed via maintainers.
Yeah, that seems like the best solution.
> Or I can push this into the poor ptrace branch... but I am
> thinking with horror about the possible conflicts in arch/ code...
>
> What do you think?
I'll let all the maintainers take it through their trees.
> Damn. And I can't relax. This is very minor, but I still think
> it would be nice to have a common helper which plays sa_mask /
> SA_NODEFER to set ->blocked. But in this case, you probably need
> to resend the whole series ;)
>
> I mean something like the patch below. OK, nevermind. Probably
> this factorization doesn't buy too much, but it will complicate
> the merging.
This does look like a decent clean up, especially because so many
architectures got this sequence wrong in the past. But yeah, it will
require me to rewrite half my series ;-)
Is it worth rewriting the series? Dunno. I'm not convinced that the
wrapper buys us enough for that, plus it'd increase the number of
patches.
Maybe it'd be better to do it as a separate set of patches?
--
Matt Fleming, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 00/41] signal: Use set_current_blocked()
2011-08-17 22:17 ` Matt Fleming
@ 2011-08-18 11:09 ` Matt Fleming
2011-08-18 18:36 ` Oleg Nesterov
0 siblings, 1 reply; 87+ messages in thread
From: Matt Fleming @ 2011-08-18 11:09 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel
On Wed, 2011-08-17 at 23:17 +0100, Matt Fleming wrote:
>
> This does look like a decent clean up, especially because so many
> architectures got this sequence wrong in the past. But yeah, it will
> require me to rewrite half my series ;-)
>
> Is it worth rewriting the series? Dunno. I'm not convinced that the
> wrapper buys us enough for that, plus it'd increase the number of
> patches.
>
> Maybe it'd be better to do it as a separate set of patches?
OK so, I went ahead and rewrote the series using your clean up. It's in
the 'oleg/set-current-blocked-v2' branch at,
git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/linux-2.6.git
Would you mind taking a quick look and see which series you prefer? I'll
wait for your reply before I resend any patches.
--
Matt Fleming, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 00/41] signal: Use set_current_blocked()
2011-08-18 11:09 ` Matt Fleming
@ 2011-08-18 18:36 ` Oleg Nesterov
0 siblings, 0 replies; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-18 18:36 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-kernel
On 08/18, Matt Fleming wrote:
>
> On Wed, 2011-08-17 at 23:17 +0100, Matt Fleming wrote:
> >
> > This does look like a decent clean up, especially because so many
> > architectures got this sequence wrong in the past. But yeah, it will
> > require me to rewrite half my series ;-)
> >
> > Is it worth rewriting the series? Dunno. I'm not convinced that the
> > wrapper buys us enough for that,
Oh, I am not sure too. I almost regret I mentioned this cleanup. Just
I was a bit suprised by how many arches do this wrong. See also below.
> plus it'd increase the number of
> patches.
By 1. A single patch can convert the code to use set_current_blocked()
and block_sigmask().
And probably the patch which adds block_sigmask() should change arch/x86
as well, this makes its purpose immediately clear. IOW, I'd prefer the
patch I sent (with rename). But once again, I won't persist.
> > Maybe it'd be better to do it as a separate set of patches?
I don't think so... this means another series touching arch/* to
make the very minor cleanup.
> OK so, I went ahead and rewrote the series using your clean up. It's in
> the 'oleg/set-current-blocked-v2' branch at,
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/linux-2.6.git
>
> Would you mind taking a quick look and see which series you prefer?
No, no, no. Matt, please choose the series which _you_ prefer. You
are the author. I am fine either way.
> I resend any patches.
Yes. I think you should resend the whole series in any case. Because
I think our discussion could confuse the maintainers.
Can I ask you to CC them all in 00/XX ? In this case I can reply
to 00 saying that I am going to take the whole series, unless the
maintainer want to do this. Otherwise I should reply per-arch, this
will certainly cause the confusion.
And, Matt, if I take the patch and then we have any sort of conflict,
you will have to help me with the git problems ;) My understanding
of git magic is quite limited.
Oleg.
^ permalink raw reply [flat|nested] 87+ messages in thread
* Re: [PATCH 00/41] signal: Use set_current_blocked()
2011-08-11 13:56 [PATCH 00/41] signal: Use set_current_blocked() Matt Fleming
` (41 preceding siblings ...)
2011-08-11 16:03 ` [PATCH 00/41] signal: Use set_current_blocked() Oleg Nesterov
@ 2011-08-16 19:58 ` Oleg Nesterov
42 siblings, 0 replies; 87+ messages in thread
From: Oleg Nesterov @ 2011-08-16 19:58 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-kernel
On 08/11, Matt Fleming wrote:
>
> I did a tree wide scan of all the code that modifies current->blocked
> without using the set_current_blocked() accessor. This patch series is
> the result of the conversion to the new accessor functions.
Thanks Matt.
The whole series looks fine, except the nits I sent. Still I hope the
maintainers can recheck.
One more nit... Every handle_signal() in arch/ does
current->blocked |= ka->sa.sa_mask plus SA_NODEFER code. May be we
should start with the patch below?
The only problem is, I can't invent the good name for this helper.
Also, in this case you need to rediff a lot of patches in this
series, so I won't insist.
Oleg.
--- x/arch/x86/kernel/signal.c
+++ x/arch/x86/kernel/signal.c
@@ -678,11 +678,20 @@ setup_rt_frame(int sig, struct k_sigacti
return ret;
}
+void xxx(struct k_sigaction *ka)
+{
+ sigset_t blocked;
+
+ sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
+ if (!(ka->sa.sa_flags & SA_NODEFER))
+ sigaddset(&blocked, sig);
+ set_current_blocked(&blocked);
+}
+
static int
handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka,
struct pt_regs *regs)
{
- sigset_t blocked;
int ret;
/* Are we from a system call? */
@@ -733,10 +742,7 @@ handle_signal(unsigned long sig, siginfo
*/
regs->flags &= ~X86_EFLAGS_TF;
- sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask);
- if (!(ka->sa.sa_flags & SA_NODEFER))
- sigaddset(&blocked, sig);
- set_current_blocked(&blocked);
+ xxx(ka);
tracehook_signal_handler(sig, info, ka, regs,
test_thread_flag(TIF_SINGLESTEP));
^ permalink raw reply [flat|nested] 87+ messages in thread