avr32: handle_signal() bug?

PATCH In review Matt Fleming 3 participants 12 messages Aug 3, 2011 – Aug 17, 2011

lore

Share & tools

lore

Matt Fleming

author

2011-08-03 09:04 UTC #0 lore raw

Hey guys,

I was just looking at the code in handle_signal() and I got pretty confused, specifically about this part...

	/*
	 * Set up the stack frame
	 */
	ret = setup_rt_frame(sig, ka, info, oldset, regs);

	/*
	 * Check that the resulting registers are sane
	 */
	ret |= !valid_user_regs(regs);

	/*
	 * Block the signal if we were unsuccessful.
	 */
	if (ret != 0 || !(ka->sa.sa_flags & SA_NODEFER)) {
		spin_lock_irq(&current->sighand->siglock);
		sigorsets(&current->blocked, &current->blocked,
			  &ka->sa.sa_mask);
		sigaddset(&current->blocked, sig);
		recalc_sigpending();
		spin_unlock_irq(&current->sighand->siglock);
	}

	if (ret == 0)
		return;

That doesn't look correct to me. Now, if we were unsuccessful in setting up a signal frame, say, ret == -EFAULT, do we really want to block the signal or any of the signals in the handler mask?

Is there some intricacy of the avr32 architecture that I'm missing here? It looks to me like this code was copied from the arm implementation from years ago before commit a6c61e9dfdd0 ("[ARM] 3168/1: Update ARM signal delivery and masking").

How about this?

-------------8<-----------

Quoted 1 line

From 3fabe0f205b327dd82c2349a816f2574f2b78542 Mon Sep 17 00:00:00 2001

From: Matt Fleming <[email protected]>
Date: Wed, 3 Aug 2011 09:53:38 +0100
Subject: [PATCH] avr32: Don't mask signals in the error path

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

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

Oleg Nesterov

2011-08-03 13:11 UTC #1 lore raw

Quoted 11 lines · Matt Fleming

	 * Block the signal if we were unsuccessful.
	 */
	if (ret != 0 || !(ka->sa.sa_flags & SA_NODEFER)) {
		spin_lock_irq(&current->sighand->siglock);
		sigorsets(&current->blocked, &current->blocked,
			  &ka->sa.sa_mask);
		sigaddset(&current->blocked, sig);
		recalc_sigpending();
		spin_unlock_irq(&current->sighand->siglock);
	}

Agreed, this looks "obviously wrong". We should block the !SA_NODEFER signal it was delivered.

Quoted 1 line

Is there some intricacy of the avr32 architecture that I'm missing here?

same question here ;)

Quoted 25 lines

arch/avr32/kernel/signal.c +10 -6

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

arch/avr32/kernel/signal.c handle_signal(unsigned long sig, struct k_sigaction *ka, siginfo_t *info, · line 256

Quoted 10 lines

arch/avr32/kernel/signal.c +2 -7

-		spin_unlock_irq(&current->sighand->siglock);
-	}
-
-	if (ret == 0)
-		return;
-
-	force_sigsegv(sig, current);
+	recalc_sigpending();
+	spin_unlock_irq(&current->sighand->siglock);
 }

I think this is correct.

Oleg.

Oleg Nesterov

2011-08-03 13:42 UTC #2 lore raw

PATCH 1/1 avr32: use set_current_blocked() in handle_signal/sys_rt_sigreturn

Quoted 14 lines · Oleg Nesterov

Quoted 11 lines · Matt Fleming

	 * Block the signal if we were unsuccessful.
	 */
	if (ret != 0 || !(ka->sa.sa_flags & SA_NODEFER)) {
		spin_lock_irq(&current->sighand->siglock);
		sigorsets(&current->blocked, &current->blocked,
			  &ka->sa.sa_mask);
		sigaddset(&current->blocked, sig);
		recalc_sigpending();
		spin_unlock_irq(&current->sighand->siglock);
	}

Quoted 2 lines

Agreed, this looks "obviously wrong". We should block the !SA_NODEFER signal it was delivered.

(I meant, it is was successfully delivered).

While at it, I'd also suggest another patch on top of Matt's. Uncompiled/untested.

------------------------------------------------------------------------------
[PATCH] avr32: use set_current_blocked() in handle_signal/sys_rt_sigreturn

