gate page oops

gate page oops

[Jason Baron]

Hi,

While working on the RHEL 4 kernel (based on 2.6.9), i encountered a BUG() 
in mm/memory.c:get_user_pages() at the line: BUG_ON(pte_none(*pte));

Seems like in_gate_area() returns true, but there is no pte to map the 
page. That is, the in_gate_area() covers a larger range than what is 
actually mapped. 

I'm not sure if this is a problem in 2.6.11, as i didn't get a chance to 
build and test a kernel based on 2.6.11. 

The patch below fixed the problem i was seeing by simply restricting 
in_gate_area(). Although as i look at this now it may be incorrect based 
on the HAVE_BUGGY_SERGEL #define. I'll fix that up if this patch makes 
sense to others...

thanks,

-Jason


--- linux/include/asm-ia64/page.h.bak	2005-04-22 12:30:29.000000000 -0400
+++ linux/include/asm-ia64/page.h	2005-04-22 13:50:26.000000000 -0400
@@ -195,4 +195,15 @@
 
 #define devmem_is_allowed(x) 1
 
+#define CONFIG_ARCH_GATE_AREA 1
+
+#ifdef __KERNEL__
+#ifndef __ASSEMBLY__
+struct task_struct;
+struct vm_area_struct *get_gate_vma(struct task_struct *tsk);
+int in_gate_area(struct task_struct *task, unsigned long addr);
+#endif
+#endif
+
+
 #endif /* _ASM_IA64_PAGE_H */
--- linux/arch/ia64/mm/init.c.bak	2005-04-22 12:33:15.000000000 -0400
+++ linux/arch/ia64/mm/init.c	2005-04-22 14:43:02.000000000 -0400
@@ -691,3 +691,26 @@
 	ia32_mem_init();
 #endif
 }
+
+static struct vm_area_struct gate_vma = {
+        .vm_mm = NULL,
+        .vm_start = FIXADDR_USER_START,
+        .vm_end = FIXADDR_USER_END,
+        .vm_page_prot = PAGE_READONLY,
+        .vm_flags = 0
+};
+
+struct vm_area_struct *get_gate_vma(struct task_struct *tsk)
+{
+	return &gate_vma;
+}
+
+int in_gate_area(struct task_struct *task, unsigned long addr)
+{
+        if ((addr >= FIXADDR_USER_START) && (addr < FIXADDR_USER_START + PAGE_SIZE))
+                return 1;
+        if ((addr >= FIXADDR_USER_START + PERCPU_PAGE_SIZE) && (addr < FIXADDR_USER_START + PERCPU_PAGE_SIZE + PAGE_SIZE))
+                return 1;
+        return 0;
+}
+

#include <sys/ptrace.h>
#include <sys/types.h>
#include <unistd.h>


int main() {


	pid_t pid;
	int ret;
	long val;

	pid = fork();

	if (pid = 0) {
		//spin
		while(1);
	} else {
		ret = ptrace(PTRACE_ATTACH, pid, NULL, NULL);
		if(ret) {
			perror("attach error");
			exit(1);
		}

		val = ptrace(PTRACE_PEEKDATA, pid, 0xa000000000004000, 
NULL);
		printf("val is: %i\n", val);
	}	


}

RE: gate page oops

[Luck, Tony]

>While working on the RHEL 4 kernel (based on 2.6.9), i encountered a BUG() 
>in mm/memory.c:get_user_pages() at the line: BUG_ON(pte_none(*pte));
>
>Seems like in_gate_area() returns true, but there is no pte to map the 
>page. That is, the in_gate_area() covers a larger range than what is 
>actually mapped. 
>
>I'm not sure if this is a problem in 2.6.11, as i didn't get a 
>chance to build and test a kernel based on 2.6.11. 

It looks like there is some muddle of usage of PAGE_SIZE
and PERCPU_PAGESIZE here (in the kernel code, not just in
you patch).  Perhaps left over from when the percpu area
was located at the start of region 5??

The gate page only needs to be PAGE_SIZE ... doesn't it?

-Tony

RE: gate page oops

[Jason Baron]

On Fri, 22 Apr 2005, Luck, Tony wrote:

