All of lore.kernel.org
 help / color / mirror / Atom feed
* [patch -v2] flat: fix data sections alignment
@ 2009-03-04 13:51 Johannes Weiner
  2009-03-04 18:04 ` Mike Frysinger
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Johannes Weiner @ 2009-03-04 13:51 UTC (permalink / raw)
  To: Andrew Morton
  Cc: David Howells, Russell King, Bryan Wu, Geert Uytterhoeven,
	Paul Mundt, Greg Ungerer, linux-kernel

From: Oskar Schirmer <os@emlix.com>

The flat loader uses an architecture's flat_stack_align() to align the
stack but assumes word-alignment is enough for the data sections.

However, on the Xtensa S6000 we have registers up to 128bit width
which can be used from userspace and therefor need userspace stack and
data-section alignment of at least this size.

This patch drops flat_stack_align() and uses the same alignment that
is required for slab caches, ARCH_SLAB_MINALIGN, or wordsize if it's
not defined by the architecture.

It also fixes m32r which was obviously kaput, aligning an
uninitialized stack entry instead of the stack pointer.

Signed-off-by: Oskar Schirmer <os@emlix.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Russell King <rmk@arm.linux.org.uk>
Cc: Bryan Wu <cooloney@kernel.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: Greg Ungerer <gerg@uclinux.org>
Signed-off-by: Johannes Weiner <jw@emlix.com>
---
 arch/arm/include/asm/flat.h      |    3 ---
 arch/blackfin/include/asm/flat.h |    1 -
 arch/h8300/include/asm/flat.h    |    1 -
 arch/m68k/include/asm/flat.h     |    1 -
 arch/sh/include/asm/flat.h       |    1 -
 fs/binfmt_flat.c                 |   37 ++++++++++++++++++++++---------------
 include/asm-m32r/flat.h          |    1 -
 7 files changed, 22 insertions(+), 23 deletions(-)

Paul, please note that on sh ARCH_SLAB_MINALIGN is defined to be 8
while the userspace stack was aligned to 4 before.  I suppose aligning
the stack (and data sections) to 8 as well is the right thing...?

version 2:
  Align ARCH_SLAB_MINALIGN if defined, fall back to wordsize, drop all
  the flat_stack_align()s as they mean the same

--- a/fs/binfmt_flat.c
+++ b/fs/binfmt_flat.c
@@ -41,6 +41,7 @@
 #include <asm/uaccess.h>
 #include <asm/unaligned.h>
 #include <asm/cacheflush.h>
+#include <asm/page.h>
 
 /****************************************************************************/
 
@@ -54,6 +55,12 @@
 #define	DBG_FLT(a...)
 #endif
 
+#ifdef ARCH_SLAB_MINALIGN
+#define FLAT_DATA_ALIGN	(ARCH_SLAB_MINALIGN)
+#else
+#define FLAT_DATA_ALIGN	(sizeof(void *))
+#endif
+
 #define RELOC_FAILED 0xff00ff01		/* Relocation incorrect somewhere */
 #define UNLOADED_LIB 0x7ff000ff		/* Placeholder for unused library */
 
@@ -114,20 +121,18 @@ static unsigned long create_flat_tables(
 	int envc = bprm->envc;
 	char uninitialized_var(dummy);
 
-	sp = (unsigned long *) ((-(unsigned long)sizeof(char *))&(unsigned long) p);
-
-	sp -= envc+1;
-	envp = sp;
-	sp -= argc+1;
-	argv = sp;
+	sp = (unsigned long *)p;
+	sp -= (envc + argc + 2) + 1 + (flat_argvp_envp_on_stack() ? 2 : 0);
+	sp = (unsigned long *) ((unsigned long)sp & -FLAT_DATA_ALIGN);
+	argv = sp + 1 + (flat_argvp_envp_on_stack() ? 2 : 0);
+	envp = argv + (argc + 1);
 
-	flat_stack_align(sp);
 	if (flat_argvp_envp_on_stack()) {
-		--sp; put_user((unsigned long) envp, sp);
-		--sp; put_user((unsigned long) argv, sp);
+		put_user((unsigned long) envp, sp + 2);
+		put_user((unsigned long) argv, sp + 1);
 	}
 
-	put_user(argc,--sp);
+	put_user(argc,sp);
 	current->mm->arg_start = (unsigned long) p;
 	while (argc-->0) {
 		put_user((unsigned long) p, argv++);
@@ -558,7 +563,8 @@ static int load_flat_file(struct linux_b
 			ret = realdatastart;
 			goto err;
 		}
-		datapos = realdatastart + MAX_SHARED_LIBS * sizeof(unsigned long);
+		datapos = ALIGN(realdatastart + MAX_SHARED_LIBS * sizeof(unsigned long),
+				FLAT_DATA_ALIGN);
 
 		DBG_FLT("BINFMT_FLAT: Allocated data+bss+stack (%d bytes): %x\n",
 				(int)(data_len + bss_len + stack_len), (int)datapos);
@@ -604,9 +610,10 @@ static int load_flat_file(struct linux_b
 		}
 
 		realdatastart = textpos + ntohl(hdr->data_start);
-		datapos = realdatastart + MAX_SHARED_LIBS * sizeof(unsigned long);
-		reloc = (unsigned long *) (textpos + ntohl(hdr->reloc_start) +
-				MAX_SHARED_LIBS * sizeof(unsigned long));
+		datapos = ALIGN(realdatastart + MAX_SHARED_LIBS * sizeof(unsigned long),
+				FLAT_DATA_ALIGN);
+
+		reloc = (unsigned long *) (datapos+(ntohl(hdr->reloc_start)-text_len));
 		memp = textpos;
 		memp_size = len;
 #ifdef CONFIG_BINFMT_ZFLAT
@@ -854,7 +861,7 @@ static int load_flat_binary(struct linux
 	stack_len = TOP_OF_ARGS - bprm->p;             /* the strings */
 	stack_len += (bprm->argc + 1) * sizeof(char *); /* the argv array */
 	stack_len += (bprm->envc + 1) * sizeof(char *); /* the envp array */
-
+	stack_len += FLAT_DATA_ALIGN;
 	
 	res = load_flat_file(bprm, &libinfo, 0, &stack_len);
 	if (res > (unsigned long)-4096)
--- a/arch/arm/include/asm/flat.h
+++ b/arch/arm/include/asm/flat.h
@@ -5,9 +5,6 @@
 #ifndef __ARM_FLAT_H__
 #define __ARM_FLAT_H__
 
-/* An odd number of words will be pushed after this alignment, so
-   deliberately misalign the value.  */
-#define	flat_stack_align(sp)	sp = (void *)(((unsigned long)(sp) - 4) | 4)
 #define	flat_argvp_envp_on_stack()		1
 #define	flat_old_ram_flag(flags)		(flags)
 #define	flat_reloc_valid(reloc, size)		((reloc) <= (size))
--- a/arch/blackfin/include/asm/flat.h
+++ b/arch/blackfin/include/asm/flat.h
@@ -10,7 +10,6 @@
 
 #include <asm/unaligned.h>
 
-#define	flat_stack_align(sp)	/* nothing needed */
 #define	flat_argvp_envp_on_stack()		0
 #define	flat_old_ram_flag(flags)		(flags)
 
--- a/arch/h8300/include/asm/flat.h
+++ b/arch/h8300/include/asm/flat.h
@@ -5,7 +5,6 @@
 #ifndef __H8300_FLAT_H__
 #define __H8300_FLAT_H__
 
-#define	flat_stack_align(sp)			/* nothing needed */
 #define	flat_argvp_envp_on_stack()		1
 #define	flat_old_ram_flag(flags)		1
 #define	flat_reloc_valid(reloc, size)		((reloc) <= (size))
--- a/arch/m68k/include/asm/flat.h
+++ b/arch/m68k/include/asm/flat.h
@@ -5,7 +5,6 @@
 #ifndef __M68KNOMMU_FLAT_H__
 #define __M68KNOMMU_FLAT_H__
 
-#define	flat_stack_align(sp)			/* nothing needed */
 #define	flat_argvp_envp_on_stack()		1
 #define	flat_old_ram_flag(flags)		(flags)
 #define	flat_reloc_valid(reloc, size)		((reloc) <= (size))
--- a/arch/sh/include/asm/flat.h
+++ b/arch/sh/include/asm/flat.h
@@ -12,7 +12,6 @@
 #ifndef __ASM_SH_FLAT_H
 #define __ASM_SH_FLAT_H
 
-#define	flat_stack_align(sp)			/* nothing needed */
 #define	flat_argvp_envp_on_stack()		0
 #define	flat_old_ram_flag(flags)		(flags)
 #define	flat_reloc_valid(reloc, size)		((reloc) <= (size))
--- a/include/asm-m32r/flat.h
+++ b/include/asm-m32r/flat.h
@@ -12,7 +12,6 @@
 #ifndef __ASM_M32R_FLAT_H
 #define __ASM_M32R_FLAT_H
 
-#define	flat_stack_align(sp)		(*sp += (*sp & 3 ? (4 - (*sp & 3)): 0))
 #define	flat_argvp_envp_on_stack()		0
 #define	flat_old_ram_flag(flags)		(flags)
 #define	flat_set_persistent(relval, p)		0

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [patch -v2] flat: fix data sections alignment
  2009-03-04 13:51 [patch -v2] flat: fix data sections alignment Johannes Weiner
@ 2009-03-04 18:04 ` Mike Frysinger
  2009-03-04 19:33   ` Johannes Weiner
  2009-03-04 21:48 ` Mike Frysinger
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Mike Frysinger @ 2009-03-04 18:04 UTC (permalink / raw)
  To: Johannes Weiner
  Cc: Andrew Morton, David Howells, Russell King, Bryan Wu,
	Geert Uytterhoeven, Paul Mundt, Greg Ungerer, linux-kernel

