signal: Use set_current_blocked()

PATCH In review Matt Fleming 8 participants 87 messages Aug 11, 2011 – Aug 18, 2011 1 message not in the archive

Subsystems (17)

Arm (32-bit) x86 VFS PowerPC +13 more subsystems

lore

Share & tools

lore

Matt Fleming

author

2011-08-11 13:57 UTC #0 lore raw

cover signal: Use set_current_blocked()

From: Matt Fleming <[email protected]>

Hi Oleg,

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.

This series also helps to reduce the complexity of my signal scalability patches that I posted in April, because we now have much fewer callers of recalc_sigpending() (the signal scalability patches introduce a new lock that needs to be held when calling recalc_sigpending()).

This series is available in the 'oleg/set-current-blocked' branch at,

git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/linux-2.6.git

P.S I've punted on fully converting ia64 to set_current_blocked() because bits of the signal code are written in asm. I'll follow up with a later patch that does the conversion once I've improved my ia64 foo (or coerced Tony Luck into doing it ;).

Matt Fleming (40):
alpha: Use set_current_blocked()
arm: Use set_current_blocked()
avr32: Don't mask signals in the error path
blackfin: Use set_current_blocked()
cris: Use set_current_blocked()
frv: Use set_current_blocked()
h8300: Use set_current_blocked()
ia64: Use set_current_blocked()
m32r: Use set_current_blocked()
m68k: Use set_current_blocked()
microblaze: Don't reimplement force_sigsegv()
microblaze: No need to reset handler if SA_ONESHOT
microblaze: Fix signal masking
microblaze: Use set_current_blocked()
MIPS: Use set_current_blocked()
mn10300: Use set_current_blocked()
OpenRISC: Don't reimplement force_sigsegv()
OpenRISC: No need to reset handler if SA_ONESHOT
OpenRISC: Don't mask signals if we fail to setup signal stack
OpenRISC: Use set_current_blocked()
parisc: Use set_current_blocked()
powerpc: Use set_current_blocked()
score: Don't mask signals if we fail to setup signal stack
score: Use set_current_blocked()
sh: No need to reset handler if SA_ONESHOT
sh: Use set_current_blocked()
sparc: Use set_current_blocked()
tile: Use set_current_blocked()
um: Use set_current_blocked()
um: Don't restore current->blocked on error
unicore32: Use set_current_blocked()
xtensa: Don't reimplement force_sigsegv()
xtensa: No need to reset handler if SA_ONESHOT
xtensa: Don't mask signals if we fail to setup signal stack
xtensa: Use set_current_blocked()
autofs4: Use set_current_blocked()
coda: Use set_current_blocked()
dlm: Remove another superfluous call to recalc_sigpending()
ncpfs: Use set_current_blocked()
exit: Use __set_task_blocked()

Oleg Nesterov (1):
avr32: use set_current_blocked() in handle_signal/sys_rt_sigreturn

 arch/alpha/kernel/signal.c         |   31 ++++++++++-------------
 arch/arm/kernel/signal.c           |   25 ++++++++----------
 arch/avr32/kernel/signal.c         |   30 +++++++++-------------
 arch/blackfin/kernel/signal.c      |   16 ++++-------
 arch/cris/arch-v10/kernel/signal.c |   33 ++++++++++--------------
 arch/cris/arch-v32/kernel/signal.c |   37 ++++++++++-----------------
 arch/frv/kernel/signal.c           |   32 ++++++++++--------------
 arch/h8300/kernel/signal.c         |   36 ++++++++++-----------------
 arch/ia64/kernel/signal.c          |   18 ++++---------
 arch/m32r/kernel/signal.c          |   16 +++++-------
 arch/m68k/kernel/signal_mm.c       |   24 +++++++++---------
 arch/m68k/kernel/signal_no.c       |   30 +++++++++-------------
 arch/microblaze/kernel/signal.c    |   46 ++++++++++++++++-------------------
 arch/mips/kernel/signal.c          |   29 ++++++----------------
 arch/mips/kernel/signal32.c        |   20 +++------------
 arch/mips/kernel/signal_n32.c      |   10 +------
 arch/mn10300/kernel/signal.c       |   32 ++++++++++--------------
 arch/openrisc/kernel/signal.c      |   47 ++++++++++++++++-------------------
 arch/parisc/kernel/signal.c        |   15 ++++-------
 arch/powerpc/kernel/signal.c       |   16 ++++-------
 arch/powerpc/kernel/signal_32.c    |   11 ++++----
 arch/score/kernel/signal.c         |   19 +++++++-------
 arch/sh/kernel/signal_32.c         |   35 ++++++++++----------------
 arch/sh/kernel/signal_64.c         |   40 ++++++++++--------------------
 arch/sparc/kernel/signal32.c       |   19 ++++----------
 arch/sparc/kernel/signal_32.c      |   30 +++++++++-------------
 arch/sparc/kernel/signal_64.c      |   30 +++++++++-------------
 arch/tile/kernel/compat_signal.c   |    5 +---
 arch/tile/kernel/signal.c          |   16 ++++-------
 arch/um/kernel/signal.c            |   30 ++++++++++-------------
 arch/um/sys-i386/signal.c          |   12 +-------
 arch/um/sys-x86_64/signal.c        |    6 +----
 arch/unicore32/kernel/signal.c     |   15 ++++-------
 arch/xtensa/kernel/signal.c        |   37 ++++++++++-----------------
 fs/autofs4/waitq.c                 |   15 ++++-------
 fs/coda/upcall.c                   |   19 ++++++--------
 fs/dlm/user.c                      |    1 -
 fs/ncpfs/sock.c                    |   12 ++++-----
 kernel/exit.c                      |    7 ++++-
 39 files changed, 351 insertions(+), 551 deletions(-)

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 14:06 UTC #1 lore raw

PATCH 1/41 alpha: Use set_current_blocked()

From: Matt Fleming <[email protected]>

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 <[email protected]>

Cc Richard Henderson <[email protected]>

Cc Ivan Kokshaysky <[email protected]>

Cc Matt Turner <[email protected]>

Cc Al Viro <[email protected]>

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

---
 arch/alpha/kernel/signal.c |   31 +++++++++++++------------------
 1 files changed, 13 insertions(+), 18 deletions(-)

arch/alpha/kernel/signal.c +13 ~~-18~~

index 6f7feb5..ec6fe13 100644
@@ -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(&current->sighand->siglock);
+	sigset_t blocked;
+
 	current->saved_sigmask = current->blocked;
-	siginitset(&current->blocked, mask);
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-		sigorsets(&current->blocked,&current->blocked,&ka->sa.sa_mask);
+		sigset_t blocked;
+
+		sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 		if (!(ka->sa.sa_flags & SA_NODEFER)) 
-			sigaddset(&current->blocked,sig);
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
+			sigaddset(&blocked, sig);
+		set_current_blocked(&blocked);
 	}
 
 	return ret;

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 13:57 UTC #2 lore raw

PATCH 2/41 arm: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Russell King <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Dave Martin <[email protected]>

Cc Nicolas Pitre <[email protected]>

CC Will Deacon <[email protected]>

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

---
 arch/arm/kernel/signal.c |   25 +++++++++++--------------
 1 files changed, 11 insertions(+), 14 deletions(-)

arch/arm/kernel/signal.c +11 ~~-14~~

index 0340224..b97668a 100644
@@ -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(&current->sighand->siglock);
+	sigset_t blocked;
+
 	current->saved_sigmask = current->blocked;
-	siginitset(&current->blocked, mask);
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-		current->blocked = set;
-		recalc_sigpending();
-		spin_unlock_irq(&current->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;
 }

Signature · 1 line

1.7.4.4

Will Deacon

Acked-by on v1 2/41

2011-08-11 17:05 UTC #3 lore raw

Re: [PATCH 02/41] arm: Use set_current_blocked()

Hi Matt,

Quoted 13 lines · Matt Fleming

From: Matt Fleming <[email protected]>
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 <[email protected]>
Cc Russell King <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Dave Martin <[email protected]>
Cc Nicolas Pitre <[email protected]>
CC Will Deacon <[email protected]>
Signed-off-by Matt Fleming <[email protected]>

Quoted 3 lines

---
 arch/arm/kernel/signal.c |   25 +++++++++++--------------
 1 files changed, 11 insertions(+), 14 deletions(-)

Looks good to me.

Acked-by Will Deacon <[email protected]>

Cheers,

Will

Matt Fleming

author

2011-08-11 14:06 UTC #4 lore raw

PATCH 3/41 avr32: Don't mask signals in the error path

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

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 <[email protected]>

Cc Hans-Christian Egtvedt <[email protected]>

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

---
 arch/avr32/kernel/signal.c |   25 ++++++++++++-------------
 1 files changed, 12 insertions(+), 13 deletions(-)

arch/avr32/kernel/signal.c +12 ~~-13~~

