N32 sigset and __COMPAT_ENDIAN_SWAP__

N32 sigset and __COMPAT_ENDIAN_SWAP__

[Joseph S. Myers]

I'm testing glibc on MIPS64, little-endian, N32, O32 and N64
multilibs.

Among the NPTL test failures seen are some arising from sigsuspend
problems for N32: it blocks the wrong signals, so SIGCANCEL (SIGRTMIN)
is blocked despite glibc's carefully excluding it from sets of signals
to block.  Specifically, testing suggests it blocks signal N^32
instead of signal N, so (in the example tested) blocking SIGUSR1 (17)
blocks signal 49 instead.

glibc's sigset_t uses an array of unsigned long, as does the kernel.
In both cases, signal N+1 is represented as
(1UL << (N % (8 * sizeof (unsigned long)))) in word number
(N / (8 * sizeof (unsigned long))).

Thus the N32 glibc uses an array of 32-bit words and the N64 kernel
uses an array of 64-bit words.  For little-endian, the layout is the
same, with signals 1-32 in the first 4 bytes, signals 33-64 in the
second, etc.; for big-endian, userspace has that layout while in the
kernel each 8 bytes have the two halves swapped from the userspace
layout.

The N32 sigsuspend syscall uses sigset_from_compat to convert the
userspace sigset to kernel format.  If __COMPAT_ENDIAN_SWAP__ is *not*
set, this uses logic of the form

  set->sig[0] = compat->sig[0] | (((long)compat->sig[1]) << 32 )

to convert the userspace sigset to a kernel one.  This looks correct
to me for both big and little endian, given that in userspace
compat->sig[1] will represent signals 33-64, and so will the high 32
bits of set->sig[0] in the kernel.  If however __COMPAT_ENDIAN_SWAP__
*is* set, as it is for __MIPSEL__, it uses

  set->sig[0] = compat->sig[1] | (((long)compat->sig[0]) << 32 );

which seems incorrect for both big and little endian, and would
explain the observed symptoms.

This code is the only use of __COMPAT_ENDIAN_SWAP__, so if incorrect
then that macro serves no purpose, in which case something like the
following patch would seem appropriate to remove it.


Signed-off-by: Joseph Myers <joseph@codesourcery.com>

---

diff -rupN linux-2.6.17-rc6.orig/include/asm-mips/compat.h linux-2.6.17-rc6/include/asm-mips/compat.h
--- linux-2.6.17-rc6.orig/include/asm-mips/compat.h	2006-06-08 01:05:13.000000000 +0000
+++ linux-2.6.17-rc6/include/asm-mips/compat.h	2006-06-08 01:04:10.000000000 +0000
@@ -145,8 +145,5 @@ static inline void __user *compat_alloc_
 
 	return (void __user *) (regs->regs[29] - len);
 }
-#if defined (__MIPSEL__)
-#define __COMPAT_ENDIAN_SWAP__ 	1
-#endif
 
 #endif /* _ASM_COMPAT_H */
diff -rupN linux-2.6.17-rc6.orig/kernel/compat.c linux-2.6.17-rc6/kernel/compat.c
--- linux-2.6.17-rc6.orig/kernel/compat.c	2006-06-08 01:05:13.000000000 +0000
+++ linux-2.6.17-rc6/kernel/compat.c	2006-06-08 01:03:47.000000000 +0000
@@ -729,17 +729,10 @@ void
 sigset_from_compat (sigset_t *set, compat_sigset_t *compat)
 {
 	switch (_NSIG_WORDS) {
-#if defined (__COMPAT_ENDIAN_SWAP__)
-	case 4: set->sig[3] = compat->sig[7] | (((long)compat->sig[6]) << 32 );
-	case 3: set->sig[2] = compat->sig[5] | (((long)compat->sig[4]) << 32 );
-	case 2: set->sig[1] = compat->sig[3] | (((long)compat->sig[2]) << 32 );
-	case 1: set->sig[0] = compat->sig[1] | (((long)compat->sig[0]) << 32 );
-#else
 	case 4: set->sig[3] = compat->sig[6] | (((long)compat->sig[7]) << 32 );
 	case 3: set->sig[2] = compat->sig[4] | (((long)compat->sig[5]) << 32 );
 	case 2: set->sig[1] = compat->sig[2] | (((long)compat->sig[3]) << 32 );
 	case 1: set->sig[0] = compat->sig[0] | (((long)compat->sig[1]) << 32 );
-#endif
 	}
 }
 


-- 
Joseph S. Myers
joseph@codesourcery.com

Re: N32 sigset and __COMPAT_ENDIAN_SWAP__

[Kumba]

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