On Wed, Mar 4, 2009 at 08:51, Johannes Weiner wrote:
> The flat loader uses an architecture's flat_stack_align() to align the
> stack but assumes word-alignment is enough for the data sections.
>
> However, on the Xtensa S6000 we have registers up to 128bit width
> which can be used from userspace and therefor need userspace stack and
> data-section alignment of at least this size.

could this perhaps be a gcc problem ?  x86 has a similar problem with
sse and they addressed it with a function attribute.  after all, just
because your stack started out 128bit aligned doesnt mean gcc will
keep it that way when calling other functions.  so having the stack
start out aligned would only "fix" the stack for the application's
entry point right (which would in practice bubble up to main()) ?  so
you'd be right back where you started ...
-mike

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [patch -v2] flat: fix data sections alignment
  2009-03-04 18:04 ` Mike Frysinger
@ 2009-03-04 19:33   ` Johannes Weiner
  2009-03-04 20:00     ` Mike Frysinger
  0 siblings, 1 reply; 12+ messages in thread
From: Johannes Weiner @ 2009-03-04 19:33 UTC (permalink / raw)
  To: Mike Frysinger
  Cc: Andrew Morton, David Howells, Russell King, Bryan Wu,
	Geert Uytterhoeven, Paul Mundt, Greg Ungerer, linux-kernel

On Wed, Mar 04, 2009 at 01:04:00PM -0500, Mike Frysinger wrote:
> On Wed, Mar 4, 2009 at 08:51, Johannes Weiner wrote:
> > The flat loader uses an architecture's flat_stack_align() to align the
> > stack but assumes word-alignment is enough for the data sections.
> >
> > However, on the Xtensa S6000 we have registers up to 128bit width
> > which can be used from userspace and therefor need userspace stack and
> > data-section alignment of at least this size.
> 
> could this perhaps be a gcc problem ?  x86 has a similar problem with
> sse and they addressed it with a function attribute.  after all, just
> because your stack started out 128bit aligned doesnt mean gcc will
> keep it that way when calling other functions.  so having the stack
> start out aligned would only "fix" the stack for the application's
> entry point right (which would in practice bubble up to main()) ?  so
> you'd be right back where you started ...

gcc generates sp changes only ever in multiples of 16 deltas, I just
checked it again with various amounts of stack variables.

The stack frames allocate themselves with an ENTRY instruction and the
generated code I read here allocates stack frames of n * 16 bytes.

So we are good to go as long as the initial stack frame is properly
aligned.

> -mike

	Hannes

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [patch -v2] flat: fix data sections alignment
  2009-03-04 19:33   ` Johannes Weiner
