Skip to contentSkip to conversation

consolidate do_signal

PATCH In review Christoph Hellwig 2 participants 3 messages Jun 2, 2007 – Jun 4, 2007 PowerPC

lore
Share & tools
lore
#0 lore raw

PATCH 3/3 consolidate do_signal

do_signal has exactly the same behaviour on 32bit and 64bit and 32bit compat on 64bit for handling 32bit signals. Consolidate all these into one common function in signal.c. The oly odd left over is the try_to_free in the 32bit version that no other architecture has in mainline (only in i386 for some odd SuSE release). We should probably get rid of it in a separate patch.

arch/powerpc/kernel/signal.c +104 -1
Index: linux-2.6/arch/powerpc/kernel/signal.c
===================================================================
@@ -7,12 +7,26 @@
  * this archive for more details.
  */
 
+#include <linux/freezer.h>
 #include <linux/ptrace.h>
 #include <linux/signal.h>
 
 #include "signal.h"
 
 
+#ifdef CONFIG_PPC64
+static inline int is_32bit_task(void)
+{
+	return test_thread_flag(TIF_32BIT);
+}
+#else
+static inline int is_32bit_task(void)
+{
+	return 1;
+}
+#endif
+
+
 /*
  * Restore the user process's signal mask
  */
@@ -25,7 +39,7 @@ void restore_sigmask(sigset_t *set)
 	spin_unlock_irq(&current->sighand->siglock);
 }
 
-void check_syscall_restart(struct pt_regs *regs, struct k_sigaction *ka,
+static void check_syscall_restart(struct pt_regs *regs, struct k_sigaction *ka,
 		int has_handler)
 {
 	unsigned long ret = regs->gpr[3];
@@ -76,6 +90,95 @@ void check_syscall_restart(struct pt_reg
 	}
 }
 
+int do_signal(sigset_t *oldset, struct pt_regs *regs)
+{
+	siginfo_t info;
+	int signr;
+	struct k_sigaction ka;
+	int ret;
+	int is32 = is_32bit_task();
+
+#ifdef CONFIG_PPC32
+	if (try_to_freeze()) {
+		signr = 0;
+		if (!signal_pending(current))
+			goto no_signal;
+	}
+#endif
+
+	if (test_thread_flag(TIF_RESTORE_SIGMASK))
+		oldset = &current->saved_sigmask;
+	else if (!oldset)
+		oldset = &current->blocked;
+
+	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
+
+#ifdef CONFIG_PPC32
+no_signal:
+#endif
+	/* Is there any syscall restart business here ? */
+	check_syscall_restart(regs, &ka, signr > 0);
+
+	if (signr <= 0) {
+		/* No signal to deliver -- put the saved sigmask back */
+		if (test_thread_flag(TIF_RESTORE_SIGMASK)) {
+			clear_thread_flag(TIF_RESTORE_SIGMASK);
+			sigprocmask(SIG_SETMASK, &current->saved_sigmask, NULL);
+		}
+		return 0;               /* no signals delivered */
+	}
+
+#ifdef CONFIG_PPC64
+        /*
+	 * Reenable the DABR before delivering the signal to
+	 * user space. The DABR will have been cleared if it
+	 * triggered inside the kernel.
+	 */
+	if (current->thread.dabr)
+		set_dabr(current->thread.dabr);
+#endif
+
+	if (is32) {
+		unsigned int newsp;
+
+		if ((ka.sa.sa_flags & SA_ONSTACK) &&
+		    current->sas_ss_size && !on_sig_stack(regs->gpr[1]))
+			newsp = current->sas_ss_sp + current->sas_ss_size;
+		else
+			newsp = regs->gpr[1];
+
+        	if (ka.sa.sa_flags & SA_SIGINFO)
+			ret = handle_rt_signal32(signr, &ka, &info, oldset,
+					regs, newsp);
+		else
+			ret = handle_signal32(signr, &ka, &info, oldset,
+					regs, newsp);
+#ifdef CONFIG_PPC64
+	} else {
+		ret = handle_rt_signal64(signr, &ka, &info, oldset, regs);
+#endif
+	}
+
+	if (ret) {
+		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, signr);
+		recalc_sigpending();
+		spin_unlock_irq(&current->sighand->siglock);
+
+		/*
+		 * A signal was successfully delivered; the saved sigmask is in
+		 * its frame, and we can clear the TIF_RESTORE_SIGMASK flag.
+		 */
+		if (test_thread_flag(TIF_RESTORE_SIGMASK))
+			clear_thread_flag(TIF_RESTORE_SIGMASK);
+	}
+
+	return ret;
+}
+
 long sys_sigaltstack(const stack_t __user *uss, stack_t __user *uoss,
 		unsigned long r5, unsigned long r6, unsigned long r7,
 		unsigned long r8, struct pt_regs *regs)
arch/powerpc/kernel/signal.h +8 -2
Index: linux-2.6/arch/powerpc/kernel/signal.h
===================================================================
@@ -2,5 +2,11 @@
 #define _BLOCKABLE (~(sigmask(SIGKILL) | sigmask(SIGSTOP)))
 
 void restore_sigmask(sigset_t *set);