index 64f886f..9c075e1 100644
@@ -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(&current->sighand->siglock);
-		sigorsets(&current->blocked, &current->blocked,
-			  &ka->sa.sa_mask);
+	spin_lock_irq(&current->sighand->siglock);
+	sigorsets(&current->blocked, &current->blocked,
+		  &ka->sa.sa_mask);
+	if (!(ka->sa.sa_flags & SA_NODEFER))
 		sigaddset(&current->blocked, sig);
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
-	}
-
-	if (ret == 0)
-		return;
-
-	force_sigsegv(sig, current);
+	recalc_sigpending();
+	spin_unlock_irq(&current->sighand->siglock);
 }
 
 /*

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 14:05 UTC #5 lore raw

PATCH 4/41 avr32: use set_current_blocked() in handle_signal/sys_rt_sigreturn

In reply to Matt Fleming

From: Oleg Nesterov <[email protected]>

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 <[email protected]>

Cc Hans-Christian Egtvedt <[email protected]>

Reviewed-by Matt Fleming <[email protected]>

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

---
 arch/avr32/kernel/signal.c |   15 +++++----------
 1 files changed, 5 insertions(+), 10 deletions(-)

arch/avr32/kernel/signal.c +5 ~~-10~~

index 9c075e1..06f4293 100644
@@ -87,10 +87,7 @@ asmlinkage int sys_rt_sigreturn(struct pt_regs *regs)
 		goto badframe;
 
 	sigdelsetmask(&set, ~_BLOCKABLE);
-	spin_lock_irq(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&set);
 
 	if (restore_sigcontext(regs, &frame->uc.uc_mcontext))
 		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(&current->sighand->siglock);
-	sigorsets(&current->blocked, &current->blocked,
-		  &ka->sa.sa_mask);
+	sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 	if (!(ka->sa.sa_flags & SA_NODEFER))
-		sigaddset(&current->blocked, sig);
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+		sigaddset(&blocked, sig);
+	set_current_blocked(&blocked);
 }
 
 /*

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 13:57 UTC #6 lore raw

PATCH 5/41 blackfin: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

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 <[email protected]>

Cc Mike Frysinger <[email protected]>

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

---
 arch/blackfin/kernel/signal.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

arch/blackfin/kernel/signal.c +6 ~~-10~~

index d536f35..96b8666 100644
@@ -99,10 +99,7 @@ asmlinkage int do_rt_sigreturn(unsigned long __unused)
 		goto badframe;
 
 	sigdelsetmask(&set, ~_BLOCKABLE);
-	spin_lock_irq(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-		sigorsets(&current->blocked, &current->blocked,
-			  &ka->sa.sa_mask);
+		sigset_t blocked;
+
+		sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 		if (!(ka->sa.sa_flags & SA_NODEFER))
-			sigaddset(&current->blocked, sig);
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
+			sigaddset(&blocked, sig);
+		set_current_blocked(&blocked);
 	}
 	return ret;
 }

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 13:57 UTC #7 lore raw

PATCH 6/41 cris: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Mikael Starvik <[email protected]>

Cc Jesper Nilsson <[email protected]>

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

---
 arch/cris/arch-v10/kernel/signal.c |   33 +++++++++++++------------------
 arch/cris/arch-v32/kernel/signal.c |   37 +++++++++++++----------------------
 2 files changed, 28 insertions(+), 42 deletions(-)

arch/cris/arch-v10/kernel/signal.c +14 ~~-19~~

index e78fe49..9f8c29c 100644
@@ -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(&current->sighand->siglock);
+	sigset_t blocked;
+
 	current->saved_sigmask = current->blocked;
-	siginitset(&current->blocked, mask);
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&set);
 
 	if (restore_sigcontext(regs, &frame->uc.uc_mcontext))
 		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(&current->sighand->siglock);
-		sigorsets(&current->blocked, &current->blocked,
-			&ka->sa.sa_mask);
+		sigset_t blocked;
+
+		sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 		if (!(ka->sa.sa_flags & SA_NODEFER))
-			sigaddset(&current->blocked, sig);
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
+			sigaddset(&blocked, sig);
+		set_current_blocked(&blocked);
 	}
 	return ret;
 }

arch/cris/arch-v32/kernel/signal.c +14 ~~-23~~

index ce4ab1a..e493656 100644
@@ -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(&current->sighand->siglock);
+	sigset_t blocked;
+
 	current->saved_sigmask = current->blocked;
-	siginitset(&current->blocked, mask);
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-
-	current->blocked = set;
-
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-
-	current->blocked = set;
-
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&set);
 
 	if (restore_sigcontext(regs, &frame->uc.uc_mcontext))
 		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(&current->sighand->siglock);
-		sigorsets(&current->blocked, &current->blocked,
-			&ka->sa.sa_mask);
+		sigset_t blocked;
+
+		sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 		if (!(ka->sa.sa_flags & SA_NODEFER))
-			sigaddset(&current->blocked, sig);
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
+			sigaddset(&blocked, sig);
+		set_current_blocked(&blocked);
 	}
 
 	return ret;

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 13:57 UTC #8 lore raw

PATCH 7/41 frv: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc David Howells <[email protected]>

Cc Al Viro <[email protected]>

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

---
 arch/frv/kernel/signal.c |   32 +++++++++++++-------------------
 1 files changed, 13 insertions(+), 19 deletions(-)

arch/frv/kernel/signal.c +13 ~~-19~~

index bab0129..7f624e4 100644
@@ -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(&current->sighand->siglock);
+	sigset_t blocked;
+
 	current->saved_sigmask = current->blocked;
-	siginitset(&current->blocked, mask);
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-		sigorsets(&current->blocked, &current->blocked,
-			  &ka->sa.sa_mask);
+		sigset_t blocked;
+
+		sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 		if (!(ka->sa.sa_flags & SA_NODEFER))
-			sigaddset(&current->blocked, sig);
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
+			sigaddset(&blocked, sig);
+		set_current_blocked(&blocked);
 	}
 
 	return ret;

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 13:57 UTC #9 lore raw

PATCH 8/41 h8300: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Yoshinori Sato <[email protected]>

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

---
 arch/h8300/kernel/signal.c |   36 +++++++++++++-----------------------
 1 files changed, 13 insertions(+), 23 deletions(-)

arch/h8300/kernel/signal.c +13 ~~-23~~

index af842c3..4c06f98 100644
@@ -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(&current->sighand->siglock);
 	saveset = current->blocked;
-	siginitset(&current->blocked, mask);
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
 	saveset = current->blocked;
-	current->blocked = newset;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_lock_irq(&current->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(&current->sighand->siglock);
-	sigorsets(&current->blocked,&current->blocked,&ka->sa.sa_mask);
+	sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 	if (!(ka->sa.sa_flags & SA_NODEFER))
-		sigaddset(&current->blocked,sig);
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+		sigaddset(&blocked, sig);
+	set_current_blocked(&blocked);
 }
 
 /*

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 13:57 UTC #10 lore raw

PATCH 9/41 ia64: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Tony Luck <[email protected]>

Cc Fenghua Yu <[email protected]>

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

---
 arch/ia64/kernel/signal.c |   18 ++++++------------
 1 files changed, 6 insertions(+), 12 deletions(-)

arch/ia64/kernel/signal.c +6 ~~-12~~

index 7bdafc8..1261b2f 100644
@@ -201,13 +201,7 @@ ia64_rt_sigreturn (struct sigscratch *scr)
 		goto give_sigsegv;
 
 	sigdelsetmask(&set, ~_BLOCKABLE);
-
-	spin_lock_irq(&current->sighand->siglock);
-	{
-		current->blocked = set;
-		recalc_sigpending();
-	}
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&set);
 
 	if (restore_sigcontext(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(&current->sighand->siglock);
-	sigorsets(&current->blocked, &current->blocked, &ka->sa.sa_mask);
+	sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 	if (!(ka->sa.sa_flags & SA_NODEFER))
-		sigaddset(&current->blocked, sig);
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+		sigaddset(&blocked, sig);
+	set_current_blocked(&blocked);
 
 	/*
 	 * Let tracing know that we've done the handler setup.

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 14:05 UTC #11 lore raw

PATCH 10/41 m32r: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Hirokazu Takata <[email protected]>

Cc Al Viro <[email protected]>

Cc Kyle McMartin <[email protected]>

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

---
 arch/m32r/kernel/signal.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

arch/m32r/kernel/signal.c +7 -9

index a08697f..98f016a 100644
@@ -112,10 +112,7 @@ sys_rt_sigreturn(unsigned long r0, unsigned long r1,
 		goto badframe;
 
 	sigdelsetmask(&set, ~_BLOCKABLE);
-	spin_lock_irq(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&set);
 
 	if (restore_sigcontext(regs, &frame->uc.uc_mcontext, &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(&current->sighand->siglock);
-	sigorsets(&current->blocked,&current->blocked,&ka->sa.sa_mask);
+	sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 	if (!(ka->sa.sa_flags & SA_NODEFER))
-		sigaddset(&current->blocked,sig);
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+		sigaddset(&blocked, sig);
+	set_current_blocked(&blocked);
+
 	return 0;
 }

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 13:57 UTC #12 lore raw

PATCH 11/41 m68k: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Geert Uytterhoeven <[email protected]>

Cc Greg Ungerer <[email protected]>

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

---
 arch/m68k/kernel/signal_mm.c |   24 ++++++++++++------------
 arch/m68k/kernel/signal_no.c |   30 ++++++++++++------------------
 2 files changed, 24 insertions(+), 30 deletions(-)

arch/m68k/kernel/signal_mm.c +12 ~~-12~~

index a0afc23..eeeaf3c 100644
@@ -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(&current->sighand->siglock);
+	sigset_t blocked;
+
 	current->saved_sigmask = current->blocked;
-	siginitset(&current->blocked, mask);
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->blocked,&current->blocked,&ka->sa.sa_mask);
+	sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 	if (!(ka->sa.sa_flags & SA_NODEFER))
-		sigaddset(&current->blocked,sig);
-	recalc_sigpending();
+		sigaddset(&blocked, sig);
+	set_current_blocked(&blocked);
 
 	if (test_thread_flag(TIF_DELAYED_TRACE)) {
 		regs->sr &= ~0x8000;

arch/m68k/kernel/signal_no.c +12 ~~-18~~

index 36a81bb..97f3c39 100644
@@ -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(&current->sighand->siglock);
+	sigset_t blocked;
+
 	current->saved_sigmask = current->blocked;
-	siginitset(&current->blocked, mask);
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	sigorsets(&current->blocked,&current->blocked,&ka->sa.sa_mask);
+	sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 	if (!(ka->sa.sa_flags & SA_NODEFER))
-		sigaddset(&current->blocked,sig);
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+		sigaddset(&blocked, sig);
+	set_current_blocked(&blocked);
 
 	clear_thread_flag(TIF_RESTORE_SIGMASK);
 }

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 14:04 UTC #13 lore raw

PATCH 12/41 microblaze: Don't reimplement force_sigsegv()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

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 <[email protected]>

Cc Michal Simek <[email protected]>

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

---
 arch/microblaze/kernel/signal.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

arch/microblaze/kernel/signal.c +1 -3

index 5996711..90de06d 100644
@@ -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 */