@ 2009-03-04 20:00     ` Mike Frysinger
  2009-03-04 20:13       ` Johannes Weiner
  0 siblings, 1 reply; 12+ messages in thread
From: Mike Frysinger @ 2009-03-04 20:00 UTC (permalink / raw)
  To: Johannes Weiner
  Cc: Andrew Morton, David Howells, Russell King, Bryan Wu,
	Geert Uytterhoeven, Paul Mundt, Greg Ungerer, linux-kernel

On Wed, Mar 4, 2009 at 14:33, Johannes Weiner wrote:
> On Wed, Mar 04, 2009 at 01:04:00PM -0500, Mike Frysinger wrote:
>> On Wed, Mar 4, 2009 at 08:51, Johannes Weiner wrote:
>> > The flat loader uses an architecture's flat_stack_align() to align the
>> > stack but assumes word-alignment is enough for the data sections.
>> >
>> > However, on the Xtensa S6000 we have registers up to 128bit width
>> > which can be used from userspace and therefor need userspace stack and
>> > data-section alignment of at least this size.
>>
>> could this perhaps be a gcc problem ?  x86 has a similar problem with
>> sse and they addressed it with a function attribute.  after all, just
>> because your stack started out 128bit aligned doesnt mean gcc will
>> keep it that way when calling other functions.  so having the stack
>> start out aligned would only "fix" the stack for the application's
>> entry point right (which would in practice bubble up to main()) ?  so
>> you'd be right back where you started ...
>
> gcc generates sp changes only ever in multiples of 16 deltas, I just
> checked it again with various amounts of stack variables.
>
> The stack frames allocate themselves with an ENTRY instruction and the
> generated code I read here allocates stack frames of n * 16 bytes.
>
> So we are good to go as long as the initial stack frame is properly
> aligned.