> >While working on the RHEL 4 kernel (based on 2.6.9), i encountered a BUG() 
> >in mm/memory.c:get_user_pages() at the line: BUG_ON(pte_none(*pte));
> >
> >Seems like in_gate_area() returns true, but there is no pte to map the 
> >page. That is, the in_gate_area() covers a larger range than what is 
> >actually mapped. 
> >
> >I'm not sure if this is a problem in 2.6.11, as i didn't get a 
> >chance to build and test a kernel based on 2.6.11. 
> 
> It looks like there is some muddle of usage of PAGE_SIZE
> and PERCPU_PAGESIZE here (in the kernel code, not just in
> you patch).  Perhaps left over from when the percpu area
> was located at the start of region 5??
> 
> The gate page only needs to be PAGE_SIZE ... doesn't it?
> 

I'm not really sure...i was hoping somebody on the list would know :) If 
you look at setup_gate(), it establishes two gate pages, the comment says:

* Map the gate page twice: once read-only to export the ELF headers etc. and once
* execute-only page to enable privilege-promotion via "epc":

So the patch i proposed was intended to allow user access to these two 
regions. Currently, that isn't being done properly, leading to an oops.

-Jason

RE: gate page oops

[David Mosberger]

>>>>> On Tue, 26 Apr 2005 13:10:06 -0400 (EDT), Jason Baron <jbaron@redhat.com> said:

  Jason> On Fri, 22 Apr 2005, Luck, Tony wrote:

  >> The gate page only needs to be PAGE_SIZE ... doesn't it?

  Jason> I'm not really sure...i was hoping somebody on the list would
  Jason> know :) If you look at setup_gate(), it establishes two gate
  Jason> pages, the comment says:

  Jason> * Map the gate page twice: once read-only to export the ELF
  Jason> headers etc. and once * execute-only page to enable
  Jason> privilege-promotion via "epc":

  Jason> So the patch i proposed was intended to allow user access to
  Jason> these two regions. Currently, that isn't being done properly,
  Jason> leading to an oops.

Yes, IIRC, we need to give ptrace() access to the execute-only page so
that a debugger can read and decode the instruction at the address
(yes, that circumvents the execute-only part of the mapping, but we
don't really care about that; the only reason it's execute-only is
because that's the only way to get the promote-privilege-on-epc
behavior).

	--david

RE: gate page oops

[Jason Baron]

On Fri, 29 Apr 2005, David Mosberger wrote:

> >>>>> On Tue, 26 Apr 2005 13:10:06 -0400 (EDT), Jason Baron <jbaron@redhat.com> said:
> 
>   Jason> On Fri, 22 Apr 2005, Luck, Tony wrote:
> 
>   >> The gate page only needs to be PAGE_SIZE ... doesn't it?
> 
>   Jason> I'm not really sure...i was hoping somebody on the list would
>   Jason> know :) If you look at setup_gate(), it establishes two gate
>   Jason> pages, the comment says:
> 
>   Jason> * Map the gate page twice: once read-only to export the ELF
>   Jason> headers etc. and once * execute-only page to enable
>   Jason> privilege-promotion via "epc":
> 
>   Jason> So the patch i proposed was intended to allow user access to
>   Jason> these two regions. Currently, that isn't being done properly,
>   Jason> leading to an oops.
> 
> Yes, IIRC, we need to give ptrace() access to the execute-only page so
> that a debugger can read and decode the instruction at the address
> (yes, that circumvents the execute-only part of the mapping, but we
> don't really care about that; the only reason it's execute-only is
> because that's the only way to get the promote-privilege-on-epc
> behavior).
> 
> 	--david
> 

So are there any objections to the patch? It seems to be consistent with 
this, and it fixes a local DOS.

thanks,

-Jason

RE: gate page oops

[David Mosberger]

