[PATCH 1/2] Support TIF_RESTORE_SIGMASK on x86_64

[PATCH 1/2] Support TIF_RESTORE_SIGMASK on x86_64

[David Woodhouse]

We need TIF_RESTORE_SIGMASK in order to support ppoll() and pselect()
system calls. This patch originally came from Andi, and was based
heavily on David Howells' implementation of same on i386. I fixed a typo
which was causing do_signal() to use the wrong signal mask.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>

diff --git a/arch/x86_64/ia32/ia32_signal.c b/arch/x86_64/ia32/ia32_signal.c
index 25e5ca2..549de43 100644
--- a/arch/x86_64/ia32/ia32_signal.c
+++ b/arch/x86_64/ia32/ia32_signal.c
@@ -113,25 +113,19 @@ int copy_siginfo_from_user32(siginfo_t *
 }
 
 asmlinkage long
-sys32_sigsuspend(int history0, int history1, old_sigset_t mask,
-		 struct pt_regs *regs)
+sys32_sigsuspend(int history0, int history1, old_sigset_t mask)
 {
-	sigset_t saveset;
-
 	mask &= _BLOCKABLE;
 	spin_lock_irq(&current->sighand->siglock);
-	saveset = current->blocked;
+	current->saved_sigmask = current->blocked;
 	siginitset(&current->blocked, mask);
 	recalc_sigpending();
 	spin_unlock_irq(&current->sighand->siglock);
 
-	regs->rax = -EINTR;
-	while (1) {
-		current->state = TASK_INTERRUPTIBLE;
-		schedule();
-		if (do_signal(regs, &saveset))
-			return -EINTR;
-	}
+	current->state = TASK_INTERRUPTIBLE;
+	schedule();
+	set_thread_flag(TIF_RESTORE_SIGMASK);
+	return -ERESTARTNOHAND;
 }
 
 asmlinkage long
@@ -508,11 +502,11 @@ #if DEBUG_SIG
 		current->comm, current->pid, frame, regs->rip, frame->pretcode);
 #endif
 
-	return 1;
+	return 0;
 
 give_sigsegv:
 	force_sigsegv(sig, current);
-	return 0;
+	return -EFAULT;
 }
 
 int ia32_setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
@@ -595,7 +589,7 @@ int ia32_setup_rt_frame(int sig, struct 
 	regs->ss = __USER32_DS; 
 
 	set_fs(USER_DS);
-    regs->eflags &= ~TF_MASK;
+	regs->eflags &= ~TF_MASK;
     if (test_thread_flag(TIF_SINGLESTEP))
         ptrace_notify(SIGTRAP);
 
@@ -604,9 +598,9 @@ #if DEBUG_SIG
 		current->comm, current->pid, frame, regs->rip, frame->pretcode);
 #endif
 
-	return 1;
+	return 0;
 
 give_sigsegv:
 	force_sigsegv(sig, current);
-	return 0;
+	return -EFAULT;
 }
diff --git a/arch/x86_64/kernel/signal.c b/arch/x86_64/kernel/signal.c
index 2816117..7f58bc9 100644
--- a/arch/x86_64/kernel/signal.c
+++ b/arch/x86_64/kernel/signal.c
@@ -38,37 +38,6 @@ int ia32_setup_frame(int sig, struct k_s
             sigset_t *set, struct pt_regs * regs); 
 
 asmlinkage long
-sys_rt_sigsuspend(sigset_t __user *unewset, size_t sigsetsize, struct pt_regs *regs)
-{
-	sigset_t saveset, newset;
-
-	/* XXX: Don't preclude handling different sized sigset_t's.  */
-	if (sigsetsize != sizeof(sigset_t))
-		return -EINVAL;
-
-	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);
-#ifdef DEBUG_SIG
-	printk("rt_sigsuspend savset(%lx) newset(%lx) regs(%p) rip(%lx)\n",
-		saveset, newset, regs, regs->rip);
-#endif 
-	regs->rax = -EINTR;
-	while (1) {
-		current->state = TASK_INTERRUPTIBLE;
-		schedule();
-		if (do_signal(regs, &saveset))
-			return -EINTR;
-	}
-}
-
-asmlinkage long
 sys_sigaltstack(const stack_t __user *uss, stack_t __user *uoss,
 		struct pt_regs *regs)
 {
@@ -341,11 +310,11 @@ #ifdef DEBUG_SIG
 		current->comm, current->pid, frame, regs->rip, frame->pretcode);
 #endif
 