throwing a few random cases at gcc isnt really a good way to validate.
 this would have worked for x86 too with older versions.  only when
common code in later gcc versions got more aggressive with stack
packing did people notice the issue.

so, lets look at the authoritative place: the gcc source code for xtensa

$ grep define.*STACK_BOUNDARY -B 2 gcc/config/xtensa/*.h
xtensa.h-/* Align stack frames on 128 bits for Xtensa.  This is necessary for
xtensa.h-   128-bit datatypes defined in TIE (e.g., for Vectra).  */
xtensa.h:#define STACK_BOUNDARY 128

ok, now i believe that forcing a stack alignment of 128bits in the
kernel is correct ;)
-mike

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [patch -v2] flat: fix data sections alignment
  2009-03-04 20:00     ` Mike Frysinger
@ 2009-03-04 20:13       ` Johannes Weiner
  0 siblings, 0 replies; 12+ messages in thread
From: Johannes Weiner @ 2009-03-04 20:13 UTC (permalink / raw)
  To: Mike Frysinger
  Cc: Andrew Morton, David Howells, Russell King, Bryan Wu,
	Geert Uytterhoeven, Paul Mundt, Greg Ungerer, linux-kernel

On Wed, Mar 04, 2009 at 03:00:25PM -0500, Mike Frysinger wrote:
> On Wed, Mar 4, 2009 at 14:33, Johannes Weiner wrote:
> > On Wed, Mar 04, 2009 at 01:04:00PM -0500, Mike Frysinger wrote:
> >> On Wed, Mar 4, 2009 at 08:51, Johannes Weiner wrote:
> >> > The flat loader uses an architecture's flat_stack_align() to align the
> >> > stack but assumes word-alignment is enough for the data sections.
> >> >
> >> > However, on the Xtensa S6000 we have registers up to 128bit width
> >> > which can be used from userspace and therefor need userspace stack and
> >> > data-section alignment of at least this size.
> >>
> >> could this perhaps be a gcc problem ?  x86 has a similar problem with
> >> sse and they addressed it with a function attribute.  after all, just
> >> because your stack started out 128bit aligned doesnt mean gcc will
> >> keep it that way when calling other functions.  so having the stack
> >> start out aligned would only "fix" the stack for the application's
> >> entry point right (which would in practice bubble up to main()) ?  so
> >> you'd be right back where you started ...
> >
> > gcc generates sp changes only ever in multiples of 16 deltas, I just
> > checked it again with various amounts of stack variables.
> >
> > The stack frames allocate themselves with an ENTRY instruction and the
> > generated code I read here allocates stack frames of n * 16 bytes.
> >
> > So we are good to go as long as the initial stack frame is properly
> > aligned.
> 
> throwing a few random cases at gcc isnt really a good way to validate.
>  this would have worked for x86 too with older versions.  only when
> common code in later gcc versions got more aggressive with stack
> packing did people notice the issue.
> 
> so, lets look at the authoritative place: the gcc source code for xtensa
> 
> $ grep define.*STACK_BOUNDARY -B 2 gcc/config/xtensa/*.h
> xtensa.h-/* Align stack frames on 128 bits for Xtensa.  This is necessary for
> xtensa.h-   128-bit datatypes defined in TIE (e.g., for Vectra).  */
> xtensa.h:#define STACK_BOUNDARY 128
> 
> ok, now i believe that forcing a stack alignment of 128bits in the
> kernel is correct ;)

Now I do too.  Heh.