It is wrong to change ->blocked directly, see e6fa16ab. Change handle_signal() and sys_rt_sigreturn() to use the right helper, set_current_blocked().

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

 signal.c |   15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

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

@@ -87,10 +87,7 @@ asmlinkage int sys_rt_sigreturn(struct p
 		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 
 	/*
 	 * 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);
 }
 
 /*

Matt Fleming

author

2011-08-03 13:56 UTC #3 lore raw

Re: [PATCH] avr32: use set_current_blocked() in handle_signal/sys_rt_sigreturn

Quoted 18 lines · Oleg Nesterov

Quoted 14 lines · Oleg Nesterov
Quoted 11 lines · Matt Fleming
	 * Block the signal if we were unsuccessful.
	 */
	if (ret != 0 || !(ka->sa.sa_flags & SA_NODEFER)) {
		spin_lock_irq(&current->sighand->siglock);
		sigorsets(&current->blocked, &current->blocked,
			  &ka->sa.sa_mask);
		sigaddset(&current->blocked, sig);
		recalc_sigpending();
		spin_unlock_irq(&current->sighand->siglock);
	}
Quoted 2 lines
Agreed, this looks "obviously wrong". We should block the !SA_NODEFER signal it was delivered.

Quoted 7 lines

(I meant, it is was successfully delivered).
While at it, I'd also suggest another patch on top of Matt's. Uncompiled/untested.
------------------------------------------------------------------------------
[PATCH] avr32: use set_current_blocked() in handle_signal/sys_rt_sigreturn

Actually this was the reason that I spotted this bug. I'm going through all the code that sets current->blocked manually and making it use set_current_blocked() ;-)

Moving most of the recalc_pending() callers into one place (set_current_blocked()) makes my signal scalability series a lot smaller.

Håvard Skinnemoen

2011-08-07 17:20 UTC #4 lore raw

In reply to Matt Fleming

Hi Matt,

Quoted 3 lines · Matt Fleming

That doesn't look correct to me. Now, if we were unsuccessful in setting up a signal frame, say, ret == -EFAULT, do we really want to block the signal or any of the signals in the handler mask?

I'm assuming this is a rhetorical question :-)

Quoted 4 lines

Is there some intricacy of the avr32 architecture that I'm missing here? It looks to me like this code was copied from the arm implementation from years ago before commit a6c61e9dfdd0 ("[ARM] 3168/1: Update ARM signal delivery and masking").

I don't think there are any avr32-specific intricacies to consider here, and ARM was indeed one of the architectures I looked at when writing the signal handling code, so I probably picked up that bug from there.

Quoted 1 line

How about this?

Looks good to me. I'm not sure how to test it though...I can try to build a kernel, run it on my board and see if it explodes, but I suspect this bug is a lot more subtle than that.

Havard

Matt Fleming

author

2011-08-08 10:25 UTC #5 lore raw

Quoted 8 lines · Håvard Skinnemoen

Hi Matt,
Quoted 3 lines · Matt Fleming
That doesn't look correct to me. Now, if we were unsuccessful in setting up a signal frame, say, ret == -EFAULT, do we really want to block the signal or any of the signals in the handler mask?
I'm assuming this is a rhetorical question :-)

Sort of. I phrased it as a question so that someone could point out whether my analysis was correct or not ;-)

Quoted 3 lines

Looks good to me. I'm not sure how to test it though...I can try to build a kernel, run it on my board and see if it explodes, but I suspect this bug is a lot more subtle than that.

I suspect the best test would be one that makes use of SA_NODEFER. Something like this,

#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

void handler(int signum)

{
	sigset_t mask;

	sigprocmask(SIG_BLOCK, NULL, &mask);
	printf("SIGUSR2: %s\n",
	       sigismember(&mask, SIGUSR2) ? "blocked" : "not blocked");
	printf("SIGTERM: %s\n",
	       sigismember(&mask, SIGTERM) ? "blocked" : "not blocked");
}

int main(int argc, char **argv)