-	return 1;
+	return 0;
 
 give_sigsegv:
 	force_sigsegv(sig, current);
-	return 0;
+	return -EFAULT;
 }
 
 /*
@@ -408,7 +377,7 @@ #ifdef CONFIG_IA32_EMULATION
 #endif
 	ret = setup_rt_frame(sig, ka, info, oldset, regs);
 
-	if (ret) {
+	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))
@@ -425,11 +394,12 @@ #endif
  * want to handle. Thus you cannot kill init even with a SIGKILL even by
  * mistake.
  */
-int do_signal(struct pt_regs *regs, sigset_t *oldset)
+static void do_signal(struct pt_regs *regs)
 {
 	struct k_sigaction ka;
 	siginfo_t info;
 	int signr;
+	sigset_t *oldset;
 
 	/*
 	 * We want the common case to go fast, which
@@ -438,9 +408,11 @@ int do_signal(struct pt_regs *regs, sigs
 	 * if so.
 	 */
 	if (!user_mode(regs))
-		return 1;
+		return;
 
-	if (!oldset)
+	if (test_thread_flag(TIF_RESTORE_SIGMASK))
+		oldset = &current->saved_sigmask;
+	else
 		oldset = &current->blocked;
 
 	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
@@ -454,30 +426,46 @@ int do_signal(struct pt_regs *regs, sigs
 			set_debugreg(current->thread.debugreg7, 7);
 
 		/* Whee!  Actually deliver the signal.  */
-		return handle_signal(signr, &info, &ka, oldset, regs);
+		if (handle_signal(signr, &info, &ka, oldset, regs) == 0) {
+			/* 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);
+		}
+		return;
 	}
 
 	/* Did we come from a system call? */
 	if ((long)regs->orig_rax >= 0) {
 		/* Restart the system call - no handlers present */
 		long res = regs->rax;
-		if (res == -ERESTARTNOHAND ||
-		    res == -ERESTARTSYS ||
-		    res == -ERESTARTNOINTR) {
+		switch (res) {
+		case -ERESTARTNOHAND:
+		case -ERESTARTSYS:
+		case -ERESTARTNOINTR:
 			regs->rax = regs->orig_rax;
 			regs->rip -= 2;
-		}
-		if (regs->rax == (unsigned long)-ERESTART_RESTARTBLOCK) {
+			break;
+		case -ERESTART_RESTARTBLOCK:
 			regs->rax = test_thread_flag(TIF_IA32) ?
 					__NR_ia32_restart_syscall :
 					__NR_restart_syscall;
 			regs->rip -= 2;
+			break;
 		}
 	}
-	return 0;
+
+	/* if there's no signal to deliver, we just 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);
+	}
 }
 
-void do_notify_resume(struct pt_regs *regs, sigset_t *oldset, __u32 thread_info_flags)
+void
+do_notify_resume(struct pt_regs *regs, void *unused, __u32 thread_info_flags)
 {
 #ifdef DEBUG_SIG
 	printk("do_notify_resume flags:%x rip:%lx rsp:%lx caller:%lx pending:%lx\n",
@@ -491,8 +479,8 @@ #endif
 	}
 
 	/* deal with pending signal delivery */
-	if (thread_info_flags & _TIF_SIGPENDING)
-		do_signal(regs,oldset);
+	if (thread_info_flags & (_TIF_SIGPENDING|_TIF_RESTORE_SIGMASK))
+		do_signal(regs);
 }
 
 void signal_fault(struct pt_regs *regs, void __user *frame, char *where)
diff --git a/include/asm-x86_64/signal.h b/include/asm-x86_64/signal.h
index cef7a7d..00f2d38 100644
--- a/include/asm-x86_64/signal.h
+++ b/include/asm-x86_64/signal.h
@@ -24,10 +24,6 @@ typedef struct {
 } sigset_t;
 
 
-struct pt_regs; 
-asmlinkage int do_signal(struct pt_regs *regs, sigset_t *oldset);
-
-
 #else
 /* Here we must cater to libcs that poke about in kernel headers.  */
 
diff --git a/include/asm-x86_64/thread_info.h b/include/asm-x86_64/thread_info.h
index 2029b00..790c512 100644
--- a/include/asm-x86_64/thread_info.h
+++ b/include/asm-x86_64/thread_info.h
@@ -114,6 +114,7 @@ #define TIF_SINGLESTEP		4	/* reenable si
 #define TIF_IRET		5	/* force IRET */
 #define TIF_SYSCALL_AUDIT	7	/* syscall auditing active */
 #define TIF_SECCOMP		8	/* secure computing */
+#define TIF_RESTORE_SIGMASK	9	/* restore signal mask in do_signal */
 /* 16 free */
 #define TIF_IA32		17	/* 32bit process */ 
 #define TIF_FORK		18	/* ret_from_fork */
@@ -128,6 +129,7 @@ #define _TIF_NEED_RESCHED	(1<<TIF_NEED_R
 #define _TIF_IRET		(1<<TIF_IRET)
 #define _TIF_SYSCALL_AUDIT	(1<<TIF_SYSCALL_AUDIT)
 #define _TIF_SECCOMP		(1<<TIF_SECCOMP)
+#define _TIF_RESTORE_SIGMASK	(1<<TIF_RESTORE_SIGMASK)
 #define _TIF_IA32		(1<<TIF_IA32)
 #define _TIF_FORK		(1<<TIF_FORK)
 #define _TIF_ABI_PENDING	(1<<TIF_ABI_PENDING)
diff --git a/include/asm-x86_64/unistd.h b/include/asm-x86_64/unistd.h
index 94387c9..c9db37d 100644
--- a/include/asm-x86_64/unistd.h
+++ b/include/asm-x86_64/unistd.h
@@ -658,6 +658,7 @@ #define __ARCH_WANT_SYS_OLDUMOUNT
 #define __ARCH_WANT_SYS_SIGPENDING
 #define __ARCH_WANT_SYS_SIGPROCMASK
 #define __ARCH_WANT_SYS_RT_SIGACTION
+#define __ARCH_WANT_SYS_RT_SIGSUSPEND
 #define __ARCH_WANT_SYS_TIME
 #define __ARCH_WANT_COMPAT_SYS_TIME
 


-- 
dwmw2

[PATCH 2/2] pselect/ppoll support on x86_64

[David Woodhouse]

This adds support for the pselect and ppoll system calls on x86_64. 

Andi suggests that it might be too late for the 2.6.18 merge window -- I
disagree. I consider it a bug fix, since I don't think we intend x86_64
to be a secondary architecture and lag behind i386 and PowerPC in its
system call support. Andi's TIF_RESTORE_SIGMASK implementation is
heavily based on the i386 version, where it's had lots of testing
already.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>

diff --git a/arch/x86_64/ia32/ia32entry.S b/arch/x86_64/ia32/ia32entry.S
index 5a92fed..07399fe 100644
--- a/arch/x86_64/ia32/ia32entry.S
+++ b/arch/x86_64/ia32/ia32entry.S
@@ -687,8 +687,8 @@ #endif
 	.quad sys_readlinkat		/* 305 */
 	.quad sys_fchmodat
 	.quad sys_faccessat
-	.quad quiet_ni_syscall		/* pselect6 for now */
-	.quad quiet_ni_syscall		/* ppoll for now */
+	.quad compat_sys_pselect6
+	.quad compat_sys_ppoll
 	.quad sys_unshare		/* 310 */
 	.quad compat_sys_set_robust_list
 	.quad compat_sys_get_robust_list
diff --git a/include/asm-x86_64/unistd.h b/include/asm-x86_64/unistd.h
index feb77cb..0a08bbf 100644
--- a/include/asm-x86_64/unistd.h
+++ b/include/asm-x86_64/unistd.h
@@ -600,9 +600,9 @@ __SYSCALL(__NR_fchmodat, sys_fchmodat)
 #define __NR_faccessat		269
 __SYSCALL(__NR_faccessat, sys_faccessat)
 #define __NR_pselect6		270
-__SYSCALL(__NR_pselect6, sys_ni_syscall)	/* for now */
+__SYSCALL(__NR_pselect6, sys_pselect6)
 #define __NR_ppoll		271
-__SYSCALL(__NR_ppoll,	sys_ni_syscall)		/* for now */
+__SYSCALL(__NR_ppoll,	sys_ppoll)
 #define __NR_unshare		272
 __SYSCALL(__NR_unshare,	sys_unshare)
 #define __NR_set_robust_list	273


-- 
dwmw2

Re: [PATCH 1/2] Support TIF_RESTORE_SIGMASK on x86_64

[Andrew Morton]

On Mon, 03 Jul 2006 10:41:51 +0100
David Woodhouse <dwmw2@infradead.org> wrote:

> We need TIF_RESTORE_SIGMASK in order to support ppoll() and pselect()
> system calls. This patch originally came from Andi, and was based
> heavily on David Howells' implementation of same on i386. I fixed a typo
> which was causing do_signal() to use the wrong signal mask.
> 

We struggled with these patches quite a lot when they were in Andi's tree. 
My test box would get stuck during kernel compiles and Andi could never
reproduce it.

<digs back through emails>

x:/home/akpm> ps aux|grep make
akpm      5064  0.0  0.0 40844 1112 pts/0    S+   17:30   0:00 make -j 6 CC= gcc bzImage modules
akpm      5364  0.0  0.0     0    0 pts/0    Z+   17:30   0:00 [make] <defunct>
akpm      5381  0.1  0.0     0    0 pts/0    Z+   17:30   0:00 [make] <defunct>
akpm      5451  0.0  0.0     0    0 pts/0    Z+   17:30   0:00 [make] <defunct>
akpm      5480  0.0  0.0     0    0 pts/0    Z+   17:30   0:00 [make] <defunct>
akpm      6004  0.0  0.0 41496  608 pts/1    S+   17:32   0:00 grep make

make          S ffff81017683fdc8     0  5064   5056  5364               (NOTLB)
ffff81017683fdc8 ffff81017aa7ca98 ffff81017982d4b8 ffff810178b7f810 
       0000000080168760 ffff81000705baa0 ffff81017fc0b480 ffff810179a24790 
       000000037aa7caa0 0000000000008944 
Call Trace: <ffffffff803cd4d4>{_spin_unlock+19} <ffffffff80178feb>{pipe_wait+119}
       <ffffffff8013eff9>{autoremove_wake_function+0} <ffffffff8013eff9>{autoremove_wake_function+0}
       <ffffffff8017934a>{pipe_readv+621} <ffffffff801793f1>{pipe_read+30}
       <ffffffff8016d29b>{vfs_read+175} <ffffffff8016d5fc>{sys_read+71}
       <ffffffff8010aa5e>{system_call+126}
make          X ffff810176389ed8     0  5364   5064          5381       (L-TLB)
ffff810176389ed8 ffff810176389e28 ffffffff803cd4a4 0000000000000000 
       ffff810175596850 ffff810176389ed8 0000000000000003 ffff810175596850 
       0000000300040001 000000000000c50a 
Call Trace: <ffffffff803cd4a4>{_spin_unlock_irqrestore+27}
       <ffffffff803cd4d4>{_spin_unlock+19} <ffffffff8012e5c5>{do_exit+2225}
       <ffffffff8016c00a>{sys_chdir+90} <ffffffff8012e6dc>{sys_exit_group+0}
       <ffffffff8012e6ee>{sys_exit_group+18} <ffffffff8010aa5e>{system_call+126}
make          X ffff810175199ed8     0  5381   5064          5451  5364 (L-TLB)
ffff810175199ed8 ffff810175199e28 ffffffff803cd4a4 0000000000000000 
       ffff810179a930c0 ffff810175199ed8 0000000000000002 ffff810179a930c0 
       0000000200040001 000000000000ca4c 
Call Trace: <ffffffff803cd4a4>{_spin_unlock_irqrestore+27}
       <ffffffff803cd4d4>{_spin_unlock+19} <ffffffff8012e5c5>{do_exit+2225}
       <ffffffff8016c00a>{sys_chdir+90} <ffffffff8012e6dc>{sys_exit_group+0}
       <ffffffff8012e6ee>{sys_exit_group+18} <ffffffff8010aa5e>{system_call+126}
make          X ffff810176249ed8     0  5451   5064          5480  5381 (L-TLB)
ffff810176249ed8 ffff810176249e28 ffffffff803cd4a4 0000000000000000 
       ffff8101790fe7d0 ffff810176249ed8 0000000000000001 ffff8101790fe7d0 
       0000000100040001 000000000000d408 
Call Trace: <ffffffff803cd4a4>{_spin_unlock_irqrestore+27}
       <ffffffff803cd4d4>{_spin_unlock+19} <ffffffff8012e5c5>{do_exit+2225}
       <ffffffff8016c00a>{sys_chdir+90} <ffffffff8012e6dc>{sys_exit_group+0}
       <ffffffff8012e6ee>{sys_exit_group+18} <ffffffff8010aa5e>{system_call+126}
make          X ffff810174f91ed8     0  5480   5064                5451 (L-TLB)
ffff810174f91ed8 ffff810174f91e28 ffffffff803cd4a4 0000000000000046 
       ffff810178b7f810 ffff810174f91ed8 0000000000000001 ffff810178b7f810 
       0000000100040001 00000000000106aa 
Call Trace: <ffffffff803cd4a4>{_spin_unlock_irqrestore+27}
       <ffffffff803cd4d4>{_spin_unlock+19} <ffffffff8012e5c5>{do_exit+2225}
       <ffffffff8016c00a>{sys_chdir+90} <ffffffff8012e6dc>{sys_exit_group+0}
       <ffffffff8012e6ee>{sys_exit_group+18} <ffffffff8010aa5e>{system_call+126}


It seems OK now with these patches.

Could you please describe the signal mask fix?  Is that likely to have
caused the above symptoms?

>  asmlinkage long
> -sys32_sigsuspend(int history0, int history1, old_sigset_t mask,
> -		 struct pt_regs *regs)
> +sys32_sigsuspend(int history0, int history1, old_sigset_t mask)
>  {
> -	sigset_t saveset;
> -
>  	mask &= _BLOCKABLE;
>  	spin_lock_irq(&current->sighand->siglock);
> -	saveset = current->blocked;
> +	current->saved_sigmask = current->blocked;
>  	siginitset(&current->blocked, mask);
>  	recalc_sigpending();
>  	spin_unlock_irq(&current->sighand->siglock);
>  
> -	regs->rax = -EINTR;
> -	while (1) {
> -		current->state = TASK_INTERRUPTIBLE;
> -		schedule();
> -		if (do_signal(regs, &saveset))
> -			return -EINTR;
> -	}
> +	current->state = TASK_INTERRUPTIBLE;
> +	schedule();
> +	set_thread_flag(TIF_RESTORE_SIGMASK);
> +	return -ERESTARTNOHAND;
>  }

Should we be setting TASK_INTERRUPTIBLE before releasing that lock?

Re: [PATCH 1/2] Support TIF_RESTORE_SIGMASK on x86_64

[David Woodhouse]

On Mon, 2006-07-03 at 03:19 -0700, Andrew Morton wrote:
> Could you please describe the signal mask fix? 

@@ -583,7 +583,7 @@
        if (!user_mode(regs))
                return;

-       if (!test_thread_flag(TIF_RESTORE_SIGMASK))
+       if (test_thread_flag(TIF_RESTORE_SIGMASK))
                oldset = &current->saved_sigmask;
        else
                oldset = &current->blocked;


>  Is that likely to have caused the above symptoms?

Yeah, it'll screw up the signal mask all over the place. 
cf. https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=180567
also https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=179228

> Should we be setting TASK_INTERRUPTIBLE before releasing that lock?

Before releasing current->sighand->siglock? Nah, schedule() will check
for pending signals -- it's not like racing with a wake_up() 

-- 
dwmw2

Re: [PATCH 2/2] pselect/ppoll support on x86_64

[Andi Kleen]

On Mon, Jul 03, 2006 at 10:47:15AM +0100, David Woodhouse wrote:
> This adds support for the pselect and ppoll system calls on x86_64. 
> 
> Andi suggests that it might be too late for the 2.6.18 merge window -- I
> disagree. I consider it a bug fix, since I don't think we intend x86_64
> to be a secondary architecture and lag behind i386 and PowerPC in its
> system call support. Andi's TIF_RESTORE_SIGMASK implementation is
> heavily based on the i386 version, where it's had lots of testing
> already.

I've got burned badly by these patches last time and the 2.6.18 merge
window is closed.

I'll add them for testing, but it's 2.6.19 material.

-Andi

Re: [PATCH 1/2] Support TIF_RESTORE_SIGMASK on x86_64

[Ulrich Drepper]

[-- Attachment #1: Type: text/plain, Size: 427 bytes --]

Andrew Morton wrote:
> We struggled with these patches quite a lot when they were in Andi's tree. 
> My test box would get stuck during kernel compiles and Andi could never
> reproduce it.

Try the patch.   dwmw2's change made all the difference for me.  The old
kernel wasn't able to handle the glibc test suite, this one does.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 251 bytes --]