Signature · 1 line

1.7.4.4

Oleg Nesterov

2011-08-16 17:23 UTC #14 lore raw

Re: [PATCH 12/41] microblaze: Don't reimplement force_sigsegv()

Quoted 17 lines · Matt Fleming

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 <[email protected]>
Cc Michal Simek <[email protected]>
Signed-off-by Matt Fleming <[email protected]>
---
 arch/microblaze/kernel/signal.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)
arch/microblaze/kernel/signal.c +0 -0
index 5996711..90de06d 100644

arch/microblaze/kernel/signal.c static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info, · line 274

Quoted 8 lines

arch/microblaze/kernel/signal.c +1 -3

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

Oleg Nesterov

2011-08-16 17:23 UTC #15 lore raw

Re: [PATCH 12/41] microblaze: Don't reimplement force_sigsegv()

Quoted 5 lines · Oleg Nesterov

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.

Michal Simek

maintainer Acked-by

2011-08-17 13:01 UTC #16 lore raw

Re: [PATCH 12/41] microblaze: Don't reimplement force_sigsegv()

Quoted 7 lines · Oleg Nesterov

Quoted 4 lines · Oleg Nesterov
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 <[email protected]> ?

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 <[email protected]>

If you want to propose 12-15 through my Microblaze tree, please let me know.

Thanks,
Michal

Signature · 4 lines

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

Oleg Nesterov

Acked-by

2011-08-17 13:17 UTC #17 lore raw

Re: [PATCH 12/41] microblaze: Don't reimplement force_sigsegv()

Quoted 6 lines · Michal Simek

Quoted 4 lines · Oleg Nesterov
Quoted 3 lines · Oleg Nesterov
This and the next patch seem to have the same problems with the changelogs as 18 and 19.

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
13-14 looks correct to me...
I meant 14-15, sorry for noise
Do you mean that there is
From: Matt Fleming <[email protected]> ?

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.

Quoted 3 lines

Anyway: I have looked at patches 12-15 and they look good to me.
Acked-by Michal Simek <[email protected]>

Assuming the changelog is fixed,

Acked-by Oleg Nesterov <[email protected]>

Quoted 1 line

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.

Matt Fleming

author

2011-08-17 13:25 UTC #18 lore raw

Re: [PATCH 12/41] microblaze: Don't reimplement force_sigsegv()

Quoted 4 lines · Oleg Nesterov

Assuming the changelog is fixed,
Acked-by Oleg Nesterov <[email protected]>

Thanks!

Quoted 3 lines

Quoted 1 line
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.

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Matt Fleming

author

2011-08-17 13:18 UTC #19 lore raw

Re: [PATCH 12/41] microblaze: Don't reimplement force_sigsegv()

In reply to Michal Simek

Quoted 7 lines · Michal Simek

Quoted 6 lines · Oleg Nesterov
Quoted 4 lines · Oleg Nesterov
This and the next patch seem to have the same problems with the changelogs as 18 and 19.
13-14 looks correct to me...

Quoted 8 lines

Quoted 1 line
I meant 14-15, sorry for noise
Do you mean that there is
From: Matt Fleming <[email protected]> ?
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.

Quoted 1 line

Acked-by Michal Simek <[email protected]>

Thanks.

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Michal Simek

maintainer

2011-08-17 13:22 UTC #20 lore raw

Re: [PATCH 12/41] microblaze: Don't reimplement force_sigsegv()

Quoted 11 lines · Matt Fleming

Quoted 10 lines · Michal Simek
Quoted 6 lines · Oleg Nesterov
Quoted 4 lines · Oleg Nesterov
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 <[email protected]> ?

Quoted 6 lines

Quoted 3 lines
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

Signature · 4 lines

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

Matt Fleming

author

2011-08-11 14:04 UTC #21 lore raw

PATCH 13/41 microblaze: No need to reset handler if SA_ONESHOT

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

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 <[email protected]>

Cc Michal Simek <[email protected]>

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

---
 arch/microblaze/kernel/signal.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

arch/microblaze/kernel/signal.c +0 -3

index 90de06d..9e749c0 100644
@@ -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(&current->sighand->siglock);
 		sigorsets(&current->blocked,

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 14:03 UTC #22 lore raw

PATCH 14/41 microblaze: Fix signal masking

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

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 <[email protected]>

Cc Michal Simek <[email protected]>

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

---
 arch/microblaze/kernel/signal.c |   31 ++++++++++++++++++-------------
 1 files changed, 18 insertions(+), 13 deletions(-)

arch/microblaze/kernel/signal.c +18 ~~-13~~

index 9e749c0..f2c13d5 100644
@@ -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(&current->sighand->siglock);
-		sigorsets(&current->blocked,
-				&current->blocked, &ka->sa.sa_mask);
+	spin_lock_irq(&current->sighand->siglock);
+	sigorsets(&current->blocked, &current->blocked, &ka->sa.sa_mask);
+	if (!(ka->sa.sa_flags & SA_NODEFER))
 		sigaddset(&current->blocked, sig);
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
-	}
-	return 1;
+	recalc_sigpending();
+	spin_unlock_irq(&current->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,

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 13:57 UTC #23 lore raw

PATCH 15/41 microblaze: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Michal Simek <[email protected]>

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

---
 arch/microblaze/kernel/signal.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)

arch/microblaze/kernel/signal.c +5 -9

index f2c13d5..aaf7c27 100644
@@ -105,10 +105,7 @@ asmlinkage long sys_rt_sigreturn(struct pt_regs *regs)
 		goto badframe;
 
 	sigdelsetmask(&set, ~_BLOCKABLE);
-	spin_lock_irq(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&set);
 
 	if (restore_sigcontext(regs, &frame->uc.uc_mcontext, &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(&current->sighand->siglock);
-	sigorsets(&current->blocked, &current->blocked, &ka->sa.sa_mask);
+	sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 	if (!(ka->sa.sa_flags & SA_NODEFER))
-		sigaddset(&current->blocked, sig);
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+		sigaddset(&blocked, sig);
+	set_current_blocked(&blocked);
 
 	return 0;
 }

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 13:58 UTC #24 lore raw

PATCH 16/41 MIPS: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Ralf Baechle <[email protected]>

Cc Al Viro <[email protected]>

Cc David Daney <[email protected]>

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

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

arch/mips/kernel/signal.c +8 ~~-21~~

index dbbe0ce..bd3f75b 100644
@@ -255,11 +255,8 @@ asmlinkage int sys_sigsuspend(nabi_no_regargs struct pt_regs regs)
 		return -EFAULT;
 	sigdelsetmask(&newset, ~_BLOCKABLE);
 
-	spin_lock_irq(&current->sighand->siglock);
 	current->saved_sigmask = current->blocked;
-	current->blocked = newset;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
 	current->saved_sigmask = current->blocked;
-	current->blocked = newset;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = blocked;
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&blocked);
 
 	sig = restore_sigcontext(&regs, &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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&set);
 
 	sig = restore_sigcontext(&regs, &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(&current->sighand->siglock);
-	sigorsets(&current->blocked, &current->blocked, &ka->sa.sa_mask);
+	sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 	if (!(ka->sa.sa_flags & SA_NODEFER))
-		sigaddset(&current->blocked, sig);
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+		sigaddset(&blocked, sig);
+	set_current_blocked(&blocked);
 
 	return ret;
 }

arch/mips/kernel/signal32.c +4 ~~-16~~

index aae9866..902a889 100644
@@ -290,11 +290,8 @@ asmlinkage int sys32_sigsuspend(nabi_no_regargs struct pt_regs regs)
 		return -EFAULT;
 	sigdelsetmask(&newset, ~_BLOCKABLE);
 
-	spin_lock_irq(&current->sighand->siglock);
 	current->saved_sigmask = current->blocked;
-	current->blocked = newset;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
 	current->saved_sigmask = current->blocked;
-	current->blocked = newset;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = blocked;
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&blocked);
 
 	sig = restore_sigcontext32(&regs, &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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&set);
 
 	sig = restore_sigcontext32(&regs, &frame->rs_uc.uc_mcontext);
 	if (sig < 0)

arch/mips/kernel/signal_n32.c +2 -8

index ee24d81..30fc7ff 100644
@@ -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(&current->sighand->siglock);
 	current->saved_sigmask = current->blocked;
-	current->blocked = newset;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&set);
 
 	sig = restore_sigcontext(&regs, &frame->rs_uc.uc_mcontext);
 	if (sig < 0)

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 13:58 UTC #25 lore raw

PATCH 17/41 mn10300: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc David Howells <[email protected]>