Joseph S. Myers wrote:
> I'm testing glibc on MIPS64, little-endian, N32, O32 and N64
> multilibs.
> 
> Among the NPTL test failures seen are some arising from sigsuspend
> problems for N32: it blocks the wrong signals, so SIGCANCEL (SIGRTMIN)
> is blocked despite glibc's carefully excluding it from sets of signals
> to block.  Specifically, testing suggests it blocks signal N^32
> instead of signal N, so (in the example tested) blocking SIGUSR1 (17)
> blocks signal 49 instead.
> 
> glibc's sigset_t uses an array of unsigned long, as does the kernel.
> In both cases, signal N+1 is represented as
> (1UL << (N % (8 * sizeof (unsigned long)))) in word number
> (N / (8 * sizeof (unsigned long))).
> 
> Thus the N32 glibc uses an array of 32-bit words and the N64 kernel
> uses an array of 64-bit words.  For little-endian, the layout is the
> same, with signals 1-32 in the first 4 bytes, signals 33-64 in the
> second, etc.; for big-endian, userspace has that layout while in the
> kernel each 8 bytes have the two halves swapped from the userspace
> layout.
> 
> The N32 sigsuspend syscall uses sigset_from_compat to convert the
> userspace sigset to kernel format.  If __COMPAT_ENDIAN_SWAP__ is *not*
> set, this uses logic of the form
> 
>   set->sig[0] = compat->sig[0] | (((long)compat->sig[1]) << 32 )
> 
> to convert the userspace sigset to a kernel one.  This looks correct
> to me for both big and little endian, given that in userspace
> compat->sig[1] will represent signals 33-64, and so will the high 32
> bits of set->sig[0] in the kernel.  If however __COMPAT_ENDIAN_SWAP__
> *is* set, as it is for __MIPSEL__, it uses
> 
>   set->sig[0] = compat->sig[1] | (((long)compat->sig[0]) << 32 );
> 
> which seems incorrect for both big and little endian, and would
> explain the observed symptoms.
> 
> This code is the only use of __COMPAT_ENDIAN_SWAP__, so if incorrect
> then that macro serves no purpose, in which case something like the
> following patch would seem appropriate to remove it.



You might find the attached patch of interest.

We've been using it gentoo-side for awhile now, as it allowed some N32 programs 
to work correctly.  Namely, a configure test in glib would trigger a non-fatal 
oops in the kernel due to an issue this patch fixes.  Daniel Jacobwitz wrote it 
up when he apparently stumbled across the issue (something wonky in n32's 
sigsuspend, as the name indicates.  I forget the specifics, though), but I'm 
unsure if the issue is in any way connected to what you're seeing as well.


--Kumba

-- 
Gentoo/MIPS Team Lead
Gentoo Foundation Board of Trustees

"Such is oft the course of deeds that move the wheels of the world: small hands 
do them because they must, while the eyes of the great are elsewhere."  --Elrond