>>>>> On Tue, 3 May 2005 16:47:47 -0400 (EDT), Jason Baron <jbaron@redhat.com> said:

  Jason> On Fri, 29 Apr 2005, David Mosberger wrote:

  >> >>>>> On Tue, 26 Apr 2005 13:10:06 -0400 (EDT), Jason Baron <jbaron@redhat.com> said:
  >>
  Jason> On Fri, 22 Apr 2005, Luck, Tony wrote:
  >>
  >> >> The gate page only needs to be PAGE_SIZE ... doesn't it?
  >>
  Jason> I'm not really sure...i was hoping somebody on the list would
  Jason> know :) If you look at setup_gate(), it establishes two gate
  Jason> pages, the comment says:
  >>
  Jason> * Map the gate page twice: once read-only to export the ELF
  Jason> headers etc. and once * execute-only page to enable
  Jason> privilege-promotion via "epc":
  >>
  Jason> So the patch i proposed was intended to allow user access to
  Jason> these two regions. Currently, that isn't being done properly,
  Jason> leading to an oops.

  >> Yes, IIRC, we need to give ptrace() access to the execute-only page so
  >> that a debugger can read and decode the instruction at the address
  >> (yes, that circumvents the execute-only part of the mapping, but we
  >> don't really care about that; the only reason it's execute-only is
  >> because that's the only way to get the promote-privilege-on-epc
  >> behavior).

  Jason> So are there any objections to the patch? It seems to be
  Jason> consistent with this, and it fixes a local DOS.

I (finally) looked into this again and my current thinking is that it
may be better to go back to mapping the two pages consecutively.  The
gate-related code has an implicit assumption that the gate-area is
occupying a single region of memory (that _could_ be changed, though).
Re-enabling the HAVE_BUGG_SEGREL code unconditionally should do that,
at the expense of increasing the size of the kernel's ELF image by
16KB.  I need to double-check, but I think there won't be any other
negative side-effects.

More on this a bit later...

	--david

RE: gate page oops

[David Mosberger]

>>>>> On Mon, 6 Jun 2005 17:06:23 -0700, David Mosberger <davidm@linux.hpl.hp.com> said:

  David> I (finally) looked into this again and my current thinking is
  David> that it may be better to go back to mapping the two pages
  David> consecutively.  The gate-related code has an implicit
  David> assumption that the gate-area is occupying a single region of
  David> memory (that _could_ be changed, though).  Re-enabling the
  David> HAVE_BUGG_SEGREL code unconditionally should do that, at the
  David> expense of increasing the size of the kernel's ELF image by
  David> 16KB.  I need to double-check, but I think there won't be any
  David> other negative side-effects.

Well, it's easier to just map the holes with zero-pages.  The cost is
trivial (a few more non-zero kernel page-table entries) and it passes
all my testing (including coredumping and gdb single-stepping over an
EPC syscall).

Unless there are any objections, please apply (for 2.6.12, hopefully).

	--david

[IA64] Fill holes in FIXADDR_USER space with zero pages.

This fixes an oops reported by Jason Baron.

 arch/ia64/mm/init.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
--- a/arch/ia64/mm/init.c
+++ b/arch/ia64/mm/init.c
@@ -305,8 +305,9 @@ setup_gate (void)
 	struct page *page;
 
 	/*
-	 * Map the gate page twice: once read-only to export the ELF headers etc. and once
-	 * execute-only page to enable privilege-promotion via "epc":
+	 * Map the gate page twice: once read-only to export the ELF
+	 * headers etc. and once execute-only page to enable
+	 * privilege-promotion via "epc":
 	 */
 	page = virt_to_page(ia64_imva(__start_gate_section));
 	put_kernel_page(page, GATE_ADDR, PAGE_READONLY);
@@ -315,6 +316,20 @@ setup_gate (void)
 	put_kernel_page(page, GATE_ADDR + PAGE_SIZE, PAGE_GATE);
 #else
 	put_kernel_page(page, GATE_ADDR + PERCPU_PAGE_SIZE, PAGE_GATE);
+	/* Fill in the holes (if any) with read-only zero pages: */
+	{
+		unsigned long addr;
+
+		for (addr = GATE_ADDR + PAGE_SIZE;
+		     addr < GATE_ADDR + PERCPU_PAGE_SIZE;
+		     addr += PAGE_SIZE)
+		{
+			put_kernel_page(ZERO_PAGE(0), addr,
+					PAGE_READONLY);
+			put_kernel_page(ZERO_PAGE(0), addr + PERCPU_PAGE_SIZE,
+					PAGE_READONLY);
+		}
+	}
 #endif
 	ia64_patch_gate();
 }

RE: gate page oops

[Jason Baron]

On Tue, 7 Jun 2005, David Mosberger wrote:

> >>>>> On Mon, 6 Jun 2005 17:06:23 -0700, David Mosberger <davidm@linux.hpl.hp.com> said:
> 
>   David> I (finally) looked into this again and my current thinking is
>   David> that it may be better to go back to mapping the two pages
>   David> consecutively.  The gate-related code has an implicit
>   David> assumption that the gate-area is occupying a single region of
>   David> memory (that _could_ be changed, though).  Re-enabling the
>   David> HAVE_BUGG_SEGREL code unconditionally should do that, at the
>   David> expense of increasing the size of the kernel's ELF image by
>   David> 16KB.  I need to double-check, but I think there won't be any
>   David> other negative side-effects.
> 
> Well, it's easier to just map the holes with zero-pages.  The cost is
> trivial (a few more non-zero kernel page-table entries) and it passes
> all my testing (including coredumping and gdb single-stepping over an
> EPC syscall).
> 
> Unless there are any objections, please apply (for 2.6.12, hopefully).
> 

Fixes my test case too. thanks.

-Jason

RE: gate page oops

[Luck, Tony]

>Unless there are any objections, please apply (for 2.6.12, hopefully).

I still don't really understand why there are still any "PERCPU_PAGE_SIZE"
references in this code.

It looks like if we are building with a buggy linker, then we workaround
problems by having two separate pages in our vmlinux image, and we map
them at consecutive virtual pages (of size PAGE_SIZE) at the start of region
5 (GATE_ADDR = 0xa000000000000000).

With a good linker, there is only one page in the vmlinux image, and we map
it twice ... but for some unspecified reason we put the mappings on
PERCPU_PAGE_SIZE boundaries at the start of region 5.  This leaves two holes
(assuming PERCPU_PAGE_SIZE > PAGE_SIZE ... which it is unless PAGE_SIZEdk).

Why do we need the holes?

Should we also map zero pages in the buggy linker case (since there is a hole
from GATE_ADDR + 2*PAGE_SIZE to FIXADDR_USER_END where there are no mappings).
Or perhaps FIXADDR_USER_END should only be GATE_ADDR + 2*PAGE_SIZE in the
buggy linker case?

-Tony

RE: gate page oops

[David Mosberger]

>>>>> On Wed, 8 Jun 2005 10:11:05 -0700, "Luck, Tony" <tony.luck@intel.com> said:

  >> Unless there are any objections, please apply (for 2.6.12, hopefully).

  Tony> I still don't really understand why there are still any
  Tony> "PERCPU_PAGE_SIZE" references in this code.

  Tony> It looks like if we are building with a buggy linker, then we
  Tony> workaround problems by having two separate pages in our
  Tony> vmlinux image, and we map them at consecutive virtual pages
  Tony> (of size PAGE_SIZE) at the start of region 5 (GATE_ADDR   Tony> 0xa000000000000000).

  Tony> With a good linker, there is only one page in the vmlinux
  Tony> image, and we map it twice ... but for some unspecified reason
  Tony> we put the mappings on PERCPU_PAGE_SIZE boundaries at the
  Tony> start of region 5.  This leaves two holes (assuming
  Tony> PERCPU_PAGE_SIZE > PAGE_SIZE ... which it is unless
  Tony> PAGE_SIZEdk).

  Tony> Why do we need the holes?

Because ia64 linux ELF images can only alias at a 64KB boundary (the
max. supported kernel page size).

It doesn't make a huge difference and in fact I tried out a patch
which was just always using the BUGGY_SEGREL code.  That works too,
but it strikes me as less elegant (increases the size of the ELF image
needlessly; not that it makes a huge difference) and it's definitely
a larger patch (which affects several files).

  Tony> Should we also map zero pages in the buggy linker case (since
  Tony> there is a hole from GATE_ADDR + 2*PAGE_SIZE to
  Tony> FIXADDR_USER_END where there are no mappings).  Or perhaps
  Tony> FIXADDR_USER_END should only be GATE_ADDR + 2*PAGE_SIZE in the
  Tony> buggy linker case?

Good catch.  I think the latter (adjusting FIXADDR_USER_END) would be
better.

	--david

RE: gate page oops

[David Mosberger]

>>>>> On Wed, 8 Jun 2005 10:11:05 -0700, "Luck, Tony" <tony.luck@intel.com> said:

  Tony> Should we also map zero pages in the buggy linker case (since
  Tony> there is a hole from GATE_ADDR + 2*PAGE_SIZE to
  Tony> FIXADDR_USER_END where there are no mappings).  Or perhaps
  Tony> FIXADDR_USER_END should only be GATE_ADDR + 2*PAGE_SIZE in the
  Tony> buggy linker case?