Cc Koichi Yasutake <[email protected]>

Cc Al Viro <[email protected]>

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

---
 arch/mn10300/kernel/signal.c |   32 +++++++++++++-------------------
 1 files changed, 13 insertions(+), 19 deletions(-)

arch/mn10300/kernel/signal.c +13 ~~-19~~

index 690f4e9..107fb4e 100644
@@ -38,12 +38,13 @@
  */
 asmlinkage long sys_sigsuspend(int history0, int history1, old_sigset_t mask)
 {
-	mask &= _BLOCKABLE;
-	spin_lock_irq(&current->sighand->siglock);
+	sigset_t blocked;
+
 	current->saved_sigmask = current->blocked;
-	siginitset(&current->blocked, mask);
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-		sigorsets(&current->blocked, &current->blocked,
-			  &ka->sa.sa_mask);
+		sigset_t blocked;
+
+		sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 		if (!(ka->sa.sa_flags & SA_NODEFER))
-			sigaddset(&current->blocked, sig);
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
+			sigaddset(&blocked, sig);
+		set_current_blocked(&blocked);
 	}
 
 	return ret;

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 13:58 UTC #26 lore raw

PATCH 18/41 OpenRISC: Don't reimplement force_sigsegv()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

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 <[email protected]>

Cc Jonas Bonn <[email protected]>

Cc Arnd Bergmann <[email protected]>

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

---
 arch/openrisc/kernel/signal.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

arch/openrisc/kernel/signal.c +1 -3

index 5f759c7..c023db9 100644
@@ -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

Signature · 1 line

1.7.4.4

Oleg Nesterov

2011-08-16 17:53 UTC #27 lore raw

Re: [PATCH 18/41] OpenRISC: Don't reimplement force_sigsegv()

Quoted 8 lines · Matt Fleming

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.

arch/openrisc/kernel/signal.c static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info, · line 261

Quoted 8 lines

arch/openrisc/kernel/signal.c +1 -3

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

Matt Fleming

author

2011-08-16 19:33 UTC #28 lore raw

Re: [PATCH 18/41] OpenRISC: Don't reimplement force_sigsegv()

Quoted 20 lines · Oleg Nesterov

Quoted 8 lines · Matt Fleming
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.
arch/openrisc/kernel/signal.c static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info, · line 261
Quoted 8 lines
arch/openrisc/kernel/signal.c +1 -3
@@ -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...

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 1 line

Unless I missed something, could you fix the changelog and resend?

Nope you haven't missed anything, you're spot on. I will resend.

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Oleg Nesterov

2011-08-18 17:50 UTC #29 lore raw

Re: [PATCH 18/41] OpenRISC: Don't reimplement force_sigsegv()

Quoted 7 lines · Matt Fleming

Quoted 3 lines · Oleg Nesterov
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.

Matt Fleming

author

2011-08-11 13:58 UTC #30 lore raw

PATCH 19/41 OpenRISC: No need to reset handler if SA_ONESHOT

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Jonas Bonn <[email protected]>

Cc Arnd Bergmann <[email protected]>

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

---
 arch/openrisc/kernel/signal.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

arch/openrisc/kernel/signal.c +0 -3

index c023db9..2f75e74 100644
@@ -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(&current->sighand->siglock);
 	sigorsets(&current->blocked, &current->blocked, &ka->sa.sa_mask);
 	if (!(ka->sa.sa_flags & SA_NODEFER))

Signature · 1 line

1.7.4.4

Oleg Nesterov

2011-08-16 16:56 UTC #31 lore raw

Re: [PATCH 19/41] OpenRISC: No need to reset handler if SA_ONESHOT

Quoted 3 lines · Matt Fleming

get_signal_to_deliver() already resets the signal handler if SA_ONESHOT is set in ka->sa.sa_flags,

Yes.

Quoted 2 lines

there's no need to do it again
in handle_signal().

Again, this code in fact does nothing, it plays with the copy.

Quoted 4 lines

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.

Matt Fleming

author

2011-08-11 14:03 UTC #32 lore raw

PATCH 20/41 OpenRISC: Don't mask signals if we fail to setup signal stack

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

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 <[email protected]>

Cc Jonas Bonn <[email protected]>

Cc Arnd Bergmann <[email protected]>

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

---
 arch/openrisc/kernel/signal.c |   29 ++++++++++++++++++-----------
 1 files changed, 18 insertions(+), 11 deletions(-)

arch/openrisc/kernel/signal.c +18 ~~-11~~

index 2f75e74..b0bdfda 100644
@@ -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(&current->sighand->siglock);
 	sigorsets(&current->blocked, &current->blocked, &ka->sa.sa_mask);

