Re: serious performance regression due to NX patch

Re: serious performance regression due to NX patch

[Andi Kleen]

Ingo Molnar <mingo@redhat.com> writes:
>  		}
> +#ifdef __i386_

Won't do on x86-64.

-Andi

Re: serious performance regression due to NX patch

[Ingo Molnar]

On Sun, 11 Jul 2004, Andi Kleen wrote:

> > +#ifdef __i386__
> 
> Won't do on x86-64.

well on x86-64 'non-executable' really means non-executable, and always
did, right? (and this is completely separate from the issue of whether the
process stack is executable or not. This is about x86 that didnt enforce
the vma's protection bit.)

	Ingo

Re: serious performance regression due to NX patch

[Andi Kleen]

On Sun, Jul 11, 2004 at 07:56:10AM -0400, Ingo Molnar wrote:
> 
> On Sun, 11 Jul 2004, Andi Kleen wrote:
> 
> > > +#ifdef __i386__
> > 
> > Won't do on x86-64.
> 
> well on x86-64 'non-executable' really means non-executable, and always
> did, right? (and this is completely separate from the issue of whether the
> process stack is executable or not. This is about x86 that didnt enforce
> the vma's protection bit.)

Not quite - there is 32bit emulation. And the 64bit version also doesn't
do NX by default, but also relies on ELF header bits for this.

-Andi

Re: serious performance regression due to NX patch

[Andi Kleen]

Matthew Wilcox <willy@debian.org> writes:

> On Sun, Jul 11, 2004 at 03:02:25AM -0700, Andrew Morton wrote:
>> Apropos of nothing much, CONFIG_X86 would be preferreed here, but x86_64
>> defines that too.
>
> IMO, x86-64 should stop defining CONFIG_X86.  It's far more common
> to say "X86 && !X86_64" than it is to say X86.  How about defining
> CONFIG_X86_COMMON and migrating usage of X86 to X86_COMMON?

Definitely not in 2.6 because it has far too much potential to 
add subtle bugs, and that is not appropiate for a stable release. 
In 2.7 maybe.

Buy I would prefer to just add an truly i386 specific define 
like Andrew proposed.

-Andi

Re: serious performance regression due to NX patch

[Matthew Wilcox]

On Sun, Jul 11, 2004 at 03:38:44PM +0200, Andi Kleen wrote:
> Matthew Wilcox <willy@debian.org> writes:
> 
> > On Sun, Jul 11, 2004 at 03:02:25AM -0700, Andrew Morton wrote:
> >> Apropos of nothing much, CONFIG_X86 would be preferreed here, but x86_64
> >> defines that too.
> >
> > IMO, x86-64 should stop defining CONFIG_X86.  It's far more common
> > to say "X86 && !X86_64" than it is to say X86.  How about defining
> > CONFIG_X86_COMMON and migrating usage of X86 to X86_COMMON?
> 
> Definitely not in 2.6 because it has far too much potential to 
> add subtle bugs, and that is not appropiate for a stable release. 
> In 2.7 maybe.
> 
> Buy I would prefer to just add an truly i386 specific define 
> like Andrew proposed.

We already had an i386 specific define.  You chose to hijack it.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

serious performance regression due to NX patch

[David Mosberger]

The "No eXecute patch for x86" causes a serious exec() performance
degradation on ia64 since it ends up incorrectly turning on the
execute protection on all segments (since most ia64 binaries don't
have a gnu_stack program-header).

The patch below fixes the problem by turning on VM_EXEC and VM_MAYEXEC
only if VM_DATA_DEFAULT_FLAGS mentions them.  Note that on ia64, the
value of VM_DATA_DEFAULT_FLAGS depends on the personality (to support
x86 binaries) and hence I had to move the setting of "def_flags" down
a bit to a point after SET_PERSONALITY() has been done.

Please apply.

	--david

===== fs/binfmt_elf.c 1.78 vs edited =====
--- 1.78/fs/binfmt_elf.c	2004-06-29 07:43:10 -07:00
+++ edited/fs/binfmt_elf.c	2004-07-09 21:53:05 -07:00
@@ -493,8 +493,8 @@
 	char passed_fileno[6];
 	struct files_struct *files;
 	int executable_stack = EXSTACK_DEFAULT;
-	unsigned long def_flags = 0;
-	
+	unsigned long no_gnu_stack, def_flags = 0;
+
 	/* Get the exec-header */
 	elf_ex = *((struct elfhdr *) bprm->buf);
 
@@ -627,8 +627,7 @@
 				executable_stack = EXSTACK_DISABLE_X;
 			break;
 		}
-	if (i == elf_ex.e_phnum)
-		def_flags |= VM_EXEC | VM_MAYEXEC;
+	no_gnu_stack = (i == elf_ex.e_phnum);
 
 	/* Some simple consistency checks for the interpreter */
 	if (elf_interpreter) {
@@ -662,6 +661,10 @@
 		/* Executables without an interpreter also need a personality  */
 		SET_PERSONALITY(elf_ex, ibcs2_interpreter);
 	}
+
+	/* Now that personality is set, we can use VM_DATA_DEFAULT_FLAGS.  */
+	if (no_gnu_stack)
+		def_flags |= VM_DATA_DEFAULT_FLAGS & (VM_EXEC | VM_MAYEXEC);
 
 	/* OK, we are done with that, now set up the arg stuff,
 	   and then start this sucker up */

Re: serious performance regression due to NX patch

[Ingo Molnar]

On Fri, 9 Jul 2004, David Mosberger wrote:

> The "No eXecute patch for x86" causes a serious exec() performance
> degradation on ia64 since it ends up incorrectly turning on the execute
> protection on all segments (since most ia64 binaries don't have a
> gnu_stack program-header).
> 
> The patch below fixes the problem by turning on VM_EXEC and VM_MAYEXEC
> only if VM_DATA_DEFAULT_FLAGS mentions them.  Note that on ia64, the
> value of VM_DATA_DEFAULT_FLAGS depends on the personality (to support
> x86 binaries) and hence I had to move the setting of "def_flags" down a
> bit to a point after SET_PERSONALITY() has been done.
> 
> Please apply.

ok, agreed. I'll check that it still does the right thing on x86.

	Ingo

Re: serious performance regression due to NX patch

[Ingo Molnar]

On Sun, 11 Jul 2004, Ingo Molnar wrote:

> ok, agreed. I'll check that it still does the right thing on x86.

it doesnt seem to do the right thing for !PT_GNU_STACK applications on 
x86:

$ uname -a
Linux saturn 2.6.7-mm7 #13 SMP Sun Jul 11 10:20:45 CEST 2004 i686 athlon 
i386 GNU/Linux

$ ./cat-verylowaddress /proc/self/maps
00048000-0004c000 r-xp 00000000 03:41 1046754    /home/mingo/noexec/cat-verylowaddr
0004c000-0004d000 rw-p 00003000 03:41 1046754    /home/mingo/noexec/cat-verylowaddr
0004d000-0006e000 rw-p 0004d000 00:00 0
00aca000-00adf000 r-xp 00000000 03:41 3434109    /lib/ld-2.3.3.so
00adf000-00ae0000 r--p 00014000 03:41 3434109    /lib/ld-2.3.3.so
00ae0000-00ae1000 rw-p 00015000 03:41 3434109    /lib/ld-2.3.3.so
00ae3000-00bf8000 r-xp 00000000 03:41 3434110    /lib/tls/libc-2.3.3.so
00bf8000-00bfa000 r--p 00115000 03:41 3434110    /lib/tls/libc-2.3.3.so
00bfa000-00bfc000 rw-p 00117000 03:41 3434110    /lib/tls/libc-2.3.3.so
00bfc000-00bfe000 rw-p 00bfc000 00:00 0
b7db5000-b7db6000 r--p 00cfd000 03:41 3735973    /usr/lib/locale/locale-archive
b7db6000-b7de3000 r--p 00ccc000 03:41 3735973    /usr/lib/locale/locale-archive
b7de3000-b7de9000 r--p 00cc3000 03:41 3735973    /usr/lib/locale/locale-archive
b7de9000-b7fe9000 r--p 00000000 03:41 3735973    /usr/lib/locale/locale-archive
b7fe9000-b7fea000 rw-p b7fe9000 00:00 0
bfffe000-c0000000 rw-p bfffe000 00:00 0
ffffe000-fffff000 ---p 00000000 00:00 0

i'll come up with a patch that solves the ia64 problem and keeps x86
working too (unless you beat me to it).

	Ingo

Re: serious performance regression due to NX patch

[Ingo Molnar]

On Sun, 11 Jul 2004, Ingo Molnar wrote:

> > ok, agreed. I'll check that it still does the right thing on x86.
> 
> it doesnt seem to do the right thing for !PT_GNU_STACK applications on 
> x86:

how about the patch below? This way we recognize the fact that x86 didnt
have any executability check previously at the point where we discover
that it's a 'legacy' binary.

	Ingo

--- linux/fs/binfmt_elf.c.orig3	
+++ linux/fs/binfmt_elf.c	
@@ -627,8 +627,14 @@ static int load_elf_binary(struct linux_
 				executable_stack = EXSTACK_DISABLE_X;
 			break;
 		}
+#ifdef __i386_
+	/*
+	 * Legacy x86 binaries have an expectation of executability for
+	 * virtually all their address-space - turn executability on:
+	 */
 	if (i == elf_ex.e_phnum)
 		def_flags |= VM_EXEC | VM_MAYEXEC;
+#endif
 
 	/* Some simple consistency checks for the interpreter */
 	if (elf_interpreter) {

Re: serious performance regression due to NX patch

[Andrew Morton]

Ingo Molnar <mingo@redhat.com> wrote:
>
> +#ifdef __i386_

You'll be wanting __i386__ there.

Apropos of nothing much, CONFIG_X86 would be preferreed here, but x86_64
defines that too.

Is there a CONFIG symbol which is unique to i386?  If not, perhaps we
should define one (CONFIG_I386?)

Re: serious performance regression due to NX patch

[Matthew Wilcox]

On Sun, Jul 11, 2004 at 03:02:25AM -0700, Andrew Morton wrote:
> Apropos of nothing much, CONFIG_X86 would be preferreed here, but x86_64
> defines that too.

IMO, x86-64 should stop defining CONFIG_X86.  It's far more common
to say "X86 && !X86_64" than it is to say X86.  How about defining
CONFIG_X86_COMMON and migrating usage of X86 to X86_COMMON?

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: serious performance regression due to NX patch

[Christoph Hellwig]

On Sun, Jul 11, 2004 at 05:52:59AM -0400, Ingo Molnar wrote:
> 
> On Sun, 11 Jul 2004, Ingo Molnar wrote:
> 
> > > ok, agreed. I'll check that it still does the right thing on x86.
> > 
> > it doesnt seem to do the right thing for !PT_GNU_STACK applications on 
> > x86:
> 
> how about the patch below? This way we recognize the fact that x86 didnt
> have any executability check previously at the point where we discover
> that it's a 'legacy' binary.

Please don't add per-architecture ifdefs to generic code.  And I'm pretty
sure there's quite a few more architectures with the same issue.

Re: serious performance regression due to NX patch

[Jakub Jelinek]

On Sun, Jul 11, 2004 at 05:52:59AM -0400, Ingo Molnar wrote:
> 
> On Sun, 11 Jul 2004, Ingo Molnar wrote:
> 
> > > ok, agreed. I'll check that it still does the right thing on x86.
> > 
> > it doesnt seem to do the right thing for !PT_GNU_STACK applications on 
> > x86:
> 
> how about the patch below? This way we recognize the fact that x86 didnt
> have any executability check previously at the point where we discover
> that it's a 'legacy' binary.
> 
> 	Ingo
> 
> --- linux/fs/binfmt_elf.c.orig3	
> +++ linux/fs/binfmt_elf.c	
> @@ -627,8 +627,14 @@ static int load_elf_binary(struct linux_
>  				executable_stack = EXSTACK_DISABLE_X;
>  			break;
>  		}
> +#ifdef __i386_
> +	/*
> +	 * Legacy x86 binaries have an expectation of executability for
> +	 * virtually all their address-space - turn executability on:
> +	 */
>  	if (i == elf_ex.e_phnum)
>  		def_flags |= VM_EXEC | VM_MAYEXEC;
> +#endif

This looks incorrect.
There are many arches where legacy binaries expect the executability for
virtually all their address-space (my guess is all but x86-64 and ia64),
and even on those two legacy binaries expected at least stack executable.

And on x86-64 and ia64 ia32 binaries (i.e. when binfmt_elf.c is included
in their binfmt_elf32.c or how is it called) should have VM_EXEC and
VM_MAYEXEC in def_flags.

	Jakub

Re: serious performance regression due to NX patch

[Ingo Molnar]

On Sun, 11 Jul 2004, Jakub Jelinek wrote:

> > --- linux/fs/binfmt_elf.c.orig3	
> > +++ linux/fs/binfmt_elf.c	
> > @@ -627,8 +627,14 @@ static int load_elf_binary(struct linux_
> >  				executable_stack = EXSTACK_DISABLE_X;
> >  			break;
> >  		}
> > +#ifdef __i386_
> > +	/*
> > +	 * Legacy x86 binaries have an expectation of executability for
> > +	 * virtually all their address-space - turn executability on:
> > +	 */
> >  	if (i == elf_ex.e_phnum)
> >  		def_flags |= VM_EXEC | VM_MAYEXEC;
> > +#endif
> 
> This looks incorrect. There are many arches where legacy binaries expect
> the executability for virtually all their address-space (my guess is all
> but x86-64 and ia64), and even on those two legacy binaries expected at
> least stack executable.

so ... this should be #ifndef ia64?

to all the purists: this cannot be done via VM_DATA_DEFAULT_FLAGS, because
some architectures enforce the X bit, some dont. In fact only ia64 seems
to enforce it reliably for all binaries.

the #ifdef could be made an arch inline or define. But it's really
academic as only ia64 seems to have this problem. So i'd suggest the patch
below.

	Ingo

--- linux/fs/binfmt_elf.c.orig
+++ linux/fs/binfmt_elf.c
@@ -627,8 +627,14 @@ static int load_elf_binary(struct linux_
 				executable_stack = EXSTACK_DISABLE_X;
 			break;
 		}
+#ifndef __ia64__
+	/*
+	 * Legacy binaries (except ia64) have an expectation of executability
+	 * for virtually all their address-space - turn executability on:
+	 */
 	if (i == elf_ex.e_phnum)
 		def_flags |= VM_EXEC | VM_MAYEXEC;
+#endif
 
 	/* Some simple consistency checks for the interpreter */
 	if (elf_interpreter) {

Re: serious performance regression due to NX patch

[Christoph Hellwig]

On Mon, Jul 12, 2004 at 02:08:11PM -0400, Ingo Molnar wrote:
> the #ifdef could be made an arch inline or define. But it's really
> academic as only ia64 seems to have this problem. So i'd suggest the patch
> below.

Well, it's not.  We probably want each new port start to have the ia64
behaviour, so it should be abstracted out nicer.

Re: serious performance regression due to NX patch

[Ingo Molnar]

On Mon, 12 Jul 2004, Christoph Hellwig wrote:

> On Mon, Jul 12, 2004 at 02:08:11PM -0400, Ingo Molnar wrote:
> > the #ifdef could be made an arch inline or define. But it's really
> > academic as only ia64 seems to have this problem. So i'd suggest the patch
> > below.
> 
> Well, it's not.  We probably want each new port start to have the ia64
> behaviour, so it should be abstracted out nicer.

is it an issue? Each new port will have PT_GNU_STACK, unless they base
themselves on old compilers.

	Ingo

Re: serious performance regression due to NX patch

[David Mosberger]

>>>>> On Mon, 12 Jul 2004 14:29:16 -0400 (EDT), Ingo Molnar <mingo@redhat.com> said:

  >> Well, it's not.  We probably want each new port start to have the ia64
  >> behaviour, so it should be abstracted out nicer.

  Ingo> is it an issue? Each new port will have PT_GNU_STACK, unless they base
  Ingo> themselves on old compilers.

PT_GNU_STACK is pure bloat on new architectures (and ia64).

	--david

Re: serious performance regression due to NX patch

[Ingo Molnar]

On Mon, 12 Jul 2004, David Mosberger wrote:

>   Ingo> is it an issue? Each new port will have PT_GNU_STACK, unless they base
>   Ingo> themselves on old compilers.
> 
> PT_GNU_STACK is pure bloat on new architectures (and ia64).

EF_IA_64_LINUX_EXECUTABLE_STACK is using elf_ex->e_flags. I did it the
same way for x86 originally, but the tools people specifically rejected it
as a hack. We dont control the ELF specification, but a new gcc section
like PT_GNU_STACK is fair game. So it might be 'bloat' but it's clean and
doesnt try to hijack.

	Ingo

Re: serious performance regression due to NX patch

[David Mosberger]

>>>>> On Mon, 12 Jul 2004 15:54:56 -0400 (EDT), Ingo Molnar <mingo@redhat.com> said:

  Ingo> On Mon, 12 Jul 2004, David Mosberger wrote:

  Ingo> is it an issue? Each new port will have PT_GNU_STACK, unless they base
  Ingo> themselves on old compilers.

  >> PT_GNU_STACK is pure bloat on new architectures (and ia64).

  Ingo> EF_IA_64_LINUX_EXECUTABLE_STACK is using elf_ex->e_flags. I
  Ingo> did it the same way for x86 originally, but the tools people
  Ingo> specifically rejected it as a hack. We dont control the ELF
  Ingo> specification, but a new gcc section like PT_GNU_STACK is fair
  Ingo> game. So it might be 'bloat' but it's clean and doesnt try to
  Ingo> hijack.

I know.  All I'm saying is that when the stack permissions match the
permission which the platform uses by default, the PT_GNU_STACK header
should be omitted.

	--david

Re: serious performance regression due to NX patch

[Linus Torvalds]

On Mon, 12 Jul 2004, Ingo Molnar wrote:
> 
> so ... this should be #ifndef ia64?

No. Make it a CONFIG_DEFAULT_NOEXEC and make the relevant architectures do
a

	define_bool DEFAULT_NOEXEC y

in their Kconfig files.

In general, we should _never_ use an architecture-define. They just always
end up becoming more and more hairy, and less and less obvious what they 
are all about.

So instead, make a readable and explicit config define, and let each 
architecture just set it (or not) as they wish.

		Linus

Re: serious performance regression due to NX patch

[David Mosberger]

>>>>> On Mon, 12 Jul 2004 13:17:20 -0700 (PDT), Linus Torvalds <torvalds@osdl.org> said:

  Linus> On Mon, 12 Jul 2004, Ingo Molnar wrote:
  >>  so ... this should be #ifndef ia64?

  Linus> No. Make it a CONFIG_DEFAULT_NOEXEC and make the relevant
  Linus> architectures do a

  Linus> 	define_bool DEFAULT_NOEXEC y

  Linus> in their Kconfig files.

Would it be better to reverse the sense (i.e., make it DEFAULT_EXEC),
since the latter new platforms by default almost certainly do NOT want
DEFAULT_EXEC?

	--david

Re: serious performance regression due to NX patch

[David Mosberger]

>>>>> On Mon, 12 Jul 2004 13:17:20 -0700 (PDT), Linus Torvalds <torvalds@osdl.org> said:

  Linus> No. Make it a CONFIG_DEFAULT_NOEXEC and make the relevant
  Linus> architectures do a

  Linus> 	define_bool DEFAULT_NOEXEC y

  Linus> in their Kconfig files.

  Linus> In general, we should _never_ use an
  Linus> architecture-define. They just always end up becoming more
  Linus> and more hairy, and less and less obvious what they are all
  Linus> about.

  Linus> So instead, make a readable and explicit config define, and
  Linus> let each architecture just set it (or not) as they wish.

Oops, I responded too fast here.  This is still wrong: on ia64 (and
x86-64, I believe), you'll want DEFAULT_NOEXEC for native binaries,
but DEFAULT_EXEC for x86 binaries.

So I think it would be better to have a VM_STACK_EXEC_FLAGS macro in
an asm header file (with suitable default in asm-generic).

	--david

Re: serious performance regression due to NX patch

[Ingo Molnar]

On Mon, 12 Jul 2004, David Mosberger wrote:

> So I think it would be better to have a VM_STACK_EXEC_FLAGS macro in an
> asm header file (with suitable default in asm-generic).

it's not just about the stack! It's a "is the value of the PROT_EXEC bit
just an embelishment of /proc output or is it taken seriously" thing. My
change enforces the X bit for _all_ applications and gives the X bit to
almost all mappings created by legacy applications:

 005a1000-005b6000 r-xp 00000000 09:00 1786072  /lib/ld-2.3.3.so
 005b6000-005b7000 r--p 00014000 09:00 1786072  /lib/ld-2.3.3.so
 005b7000-005b8000 rwxp 00015000 09:00 1786072  /lib/ld-2.3.3.so
 005be000-006d3000 r-xp 00000000 09:00 1786073  /lib/tls/libc-2.3.3.so
 006d3000-006d5000 r--p 00115000 09:00 1786073  /lib/tls/libc-2.3.3.so
 006d5000-006d7000 rwxp 00117000 09:00 1786073  /lib/tls/libc-2.3.3.so
 006d7000-006d9000 rwxp 006d7000 00:00 0
 00da2000-00da3000 r-xp 00da2000 00:00 0
 01000000-01004000 r-xp 00000000 09:01 13356378 /home/mingo/cat-lowaddr
 01004000-01005000 rwxp 00003000 09:01 13356378 /home/mingo/cat-lowaddr
 08590000-085b1000 rwxp 08590000 00:00 0
 f6e48000-f6e49000 r-xp 00e4b000 09:00 2439993  /usr/lib/locale/locale-archive
 f6e49000-f6e7b000 r-xp 00dc3000 09:00 2439993  /usr/lib/locale/locale-archive
 f6e7b000-f707b000 r-xp 00000000 09:00 2439993  /usr/lib/locale/locale-archive
 f707b000-f707c000 rwxp f707b000 00:00 0
 fef8a000-ff000000 rwxp fef8a000 00:00 0
 ffffd000-ffffe000 ---p 00000000 00:00 0

this way you get what you see. An X mapping is executable, a !X one isnt.  
No magic "this applications' mappings means this, that application's
mappings mean that". This also streamlines the kernel side of any NX
solution added to an arch where applications had executability
expectations: you can just add the capability because the mappings done
lie anymore and compatibility is done by following that old expectation
for old binaries. No hackery with personalities, split decisions in the
pte handling paths, etc.

So as you can see in the above maps file, the change impacts the default
mappings for the stack, heap and mmap()s. The only narrow exeception is
that if legacy userspace asks for !PROT_EXEC via mprotect() explicitly and
then expects executability _that_ will be denied (fortunately we havent
met such a case yet) - but all the other cases will result in executable
mappings, to preserve compatibility. E.g. there are only two
non-executable mappings in the above layout, both were created by glibc
via mprotect() and are fully expected to be non-executable.

the process stack's executability itself is controlled via the value of
PT_GNU_STACK - either X or !X. (subsequently any newly loaded shared
library might also change the process' stack. So if you link against an
older library without PT_GNU_STACK then the presumption will be the
compatible one: to have an executable stack. This is not an issue in new
distros, but might help with using third party libraries.)

all of this is needed to have a smooth sailing into the NX world.

	Ingo

Re: serious performance regression due to NX patch

[David Mosberger]

>>>>> On Tue, 13 Jul 2004 00:23:29 -0400 (EDT), Ingo Molnar <mingo@redhat.com> said:

  Ingo> it's not just about the stack! It's a "is the value of the
  Ingo> PROT_EXEC bit just an embelishment of /proc output or is it
  Ingo> taken seriously" thing.

Fine, but it seems to me NX bit patch wasn't properly integrated with
VM_DATA_DEFAULT_FLAGS.  In fact, if you hadn't changed the x86 version
of VM_DATA_DEFAULT_FLAGS and instead had in mm/mmap.c replaced the
macro with (VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE), then
things would have behaved exactly right with my patch applied and the
logic would make much more sense.

There would also need to be a small change to arch/ia64/mm/init.c, but
I'd be happy to take care of that.

	--david

Re: serious performance regression due to NX patch

[Mark Haverkamp]

On Mon, 2004-07-12 at 21:23, Ingo Molnar wrote:
> On Mon, 12 Jul 2004, David Mosberger wrote:
> 
> > So I think it would be better to have a VM_STACK_EXEC_FLAGS macro in an
> > asm header file (with suitable default in asm-generic).
> 
> it's not just about the stack! It's a "is the value of the PROT_EXEC bit
> just an embelishment of /proc output or is it taken seriously" thing. My
> change enforces the X bit for _all_ applications and gives the X bit to
> almost all mappings created by legacy applications:
> 
>  005a1000-005b6000 r-xp 00000000 09:00 1786072  /lib/ld-2.3.3.so
>  005b6000-005b7000 r--p 00014000 09:00 1786072  /lib/ld-2.3.3.so
>  005b7000-005b8000 rwxp 00015000 09:00 1786072  /lib/ld-2.3.3.so
>  005be000-006d3000 r-xp 00000000 09:00 1786073  /lib/tls/libc-2.3.3.so
>  006d3000-006d5000 r--p 00115000 09:00 1786073  /lib/tls/libc-2.3.3.so
>  006d5000-006d7000 rwxp 00117000 09:00 1786073  /lib/tls/libc-2.3.3.so
>  006d7000-006d9000 rwxp 006d7000 00:00 0
>  00da2000-00da3000 r-xp 00da2000 00:00 0
>  01000000-01004000 r-xp 00000000 09:01 13356378 /home/mingo/cat-lowaddr
>  01004000-01005000 rwxp 00003000 09:01 13356378 /home/mingo/cat-lowaddr
>  08590000-085b1000 rwxp 08590000 00:00 0
>  f6e48000-f6e49000 r-xp 00e4b000 09:00 2439993  /usr/lib/locale/locale-archive
>  f6e49000-f6e7b000 r-xp 00dc3000 09:00 2439993  /usr/lib/locale/locale-archive
>  f6e7b000-f707b000 r-xp 00000000 09:00 2439993  /usr/lib/locale/locale-archive
>  f707b000-f707c000 rwxp f707b000 00:00 0
>  fef8a000-ff000000 rwxp fef8a000 00:00 0
>  ffffd000-ffffe000 ---p 00000000 00:00 0
> 
> this way you get what you see. An X mapping is executable, a !X one isnt.  
> No magic "this applications' mappings means this, that application's
> mappings mean that". This also streamlines the kernel side of any NX
> solution added to an arch where applications had executability
> expectations: you can just add the capability because the mappings done
> lie anymore and compatibility is done by following that old expectation
> for old binaries. No hackery with personalities, split decisions in the
> pte handling paths, etc.
> 
> So as you can see in the above maps file, the change impacts the default
> mappings for the stack, heap and mmap()s. The only narrow exeception is
> that if legacy userspace asks for !PROT_EXEC via mprotect() explicitly and
> then expects executability _that_ will be denied (fortunately we havent
> met such a case yet) - but all the other cases will result in executable
> mappings, to preserve compatibility. E.g. there are only two
> non-executable mappings in the above layout, both were created by glibc
> via mprotect() and are fully expected to be non-executable.
> 
> the process stack's executability itself is controlled via the value of
> PT_GNU_STACK - either X or !X. (subsequently any newly loaded shared
> library might also change the process' stack. So if you link against an
> older library without PT_GNU_STACK then the presumption will be the
> compatible one: to have an executable stack. This is not an issue in new
> distros, but might help with using third party libraries.)
> 
> all of this is needed to have a smooth sailing into the NX world.
> 
> 	Ingo

I think that there is  a problem with this piece of code in
binfmt_elf.c:

        if (i == elf_ex.e_phnum)
                def_flags |= VM_EXEC | VM_MAYEXEC;

I've seen that if this code is executed that a mmap with PROT_NONE will
have the x flag set on the page.  because of code in mmap.c for
do_mmp_pgoff:

	vm_flags = calc_vm_prot_bits(prot) | calc_vm_flag_bits(flags) |
			mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;

or possibly here in do_brk:

	flags = VM_DATA_DEFAULT_FLAGS | VM_ACCOUNT | mm->def_flags;


the mm->def_flags have VM_EXEC and VM_MAYEXEC which means they are set all the time.

Mark.

-- 
Mark Haverkamp <markh@osdl.org>

Re: serious performance regression due to NX patch

[Daniel McNeil]

On Tue, 2004-07-13 at 09:05, Mark Haverkamp wrote:
> I think that there is  a problem with this piece of code in
> binfmt_elf.c:
> 
>         if (i == elf_ex.e_phnum)
>                 def_flags |= VM_EXEC | VM_MAYEXEC;
> 
> I've seen that if this code is executed that a mmap with PROT_NONE will
> have the x flag set on the page.  because of code in mmap.c for
> do_mmp_pgoff:
> 
> 	vm_flags = calc_vm_prot_bits(prot) | calc_vm_flag_bits(flags) |
> 			mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
> 
> or possibly here in do_brk:
> 
> 	flags = VM_DATA_DEFAULT_FLAGS | VM_ACCOUNT | mm->def_flags;
> 
> 
> the mm->def_flags have VM_EXEC and VM_MAYEXEC which means they are set all the time.
> 
> Mark.

This was my email to Andrew yesterday that did not seem
to make it to LKML (probably because I attached some elf
binaries -- I made them links now).  The below program shows
the old binaries without the PT_GNU_STACK do not get PROT_NONE
mmaps. See info below.

Daniel

On Fri, 2004-07-09 at 23:45, Andrew Morton wrote:

> #include <unistd.h>
> #include <errno.h>
> #include <sys/mman.h>
> 
> 
> main()
> {
>       char *p0 = 0;
>       char *p1 = (char *)-1;
>       char *p2;
>       int err;
> 
>       p2 = mmap(0, 4096, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS, 0, 0);
> 
>       printf("p2=%p\n", p2);
>       printf("pid=%d\n", getpid());
> 
>       getchar();
> 
>       errno = 0;
>       err = access(p0, R_OK);
>       printf("access 0 ptr %p return code %d errno %d\n", p0, err,
errno);
>       perror("access result:");
>       errno = 0;
>       err = access(p1, R_OK);
>       printf("access 1 ptr %p return code %d errno %d\n", p1, err,
errno);
>       perror("access result:");
>       errno = 0;
>       err = access(p2, R_OK);
>       printf("access 2 ptr %p return code %d errno %d\n", p2, err,
errno);
>       perror("access result:");
> }
> 
> vmm:/home/akpm> ./x
> p2=0x40000000
> pid=2038
> 
> access 0 ptr (nil) return code -1 errno 14
> access result:: Bad address
> access 1 ptr 0xffffffff return code -1 errno 14
> access result:: Bad address
> access 2 ptr 0x40000000 return code -1 errno 14
> access result:: Bad address
> 
> I get this result on both p4 xeon and p3 xeon.



It is the elf file that makes the difference. I was using
gcc 3.2.2 and ld (GNU ld version 2.13.90.0.18 20030206).
This produces elf binary without the PT_GNU_STACK program
section (from objdump -p x.gcc322)

Program Header:
    PHDR off    0x00000034 vaddr 0x08048034 paddr 0x08048034 align 2**2
         filesz 0x000000c0 memsz 0x000000c0 flags r-x
  INTERP off    0x000000f4 vaddr 0x080480f4 paddr 0x080480f4 align 2**0
         filesz 0x00000013 memsz 0x00000013 flags r--
    LOAD off    0x00000000 vaddr 0x08048000 paddr 0x08048000 align 2**12
         filesz 0x00000750 memsz 0x00000750 flags r-x
    LOAD off    0x00000750 vaddr 0x08049750 paddr 0x08049750 align 2**12
         filesz 0x00000118 memsz 0x0000011c flags rw-
 DYNAMIC off    0x0000075c vaddr 0x0804975c paddr 0x0804975c align 2**2
         filesz 0x000000c8 memsz 0x000000c8 flags rw-
    NOTE off    0x00000108 vaddr 0x08048108 paddr 0x08048108 align 2**2
         filesz 0x00000020 memsz 0x00000020 flags r--

With gcc 3.4.1 and GNU ld version 2.14.90.0.6 20030820 and
compile with cc -o x.rw -z noexecstack x.c produces a 
elf binary without execute permission on the stack.
(from objdump -p x.rw)

Program Header:
    PHDR off    0x00000034 vaddr 0x08048034 paddr 0x08048034 align 2**2
         filesz 0x000000e0 memsz 0x000000e0 flags r-x
  INTERP off    0x00000114 vaddr 0x08048114 paddr 0x08048114 align 2**0
         filesz 0x00000013 memsz 0x00000013 flags r--
    LOAD off    0x00000000 vaddr 0x08048000 paddr 0x08048000 align 2**12
         filesz 0x00000735 memsz 0x00000735 flags r-x
    LOAD off    0x00000738 vaddr 0x08049738 paddr 0x08049738 align 2**12
         filesz 0x0000011c memsz 0x00000120 flags rw-
 DYNAMIC off    0x00000748 vaddr 0x08049748 paddr 0x08049748 align 2**2
         filesz 0x000000c8 memsz 0x000000c8 flags rw-
    NOTE off    0x00000128 vaddr 0x08048128 paddr 0x08048128 align 2**2
         filesz 0x00000020 memsz 0x00000020 flags r--
   STACK off    0x00000000 vaddr 0x00000000 paddr 0x00000000 align 2**2
         filesz 0x00000000 memsz 0x00000000 flags rw-

With gcc 3.4.1 and GNU ld version 2.14.90.0.6 20030820 and
compiled with cc -o x.rwx -z execstack x.c.  This produces
the same binary as with out the "-z execstack".

Program Header:
    PHDR off    0x00000034 vaddr 0x08048034 paddr 0x08048034 align 2**2
         filesz 0x000000e0 memsz 0x000000e0 flags r-x
  INTERP off    0x00000114 vaddr 0x08048114 paddr 0x08048114 align 2**0
         filesz 0x00000013 memsz 0x00000013 flags r--
    LOAD off    0x00000000 vaddr 0x08048000 paddr 0x08048000 align 2**12
         filesz 0x00000735 memsz 0x00000735 flags r-x
    LOAD off    0x00000738 vaddr 0x08049738 paddr 0x08049738 align 2**12
         filesz 0x0000011c memsz 0x00000120 flags rw-
 DYNAMIC off    0x00000748 vaddr 0x08049748 paddr 0x08049748 align 2**2
         filesz 0x000000c8 memsz 0x000000c8 flags rw-
    NOTE off    0x00000128 vaddr 0x08048128 paddr 0x08048128 align 2**2
         filesz 0x00000020 memsz 0x00000020 flags r--
0x6474e551 off    0x00000000 vaddr 0x00000000 paddr 0x00000000 align 2**2
         filesz 0x00000000 memsz 0x00000000 flags rwx

Here's the corresponding /proc/pid/maps output from the
3 different binaries:


x.gcc322:	40013000-40014000 --xp 40013000 00:00 0
x.rw:		40013000-40014000 ---p 40013000 00:00 0
x.rwx:		40013000-40014000 ---p 40013000 00:00 0


So binaries compiled with an old compiler are not being
handled correctly with the nx-update.patch changes.

In load_elf_binary()

        for (i = 0; i < elf_ex.e_phnum; i++, elf_ppnt++)
                if (elf_ppnt->p_type == PT_GNU_STACK) {
                        if (elf_ppnt->p_flags & PF_X)
                                executable_stack = EXSTACK_ENABLE_X;
                        else
                                executable_stack = EXSTACK_DISABLE_X;
                        break;
                }
        if (i == elf_ex.e_phnum)
                def_flags |= VM_EXEC | VM_MAYEXEC;

This setting of def_flags if the PT_GNU_STACK is NOT found
is causing the old binaries to get exec on PROT_NONE mmaps.

The 3 elf binaries can be found here:
http://developer.osdl.org/daniel/mmap.PROT_NONE.bug/

Daniel

Re: serious performance regression due to NX patch

[David Mosberger]

>>>>> On Tue, 13 Jul 2004 09:05:29 -0700, Mark Haverkamp <markh@osdl.org> said:

  Mark> I think that there is  a problem with this piece of code in
  Mark> binfmt_elf.c:

  Mark> if (i == elf_ex.e_phnum)
  Mark>    def_flags |= VM_EXEC | VM_MAYEXEC;

I think there are other problems, too:

 - any fork() will reset mm->def_flags to zero so if you exec
   an old binary and it does a fork, future mmaps() won't
   have the execute-bit turned on any more; perhaps a rare problem,
   but it certainly seems an illogical behavior

 - likewise for do_mlockall(): it stomps on def_flags without preserving
   the old bits

Am I missing something?

	--david

Re: serious performance regression due to NX patch

[Linus Torvalds]

On Fri, 16 Jul 2004, David Mosberger wrote:
> 
> Am I missing something?

No. Using "def_flags" was a mistake for the whole VM_EXEC thing. It's not 
designed for that, and it doesn't work that way. I applied the paper-over 
fix that gets it almost-working, but I'm waiting for Ingo to rewrite it by 
just saving the "executable_stack" information at exec time, and not 
playing with def_flags at all.

And if somebody else beats him to it, all the more power to that person,
hint hint ;)

		Linus

Re: serious performance regression due to NX patch

[David Mosberger]

>>>>> On Fri, 16 Jul 2004 18:39:40 -0700 (PDT), Linus Torvalds <torvalds@osdl.org> said:

  Linus> No. Using "def_flags" was a mistake for the whole VM_EXEC
  Linus> thing. It's not designed for that, and it doesn't work that
  Linus> way. I applied the paper-over fix that gets it
  Linus> almost-working, but I'm waiting for Ingo to rewrite it by
  Linus> just saving the "executable_stack" information at exec time,
  Linus> and not playing with def_flags at all.

I'm very happy to hear that.

Thanks,

	--david

Re: serious performance regression due to NX patch

[Ingo Molnar]

On Mon, 12 Jul 2004, Linus Torvalds wrote:

> > so ... this should be #ifndef ia64?
> 
> No. Make it a CONFIG_DEFAULT_NOEXEC and make the relevant architectures do
> a
> 
> 	define_bool DEFAULT_NOEXEC y
> 
> in their Kconfig files.
> 
> In general, we should _never_ use an architecture-define. They just
> always end up becoming more and more hairy, and less and less obvious
> what they are all about.
> 
> So instead, make a readable and explicit config define, and let each
> architecture just set it (or not) as they wish.

ok - patch below. ia64 can now define it - the default is that legacy
binaries have executability expectations.

	Ingo

--- linux/fs/binfmt_elf.c.orig
+++ linux/fs/binfmt_elf.c
@@ -627,8 +627,14 @@ static int load_elf_binary(struct linux_
 				executable_stack = EXSTACK_DISABLE_X;
 			break;
 		}
+#ifndef CONFIG_DEFAULT_NOEXEC
+	/*
+	 * Legacy binaries (unless the arch defaults to noexec) have an
+	 * expectation of executability - turn it on:
+	 */
 	if (i == elf_ex.e_phnum)
 		def_flags |= VM_EXEC | VM_MAYEXEC;
+#endif
 
 	/* Some simple consistency checks for the interpreter */
 	if (elf_interpreter) {

Re: serious performance regression due to NX patch

[David Mosberger]

>>>>> On Sun, 11 Jul 2004 05:39:23 -0400 (EDT), Ingo Molnar <mingo@redhat.com> said:

  Ingo> On Sun, 11 Jul 2004, Ingo Molnar wrote:

  >> ok, agreed. I'll check that it still does the right thing on x86.

  Ingo> it doesnt seem to do the right thing for !PT_GNU_STACK applications on 
  Ingo> x86:

How about the patch below (on top of Linus' current bk tree)?  Only
platforms which had VM_DATA_DEFAULT_FLAGS changed (as a result of the
NX patch) will need to define LEGACY_VM_DATA_DEFAULT_FLAGS to the old
value.  AFAIK, that's x86 only, so the impact is very minimal and
should retain the existing behavior on all other platforms.

	--david

===== fs/binfmt_elf.c 1.80 vs edited =====
--- 1.80/fs/binfmt_elf.c	2004-07-16 16:46:22 -07:00
+++ edited/fs/binfmt_elf.c	2004-07-16 17:25:26 -07:00
@@ -662,9 +662,9 @@
 		SET_PERSONALITY(elf_ex, ibcs2_interpreter);
 	}
 
-	/* Now that personality is set, we can use VM_DATA_DEFAULT_FLAGS.  */
+	/* Now that personality is set, we can use LEGACY_VM_DATA_DEFAULT_FLAGS.  */
 	if (no_gnu_stack)
-		def_flags |= VM_DATA_DEFAULT_FLAGS & (VM_EXEC | VM_MAYEXEC);
+		def_flags |= LEGACY_VM_DATA_DEFAULT_FLAGS & (VM_EXEC | VM_MAYEXEC);
 
 	/* OK, we are done with that, now set up the arg stuff,
 	   and then start this sucker up */
===== include/asm-i386/page.h 1.26 vs edited =====
--- 1.26/include/asm-i386/page.h	2004-07-01 17:00:00 -07:00
+++ edited/include/asm-i386/page.h	2004-07-16 17:24:26 -07:00
@@ -142,6 +142,7 @@
 
 #define VM_DATA_DEFAULT_FLAGS	(VM_READ | VM_WRITE | \
 				 VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC)
+#define LEGACY_VM_DATA_DEFAULT_FLAGS	(VM_DATA_DEFAULT_FLAGS | VM_EXEC)
 
 #endif /* __KERNEL__ */
 
===== include/linux/mm.h 1.138 vs edited =====
--- 1.138/include/linux/mm.h	2004-07-06 22:19:25 -07:00
+++ edited/include/linux/mm.h	2004-07-16 17:23:41 -07:00
@@ -136,6 +136,10 @@
 #define VM_HUGETLB	0x00400000	/* Huge TLB Page VM */
 #define VM_NONLINEAR	0x00800000	/* Is non-linear (remap_file_pages) */
 
+#ifndef LEGACY_VM_DATA_DEFAULT_FLAGS
+#define LEGACY_VM_DATA_DEFAULT_FLAGS VM_DATA_DEFAULT_FLAGS
+#endif
+
 #ifndef VM_STACK_DEFAULT_FLAGS		/* arch can override this */
 #define VM_STACK_DEFAULT_FLAGS VM_DATA_DEFAULT_FLAGS
 #endif