[-- Attachment #2: misc-2.6.13-n32-fix-sigsuspend.patch --]
[-- Type: text/plain, Size: 3572 bytes --]

diff -Naurp linux-2.6.13.3.orig/arch/mips/kernel/linux32.c linux-2.6.13.3/arch/mips/kernel/linux32.c
--- linux-2.6.13.3.orig/arch/mips/kernel/linux32.c	2005-10-05 22:46:31.000000000 -0400
+++ linux-2.6.13.3/arch/mips/kernel/linux32.c	2005-10-05 22:34:45.000000000 -0400
@@ -1453,25 +1453,6 @@ sys32_timer_create(u32 clock, struct sig
 	return sys_timer_create(clock, p, timer_id);
 }
 
-asmlinkage long
-sysn32_rt_sigtimedwait(const sigset_t __user *uthese,
-		       siginfo_t __user *uinfo,
-		       const struct compat_timespec __user *uts32,
-		       size_t sigsetsize)
-{
-	struct timespec __user *uts = NULL;
-
-	if (uts32) {
-		struct timespec ts;
-		uts = compat_alloc_user_space(sizeof(struct timespec));
-		if (get_user(ts.tv_sec, &uts32->tv_sec) ||
-		    get_user(ts.tv_nsec, &uts32->tv_nsec) ||
-		    copy_to_user (uts, &ts, sizeof (ts)))
-			return -EFAULT;
-	}
-	return sys_rt_sigtimedwait(uthese, uinfo, uts, sigsetsize);
-}
-
 save_static_function(sys32_clone);
 __attribute_used__ noinline static int
 _sys32_clone(nabi_no_regargs struct pt_regs regs)
diff -Naurp linux-2.6.13.3.orig/arch/mips/kernel/scall64-n32.S linux-2.6.13.3/arch/mips/kernel/scall64-n32.S
--- linux-2.6.13.3.orig/arch/mips/kernel/scall64-n32.S	2005-10-05 22:46:31.000000000 -0400
+++ linux-2.6.13.3/arch/mips/kernel/scall64-n32.S	2005-10-05 22:34:45.000000000 -0400
@@ -243,9 +243,9 @@ EXPORT(sysn32_call_table)
 	PTR	sys_capget
 	PTR	sys_capset
 	PTR	sys32_rt_sigpending		/* 6125 */
-	PTR	sysn32_rt_sigtimedwait
+	PTR	compat_sys_rt_sigtimedwait
 	PTR	sys_rt_sigqueueinfo
-	PTR	sys32_rt_sigsuspend
+	PTR	sysn32_rt_sigsuspend
 	PTR	sys32_sigaltstack
 	PTR	compat_sys_utime		/* 6130 */
 	PTR	sys_mknod
diff -Naurp linux-2.6.13.3.orig/arch/mips/kernel/signal.c linux-2.6.13.3/arch/mips/kernel/signal.c
--- linux-2.6.13.3.orig/arch/mips/kernel/signal.c	2005-10-05 22:46:31.000000000 -0400
+++ linux-2.6.13.3/arch/mips/kernel/signal.c	2005-10-05 22:47:07.000000000 -0400
@@ -21,6 +21,7 @@
 #include <linux/ptrace.h>
 #include <linux/unistd.h>
 #include <linux/compiler.h>
+#include <linux/compat.h>
 
 #include <asm/abi.h>
 #include <asm/asm.h>
@@ -39,6 +40,10 @@
 
 #define _BLOCKABLE (~(sigmask(SIGKILL) | sigmask(SIGSTOP)))
 
+#ifdef CONFIG_MIPS32_N32
+extern void sigset_from_compat (sigset_t *set, compat_sigset_t *compat);
+#endif
+
 int do_signal(sigset_t *oldset, struct pt_regs *regs);
 
 /*
@@ -109,6 +114,43 @@ _sys_rt_sigsuspend(nabi_no_regargs struc
 	}
 }
 
+#ifdef CONFIG_MIPS32_N32
+save_static_function(sysn32_rt_sigsuspend);
+__attribute_used__ noinline static int
+_sysn32_rt_sigsuspend(nabi_no_regargs struct pt_regs regs)
+{
+	sigset_t saveset, newset;
+	compat_sigset_t __user *unewset, uset;
+	size_t sigsetsize;
+
+	/* XXX Don't preclude handling different sized sigset_t's.  */
+	sigsetsize = regs.regs[5];
+	if (sigsetsize != sizeof(sigset_t))
+		return -EINVAL;
+
+	unewset = (compat_sigset_t __user *) regs.regs[4];
+	if (copy_from_user(&uset, unewset, sizeof(uset)))
+		return -EFAULT;
+	sigset_from_compat (&newset, &uset);
+	sigdelsetmask(&newset, ~_BLOCKABLE);
+
+	spin_lock_irq(&current->sighand->siglock);
+	saveset = current->blocked;
+	current->blocked = newset;
+        recalc_sigpending();
+	spin_unlock_irq(&current->sighand->siglock);
+
+	regs.regs[2] = EINTR;
+	regs.regs[7] = 1;
+	while (1) {
+		current->state = TASK_INTERRUPTIBLE;
+		schedule();
+		if (do_signal(&saveset, &regs))
+			return -EINTR;
+	}
+}
+#endif
+
 #ifdef CONFIG_TRAD_SIGNALS
 asmlinkage int sys_sigaction(int sig, const struct sigaction *act,
 	struct sigaction *oact)

Re: N32 sigset and __COMPAT_ENDIAN_SWAP__

[Daniel Jacobowitz]

On Wed, Jun 07, 2006 at 10:25:46PM -0400, Kumba wrote:
> You might find the attached patch of interest.
> 
> We've been using it gentoo-side for awhile now, as it allowed some N32 
> programs to work correctly.  Namely, a configure test in glib would trigger 
> a non-fatal oops in the kernel due to an issue this patch fixes.  Daniel 
> Jacobwitz wrote it up when he apparently stumbled across the issue 
> (something wonky in n32's sigsuspend, as the name indicates.  I forget the 
> specifics, though), but I'm unsure if the issue is in any way connected to 
> what you're seeing as well.

It's already in the linux-mips.org tree, though, I believe.  That
was a different problem.

-- 
Daniel Jacobowitz
CodeSourcery

Re: N32 sigset and __COMPAT_ENDIAN_SWAP__

[Kumba]

Daniel Jacobowitz wrote:
> 
> It's already in the linux-mips.org tree, though, I believe.  That
> was a different problem.

This patch still applies to a 2.6.17 tree, so I don't think it's gone it yet.


--Kumba

-- 
Gentoo/MIPS Team Lead
Gentoo Foundation Board of Trustees

"Such is oft the course of deeds that move the wheels of the world: small hands 
do them because they must, while the eyes of the great are elsewhere."  --Elrond

Re: N32 sigset and __COMPAT_ENDIAN_SWAP__

[Kumba]

Kumba wrote:
> 
> This patch still applies to a 2.6.17 tree, so I don't think it's gone it 
> yet.

Ignore.  PEBKAC :P


--Kumba

-- 
Gentoo/MIPS Team Lead
Gentoo Foundation Board of Trustees

"Such is oft the course of deeds that move the wheels of the world: small hands 
do them because they must, while the eyes of the great are elsewhere."  --Elrond

Re: N32 sigset and __COMPAT_ENDIAN_SWAP__

[Ralf Baechle]

On Thu, Jun 08, 2006 at 01:36:29AM +0000, Joseph S. Myers wrote:

Interesting that a bug of this sort manages to survive for that long.
I guess it is proof that barely anybody is using 64-bit little endian,
yet we're cursed to support it.

  Ralf

Re: N32 sigset and __COMPAT_ENDIAN_SWAP__

[Daniel Jacobowitz]

On Thu, Jun 08, 2006 at 05:51:36PM +0100, Ralf Baechle wrote:
> On Thu, Jun 08, 2006 at 01:36:29AM +0000, Joseph S. Myers wrote:
> 
> Interesting that a bug of this sort manages to survive for that long.
> I guess it is proof that barely anybody is using 64-bit little endian,
> yet we're cursed to support it.

I expect more people will be using it someday...

Anyway, I was curious if you knew where this code had come from.  I
didn't see anything to suggest that anyone besides mipsel ever
used it, but it entered linux-mips.org via a merge from kernel.org,
just before git history.

Oh, right, there's a historical import:

http://www.kernel.org/git/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;a=commitdiff;h=32ed691a4efbc1c43584b7b7a6d782528241bb27

It was copied from sys32_rt_sigtimedwait, which was wrong at least back
to the initial revision of signal32.c.  I didn't go back any further.


-- 
Daniel Jacobowitz
CodeSourcery

Re: N32 sigset and __COMPAT_ENDIAN_SWAP__

[Joseph S. Myers]

On Thu, 8 Jun 2006, Ralf Baechle wrote:

> Interesting that a bug of this sort manages to survive for that long.
> I guess it is proof that barely anybody is using 64-bit little endian,
> yet we're cursed to support it.

I might conclude that barely anybody is *yet* using NPTL on 64-bit MIPS at 
all, for either endianness, given that most of the problems I've been 
finding, in glibc as well as the kernel, don't seem endian-specific and 
would probably show up in a glibc testsuite run for either endianness.  
MIPS64 NPTL is very new and seems to do a good job of showing up bugs in 
the three syscall interfaces.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: N32 sigset and __COMPAT_ENDIAN_SWAP__

[Ralf Baechle]

On Thu, Jun 08, 2006 at 01:03:10PM -0400, Daniel Jacobowitz wrote:

> Anyway, I was curious if you knew where this code had come from.  I
> didn't see anything to suggest that anyone besides mipsel ever
> used it, but it entered linux-mips.org via a merge from kernel.org,
> just before git history.
> 
> Oh, right, there's a historical import:
> 
> http://www.kernel.org/git/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;a=commitdiff;h=32ed691a4efbc1c43584b7b7a6d782528241bb27

> It was copied from sys32_rt_sigtimedwait, which was wrong at least back
> to the initial revision of signal32.c.  I didn't go back any further.

I can further track it into 2.4 or even pre-2.4 where such
__MIPSEB__ / __MIPSEL__ dependencies did exist under arch/mips64/.
I think it was right until a certain point in 2.5 when
get_sigset and put_sigset were implement in a clever way that
automatically takes care of the endianess issue - but the swapping code
outside arch/mips got forgotten.

  Ralf

Re: N32 sigset and __COMPAT_ENDIAN_SWAP__

[Kumba]

Joseph S. Myers wrote:
> 
> I might conclude that barely anybody is *yet* using NPTL on 64-bit MIPS at 
> all, for either endianness, given that most of the problems I've been 
> finding, in glibc as well as the kernel, don't seem endian-specific and 
> would probably show up in a glibc testsuite run for either endianness.  
> MIPS64 NPTL is very new and seems to do a good job of showing up bugs in 
> the three syscall interfaces.

I'm actually going to start running some automated builds of a nptl/o32 and 
nptl/n32 userland over the next few days.  If you have any patches that need 
testing to correct known oddities, I can give them a run in these builds.


--Kumba

-- 
Gentoo/MIPS Team Lead
Gentoo Foundation Board of Trustees

"Such is oft the course of deeds that move the wheels of the world: small hands 
do them because they must, while the eyes of the great are elsewhere."  --Elrond