Here is an updated patch.

	--david

[IA64] Fill holes in FIXADDR_USER space with zero pages.

This fixes an oops reported by Jason Baron.

 arch/ia64/mm/init.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
--- a/arch/ia64/mm/init.c
+++ b/arch/ia64/mm/init.c
@@ -305,8 +305,9 @@ setup_gate (void)
 	struct page *page;
 
 	/*
-	 * Map the gate page twice: once read-only to export the ELF headers etc. and once
-	 * execute-only page to enable privilege-promotion via "epc":
+	 * Map the gate page twice: once read-only to export the ELF
+	 * headers etc. and once execute-only page to enable
+	 * privilege-promotion via "epc":
 	 */
 	page = virt_to_page(ia64_imva(__start_gate_section));
 	put_kernel_page(page, GATE_ADDR, PAGE_READONLY);
@@ -315,6 +316,20 @@ setup_gate (void)
 	put_kernel_page(page, GATE_ADDR + PAGE_SIZE, PAGE_GATE);
 #else
 	put_kernel_page(page, GATE_ADDR + PERCPU_PAGE_SIZE, PAGE_GATE);
+	/* Fill in the holes (if any) with read-only zero pages: */
+	{
+		unsigned long addr;
+
+		for (addr = GATE_ADDR + PAGE_SIZE;
+		     addr < GATE_ADDR + PERCPU_PAGE_SIZE;
+		     addr += PAGE_SIZE)
+		{
+			put_kernel_page(ZERO_PAGE(0), addr,
+					PAGE_READONLY);
+			put_kernel_page(ZERO_PAGE(0), addr + PERCPU_PAGE_SIZE,
+					PAGE_READONLY);
+		}
+	}
 #endif
 	ia64_patch_gate();
 }


diff --git a/include/asm-ia64/pgtable.h b/include/asm-ia64/pgtable.h
--- a/include/asm-ia64/pgtable.h
+++ b/include/asm-ia64/pgtable.h
@@ -8,7 +8,7 @@
  * This hopefully works with any (fixed) IA-64 page-size, as defined
  * in <asm/page.h>.
  *
- * Copyright (C) 1998-2004 Hewlett-Packard Co
+ * Copyright (C) 1998-2005 Hewlett-Packard Co
  *	David Mosberger-Tang <davidm@hpl.hp.com>
  */
 
@@ -551,7 +551,11 @@ do {											\
 
 /* These tell get_user_pages() that the first gate page is accessible from user-level.  */
 #define FIXADDR_USER_START	GATE_ADDR
-#define FIXADDR_USER_END	(GATE_ADDR + 2*PERCPU_PAGE_SIZE)
+#ifdef HAVE_BUGGY_SEGREL
+# define FIXADDR_USER_END	(GATE_ADDR + 2*PAGE_SIZE)
+#else
+# define FIXADDR_USER_END	(GATE_ADDR + 2*PERCPU_PAGE_SIZE)
+#endif
 
 #define __HAVE_ARCH_PTEP_TEST_AND_CLEAR_YOUNG
 #define __HAVE_ARCH_PTEP_TEST_AND_CLEAR_DIRTY

RE: gate page oops

[Luck, Tony]

>  Tony> Why do we need the holes?
>
>Because ia64 linux ELF images can only alias at a 64KB boundary (the
>max. supported kernel page size).

This means that PERCPU_PAGE_SIZE is the wrong define for this.  It
happens to have the value 64K today, but it is only a coincidence that
this is the maximum kernel page size.

In fact, we are already using more than half of the per cpu page (not
counting any modules that allocate per cpu space):

  $ grep __per_cpu_end System.map
  ffffffffffff8588 D __per_cpu_end

so at some point we may need to increase it to 256K (next supported TLB
page size).  Your code will still work, but we'll be filling in much bigger
holes than we need.

-Tony

RE: gate page oops

[David Mosberger]

>>>>> On Thu, 9 Jun 2005 10:48:56 -0700, "Luck, Tony" <tony.luck@intel.com> said:

  Tony> Why do we need the holes?

  >> Because ia64 linux ELF images can only alias at a 64KB boundary (the
  >> max. supported kernel page size).

  Tony> This means that PERCPU_PAGE_SIZE is the wrong define for this.

That is true.  Lazy bastards, those programmers --- oh, wait, it was me! ;-)

	--david