{
	pid_t pid;

	pid = fork();
	if (pid == -1) {
		perror("fork");
		exit(EXIT_FAILURE);
	} else if (!pid) {
		struct sigaction act;

		memset(&act, 0, sizeof(act));
		act.sa_handler = handler;
		act.sa_flags = SA_NODEFER;

		sigaddset(&act.sa_mask, SIGUSR2);
		sigaddset(&act.sa_mask, SIGTERM);

		sigaction(SIGUSR1, &act, NULL);

		pause();
	} else {
		int status;

		sleep(3);
		kill(pid, SIGUSR1);
		waitpid(pid, &status, 0);
	}

	return 0;
}

Without the patch applied I would expect this testcase to run the signal handler without SIGUSR2 or SIGTERM blocked. With the patch I'd hope you would see the following,

[matt@mfleming-mobl1 signal-tests]$ ./nodefer
SIGUSR2: blocked
SIGTERM: blocked

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Håvard Skinnemoen

2011-08-16 05:55 UTC #6 lore raw

Hi,

Sorry for taking so long to test this.

Quoted 7 lines · Matt Fleming

Quoted 3 lines · Håvard Skinnemoen
Looks good to me. I'm not sure how to test it though...I can try to build a kernel, run it on my board and see if it explodes, but I suspect this bug is a lot more subtle than that.
I suspect the best test would be one that makes use of SA_NODEFER. Something like this,

Thanks for the test. Unfortunately, the result is the same regardless of whether I apply the patches or not. In both cases:

/root # ./nodefer
SIGUSR2: not blocked
SIGTERM: not blocked

On my desktop, it behaves as expected:

$ ./nodefer-pc
SIGUSR2: blocked
SIGTERM: blocked

Your patch doesn't appear to do any harm though, and it looks correct to me. Perhaps there's another bug lurking somewhere as well. Some preliminary debugging makes me suspicious about libc, but I can't tell for sure yet.

Havard

Matt Fleming

author

2011-08-16 09:57 UTC #7 lore raw

Quoted 3 lines · Håvard Skinnemoen

Hi,
Sorry for taking so long to test this.

No worries!

Quoted 9 lines · Matt Fleming

Quoted 7 lines · Håvard Skinnemoen
Quoted 3 lines
Looks good to me. I'm not sure how to test it though...I can try to build a kernel, run it on my board and see if it explodes, but I suspect this bug is a lot more subtle than that.
I suspect the best test would be one that makes use of SA_NODEFER. Something like this,

Quoted 6 lines

Thanks for the test. Unfortunately, the result is the same regardless of whether I apply the patches or not. In both cases:
/root # ./nodefer
SIGUSR2: not blocked
SIGTERM: not blocked

Hmm.. that's interesting. I had a quick look through the rest of the code in the signal path and couldn't find anything obviously wrong. The only thing that looked suspicious is that you don't clear TIF_RESTORE_SIGMASK if you successsfully deliver a signal. Maybe try adding a clear_thread_flag(TIF_RESTORE_SIGMASK); to the success path in handle_signal() and see if you get better results? See the x86 implementation for more details.

Quoted 10 lines

On my desktop, it behaves as expected:
$ ./nodefer-pc
SIGUSR2: blocked
SIGTERM: blocked
Your patch doesn't appear to do any harm though, and it looks correct to me. Perhaps there's another bug lurking somewhere as well. Some preliminary debugging makes me suspicious about libc, but I can't tell for sure yet.

Which libc is this by the way?

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Oleg Nesterov

2011-08-16 15:42 UTC #8 lore raw

Quoted 6 lines · Matt Fleming

Quoted 4 lines · Håvard Skinnemoen
Thanks for the test. Unfortunately, the result is the same regardless of whether I apply the patches or not. In both cases:

Quoted 6 lines

Quoted 3 lines
/root # ./nodefer
SIGUSR2: not blocked
SIGTERM: not blocked
Hmm.. that's interesting. I had a quick look through the rest of the code in the signal path and couldn't find anything obviously wrong.

Agreed, I am puzzled too.

Quoted 3 lines

The
only thing that looked suspicious is that you don't clear TIF_RESTORE_SIGMASK if you successsfully deliver a signal.

Indeed this is wrong. TIF_RESTORE_SIGMASK should be always cleared, unless setup_rt_frame/valid_user_regs fails.

Quoted 3 lines

Maybe try
adding a clear_thread_flag(TIF_RESTORE_SIGMASK); to the success path in handle_signal() and see if you get better results?