@@ -274,6 +279,8 @@ handle_signal(unsigned long sig,
 	recalc_sigpending();
 
 	spin_unlock_irq(&current->sighand->siglock);
+
+	return 0;
 }
 
 /*

@@ -362,13 +369,13 @@ void do_signal(struct pt_regs *regs)
 			oldset = &current->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));

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 14:03 UTC #33 lore raw

PATCH 21/41 OpenRISC: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Jonas Bonn <[email protected]>

Cc Arnd Bergmann <[email protected]>

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

---
 arch/openrisc/kernel/signal.c |   15 +++++----------
 1 files changed, 5 insertions(+), 10 deletions(-)

arch/openrisc/kernel/signal.c +5 ~~-10~~

index b0bdfda..50a5156 100644
@@ -108,10 +108,7 @@ asmlinkage long _sys_rt_sigreturn(struct pt_regs *regs)
 		goto badframe;
 
 	sigdelsetmask(&set, ~_BLOCKABLE);
-	spin_lock_irq(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&set);
 
 	if (restore_sigcontext(regs, &frame->uc.uc_mcontext))
 		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(&current->sighand->siglock);
-	sigorsets(&current->blocked, &current->blocked, &ka->sa.sa_mask);
+	sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 	if (!(ka->sa.sa_flags & SA_NODEFER))
-		sigaddset(&current->blocked, sig);
-	recalc_sigpending();
-
-	spin_unlock_irq(&current->sighand->siglock);
+		sigaddset(&blocked, sig);
+	set_current_blocked(&blocked);
 
 	return 0;
 }

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 13:58 UTC #34 lore raw

PATCH 22/41 parisc: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Kyle McMartin <[email protected]>

Cc Helge Deller <[email protected]>

Cc "James E.J. Bottomley" <[email protected]>

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

---
 arch/parisc/kernel/signal.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

arch/parisc/kernel/signal.c +6 -9

index 12c1ed3..4d64f72 100644
@@ -131,10 +131,7 @@ sys_rt_sigreturn(struct pt_regs *regs, int in_syscall)
 	}
 		
 	sigdelsetmask(&set, ~_BLOCKABLE);
-	spin_lock_irq(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	sigorsets(&current->blocked,&current->blocked,&ka->sa.sa_mask);
+	sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 	if (!(ka->sa.sa_flags & SA_NODEFER))
-		sigaddset(&current->blocked,sig);
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+		sigaddset(&blocked, sig);
+	set_current_blocked(&blocked);
 
 	tracehook_signal_handler(sig, info, ka, regs, 
 		test_thread_flag(TIF_SINGLESTEP) ||

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 14:02 UTC #35 lore raw

PATCH 23/41 powerpc: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Benjamin Herrenschmidt <[email protected]>

Cc Paul Mackerras <[email protected]>

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

---
 arch/powerpc/kernel/signal.c    |   16 ++++++----------
 arch/powerpc/kernel/signal_32.c |   11 ++++++-----
 2 files changed, 12 insertions(+), 15 deletions(-)

arch/powerpc/kernel/signal.c +6 ~~-10~~

index 2300426..8f6731f 100644
@@ -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(&current->sighand->siglock);
-	current->blocked = *set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-		sigorsets(&current->blocked, &current->blocked,
-			  &ka.sa.sa_mask);
+		sigset_t blocked;
+
+		sigorsets(&blocked, &current->blocked, &ka.sa.sa_mask);
 		if (!(ka.sa.sa_flags & SA_NODEFER))
-			sigaddset(&current->blocked, signr);
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
+			sigaddset(&blocked, signr);
+		set_current_blocked(&blocked);
 
 		/*
 		 * A signal was successfully delivered; the saved sigmask is in

arch/powerpc/kernel/signal_32.c +6 -5

index 78b76dc..5fc9d19 100644
@@ -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(&current->sighand->siglock);
+	sigset_t blocked;
+
 	current->saved_sigmask = current->blocked;
-	siginitset(&current->blocked, mask);
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+
+	mask &= _BLOCKABLE;
+	siginitset(&blocked, mask);
+	set_current_blocked(&blocked);
 
  	current->state = TASK_INTERRUPTIBLE;
  	schedule();

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 13:58 UTC #36 lore raw

PATCH 24/41 score: Don't mask signals if we fail to setup signal stack

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

If setup_rt_frame() returns -EFAULT then we must not block any signals in the current process.

Cc Chen Liqin <[email protected]>

Cc Lennox Wu <[email protected]>

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

---
 arch/score/kernel/signal.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

arch/score/kernel/signal.c +8 -6

index aa57440..bf9e33e 100644
@@ -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(&current->sighand->siglock);
-	sigorsets(&current->blocked, &current->blocked, &ka->sa.sa_mask);
-	if (!(ka->sa.sa_flags & SA_NODEFER))
-		sigaddset(&current->blocked, sig);
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	if (ret == 0) {
+		spin_lock_irq(&current->sighand->siglock);
+		sigorsets(&current->blocked, &current->blocked, &ka->sa.sa_mask);
+		if (!(ka->sa.sa_flags & SA_NODEFER))
+			sigaddset(&current->blocked, sig);
+		recalc_sigpending();
+		spin_unlock_irq(&current->sighand->siglock);
+	}
 
 	return ret;
 }

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 14:02 UTC #37 lore raw

PATCH 25/41 score: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Chen Liqin <[email protected]>

Cc Lennox Wu <[email protected]>

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

---
 arch/score/kernel/signal.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

arch/score/kernel/signal.c +6 -9

index bf9e33e..3ba0298 100644
@@ -159,10 +159,7 @@ score_rt_sigreturn(struct pt_regs *regs)
 		goto badframe;
 
 	sigdelsetmask(&set, ~_BLOCKABLE);
-	spin_lock_irq(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&set);
 
 	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(&current->sighand->siglock);
-		sigorsets(&current->blocked, &current->blocked, &ka->sa.sa_mask);
+		sigset_t blocked;
+
+		sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 		if (!(ka->sa.sa_flags & SA_NODEFER))
-			sigaddset(&current->blocked, sig);
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
+			sigaddset(&blocked, sig);
+		set_current_blocked(&blocked);
 	}
 
 	return ret;

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 14:01 UTC #38 lore raw

PATCH 26/41 sh: No need to reset handler if SA_ONESHOT

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Paul Mundt <[email protected]>

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

---
 arch/sh/kernel/signal_32.c |    3 ---
 arch/sh/kernel/signal_64.c |    3 ---
 2 files changed, 0 insertions(+), 6 deletions(-)

arch/sh/kernel/signal_32.c +0 -3

index 579cd2c..7a00304 100644
@@ -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(&current->sighand->siglock);
 		sigorsets(&current->blocked,&current->blocked,&ka->sa.sa_mask);

arch/sh/kernel/signal_64.c +0 -3

index 5a9f1f1..c0e7d10 100644
@@ -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(&current->sighand->siglock);
 		sigorsets(&current->blocked,&current->blocked,&ka->sa.sa_mask);

Signature · 1 line

1.7.4.4

Oleg Nesterov

2011-08-16 17:28 UTC #39 lore raw

Re: [PATCH 26/41] sh: No need to reset handler if SA_ONESHOT

Quoted 7 lines · Matt Fleming

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.

Matt Fleming

author

2011-08-11 13:58 UTC #40 lore raw

PATCH 27/41 sh: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Paul Mundt <[email protected]>

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

---
 arch/sh/kernel/signal_32.c |   32 +++++++++++++-------------------
 arch/sh/kernel/signal_64.c |   37 +++++++++++++------------------------
 2 files changed, 26 insertions(+), 43 deletions(-)

arch/sh/kernel/signal_32.c +13 ~~-19~~

index 7a00304..c97705f 100644
@@ -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(&current->sighand->siglock);
+	sigset_t blocked;
+
 	current->saved_sigmask = current->blocked;
-	siginitset(&current->blocked, mask);
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&set);
 
 	if (restore_sigcontext(regs, &frame->uc.uc_mcontext, &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(&current->sighand->siglock);
-		sigorsets(&current->blocked,&current->blocked,&ka->sa.sa_mask);
+		sigset_t blocked;
+
+		sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 		if (!(ka->sa.sa_flags & SA_NODEFER))
-			sigaddset(&current->blocked,sig);
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
+			sigaddset(&blocked, sig);
+		set_current_blocked(&blocked);
 	}
 
 	return ret;

arch/sh/kernel/signal_64.c +13 ~~-24~~

index c0e7d10..14d5ed4 100644
@@ -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(&current->sighand->siglock);
 	saveset = current->blocked;
-	siginitset(&current->blocked, mask);
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
 	saveset = current->blocked;
-	current->blocked = newset;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&set);
 
 	if (restore_sigcontext(regs, &frame->uc.uc_mcontext, &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(&current->sighand->siglock);
-		sigorsets(&current->blocked,&current->blocked,&ka->sa.sa_mask);
+		sigset_t blocked;
+
+		sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 		if (!(ka->sa.sa_flags & SA_NODEFER))
-			sigaddset(&current->blocked,sig);
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
+			sigaddset(&blocked, sig);
+		set_current_blocked(&blocked);
 	}
 
 	return ret;

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 14:01 UTC #41 lore raw

PATCH 28/41 sparc: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc "David S. Miller" <[email protected]>

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

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

arch/sparc/kernel/signal32.c +6 ~~-13~~

index 75fad42..4f381ce 100644
@@ -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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	sigorsets(&current->blocked,&current->blocked,&ka->sa.sa_mask);
+	sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 	if (!(ka->sa.sa_flags & SA_NOMASK))
-		sigaddset(&current->blocked,signr);
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+		sigaddset(&blocked, signr);
+	set_current_blocked(&blocked);
 
 	tracehook_signal_handler(signr, info, ka, regs, 0);

arch/sparc/kernel/signal_32.c +12 ~~-18~~

index 5e5c5fd..d65c31d 100644
@@ -60,12 +60,13 @@ struct rt_signal_frame {
 
 static int _sigpause_common(old_sigset_t set)
 {
-	set &= _BLOCKABLE;
-	spin_lock_irq(&current->sighand->siglock);
+	sigset_t blocked;
+
 	current->saved_sigmask = current->blocked;
-	siginitset(&current->blocked, set);
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	sigorsets(&current->blocked,&current->blocked,&ka->sa.sa_mask);
+	sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 	if (!(ka->sa.sa_flags & SA_NOMASK))
-		sigaddset(&current->blocked, signr);
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+		sigaddset(&blocked, signr);
+	set_current_blocked(&blocked);
 
 	tracehook_signal_handler(signr, info, ka, regs, 0);

arch/sparc/kernel/signal_64.c +12 ~~-18~~

index 006fe45..bb153d1 100644
@@ -69,10 +69,7 @@ asmlinkage void sparc64_set_context(struct pt_regs *regs)
 				goto do_sigsegv;
 		}
 		sigdelsetmask(&set, ~_BLOCKABLE);
-		spin_lock_irq(&current->sighand->siglock);
-		current->blocked = set;
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
+		set_current_blocked(&set);
 	}
 	if (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(&current->sighand->siglock);
+	sigset_t blocked;
+
 	current->saved_sigmask = current->blocked;
-	siginitset(&current->blocked, set);
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-	sigorsets(&current->blocked,&current->blocked,&ka->sa.sa_mask);
+	sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 	if (!(ka->sa.sa_flags & SA_NOMASK))
-		sigaddset(&current->blocked,signr);
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+		sigaddset(&blocked, signr);
+	set_current_blocked(&blocked);
 
 	tracehook_signal_handler(signr, info, ka, regs, 0);

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 14:01 UTC #42 lore raw

PATCH 29/41 tile: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Chris Metcalf <[email protected]>

Cc Arnd Bergmann <[email protected]>

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

---
 arch/tile/kernel/compat_signal.c |    5 +----
 arch/tile/kernel/signal.c        |   16 ++++++----------
 2 files changed, 7 insertions(+), 14 deletions(-)

arch/tile/kernel/compat_signal.c +1 -4

index a7869ad..77763cc 100644
@@ -303,10 +303,7 @@ long compat_sys_rt_sigreturn(struct pt_regs *regs)
 		goto badframe;
 
 	sigdelsetmask(&set, ~_BLOCKABLE);
-	spin_lock_irq(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&set);
 
 	if (restore_sigcontext(regs, &frame->uc.uc_mcontext))
 		goto badframe;

arch/tile/kernel/signal.c +6 ~~-10~~

index bedaf4e..6ddceec 100644
@@ -97,10 +97,7 @@ SYSCALL_DEFINE1(rt_sigreturn, struct pt_regs *, regs)
 		goto badframe;
 
 	sigdelsetmask(&set, ~_BLOCKABLE);
-	spin_lock_irq(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&set);
 
 	if (restore_sigcontext(regs, &frame->uc.uc_mcontext))
 		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(&current->sighand->siglock);
-		sigorsets(&current->blocked,
-			  &current->blocked, &ka->sa.sa_mask);
+		sigset_t blocked;
+
+		sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 		if (!(ka->sa.sa_flags & SA_NODEFER))
-			sigaddset(&current->blocked, sig);
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
+			sigaddset(&blocked, sig);
+		set_current_blocked(&blocked);
 	}
 
 	return ret;

Signature · 1 line

1.7.4.4

Chris Metcalf

Acked-by on v1 29/41

2011-08-11 17:10 UTC #43 lore raw

Re: [PATCH 29/41] tile: Use set_current_blocked()

Quoted 7 lines · Matt Fleming

From: Matt Fleming <[email protected]>
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.

Quoted 8 lines

Cc Oleg Nesterov <[email protected]>
Cc Chris Metcalf <[email protected]>
Cc Arnd Bergmann <[email protected]>
Signed-off-by Matt Fleming <[email protected]>
---
 arch/tile/kernel/compat_signal.c |    5 +----
 arch/tile/kernel/signal.c        |   16 ++++++----------
 2 files changed, 7 insertions(+), 14 deletions(-)

Acked-by Chris Metcalf <[email protected]>

Signature · 2 lines

Chris Metcalf, Tilera Corp.
http://www.tilera.com

Matt Fleming

author

2011-08-11 13:58 UTC #44 lore raw

PATCH 30/41 um: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Richard Weinberger <[email protected]>

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

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

arch/um/kernel/signal.c +11 ~~-11~~

index b5c094c..1812524 100644
@@ -73,13 +73,12 @@ static int handle_signal(struct pt_regs *regs, unsigned long signr,
 		spin_unlock_irq(&current->sighand->siglock);
 		force_sigsegv(signr, current);
 	} else {
-		spin_lock_irq(&current->sighand->siglock);
-		sigorsets(&current->blocked, &current->blocked,
-			  &ka->sa.sa_mask);
+		sigset_t blocked:
+
+		sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);
 		if (!(ka->sa.sa_flags & SA_NODEFER))
-			sigaddset(&current->blocked, signr);
-		recalc_sigpending();
-		spin_unlock_irq(&current->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(&current->sighand->siglock);
+	sigset_t blocked;
+
 	current->saved_sigmask = current->blocked;
-	siginitset(&current->blocked, mask);
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+
+	mask &= _BLOCKABLE;
+	siginitset(&blocked, mask);
+	set_current_blocked(&blocked);
 
 	current->state = TASK_INTERRUPTIBLE;
 	schedule();

arch/um/sys-i386/signal.c +2 ~~-10~~

index 89a4662..4403b48 100644
@@ -458,11 +458,7 @@ long sys_sigreturn(struct pt_regs regs)
 		goto segfault;
 
 	sigdelsetmask(&set, ~_BLOCKABLE);
-
-	spin_lock_irq(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&set);
 
 	if (copy_sc_from_user(&current->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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&set);
 
 	if (copy_sc_from_user(&current->thread.regs, &uc->uc_mcontext))
 		goto segfault;

arch/um/sys-x86_64/signal.c +1 -5

index b6b65c7..581b64d 100644
@@ -270,11 +270,7 @@ long sys_rt_sigreturn(struct pt_regs *regs)
 		goto segfault;
 
 	sigdelsetmask(&set, ~_BLOCKABLE);
-
-	spin_lock_irq(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&set);
 
 	if (copy_sc_from_user(&current->thread.regs, &uc->uc_mcontext,
 			      &frame->fpstate))

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 14:00 UTC #45 lore raw

PATCH 31/41 um: Don't restore current->blocked on error

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

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 <[email protected]>

Cc Richard Weinberger <[email protected]>

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

---
 arch/um/kernel/signal.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

arch/um/kernel/signal.c +2 -6

index 1812524..d177685 100644
@@ -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(&current->sighand->siglock);
-		current->blocked = *oldset;
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
+	if (err)
 		force_sigsegv(signr, current);
-	} else {
+	else {
 		sigset_t blocked:
 
 		sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);

Signature · 1 line

1.7.4.4

Oleg Nesterov

2011-08-16 17:41 UTC #46 lore raw

Re: [PATCH 31/41] um: Don't restore current->blocked on error

Quoted 4 lines · Matt Fleming

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

Quoted 30 lines

Cc Oleg Nesterov <[email protected]>

Cc Richard Weinberger <[email protected]>

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

---
 arch/um/kernel/signal.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

arch/um/kernel/signal.c +2 -6

index 1812524..d177685 100644
@@ -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(&current->sighand->siglock);
-		current->blocked = *oldset;
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
+	if (err)
 		force_sigsegv(signr, current);
-	} else {
+	else {
 		sigset_t blocked:
 
 		sigorsets(&blocked, &current->blocked, &ka->sa.sa_mask);

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 13:58 UTC #47 lore raw

PATCH 32/41 unicore32: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Guan Xuetao <[email protected]>

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

---
 arch/unicore32/kernel/signal.c |   15 +++++----------
 1 files changed, 5 insertions(+), 10 deletions(-)

arch/unicore32/kernel/signal.c +5 ~~-10~~

index b163fca..911b549 100644
@@ -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(&current->sighand->siglock);
-		current->blocked = set;
-		recalc_sigpending();
-		spin_unlock_irq(&current->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;
 }

Signature · 1 line

1.7.4.4

Guan Xuetao

Acked-by on v1 32/41

2011-08-18 08:33 UTC #48 lore raw

Re: [PATCH 32/41] unicore32: Use set_current_blocked()

It looks good to me.

And already tested by compilation.

Cc Arnd Bergmann <[email protected]>

Acked-by Guan Xuetao <[email protected]>

Thanks & Regards.

Guan Xuetao

Quoted 55 lines · Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Guan Xuetao <[email protected]>

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

---
 arch/unicore32/kernel/signal.c |   15 +++++----------
 1 files changed, 5 insertions(+), 10 deletions(-)

arch/unicore32/kernel/signal.c +5 ~~-10~~

index b163fca..911b549 100644
@@ -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(&current->sighand->siglock);
-		current->blocked = set;
-		recalc_sigpending();
-		spin_unlock_irq(&current->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;
 }

Matt Fleming

author

2011-08-11 13:58 UTC #49 lore raw

PATCH 33/41 xtensa: Don't reimplement force_sigsegv()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

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 <[email protected]>

Cc Chris Zankel <[email protected]>

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

---
 arch/xtensa/kernel/signal.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

arch/xtensa/kernel/signal.c +1 -3

index f2220b5..4f53770 100644
@@ -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);
 }
 
 /*

Signature · 1 line

1.7.4.4

Oleg Nesterov

2011-08-16 17:43 UTC #50 lore raw

Re: [PATCH 33/41] xtensa: Don't reimplement force_sigsegv()

Quoted 4 lines · Matt Fleming

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.

Quoted 25 lines

Cc Oleg Nesterov <[email protected]>

Cc Chris Zankel <[email protected]>

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

---
 arch/xtensa/kernel/signal.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

arch/xtensa/kernel/signal.c +1 -3

index f2220b5..4f53770 100644
@@ -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);
 }
 
 /*

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 13:58 UTC #51 lore raw

PATCH 34/41 xtensa: No need to reset handler if SA_ONESHOT

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Chris Zankel <[email protected]>

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

---
 arch/xtensa/kernel/signal.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

arch/xtensa/kernel/signal.c +0 -3

index 4f53770..24655e3 100644
@@ -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(&current->sighand->siglock);
 		sigorsets(&current->blocked, &current->blocked, &ka.sa.sa_mask);
 		if (!(ka.sa.sa_flags & SA_NODEFER))

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 14:00 UTC #52 lore raw

PATCH 35/41 xtensa: Don't mask signals if we fail to setup signal stack

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

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 <[email protected]>

Cc Chris Zankel <[email protected]>

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

---
 arch/xtensa/kernel/signal.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

arch/xtensa/kernel/signal.c +7 -4

index 24655e3..17ceab8 100644
@@ -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(&current->sighand->siglock);
 		sigorsets(&current->blocked, &current->blocked, &ka.sa.sa_mask);

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 13:59 UTC #53 lore raw

PATCH 36/41 xtensa: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Chris Zankel <[email protected]>

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

---
 arch/xtensa/kernel/signal.c |   19 ++++++-------------
 1 files changed, 6 insertions(+), 13 deletions(-)

arch/xtensa/kernel/signal.c +6 ~~-13~~

index 17ceab8..4df5da5 100644
@@ -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(&current->sighand->siglock);
-	current->blocked = set;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
 	saveset = current->blocked;
-	current->blocked = newset;
-	recalc_sigpending();
-	spin_unlock_irq(&current->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(&current->sighand->siglock);
-		sigorsets(&current->blocked, &current->blocked, &ka.sa.sa_mask);
+		sigorsets(&blocked, &current->blocked, &ka.sa.sa_mask);
 		if (!(ka.sa.sa_flags & SA_NODEFER))
-			sigaddset(&current->blocked, signr);
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
+			sigaddset(&blocked, signr);
+		set_current_blocked(&blocked);
 		if (current->ptrace & PT_SINGLESTEP)
 			task_pt_regs(current)->icountlevel = 1;

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 13:58 UTC #54 lore raw

PATCH 37/41 autofs4: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Ian Kent <[email protected]>

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

---
 fs/autofs4/waitq.c |   15 +++++----------
 1 files changed, 5 insertions(+), 10 deletions(-)

fs/autofs4/waitq.c +5 ~~-10~~

index e1fbdee..64ff15a 100644
@@ -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(&current->sighand->siglock, irqflags);
 		oldset = current->blocked;
-		siginitsetinv(&current->blocked, SHUTDOWN_SIGS & ~oldset.sig[0]);
-		recalc_sigpending();
-		spin_unlock_irqrestore(&current->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(&current->sighand->siglock, irqflags);
-		current->blocked = oldset;
-		recalc_sigpending();
-		spin_unlock_irqrestore(&current->sighand->siglock, irqflags);
+		
+		set_current_blocked(&oldset);
 	} else {
 		DPRINTK("skipped sleeping");
 	}

Signature · 1 line

1.7.4.4

Oleg Nesterov

2011-08-16 17:50 UTC #55 lore raw

Re: [PATCH 37/41] autofs4: Use set_current_blocked()

Quoted 5 lines · Matt Fleming

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

Quoted 13 lines

fs/autofs4/waitq.c +1 -5

@@ -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(&current->sighand->siglock, irqflags);
 		oldset = current->blocked;
-		siginitsetinv(&current->blocked, SHUTDOWN_SIGS & ~oldset.sig[0]);
-		recalc_sigpending();
-		spin_unlock_irqrestore(&current->sighand->siglock, irqflags);

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry, · line 469

Quoted 10 lines

fs/autofs4/waitq.c +3 -5

+		siginitsetinv(&blocked, SHUTDOWN_SIGS & ~oldset.sig[0]);
+		set_current_blocked(&blocked);
 
 		wait_event_interruptible(wq->queue, wq->name.name == NULL);
-
-		spin_lock_irqsave(&current->sighand->siglock, irqflags);
-		current->blocked = oldset;
-		recalc_sigpending();
-		spin_unlock_irqrestore(&current->sighand->siglock, irqflags);
+

this adds the trailing whitespaces ;)

Oleg.

Matt Fleming

author

2011-08-16 20:29 UTC #56 lore raw

Re: [PATCH 37/41] autofs4: Use set_current_blocked()

Quoted 28 lines · Oleg Nesterov

Quoted 5 lines · Matt Fleming
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...
Quoted 16 lines
fs/autofs4/waitq.c +3 -5
@@ -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(&current->sighand->siglock, irqflags);
 		oldset = current->blocked;
-		siginitsetinv(&current->blocked, SHUTDOWN_SIGS & ~oldset.sig[0]);
-		recalc_sigpending();
-		spin_unlock_irqrestore(&current->sighand->siglock, irqflags);
+		siginitsetinv(&blocked, SHUTDOWN_SIGS & ~oldset.sig[0]);
+		set_current_blocked(&blocked);
 

Quoted 9 lines

fs/autofs4/waitq.c int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry, · line 469
Quoted 7 lines
fs/autofs4/waitq.c +1 -5
 		wait_event_interruptible(wq->queue, wq->name.name == NULL);
-
-		spin_lock_irqsave(&current->sighand->siglock, irqflags);
-		current->blocked = oldset;
-		recalc_sigpending();
-		spin_unlock_irqrestore(&current->sighand->siglock, irqflags);
+		
this adds the trailing whitespaces ;)

Whoops! I fixed that and added your Acked-by. Thanks!

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Matt Fleming

author

2011-08-11 13:59 UTC #57 lore raw

PATCH 38/41 coda: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Jan Harkes <[email protected]>

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

---
 fs/coda/upcall.c |   19 ++++++++-----------
 1 files changed, 8 insertions(+), 11 deletions(-)

fs/coda/upcall.c +8 ~~-11~~

index 9727e0c..ce5925d 100644
@@ -575,24 +575,21 @@ int venus_statfs(struct dentry *dentry, struct kstatfs *sfs)
  */
 static void coda_block_signals(sigset_t *old)
 {
-	spin_lock_irq(&current->sighand->siglock);
+	sigset_t blocked;
+
 	*old = current->blocked;
 
-	sigfillset(&current->blocked);
-	sigdelset(&current->blocked, SIGKILL);
-	sigdelset(&current->blocked, SIGSTOP);
-	sigdelset(&current->blocked, SIGINT);
+	sigfillset(&blocked);
+	sigdelset(&blocked, SIGKILL);
+	sigdelset(&blocked, SIGSTOP);
+	sigdelset(&blocked, SIGINT);
 
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(&blocked);
 }
 
 static void coda_unblock_signals(sigset_t *old)
 {
-	spin_lock_irq(&current->sighand->siglock);
-	current->blocked = *old;
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
+	set_current_blocked(old);
 }
 
 /* Don't allow signals to interrupt the following upcalls before venus

Signature · 1 line

1.7.4.4

Matt Fleming

author

2011-08-11 13:59 UTC #58 lore raw

PATCH 39/41 dlm: Remove another superfluous call to recalc_sigpending()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

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 <[email protected]>

Cc David Teigland <[email protected]>

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

---
 fs/dlm/user.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

fs/dlm/user.c +0 -1

index d8ea607..b38b122 100644
@@ -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;
 }

Signature · 1 line

1.7.4.4

David Teigland

maintainer

2011-08-11 15:39 UTC #59 lore raw

Re: [PATCH 39/41] dlm: Remove another superfluous call to recalc_sigpending()

Quoted 6 lines · Matt Fleming

From: Matt Fleming <[email protected]>
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

Oleg Nesterov

2011-08-16 19:39 UTC #60 lore raw

Re: [PATCH 39/41] dlm: Remove another superfluous call to recalc_sigpending()

In reply to Matt Fleming

Quoted 3 lines · Matt Fleming

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.

Matt Fleming

author

2011-08-11 13:59 UTC #61 lore raw

PATCH 40/41 ncpfs: Use set_current_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Petr Vandrovec <[email protected]>

Cc Al Viro <[email protected]>

Cc Arnd Bergmann <[email protected]>

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

---
 fs/ncpfs/sock.c |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

fs/ncpfs/sock.c +5 -7

index 3a15872..883fdc3 100644
@@ -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(&current->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(&current->blocked, mask);
-		recalc_sigpending();
+
+		siginitsetinv(&blocked, mask);
+		__set_task_blocked(current, &blocked);
 		spin_unlock_irqrestore(&current->sighand->siglock, flags);
 		
 		result = do_ncp_rpc_call(server, size, reply, max_reply_size);
 
-		spin_lock_irqsave(&current->sighand->siglock, flags);
-		current->blocked = old_set;
-		recalc_sigpending();
-		spin_unlock_irqrestore(&current->sighand->siglock, flags);
+		set_current_blocked(&old_set);
 	}
 
 	DDPRINTK("do_ncp_rpc_call returned %d\n", result);

Signature · 1 line

1.7.4.4

Oleg Nesterov

2011-08-16 17:59 UTC #62 lore raw

Re: [PATCH 40/41] ncpfs: Use set_current_blocked()

Quoted 5 lines · Matt Fleming

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,

Quoted 10 lines

fs/ncpfs/sock.c +1 -1

@@ -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(&current->sighand->siglock, flags);

fs/ncpfs/sock.c static int ncp_do_request(struct ncp_server *server, int size, · line 775

Quoted 9 lines

fs/ncpfs/sock.c +3 -2

@@ -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(&current->blocked, mask);
-		recalc_sigpending();
+
+		siginitsetinv(&blocked, mask);
+		__set_task_blocked(current, &blocked);
 		spin_unlock_irqrestore(&current->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.

Matt Fleming

author

2011-08-16 20:56 UTC #63 lore raw

Re: [PATCH 40/41] ncpfs: Use set_current_blocked()

Quoted 8 lines · Oleg Nesterov

Quoted 5 lines · Matt Fleming
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.

Quoted 2 lines

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?

Quoted 24 lines

As for this patch, it looks (almost) fine anyway. But,

Quoted 10 lines

fs/ncpfs/sock.c +1 -1

@@ -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(&current->sighand->siglock, flags);

fs/ncpfs/sock.c static int ncp_do_request(struct ncp_server *server, int size, · line 775

Quoted 9 lines

fs/ncpfs/sock.c +3 -2

@@ -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(&current->blocked, mask);
-		recalc_sigpending();
+
+		siginitsetinv(&blocked, mask);
+		__set_task_blocked(current, &blocked);
 		spin_unlock_irqrestore(&current->sighand->siglock, flags);

Why do we take ->siglock in the first place?

Quoted 3 lines

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.

Quoted 2 lines

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.

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Oleg Nesterov

2011-08-17 12:07 UTC #64 lore raw

Re: [PATCH 40/41] ncpfs: Use set_current_blocked()

Quoted 11 lines · Matt Fleming

Quoted 8 lines · Oleg Nesterov
Quoted 5 lines · Matt Fleming
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 ;)

Quoted 8 lines

Yeah, I gave up after staring at it for about twenty minutes. I couldn't fathom the logic behind it.
Quoted 2 lines
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 ;)

Quoted 4 lines

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)

Quoted 6 lines

Quoted 5 lines
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 ;)

Quoted 7 lines

Eek! Sorry, I didn't realise this didn't compile.
Quoted 2 lines
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.

Matt Fleming

author

2011-08-17 13:58 UTC #65 lore raw

Re: [PATCH 40/41] ncpfs: Use set_current_blocked()

Quoted 11 lines · Oleg Nesterov

Quoted 6 lines · Matt Fleming
Quoted 3 lines
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<--------

Quoted 1 line

From bb1650295054bdfa96f8f4ff61507d314be8296a Mon Sep 17 00:00:00 2001

From: Matt Fleming <[email protected]>
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 <[email protected]>

---
 fs/ncpfs/sock.c |   32 +-------------------------------
 1 files changed, 1 insertions(+), 31 deletions(-)

fs/ncpfs/sock.c +1 ~~-31~~

index 3a15872..6618402 100644
@@ -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(&current->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(&current->blocked, mask);
-		recalc_sigpending();
-		spin_unlock_irqrestore(&current->sighand->siglock, flags);
-		
-		result = do_ncp_rpc_call(server, size, reply, max_reply_size);
 
-		spin_lock_irqsave(&current->sighand->siglock, flags);
-		current->blocked = old_set;
-		recalc_sigpending();
-		spin_unlock_irqrestore(&current->sighand->siglock, flags);
-	}
+	result = do_ncp_rpc_call(server, size, reply, max_reply_size);
 
 	DDPRINTK("do_ncp_rpc_call returned %d\n", result);

Signature · 1 line

1.7.4.4

Oleg Nesterov

2011-08-17 14:41 UTC #66 lore raw

Re: [PATCH 40/41] ncpfs: Use set_current_blocked()

Quoted 14 lines · Matt Fleming

Quoted 11 lines · Oleg Nesterov
Quoted 6 lines · Matt Fleming
Quoted 3 lines
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.

Quoted 7 lines

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.

Quoted 70 lines

From bb1650295054bdfa96f8f4ff61507d314be8296a Mon Sep 17 00:00:00 2001
From: Matt Fleming <[email protected]>
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 <[email protected]>
---
 fs/ncpfs/sock.c |   32 +-------------------------------
 1 files changed, 1 insertions(+), 31 deletions(-)
fs/ncpfs/sock.c +1 ~~-31~~
index 3a15872..6618402 100644
@@ -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(&current->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(&current->blocked, mask);
-		recalc_sigpending();
-		spin_unlock_irqrestore(&current->sighand->siglock, flags);
-		
-		result = do_ncp_rpc_call(server, size, reply, max_reply_size);
 
-		spin_lock_irqsave(&current->sighand->siglock, flags);
-		current->blocked = old_set;
-		recalc_sigpending();
-		spin_unlock_irqrestore(&current->sighand->siglock, flags);
-	}
+	result = do_ncp_rpc_call(server, size, reply, max_reply_size);
 
 	DDPRINTK("do_ncp_rpc_call returned %d\n", result);
 
Signature · 1 line
1.7.4.4

A message that is not in the archive (sent off-list or not yet received): CA+i2_De=mKMHj++b5=ZPdXxp7pm2KzY+PzCaG++GWSud20a_qQ@mail.gmail.com

Oleg Nesterov

2011-08-18 17:05 UTC #68 lore raw

Re: [PATCH 40/41] ncpfs: Use set_current_blocked()

Quoted 3 lines · Petr Vandrovec

I do not care about 'intr' support, but regular code path blocking all signals has to stay.

I do not care about SIGINT/SIGQUIT check, just I simply can't understand the logic.

But PF_EXITING/SIGKILL code looks absolutely strange.

Quoted 2 lines

PF_EXITING code is there because in the past something was BUG_ON-ing that nobody should touch signals while exiting,

Not sure I understand... do you mean we have a kernel bug? it should be fixed then.

Quoted 2 lines

and same code made sure you
cannot send signals to exiting process.

Again, can't understand.

A PF_EXITING doesn't need to block the signals at all. See wants_siganl().

But, otoh, even sigblock(SIGKILL) can't protect from SIGKILL or another fatal signal if this thread has a live subthread.

However, blocking SIGKILL makes the difference if it exits with the pending SIGKILL. In this case recalc_sigpending() clears TIF_SIGPENDING, this "helps" to actually sleep in TASK_INTERRUPTIBLE.

At the same time, ncp_do_request() can unblock, say, SIGINT. And this means that if this task exits because it was killed by SIGINT it won't block anyway.

And I can't understand why it if fine to kill the live task, but not fine to interrupt the exiting task. If this was intended. And if you want to block all signals, why can't you do wait_event_uninterruptible() instead? Or interruptible/uninterruptible depending on PF_EXITING.

Now suppose it is not PF_EXITING. In this case it is pointless to block any fatal signal unless this task is single-threaded.

So far I think this code does not understand what it does ;)

OK. I guess nobody cares. Matt, could you redo your original patch? just remove ->siglock and convert it to use set_current_blocked() leaving this logic alone.

Oleg.

Matt Fleming

author

2011-08-18 20:09 UTC #69 lore raw

Re: [PATCH 40/41] ncpfs: Use set_current_blocked()

Quoted 4 lines · Oleg Nesterov

OK. I guess nobody cares. Matt, could you redo your original patch? just remove ->siglock and convert it to use set_current_blocked() leaving this logic alone.

Sure, will do.

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Matt Fleming

author

2011-08-11 13:59 UTC #70 lore raw

PATCH 41/41 exit: Use __set_task_blocked()

In reply to Matt Fleming

From: Matt Fleming <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Andrew Morton <[email protected]>

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

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

kernel/exit.c +5 -2

index 2913b35..a47c0f3 100644
@@ -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(&current->sighand->siglock);
 	/* This is only needed for daemonize()'ed kthreads */
