Message-ID: <1312362279.10579.38.camel@mfleming-mobl1.ger.corp.intel.com>
Date:	Wed, 03 Aug 2011 10:04:39 +0100
From:	Matt Fleming <matt@...sole-pimps.org>
To:	Haavard Skinnemoen <hskinnemoen@...il.com>,
	Hans-Christian Egtvedt <egtvedt@...fundet.no>
Cc:	linux-kernel <linux-kernel@...r.kernel.org>,
	Oleg Nesterov <oleg@...hat.com>
Subject: avr32: handle_signal() bug?

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



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists