Patchwork sparc: Align clone and signal stacks to 16 bytes.

login
register
mail settings
Submitter David Miller
Date Feb. 10, 2010, 12:50 a.m.
Message ID <20100209.165006.142022647.davem@davemloft.net>
Download mbox | patch
Permalink /patch/44983/
State Accepted
Delegated to: David Miller
Headers show

Comments

David Miller - Feb. 10, 2010, 12:50 a.m.
[ Josip, when I hit this I thought that if it were a gcc bug it might
  explain your gcc-4.4 kernel failures.  That's why I haven't played
  with your machines yet.  Now that I know this is actually a kernel
  bug effecting only user applications, I will go pursue your
  issues. ]

I originally thought this was a gcc bug:

	http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43004

But it's actually a kernel issue.

When we clone or create a signal stack we only 8-byte align the stack
frame, it should be 16-byte aligned on 64-bit.  Use 16-byte alignment
for 32-bit too since that's harmless and it's cheaper that way.

Distribution folks you really want this in your kernels, otherwise
64-bit threads and signal handlers will have broken alloca() and
corrupt their stacks.

I'll push this to Linus and -stable.

--------------------
sparc: Align clone and signal stacks to 16 bytes.

This is mandatory for 64-bit processes, and doing it also for 32-bit
processes saves a conditional in the compat case.

This fixes the glibc/nptl/tst-stdio1 test case, as well
as many others, on 64-bit.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 arch/sparc/kernel/process_32.c |    2 +-
 arch/sparc/kernel/process_64.c |    8 ++++----
 arch/sparc/kernel/signal32.c   |   10 ++++++----
 arch/sparc/kernel/signal_32.c  |    6 ++++--
 arch/sparc/kernel/signal_64.c  |    8 +++++---
 5 files changed, 20 insertions(+), 14 deletions(-)
Dennis Gilmore - Feb. 10, 2010, 7:59 p.m.
On Tuesday 09 February 2010 06:50:06 pm David Miller wrote:
> [ Josip, when I hit this I thought that if it were a gcc bug it might
>   explain your gcc-4.4 kernel failures.  That's why I haven't played
>   with your machines yet.  Now that I know this is actually a kernel
>   bug effecting only user applications, I will go pursue your
>   issues. ]
> 
> I originally thought this was a gcc bug:
> 
> 	http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43004
> 
> But it's actually a kernel issue.
> 
> When we clone or create a signal stack we only 8-byte align the stack
> frame, it should be 16-byte aligned on 64-bit.  Use 16-byte alignment
> for 32-bit too since that's harmless and it's cheaper that way.
> 
> Distribution folks you really want this in your kernels, otherwise
> 64-bit threads and signal handlers will have broken alloca() and
> corrupt their stacks.

I thought that maybe this would help with a localdef generation issue that is 
being seen while building glibc,  at least for debian and fedora.  however it 
seems not.  I have built the latest Fedora 12 stable kernel with your patch 
applied and glibc failed to build still.

patches had been submitted to glibc 
http://sources.redhat.com/bugzilla/show_bug.cgi?id=10855
http://sources.redhat.com/bugzilla/show_bug.cgi?id=10283

however one had been outright rejected and the sparc specific one has not been 
looked at.   Is this something that should be fixed in the kernel?

Dennis
David Miller - Feb. 10, 2010, 8:12 p.m.
From: Dennis Gilmore <dennis@ausil.us>
Date: Wed, 10 Feb 2010 13:59:00 -0600

> http://sources.redhat.com/bugzilla/show_bug.cgi?id=10855

As the patch in this bug does, it should be fixed in glibc.

GCC had the same exact issues when creating precompiled headers in
c++.

> http://sources.redhat.com/bugzilla/show_bug.cgi?id=10283

Hey, Ulrich Drepper unreasonably flames someone for trying to fix a
bug, I've never seen that before! :-)

His comment about "all of userland expects page-size alignment to be
sufficient" is completely wrong, you only need look at the precompiled
header support code in GCC to see but one example.

Simple sys_errlist versioning build fixes for Sparc from mid-January
still have not been applied, and I supposed STT_GNU_IFUNC support for
sparc last week to the glibc lists and that hasn't gotten looked at
either.

So the lack of any real looking into this issue is no surprise.

This is not a kernel bug, shared mappings have stringent, architecture
specific requirement wrt. mmap() area alignment in order to handle
cache aliasing issues properly.

MAP_SHARED mappings are special, you have to be aware of the issues if
you're going to use them.  You can't just map them wherever you like.



--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Feb. 10, 2010, 8:51 p.m.
From: David Miller <davem@davemloft.net>
Date: Wed, 10 Feb 2010 12:12:17 -0800 (PST)

> From: Dennis Gilmore <dennis@ausil.us>
> Date: Wed, 10 Feb 2010 13:59:00 -0600
> 
>> http://sources.redhat.com/bugzilla/show_bug.cgi?id=10855
> 
> As the patch in this bug does, it should be fixed in glibc.

As an update I just learned about this commit:

http://sourceware.org/git/?p=glibc.git;a=commitdiff;h=6a147cc6e402e7167fa1213c31dc42a6fa38f52b

which should fix this problem.