Seriously, thanks for fishing this out.  Is this an Ack? ;)

> -mike

	Hannes

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [patch -v2] flat: fix data sections alignment
  2009-03-04 13:51 [patch -v2] flat: fix data sections alignment Johannes Weiner
  2009-03-04 18:04 ` Mike Frysinger
@ 2009-03-04 21:48 ` Mike Frysinger
  2009-03-05 10:53   ` Johannes Weiner
  2009-03-05 16:40   ` Oskar Schirmer
  2009-03-05  8:43 ` Paul Mundt
  2009-05-29  0:01 ` John Williams
  3 siblings, 2 replies; 12+ messages in thread
From: Mike Frysinger @ 2009-03-04 21:48 UTC (permalink / raw)
  To: Johannes Weiner
  Cc: Andrew Morton, David Howells, Russell King, Bryan Wu,
	Geert Uytterhoeven, Paul Mundt, Greg Ungerer, linux-kernel

On Wed, Mar 4, 2009 at 08:51, Johannes Weiner wrote:
> --- a/fs/binfmt_flat.c
> +++ b/fs/binfmt_flat.c
> +#ifdef ARCH_SLAB_MINALIGN
> +#define FLAT_DATA_ALIGN        (ARCH_SLAB_MINALIGN)
> +#else
> +#define FLAT_DATA_ALIGN        (sizeof(void *))
> +#endif
> ...
> +       sp = (unsigned long *) ((unsigned long)sp & -FLAT_DATA_ALIGN);
> ...
> -               datapos = realdatastart + MAX_SHARED_LIBS * sizeof(unsigned long);
> +               datapos = ALIGN(realdatastart + MAX_SHARED_LIBS * sizeof(unsigned long),
> +                               FLAT_DATA_ALIGN);
> ...
> -               datapos = realdatastart + MAX_SHARED_LIBS * sizeof(unsigned long);
> +               datapos = ALIGN(realdatastart + MAX_SHARED_LIBS * sizeof(unsigned long),
> +                               FLAT_DATA_ALIGN);

why not make FLAT_DATA_ALIGN into a macro ?  then it'd naturally
follow the existing ALIGN() behavior.

> -       sp = (unsigned long *) ((-(unsigned long)sizeof(char *))&(unsigned long) p);
> -
> -       sp -= envc+1;
> -       envp = sp;
> -       sp -= argc+1;
> -       argv = sp;
> +       sp = (unsigned long *)p;
> +       sp -= (envc + argc + 2) + 1 + (flat_argvp_envp_on_stack() ? 2 : 0);
> +       sp = (unsigned long *) ((unsigned long)sp & -FLAT_DATA_ALIGN);
> +       argv = sp + 1 + (flat_argvp_envp_on_stack() ? 2 : 0);
> +       envp = argv + (argc + 1);

can this be cleaned up a bit so that the argv/envp assignment happens
by using sp before aligning sp ?  that would be defensive coding wrt
preventing sp adjustment falling out of line with argv initialization,
and cut down on duplicated code.

> -       put_user(argc,--sp);
> +       put_user(argc,sp);

might as well fix the style issues here while you're changing code ...
stick a space after the comma

