avr32: handle_signal() bug?

avr32: handle_signal() bug?

[Matt Fleming]

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

>From 3fabe0f205b327dd82c2349a816f2574f2b78542 Mon Sep 17 00:00:00 2001
From: Matt Fleming <matt.fleming@linux.intel.com>
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 <matt.fleming@linux.intel.com>
---
 arch/avr32/kernel/signal.c |   25 ++++++++++++-------------
 1 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/arch/avr32/kernel/signal.c b/arch/avr32/kernel/signal.c
index 64f886f..9c075e1 100644
--- a/arch/avr32/kernel/signal.c
+++ b/arch/avr32/kernel/signal.c
@@ -238,22 +238,21 @@ handle_signal(unsigned long sig, struct k_sigaction *ka, siginfo_t *info,
 	 */
 	ret |= !valid_user_regs(regs);
 
+	if (ret != 0) {
+		force_sigsegv(sig, current);
+		return;
+	}
+
 	/*
-	 * Block the signal if we were unsuccessful.
+	 * Block the signal if we were successful.
 	 */
-	if (ret != 0 || !(ka->sa.sa_flags & SA_NODEFER)) {
-		spin_lock_irq(&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);
 }
 
 /*
-- 
1.7.4.4

Re: avr32: handle_signal() bug?

[Oleg Nesterov]

On 08/03, Matt Fleming wrote:
>
> 	 * 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.

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

same question here ;)

> --- a/arch/avr32/kernel/signal.c
> +++ b/arch/avr32/kernel/signal.c
> @@ -238,22 +238,21 @@ handle_signal(unsigned long sig, struct k_sigaction *ka, siginfo_t *info,
>  	 */
>  	ret |= !valid_user_regs(regs);
>  
> +	if (ret != 0) {
> +		force_sigsegv(sig, current);
> +		return;
> +	}
> +
>  	/*
> -	 * Block the signal if we were unsuccessful.
> +	 * Block the signal if we were successful.
>  	 */
> -	if (ret != 0 || !(ka->sa.sa_flags & SA_NODEFER)) {
> -		spin_lock_irq(&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);
>  }

I think this is correct.

Oleg.

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

[Oleg Nesterov]

On 08/03, Oleg Nesterov wrote:
>
> On 08/03, Matt Fleming wrote:
> >
> > 	 * 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.

(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 <oleg@redhat.com>

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

--- x/arch/avr32/kernel/signal.c
+++ x/arch/avr32/kernel/signal.c
@@ -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);
 }
 
 /*

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

[Matt Fleming]

On Wed, 2011-08-03 at 15:39 +0200, Oleg Nesterov wrote:
> On 08/03, Oleg Nesterov wrote:
> >
> > On 08/03, Matt Fleming wrote:
> > >
> > > 	 * 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.
> 
> (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.

Re: avr32: handle_signal() bug?

[Håvard Skinnemoen]

Hi Matt,

On Wed, Aug 3, 2011 at 2:04 AM, Matt Fleming <matt@console-pimps.org> wrote:
> 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 :-)

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

> 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

Re: avr32: handle_signal() bug?

[Matt Fleming]

On Sun, 2011-08-07 at 10:20 -0700, Håvard Skinnemoen wrote:
> Hi Matt,
> 
> On Wed, Aug 3, 2011 at 2:04 AM, Matt Fleming <matt@console-pimps.org> wrote:
> > 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 ;-)

> 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

-- 
Matt Fleming, Intel Open Source Technology Center

Re: avr32: handle_signal() bug?

[Håvard Skinnemoen]

Hi,

Sorry for taking so long to test this.

On Mon, Aug 8, 2011 at 3:25 AM, Matt Fleming <matt@console-pimps.org> wrote:
> On Sun, 2011-08-07 at 10:20 -0700, Håvard Skinnemoen wrote:
>> 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

Re: avr32: handle_signal() bug?

[Matt Fleming]

On Mon, 2011-08-15 at 22:55 -0700, Håvard Skinnemoen wrote:
> Hi,
> 
> Sorry for taking so long to test this.

No worries!

> On Mon, Aug 8, 2011 at 3:25 AM, Matt Fleming <matt@console-pimps.org> wrote:
> > On Sun, 2011-08-07 at 10:20 -0700, Håvard Skinnemoen wrote:
> >> 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

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.

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

-- 
Matt Fleming, Intel Open Source Technology Center

Re: avr32: handle_signal() bug?

[Oleg Nesterov]

On 08/16, Matt Fleming wrote:
>
> On Mon, 2011-08-15 at 22:55 -0700, Håvard Skinnemoen wrote:
> >
> > 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.

Agreed, I am puzzled too.

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

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

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

Re: avr32: handle_signal() bug?

[Håvard Skinnemoen]

On Tue, Aug 16, 2011 at 2:57 AM, Matt Fleming <matt@console-pimps.org> wrote:
> On Mon, 2011-08-15 at 22:55 -0700, Håvard Skinnemoen wrote:
>> /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.

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.

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

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

Re: avr32: handle_signal() bug?

[Håvard Skinnemoen]

On Tue, Aug 16, 2011 at 8:39 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 08/16, Matt Fleming wrote:
>> 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.

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

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 <hskinnemoen@gmail.com>

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

Re: avr32: handle_signal() bug?

[Matt Fleming]

On Tue, 2011-08-16 at 22:14 -0700, Håvard Skinnemoen wrote:
> 
> For now, please feel free to add
> 
> Acked-by: Havard Skinnemoen <hskinnemoen@gmail.com>
> 
> 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.

-- 
Matt Fleming, Intel Open Source Technology Center