I will be surprised if this helps with this particular test-case, but I agree this should be fixed anyway.

Quoted 6 lines

Quoted 4 lines
Your patch doesn't appear to do any harm though, and it looks correct to me. Perhaps there's another bug lurking somewhere as well. Some preliminary debugging makes me suspicious about libc, but I can't tell for sure yet.
Which libc is this by the way?

may be you can run the test-case under strace? On my machine strace -f -e rt_sigprocmask ./test shows

	[pid 25610] --- SIGUSR1 (User defined signal 1) @ 0 (0) ---
	[pid 25610] rt_sigprocmask(SIG_BLOCK, NULL, [USR2 TERM], 8) = 0

Oleg.

Håvard Skinnemoen

Acked-by

2011-08-17 05:14 UTC #9 lore raw

Quoted 7 lines · Oleg Nesterov

Quoted 3 lines · Matt Fleming
Maybe try
adding a clear_thread_flag(TIF_RESTORE_SIGMASK); to the success path in handle_signal() and see if you get better results?
I will be surprised if this helps with this particular test-case, but I agree this should be fixed anyway.

Yeah, we should get that fixed.

Quoted 5 lines

Quoted 5 lines
Quoted 4 lines
Your patch doesn't appear to do any harm though, and it looks correct to me. Perhaps there's another bug lurking somewhere as well. Some preliminary debugging makes me suspicious about libc, but I can't tell for sure yet.

Quoted 7 lines

Quoted 1 line
Which libc is this by the way?
may be you can run the test-case under strace? On my machine strace -f -e rt_sigprocmask ./test shows
[pid 25610] --- SIGUSR1 (User defined signal 1) @ 0 (0) --- [pid 25610] rt_sigprocmask(SIG_BLOCK, NULL, [USR2 TERM], 8) = 0

Hmm, doing that makes my board reboot. I guess we're having trouble with ptrace() as well...somehow the pid parameter to kill() gets turned into 1 when running under strace, and it just so happens that busybox's init interprets SIGUSR1 as "halt".

kill(1, SIGUSR1) = 0

Oh well, perhaps it's time to finally get those tracehooks implemented.

For now, please feel free to add

Acked-by Havard Skinnemoen <[email protected]>

to the two patches Matt sent earlier. I think they move things in the right direction even though there are other issues that need to be fixed as well.

Havard

Matt Fleming

author

2011-08-17 09:48 UTC #10 lore raw

Quoted 8 lines · Håvard Skinnemoen

For now, please feel free to add
Acked-by Havard Skinnemoen <[email protected]>
to the two patches Matt sent earlier. I think they move things in the right direction even though there are other issues that need to be fixed as well.

Thanks Håvard! I've added your Acked-by's.

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Håvard Skinnemoen

2011-08-17 04:32 UTC #11 lore raw

In reply to Matt Fleming

Quoted 5 lines · Matt Fleming

Quoted 3 lines · Håvard Skinnemoen
/root # ./nodefer
SIGUSR2: not blocked
SIGTERM: not blocked

Quoted 7 lines

Hmm.. that's interesting. I had a quick look through the rest of the code in the signal path and couldn't find anything obviously wrong. The only thing that looked suspicious is that you don't clear TIF_RESTORE_SIGMASK if you successsfully deliver a signal. Maybe try adding a clear_thread_flag(TIF_RESTORE_SIGMASK); to the success path in handle_signal() and see if you get better results? See the x86 implementation for more details.

Yeah, probably worth a try. I wonder if I should take the opportunity to make the signal handling code look more like x86. There's no good reason to be different, really, other than a natural tendency to drift apart over time.

Quoted 6 lines

Quoted 6 lines
On my desktop, it behaves as expected:
$ ./nodefer-pc
SIGUSR2: blocked
SIGTERM: blocked

Quoted 6 lines

Quoted 4 lines
Your patch doesn't appear to do any harm though, and it looks correct to me. Perhaps there's another bug lurking somewhere as well. Some preliminary debugging makes me suspicious about libc, but I can't tell for sure yet.
Which libc is this by the way?

uClibc-0.9.30. Dang, I thought I was running 0.9.31...I recall fixing several nasty bugs between those two versions, so perhaps I should upgrade it and see if it helps.

Havard