> @@ -854,7 +861,7 @@ static int load_flat_binary(struct linux
>        stack_len = TOP_OF_ARGS - bprm->p;             /* the strings */
>        stack_len += (bprm->argc + 1) * sizeof(char *); /* the argv array */
>        stack_len += (bprm->envc + 1) * sizeof(char *); /* the envp array */
> -
> +       stack_len += FLAT_DATA_ALIGN;

this seems weird.  alignment is for aligning data, not padding it out
some value ...
-mike

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [patch -v2] flat: fix data sections alignment
  2009-03-04 13:51 [patch -v2] flat: fix data sections alignment Johannes Weiner
  2009-03-04 18:04 ` Mike Frysinger
  2009-03-04 21:48 ` Mike Frysinger
@ 2009-03-05  8:43 ` Paul Mundt
  2009-05-29  0:01 ` John Williams
  3 siblings, 0 replies; 12+ messages in thread
From: Paul Mundt @ 2009-03-05  8:43 UTC (permalink / raw)
  To: Johannes Weiner
  Cc: Andrew Morton, David Howells, Russell King, Bryan Wu,
	Geert Uytterhoeven, Greg Ungerer, linux-kernel

On Wed, Mar 04, 2009 at 02:51:17PM +0100, Johannes Weiner wrote:
> Paul, please note that on sh ARCH_SLAB_MINALIGN is defined to be 8
> while the userspace stack was aligned to 4 before.  I suppose aligning
> the stack (and data sections) to 8 as well is the right thing...?
> 
This is intentional. As I noted before, the ARCH_SLAB_MINALIGN on SH
refers specifically to SH-5 (or anything implementing a 32-bit sh64 ABI),
which presently does not support nommu, but could in theory. The SH parts
that do nommu today generally need ARCH_KMALLOC_MINALIGN, but do not have
the ARCH_SLAB_MINALIGN requirement that SH-5 does.

> --- a/fs/binfmt_flat.c
> +++ b/fs/binfmt_flat.c
> @@ -41,6 +41,7 @@
>  #include <asm/uaccess.h>
>  #include <asm/unaligned.h>
>  #include <asm/cacheflush.h>
> +#include <asm/page.h>
>  
>  /****************************************************************************/
>  
> @@ -54,6 +55,12 @@
>  #define	DBG_FLT(a...)
>  #endif
>  
> +#ifdef ARCH_SLAB_MINALIGN
> +#define FLAT_DATA_ALIGN	(ARCH_SLAB_MINALIGN)
> +#else
> +#define FLAT_DATA_ALIGN	(sizeof(void *))
> +#endif
> +
As it's not entirely obvious what this is used for outside of the slab
context, you will want to have a comment here explaining the situation,
and particularly what the implication for stack alignment is.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [patch -v2] flat: fix data sections alignment
  2009-03-04 21:48 ` Mike Frysinger
@ 2009-03-05 10:53   ` Johannes Weiner
  2009-03-05 16:40   ` Oskar Schirmer
  1 sibling, 0 replies; 12+ messages in thread
From: Johannes Weiner @ 2009-03-05 10:53 UTC (permalink / raw)
  To: Mike Frysinger
  Cc: Johannes Weiner, Andrew Morton, David Howells, Russell King,
	Bryan Wu, Geert Uytterhoeven, Paul Mundt, Greg Ungerer,
	linux-kernel

On Wed, Mar 04, 2009 at 04:48:04PM -0500, Mike Frysinger wrote:
> On Wed, Mar 4, 2009 at 08:51, Johannes Weiner wrote:
> > --- a/fs/binfmt_flat.c
> > +++ b/fs/binfmt_flat.c
> > +#ifdef ARCH_SLAB_MINALIGN
> > +#define FLAT_DATA_ALIGN        (ARCH_SLAB_MINALIGN)
> > +#else
> > +#define FLAT_DATA_ALIGN        (sizeof(void *))
> > +#endif
> > ...
> > +       sp = (unsigned long *) ((unsigned long)sp & -FLAT_DATA_ALIGN);
> > ...
> > -               datapos = realdatastart + MAX_SHARED_LIBS * sizeof(unsigned long);
> > +               datapos = ALIGN(realdatastart + MAX_SHARED_LIBS * sizeof(unsigned long),
> > +                               FLAT_DATA_ALIGN);
> > ...
> > -               datapos = realdatastart + MAX_SHARED_LIBS * sizeof(unsigned long);
> > +               datapos = ALIGN(realdatastart + MAX_SHARED_LIBS * sizeof(unsigned long),
> > +                               FLAT_DATA_ALIGN);
> 
> why not make FLAT_DATA_ALIGN into a macro ?  then it'd naturally
> follow the existing ALIGN() behavior.

datapos is aligned to the next higher bound while the stack pointer
grows down and is therefor aligned to the next lower bound.  A
FLAT_DATA_ALIGN() doesn't make sense.

> > -       sp = (unsigned long *) ((-(unsigned long)sizeof(char *))&(unsigned long) p);
> > -
> > -       sp -= envc+1;
> > -       envp = sp;
> > -       sp -= argc+1;
> > -       argv = sp;
> > +       sp = (unsigned long *)p;
> > +       sp -= (envc + argc + 2) + 1 + (flat_argvp_envp_on_stack() ? 2 : 0);
> > +       sp = (unsigned long *) ((unsigned long)sp & -FLAT_DATA_ALIGN);
> > +       argv = sp + 1 + (flat_argvp_envp_on_stack() ? 2 : 0);
> > +       envp = argv + (argc + 1);

	Hannes

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [patch -v2] flat: fix data sections alignment
  2009-03-04 21:48 ` Mike Frysinger
  2009-03-05 10:53   ` Johannes Weiner
@ 2009-03-05 16:40   ` Oskar Schirmer
  1 sibling, 0 replies; 12+ messages in thread
From: Oskar Schirmer @ 2009-03-05 16:40 UTC (permalink / raw)
  To: Mike Frysinger
  Cc: Johannes Weiner, Andrew Morton, David Howells, Russell King,
	Bryan Wu, Geert Uytterhoeven, Paul Mundt, linux-kernel

On Wed, Mar 04, 2009 at 16:48:04 -0500, Mike Frysinger wrote:
> On Wed, Mar 4, 2009 at 08:51, Johannes Weiner wrote:
> > -       sp = (unsigned long *) ((-(unsigned long)sizeof(char *))&(unsigned long) p);
> > -
> > -       sp -= envc+1;
> > -       envp = sp;
> > -       sp -= argc+1;
> > -       argv = sp;
> > +       sp = (unsigned long *)p;
> > +       sp -= (envc + argc + 2) + 1 + (flat_argvp_envp_on_stack() ? 2 : 0);
> > +       sp = (unsigned long *) ((unsigned long)sp & -FLAT_DATA_ALIGN);
> > +       argv = sp + 1 + (flat_argvp_envp_on_stack() ? 2 : 0);
> > +       envp = argv + (argc + 1);
> 
> can this be cleaned up a bit so that the argv/envp assignment happens
> by using sp before aligning sp ?  that would be defensive coding wrt
> preventing sp adjustment falling out of line with argv initialization,
> and cut down on duplicated code.

The stack grows down and needs to be aligned when done,
i.e. where it's user space's turn. Therefor, we need to
first calculate the amount of space we need for argv/envp,
then align the result, and finally push argv/envp backward
into the reserved space. Note, that all this was done
before too, with one difference: Alignment was requested
in the middle of the calculation, which is nonsense (as
the comment in the ARM flat.h prooved).

> > @@ -854,7 +861,7 @@ static int load_flat_binary(struct linux
> >        stack_len = TOP_OF_ARGS - bprm->p;             /* the strings */
> >        stack_len += (bprm->argc + 1) * sizeof(char *); /* the argv array */
> >        stack_len += (bprm->envc + 1) * sizeof(char *); /* the envp array */
> > -
> > +       stack_len += FLAT_DATA_ALIGN;
> 
> this seems weird.  alignment is for aligning data, not padding it out
> some value ...

stack_len is the minimum amount of space to reserve
for the stack later on. As the stack pointer will be
aligned after pushing argv/envp (see above), we need
to reserve the additional space for maximum possible
alignment upon allocation.

  Oskar

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [patch -v2] flat: fix data sections alignment
  2009-03-04 13:51 [patch -v2] flat: fix data sections alignment Johannes Weiner
                   ` (2 preceding siblings ...)
  2009-03-05  8:43 ` Paul Mundt
@ 2009-05-29  0:01 ` John Williams
  2009-05-29  3:58   ` Paul Mundt
  3 siblings, 1 reply; 12+ messages in thread
From: John Williams @ 2009-05-29  0:01 UTC (permalink / raw)
  To: Johannes Weiner
  Cc: Andrew Morton, David Howells, Russell King, Bryan Wu,
	Geert Uytterhoeven, Paul Mundt, Greg Ungerer, linux-kernel,
	michal.simek

Hi,

On Wed, Mar 4, 2009 at 11:51 PM, Johannes Weiner <jw@emlix.com> wrote:
> From: Oskar Schirmer <os@emlix.com>
>
> The flat loader uses an architecture's flat_stack_align() to align the
> stack but assumes word-alignment is enough for the data sections.
>
> However, on the Xtensa S6000 we have registers up to 128bit width
> which can be used from userspace and therefor need userspace stack and
> data-section alignment of at least this size.
>
> This patch drops flat_stack_align() and uses the same alignment that
> is required for slab caches, ARCH_SLAB_MINALIGN, or wordsize if it's
> not defined by the architecture.
>
> It also fixes m32r which was obviously kaput, aligning an
> uninitialized stack entry instead of the stack pointer.
>

> ---
>  arch/arm/include/asm/flat.h      |    3 ---
>  arch/blackfin/include/asm/flat.h |    1 -
>  arch/h8300/include/asm/flat.h    |    1 -
>  arch/m68k/include/asm/flat.h     |    1 -
>  arch/sh/include/asm/flat.h       |    1 -
>  fs/binfmt_flat.c                 |   37 ++++++++++++++++++++++---------------
>  include/asm-m32r/flat.h          |    1 -
>  7 files changed, 22 insertions(+), 23 deletions(-)

Can you please cross-check this patch for MicroBlaze as well, we are
another active nommu architecture.

Thanks,

John

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [patch -v2] flat: fix data sections alignment
  2009-05-29  0:01 ` John Williams
@ 2009-05-29  3:58   ` Paul Mundt
  2009-05-29  5:11     ` John Williams
  0 siblings, 1 reply; 12+ messages in thread
From: Paul Mundt @ 2009-05-29  3:58 UTC (permalink / raw)
  To: John Williams
  Cc: Johannes Weiner, Andrew Morton, David Howells, Russell King,
	Bryan Wu, Geert Uytterhoeven, Greg Ungerer, linux-kernel,
	michal.simek

On Fri, May 29, 2009 at 10:01:51AM +1000, John Williams wrote:
> Hi,
> 
> On Wed, Mar 4, 2009 at 11:51 PM, Johannes Weiner <jw@emlix.com> wrote:
> > From: Oskar Schirmer <os@emlix.com>
> >
> > The flat loader uses an architecture's flat_stack_align() to align the
> > stack but assumes word-alignment is enough for the data sections.
> >
> > However, on the Xtensa S6000 we have registers up to 128bit width
> > which can be used from userspace and therefor need userspace stack and
> > data-section alignment of at least this size.
> >
> > This patch drops flat_stack_align() and uses the same alignment that
> > is required for slab caches, ARCH_SLAB_MINALIGN, or wordsize if it's
> > not defined by the architecture.
> >
> > It also fixes m32r which was obviously kaput, aligning an
> > uninitialized stack entry instead of the stack pointer.
> >
> 
> > ---
> > ?arch/arm/include/asm/flat.h ? ? ?| ? ?3 ---
> > ?arch/blackfin/include/asm/flat.h | ? ?1 -
> > ?arch/h8300/include/asm/flat.h ? ?| ? ?1 -
> > ?arch/m68k/include/asm/flat.h ? ? | ? ?1 -
> > ?arch/sh/include/asm/flat.h ? ? ? | ? ?1 -
> > ?fs/binfmt_flat.c ? ? ? ? ? ? ? ? | ? 37 ++++++++++++++++++++++---------------
> > ?include/asm-m32r/flat.h ? ? ? ? ?| ? ?1 -
> > ?7 files changed, 22 insertions(+), 23 deletions(-)
> 
> Can you please cross-check this patch for MicroBlaze as well, we are
> another active nommu architecture.
> 
I already did, see http://lkml.org/lkml/2009/5/27/753

You just need to kill off your flat_stack_align(), as it doesn't do
anything presently it is harmless one way or the other.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [patch -v2] flat: fix data sections alignment
  2009-05-29  3:58   ` Paul Mundt
@ 2009-05-29  5:11     ` John Williams
  0 siblings, 0 replies; 12+ messages in thread
From: John Williams @ 2009-05-29  5:11 UTC (permalink / raw)
  To: Paul Mundt, John Williams, Johannes Weiner, Andrew Morton,
	David Howells, Russell King, Bryan Wu, Geert Uytterhoeven,
	Greg Ungerer, linux-kernel, michal.simek

On Fri, May 29, 2009 at 1:58 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Fri, May 29, 2009 at 10:01:51AM +1000, John Williams wrote:

>> Can you please cross-check this patch for MicroBlaze as well, we are
>> another active nommu architecture.
>>
> I already did, see http://lkml.org/lkml/2009/5/27/753
>
> You just need to kill off your flat_stack_align(), as it doesn't do
> anything presently it is harmless one way or the other.

Great, thanks.  This one hit my microblaze filter which is why I only
popped up so late in the discussion!

John

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2009-05-29  5:12 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-03-04 13:51 [patch -v2] flat: fix data sections alignment Johannes Weiner
2009-03-04 18:04 ` Mike Frysinger
2009-03-04 19:33   ` Johannes Weiner
2009-03-04 20:00     ` Mike Frysinger
2009-03-04 20:13       ` Johannes Weiner
2009-03-04 21:48 ` Mike Frysinger
2009-03-05 10:53   ` Johannes Weiner
2009-03-05 16:40   ` Oskar Schirmer
2009-03-05  8:43 ` Paul Mundt
2009-05-29  0:01 ` John Williams
2009-05-29  3:58   ` Paul Mundt
2009-05-29  5:11     ` John Williams

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.