-void check_syscall_restart(struct pt_regs *regs, struct k_sigaction *ka,
-		int has_handler);
+int handle_signal32(unsigned long sig, struct k_sigaction *ka,
+		siginfo_t *info, sigset_t *oldset, struct pt_regs *regs,
+		unsigned long newsp);
+int handle_rt_signal32(unsigned long sig, struct k_sigaction *ka,
+		siginfo_t *info, sigset_t *oldset,
+		struct pt_regs *regs, unsigned long newsp);
+int handle_rt_signal64(int signr, struct k_sigaction *ka, siginfo_t *info,
+		sigset_t *set, struct pt_regs *regs);
arch/powerpc/kernel/signal_32.c +2 -86

115 lines not on this page · raw message

arch/powerpc/kernel/signal_64.c +1 -85

95 lines not on this page · raw message

#1 lore raw
Quoted 6 lines · Christoph Hellwig

do_signal has exactly the same behaviour on 32bit and 64bit and 32bit compat on 64bit for handling 32bit signals. Consolidate all these into one common function in signal.c. The oly odd left over is the try_to_free in the 32bit version that no other architecture has in mainline (only in i386 for some odd SuSE release). We should probably get rid of it in a separate patch.

Excellent, that's something I was planning to do too, nice that you beat me to it :-)

Some comments after a quick look (I haven't gone deep into comparing old/new implementation, I'll do that later from work).

arch/powerpc/kernel/signal.c void check_syscall_restart(struct pt_reg · line 118

Quoted 4 lines
arch/powerpc/kernel/signal.c +4 -0
+
+#ifdef CONFIG_PPC32
+no_signal:
+#endif

That really need to go (the freeze stuff)

Quoted 11 lines
arch/powerpc/kernel/signal.c +11 -0
+	/* Is there any syscall restart business here ? */
+	check_syscall_restart(regs, &ka, signr > 0);
+
+	if (signr <= 0) {
+		/* No signal to deliver -- put the saved sigmask back */
+		if (test_thread_flag(TIF_RESTORE_SIGMASK)) {
+			clear_thread_flag(TIF_RESTORE_SIGMASK);
+			sigprocmask(SIG_SETMASK, &current->saved_sigmask, NULL);
+		}
+		return 0;               /* no signals delivered */
+	}

arch/powerpc/kernel/signal.c void check_syscall_restart(struct pt_reg · line 139

Quoted 10 lines
arch/powerpc/kernel/signal.c +10 -0
+
+#ifdef CONFIG_PPC64
+        /*
+	 * Reenable the DABR before delivering the signal to
+	 * user space. The DABR will have been cleared if it
+	 * triggered inside the kernel.
+	 */
+	if (current->thread.dabr)
+		set_dabr(current->thread.dabr);
+#endif

One of my patches is extending DABR to 32 bits, though I might have missed that bit. I think if you apply yours on top of mines, then that ifdef can go. (Note that I need to respin mines due to small changes, so we might decide to shuffle things and put yours first, just on top of the one of mine making the check restart common, and have the rest of my changes moved on top of those).

arch/powerpc/kernel/signal.c void check_syscall_restart(struct pt_reg · line 148

Quoted 8 lines
arch/powerpc/kernel/signal.c +8 -0
+	if (is32) {
+		unsigned int newsp;
+
+		if ((ka.sa.sa_flags & SA_ONSTACK) &&
+		    current->sas_ss_size && !on_sig_stack(regs->gpr[1]))
+			newsp = current->sas_ss_sp + current->sas_ss_size;
+		else
+			newsp = regs->gpr[1];

Hrm... some gratuituous differences in the signal stack handling.. I wonder if that hides a bug in one of the implementations...

arch/powerpc/kernel/signal.c void check_syscall_restart(struct pt_reg · line 160

Quoted 11 lines
arch/powerpc/kernel/signal.c +11 -0
+        	if (ka.sa.sa_flags & SA_SIGINFO)
+			ret = handle_rt_signal32(signr, &ka, &info, oldset,
+					regs, newsp);
+		else
+			ret = handle_signal32(signr, &ka, &info, oldset,
+					regs, newsp);
+#ifdef CONFIG_PPC64
+	} else {
+		ret = handle_rt_signal64(signr, &ka, &info, oldset, regs);
+#endif
+	}

I'd rather have handle_rt_signal64() itself be defined as an empty inline function than having an ifdef in here, don't you agree ?

Cheers,
Ben.

#2 lore raw
Quoted 10 lines · Benjamin Herrenschmidt

Review

Quoted 2 lines
(quoted diff) +2 -0
+             if ((ka.sa.sa_flags & SA_ONSTACK) &&
+                 current->sas_ss_size && !

on_sig_stack(regs->gpr[1]))

Quoted 1 line

+ newsp = current->sas_ss_sp +

current->sas_ss_size;

arch/powerpc/kernel/signal.c void check_syscall_restart(struct pt_reg · line 148

Quoted 2 lines
arch/powerpc/kernel/signal.c +2 -0
+             else
+                     newsp = regs->gpr[1];

Hrm... some gratuituous differences in the signal stack handling.. I wonder if that hides a bug in one of the implementations...

Ok, so, that bit is actually a bit nicer on 64 bits, where there's a get_sigframe() that handles this and is called by setup_rt_frame (which you renamed to handle_rt_signal64).

(Though the actual implementation of get_sigframe() could use some cleanup).

I'll do a patch on top of yours making that bit common.

Cheers,
Ben.