-	sigdelset(&current->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(&current->sighand->siglock);
 	return 0;
 }

Signature · 1 line

1.7.4.4

Oleg Nesterov

2011-08-16 18:10 UTC #71 lore raw

Re: [PATCH 41/41] exit: Use __set_task_blocked()

Quoted 5 lines · Matt Fleming

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

kernel/exit.c static void set_special_pids(struct pid *pid) · line 378

Quoted 9 lines

int allow_signal(int sig)
{

kernel/exit.c +2 -0

+	sigset_t blocked;
+
 	if (!valid_signal(sig) || sig < 1)
 		return -EINVAL;
 
 	spin_lock_irq(&current->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.

Oleg Nesterov

2011-08-16 19:47 UTC #72 lore raw

cover kthreads: allow_signal: don't play with ->blocked

Quoted 8 lines · Oleg Nesterov

Quoted 8 lines
int allow_signal(int sig)
{
kernel/exit.c +2 -0
+	sigset_t blocked;
+
 	if (!valid_signal(sig) || sig < 1)
 		return -EINVAL;

Quoted 8 lines

kernel/exit.c static void set_special_pids(struct pid *pid) · line 378
Quoted 2 lines
kernel/exit.c +0 -0
 	spin_lock_irq(&current->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.

Quoted 1 line

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.

Oleg Nesterov

2011-08-16 19:47 UTC #73 lore raw

PATCH 1/1 kthreads: allow_signal: don't play with ->blocked

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 <[email protected]>

---

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

kernel/exit.c renamed from kernel/exit.c~4_allow_signal_dont_unblock +2 -3

@@ -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(&current->sighand->siglock);
-	/* This is only needed for daemonize()'ed kthreads */
-	sigdelset(&current->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

Tejun Heo

2011-08-16 19:51 UTC #74 lore raw

Re: [PATCH 1/1] kthreads: allow_signal: don't play with ->blocked

Hello, Oleg.

Quoted 10 lines · Oleg Nesterov

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 <[email protected]>

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.

Signature · 1 line

tejun

Oleg Nesterov

2011-08-16 20:10 UTC #75 lore raw

Re: [PATCH 1/1] kthreads: allow_signal: don't play with ->blocked

Quoted 13 lines · Tejun Heo

Hello, Oleg.
Quoted 9 lines · Oleg Nesterov
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.

Quoted 7 lines

Quoted 1 line
Signed-off-by Oleg Nesterov <[email protected]>
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.

Matt Fleming

author

2011-08-16 21:50 UTC #76 lore raw

Re: [PATCH 1/1] kthreads: allow_signal: don't play with ->blocked

In reply to Tejun Heo

Quoted 6 lines · Tejun Heo

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?

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Tejun Heo

2011-08-17 07:27 UTC #77 lore raw

Re: [PATCH 1/1] kthreads: allow_signal: don't play with ->blocked

Hello,

Quoted 10 lines · Matt Fleming

Quoted 5 lines · Tejun Heo
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.

Signature · 1 line

tejun

Matt Fleming

author

2011-08-17 09:57 UTC #78 lore raw

Re: [PATCH 1/1] kthreads: allow_signal: don't play with ->blocked

Quoted 26 lines · Tejun Heo

Hello,
Quoted 4 lines · Matt Fleming
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.

Quoted 8 lines

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.

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Oleg Nesterov

2011-08-17 18:30 UTC #79 lore raw

Re: [PATCH 1/1] kthreads: allow_signal: don't play with ->blocked

In reply to Tejun Heo

Quoted 5 lines · Tejun Heo

Quoted 3 lines · Matt Fleming
Quoted 2 lines · Tejun Heo
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.

Quoted 3 lines

but in this case it's an interface which is
quite unpopular and with relatively easy workaround (just use kthread).

Agreed.

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Matt Fleming

author

2011-08-16 21:51 UTC #80 lore raw

Re: [PATCH 41/41] exit: Use __set_task_blocked()

In reply to Oleg Nesterov

Quoted 7 lines · Oleg Nesterov

Quoted 5 lines · Matt Fleming
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.

Quoted 7 lines

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.

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Oleg Nesterov

2011-08-11 16:06 UTC #81 lore raw

In reply to Matt Fleming

Hi Matt,

Quoted 4 lines · Matt Fleming

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.

Matt Fleming

author

2011-08-16 19:41 UTC #82 lore raw

Quoted 4 lines · Oleg Nesterov

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?

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Oleg Nesterov

2011-08-17 17:04 UTC #83 lore raw

Quoted 4 lines · Matt Fleming

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

Quoted 2 lines

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.

include/linux/signal.h +1 -0

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

kernel/signal.c +10 -0

@@ -2314,6 +2314,16 @@ relock:
 	return signr;
 }
 
+void xxx(struct k_sigaction *ka)
+{
+	sigset_t blocked;
+
+	sigorsets(&blocked, &current->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

arch/x86/kernel/signal.c +1 -6

@@ -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, &current->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;
 }

Matt Fleming

author

2011-08-17 22:17 UTC #84 lore raw

Quoted 9 lines · Oleg Nesterov

Quoted 4 lines · Matt Fleming
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

Quoted 7 lines

Quoted 2 lines
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.

Quoted 4 lines

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.

Quoted 8 lines

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?

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Matt Fleming

author

2011-08-18 11:09 UTC #85 lore raw

Quoted 10 lines · Matt Fleming

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.

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Oleg Nesterov

2011-08-18 18:39 UTC #86 lore raw

Quoted 9 lines · Matt Fleming

Quoted 7 lines · Matt Fleming
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.

Quoted 2 lines

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.

Quoted 1 line

Quoted 1 line
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.

Quoted 6 lines

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.

Quoted 1 line

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.

Oleg Nesterov

2011-08-16 20:01 UTC #87 lore raw

In reply to Matt Fleming

Quoted 4 lines · Matt Fleming

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.

arch/x86/kernel/signal.c +11 -5

@@ -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, &current->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, &current->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));