It's in the feora branch, but not mainline.  But hopefully
it will propagate to mainline soon.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/sparc/kernel/process_32.c b/arch/sparc/kernel/process_32.c
index 2830b41..f23c8fd 100644
--- a/arch/sparc/kernel/process_32.c
+++ b/arch/sparc/kernel/process_32.c
@@ -526,7 +526,7 @@  int copy_thread(unsigned long clone_flags, unsigned long sp,
 			 * Set some valid stack frames to give to the child.
 			 */
 			childstack = (struct sparc_stackf __user *)
-				(sp & ~0x7UL);
+				(sp & ~0x15UL);
 			parentstack = (struct sparc_stackf __user *)
 				regs->u_regs[UREG_FP];
 
diff --git a/arch/sparc/kernel/process_64.c b/arch/sparc/kernel/process_64.c
index 18d6785..6679eeb 100644
--- a/arch/sparc/kernel/process_64.c
+++ b/arch/sparc/kernel/process_64.c
@@ -406,11 +406,11 @@  static unsigned long clone_stackframe(unsigned long csp, unsigned long psp)
 	} else
 		__get_user(fp, &(((struct reg_window32 __user *)psp)->ins[6]));
 
-	/* Now 8-byte align the stack as this is mandatory in the
-	 * Sparc ABI due to how register windows work.  This hides
-	 * the restriction from thread libraries etc.  -DaveM
+	/* Now align the stack as this is mandatory in the Sparc ABI
+	 * due to how register windows work.  This hides the
+	 * restriction from thread libraries etc.
 	 */
-	csp &= ~7UL;
+	csp &= ~15UL;
 
 	distance = fp - psp;
 	rval = (csp - distance);
diff --git a/arch/sparc/kernel/signal32.c b/arch/sparc/kernel/signal32.c
index ba5b09a..ea22cd3 100644
--- a/arch/sparc/kernel/signal32.c
+++ b/arch/sparc/kernel/signal32.c
@@ -120,8 +120,8 @@  struct rt_signal_frame32 {
 };
 
 /* Align macros */
-#define SF_ALIGNEDSZ  (((sizeof(struct signal_frame32) + 7) & (~7)))
-#define RT_ALIGNEDSZ  (((sizeof(struct rt_signal_frame32) + 7) & (~7)))
+#define SF_ALIGNEDSZ  (((sizeof(struct signal_frame32) + 15) & (~15)))
+#define RT_ALIGNEDSZ  (((sizeof(struct rt_signal_frame32) + 15) & (~15)))
 
 int copy_siginfo_to_user32(compat_siginfo_t __user *to, siginfo_t *from)
 {
@@ -420,15 +420,17 @@  static void __user *get_sigframe(struct sigaction *sa, struct pt_regs *regs, uns
 			sp = current->sas_ss_sp + current->sas_ss_size;
 	}
 
+	sp -= framesize;
+
 	/* Always align the stack frame.  This handles two cases.  First,
 	 * sigaltstack need not be mindful of platform specific stack
 	 * alignment.  Second, if we took this signal because the stack
 	 * is not aligned properly, we'd like to take the signal cleanly
 	 * and report that.
 	 */
-	sp &= ~7UL;
+	sp &= ~15UL;
 
-	return (void __user *)(sp - framesize);
+	return (void __user *) sp;
 }
 
 static int save_fpu_state32(struct pt_regs *regs, __siginfo_fpu_t __user *fpu)
diff --git a/arch/sparc/kernel/signal_32.c b/arch/sparc/kernel/signal_32.c
index 7ce1a10..9882df9 100644
--- a/arch/sparc/kernel/signal_32.c
+++ b/arch/sparc/kernel/signal_32.c
@@ -267,15 +267,17 @@  static inline void __user *get_sigframe(struct sigaction *sa, struct pt_regs *re
 			sp = current->sas_ss_sp + current->sas_ss_size;
 	}
 
+	sp -= framesize;
+
 	/* Always align the stack frame.  This handles two cases.  First,
 	 * sigaltstack need not be mindful of platform specific stack
 	 * alignment.  Second, if we took this signal because the stack
 	 * is not aligned properly, we'd like to take the signal cleanly
 	 * and report that.
 	 */
-	sp &= ~7UL;
+	sp &= ~15UL;
 
-	return (void __user *)(sp - framesize);
+	return (void __user *) sp;
 }
 
 static inline int
diff --git a/arch/sparc/kernel/signal_64.c b/arch/sparc/kernel/signal_64.c
index 647afbd..9fa48c3 100644
--- a/arch/sparc/kernel/signal_64.c
+++ b/arch/sparc/kernel/signal_64.c
@@ -353,7 +353,7 @@  segv:
 /* Checks if the fp is valid */
 static int invalid_frame_pointer(void __user *fp, int fplen)
 {
-	if (((unsigned long) fp) & 7)
+	if (((unsigned long) fp) & 15)
 		return 1;
 	return 0;
 }
@@ -396,15 +396,17 @@  static inline void __user *get_sigframe(struct k_sigaction *ka, struct pt_regs *
 			sp = current->sas_ss_sp + current->sas_ss_size;
 	}
 
+	sp -= framesize;
+
 	/* Always align the stack frame.  This handles two cases.  First,
 	 * sigaltstack need not be mindful of platform specific stack
 	 * alignment.  Second, if we took this signal because the stack
 	 * is not aligned properly, we'd like to take the signal cleanly
 	 * and report that.
 	 */
-	sp &= ~7UL;
+	sp &= ~15UL;
 
-	return (void __user *)(sp - framesize);
+	return (void __user *) sp;
 }
 
 static inline void