[PATCH] fix BUG: unable to handle kernel .. in free_init_pages called from mark_rodata_ro

[PATCH] fix BUG: unable to handle kernel .. in free_init_pages called from mark_rodata_ro

[Konrad Rzeszutek Wilk]

When running under Xen as PV guest, with CONFIG_DEBUG_RODATA set we get this ugly BUG:

[    0.262514] BUG: unable to handle kernel paging request at ffff8800013f4000
[    0.262526] IP: [<ffffffff8102bb0b>] free_init_pages+0xa3/0xcc
[    0.262538] PGD 1611067 PUD 1615067 PMD 556b067 PTE 100000013f4025
[    0.262554] Oops: 0003 [#1] SMP 
[    0.262564] last sysfs file: 
[    0.262569] CPU 0 
[    0.262578] Pid: 1, comm: swapper Not tainted 2.6.33-rc7NEB #67 /
[    0.262585] RIP: e030:[<ffffffff8102bb0b>]  [<ffffffff8102bb0b>] free_init_pages+0xa3/0xcc
[    0.262597] RSP: e02b:ffff88001fcfbe40  EFLAGS: 00010286
[    0.262603] RAX: 00000000cccccccc RBX: ffff880001400000 RCX: 0000000000000400
[    0.262610] RDX: ffff8800013f4000 RSI: 0000000000000000 RDI: ffff8800013f4000
[    0.262617] RBP: ffff88001fcfbe70 R08: 0000000000000000 R09: ffff88001fc02200
[    0.262624] R10: ffff88001fc02200 R11: ffff88001fcfbd00 R12: ffff8800013f4000
[    0.262631] R13: 0000000000000400 R14: ffffea0000000000 R15: 00000000cccccccc
[    0.262641] FS:  0000000000000000(0000) GS:ffff880005598000(0000) knlGS:0000000000000000
[    0.262649] CS:  e033 DS: 0000 ES: 0000 CR0: 000000008005003b
[    0.262655] CR2: ffff8800013f4000 CR3: 0000000001610000 CR4: 0000000000000660
[    0.262663] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[    0.262671] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[    0.262678] Process swapper (pid: 1, threadinfo ffff88001fcfa000, task ffff88001fd00000)
[    0.262685] Stack:
[    0.262690]  0000000000000000 0000000001400000 ffffffff813f4000 ffffffff81000000
[    0.262704] <0> ffffffff815c7000 ffffffff81600000 ffff88001fcfbf00 ffffffff8102c2cb
[    0.262721] <0> 00000000000001c7 ffffffff813f4000 ffffffff81600000 0000000000000039
[    0.262740] Call Trace:
[    0.262749]  [<ffffffff8102c2cb>] mark_rodata_ro+0x4a2/0x527
[    0.262759]  [<ffffffff810021a5>] init_post+0x2b/0x10e
[    0.262769]  [<ffffffff8169a703>] kernel_init+0x1b1/0x1bc
[    0.262777]  [<ffffffff8100a7e4>] kernel_thread_helper+0x4/0x10
[    0.262894]  [<ffffffff81009be1>] ? int_ret_from_sys_call+0x7/0x1b
[    0.262904]  [<ffffffff813e961d>] ? retint_restore_args+0x5/0x6
[    0.262912]  [<ffffffff8100a7e0>] ? kernel_thread_helper+0x0/0x10


I traced it down to the mark_rodata_ which sets the .text through .sdata to PAGE_RO.
Then it sets PAGE_NX whenever it can, and for two selective sections:
a) .__stop___ex_table -> .__start_rodata and b).__end_rodata -> ._sdata sets
them to _PAGE_RW. Both a) and b) are recycled by free_init_pages which tries to
write to the sections POISON_FREE_INITMEM and it hits the BUG().

The reason for this is that 'set_memory_rw' eventually ends up calling
'static_projections' which checks certain ranges of addresses and forbids certain
page  flags depending on the nature of the region. One of checks is to forbid _PAGE_RW
to the region from .text to ._sdata. The a) and b) section fall in that, and
the _PAGE_RW page attribute does not get set. If you are looking at the code
please note that at that stage the 'kernel_set_to_readonly' has been set.

Now this BUG() only shows up on Xen. The one big difference between baremetal
and paravirtualized is that on Xen all pages are 4KB in size. On baremetal
those two regions are marked as 2MB page.

When running this on bare-metal those sections get split from 2MB to 4KB chunks
and the _PAGE_RW is set without any trouble (even though the sections do fall in the
.text and .sdata).  I am at loss to explain why this works on bare-metal even
thought it looks to be doing the wrong thing there too. I sprinkled dump_stack()
to figure this out but got address that don't vibe with the reality, any ideas?

In summary, the patch allows the two sections a) and b) to have _PAGE_RW set
so that they can be written to and re-used. 

[PATCH] x86_64: allow sections that are recycled to set _PAGE_RW

[Konrad Rzeszutek Wilk]

This fixes BUG: unable to handle kernel paging request at ffff8800013f4000

 IP: [<ffffffff8102bb0b>] free_init_pages+0xa3/0xcc
 PGD 1611067 PUD 1615067 PMD 556b067 PTE 100000013f4025
...
  [<ffffffff8102c2cb>] mark_rodata_ro+0x4a2/0x527
  [<ffffffff810021a5>] init_post+0x2b/0x10e
...

On platforms where the pages to be recycled ("free") are 4KB (Xen PV guest).

In the mark_rodata_ro, we set the .text through .sdata section to RO,
then for selective sections (.__stop___ex_table -> .__start_rodata)
and (.__end_rodata -> ._sdata) set them to RW. The logic in
static_protections forbids this and unsets the _PAGE_RW attribute.

This is not an issue if the sections to be recycled are in 2MB pages
so on native platform this isn't seen.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/mm/pageattr.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 1d4eb93..44ebcd7 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -291,8 +291,17 @@ static inline pgprot_t static_protections(pgprot_t prot, unsigned long address,
 	 */
 	if (kernel_set_to_readonly &&
 	    within(address, (unsigned long)_text,
-		   (unsigned long)__end_rodata_hpage_align))
-		pgprot_val(forbidden) |= _PAGE_RW;
+		   (unsigned long)__end_rodata_hpage_align)) {
+		/* When 'kernel_set_to_readonly', it is OK to set PAGE_RW
+		 * on the ones that are being recycled by free_init_pages
+		 * in mark_rodata_ro.
+		 */
+		if (!within(address, (unsigned long)&__stop___ex_table,
+				(unsigned long)&__start_rodata) &&
+		    !within(address, (unsigned long)&__end_rodata,
+				(unsigned long)&_sdata))
+			pgprot_val(forbidden) |= _PAGE_RW;
+	}
 #endif
 
 	prot = __pgprot(pgprot_val(prot) & ~pgprot_val(forbidden));
-- 
1.6.2.5

Re: [PATCH] x86_64: allow sections that are recycled to set _PAGE_RW

[Suresh Siddha]

Konrad,

I don't think this patch is correct.

I am not sure if I understand why we see this failure in Xen. In native
case, we have two kernel mappings for 64bit kernel.

One is kernel text mapping:

ffffffff80000000 - ffffffffa0000000 (=512 MB)  kernel text mapping, from
phys 0

And another is:

ffff880000000000 - ffffc7ffffffffff (=64 TB) direct mapping of all phys.
memory

Checks in static_protections() ensure that we map the text mapping as
read-only (and don't bother about what permissions it uses for
underlying free pages that get freed in free_init_pages()). But the
kernel direct mappings for free pages will be RW and ensure that the
free pages can be read/written using the direct mapping.

The checks in static_protections() for kernel text mapping ensure that
we don't break the 2MB kernel text pages unnecessarily on 64bit kernels
(as it has performance implications). We should be fine as long as the
kernel identity mappings reflect the correct RW permissions.

But somehow this is working fine on native kernels but not on Xen pv
guest. Your patch will cause the performance issues that we are
addressing using the static protections checks. I will look at this more
detailed on tuesday.

thanks,
suresh

On Fri, 2010-02-12 at 20:15 -0700, Konrad Rzeszutek Wilk wrote:
> This fixes BUG: unable to handle kernel paging request at ffff8800013f4000
> 
>  IP: [<ffffffff8102bb0b>] free_init_pages+0xa3/0xcc
>  PGD 1611067 PUD 1615067 PMD 556b067 PTE 100000013f4025
> ...
>   [<ffffffff8102c2cb>] mark_rodata_ro+0x4a2/0x527
>   [<ffffffff810021a5>] init_post+0x2b/0x10e
> ...
> 
> On platforms where the pages to be recycled ("free") are 4KB (Xen PV guest).
> 
> In the mark_rodata_ro, we set the .text through .sdata section to RO,
> then for selective sections (.__stop___ex_table -> .__start_rodata)
> and (.__end_rodata -> ._sdata) set them to RW. The logic in
> static_protections forbids this and unsets the _PAGE_RW attribute.
> 
> This is not an issue if the sections to be recycled are in 2MB pages
> so on native platform this isn't seen.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  arch/x86/mm/pageattr.c |   13 +++++++++++--
>  1 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
> index 1d4eb93..44ebcd7 100644
> --- a/arch/x86/mm/pageattr.c
> +++ b/arch/x86/mm/pageattr.c
> @@ -291,8 +291,17 @@ static inline pgprot_t static_protections(pgprot_t prot, unsigned long address,
>  	 */
>  	if (kernel_set_to_readonly &&
>  	    within(address, (unsigned long)_text,
> -		   (unsigned long)__end_rodata_hpage_align))
> -		pgprot_val(forbidden) |= _PAGE_RW;
> +		   (unsigned long)__end_rodata_hpage_align)) {
> +		/* When 'kernel_set_to_readonly', it is OK to set PAGE_RW
> +		 * on the ones that are being recycled by free_init_pages
> +		 * in mark_rodata_ro.
> +		 */
> +		if (!within(address, (unsigned long)&__stop___ex_table,
> +				(unsigned long)&__start_rodata) &&
> +		    !within(address, (unsigned long)&__end_rodata,
> +				(unsigned long)&_sdata))
> +			pgprot_val(forbidden) |= _PAGE_RW;
> +	}
>  #endif
>  
>  	prot = __pgprot(pgprot_val(prot) & ~pgprot_val(forbidden));

Re: [PATCH] x86_64: allow sections that are recycled to set _PAGE_RW

[H. Peter Anvin]

On 02/13/2010 12:08 PM, Suresh Siddha wrote:
>
> Checks in static_protections() ensure that we map the text mapping as
> read-only (and don't bother about what permissions it uses for
> underlying free pages that get freed in free_init_pages()). But the
> kernel direct mappings for free pages will be RW and ensure that the
> free pages can be read/written using the direct mapping.
>
> The checks in static_protections() for kernel text mapping ensure that
> we don't break the 2MB kernel text pages unnecessarily on 64bit kernels
> (as it has performance implications). We should be fine as long as the
> kernel identity mappings reflect the correct RW permissions.
>
> But somehow this is working fine on native kernels but not on Xen pv
> guest. Your patch will cause the performance issues that we are
> addressing using the static protections checks. I will look at this more
> detailed on tuesday.
>

Xen doesn't support PSE, so it has the (otherwise impossible) 
combination of PAE and !PSE ... i.e. it only uses 4K pages.

	-hpa

Re: [LKML] Re: [PATCH] x86_64: allow sections that are recycled to set _PAGE_RW

[Konrad Rzeszutek Wilk]

On Sat, Feb 13, 2010 at 12:08:17PM -0800, Suresh Siddha wrote:
> Konrad,
> 
> I don't think this patch is correct.
> 
> I am not sure if I understand why we see this failure in Xen. In native
> case, we have two kernel mappings for 64bit kernel.
> 
> One is kernel text mapping:
> 
> ffffffff80000000 - ffffffffa0000000 (=512 MB)  kernel text mapping, from
> phys 0
> 
> And another is:
> 
> ffff880000000000 - ffffc7ffffffffff (=64 TB) direct mapping of all phys.
> memory
> 
> Checks in static_protections() ensure that we map the text mapping as
> read-only (and don't bother about what permissions it uses for
> underlying free pages that get freed in free_init_pages()). But the
> kernel direct mappings for free pages will be RW and ensure that the
> free pages can be read/written using the direct mapping.
> 
> The checks in static_protections() for kernel text mapping ensure that
> we don't break the 2MB kernel text pages unnecessarily on 64bit kernels
> (as it has performance implications). We should be fine as long as the
> kernel identity mappings reflect the correct RW permissions.
> 
> But somehow this is working fine on native kernels but not on Xen pv
> guest. Your patch will cause the performance issues that we are

That would not be good.

> addressing using the static protections checks. I will look at this more
> detailed on tuesday.

Great. Thank you for doing that. If you find yourself in a bind, here are
some steps on how to build the Xen pv-ops kernel and such:
http://wiki.xensource.com/xenwiki/XenParavirtOps

It goes without saying that I would be happy to test your patch when
you have one ready.

Re: [LKML] Re: [PATCH] x86_64: allow sections that are recycled to set _PAGE_RW

[Suresh Siddha]

On Tue, 2010-02-16 at 14:13 -0800, Konrad Rzeszutek Wilk wrote:
> On Sat, Feb 13, 2010 at 12:08:17PM -0800, Suresh Siddha wrote:
> > The checks in static_protections() for kernel text mapping ensure that
> > we don't break the 2MB kernel text pages unnecessarily on 64bit kernels
> > (as it has performance implications). We should be fine as long as the
> > kernel identity mappings reflect the correct RW permissions.
> > 
> > But somehow this is working fine on native kernels but not on Xen pv
> > guest. Your patch will cause the performance issues that we are
> 
> That would not be good.
> 
> > addressing using the static protections checks. I will look at this more
> > detailed on tuesday.
> 
> Great. Thank you for doing that. If you find yourself in a bind, here are
> some steps on how to build the Xen pv-ops kernel and such:
> http://wiki.xensource.com/xenwiki/XenParavirtOps
> 
> It goes without saying that I would be happy to test your patch when
> you have one ready.

x86 folks, can you please queue the appended patch? If you think it is
too late for 2.6.33, I added a "cc: stable", so that they can pick this
up for both .32 and .33. Thanks.
---

From: Suresh Siddha <suresh.b.siddha@intel.com>
Subject: x86_64, cpa: don't work hard in preserving kernel text 2M mapping when using 4K already

We currently enforce the !RW mapping for the kernel mapping that maps
holes between different text, rodata and data sections. However, kernel
identity mappings will have different RWX permissions to the pages mapping to
text and to the pages padding (which are freed) the text, rodata sections.
Hence kernel identity mappings will be broken to smaller pages. For 64-bit,
kernel text and kernel identity mappings are different, so we can enable
protection checks that come with CONFIG_DEBUG_RODATA, as well as retain 2MB
large page mappings for kernel text.

Konrad reported a boot failure with the Linux Xen paravirt guest because of
this. In this paravirt guest case, the kernel text mapping and the kernel
identity mapping share the same page-table pages. Thus forcing the !RW mapping
for some of the kernel mappings also cause the kernel identity mappings to be
read-only resulting in the boot failure. Linux Xen paravirt guest also
uses 4k mappings and don't use 2M mapping.

Fix this issue and retain large page performance advantage for native kernels
by not working hard and not enforcing !RW for the kernel text mapping,
if the current mapping is already using small page mapping.

Reported-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: stable@kernel.org	[2.6.32, 2.6.33]
---

index 1d4eb93..cf07c26 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -291,8 +291,29 @@ static inline pgprot_t static_protections(pgprot_t prot, unsigned long address,
 	 */
 	if (kernel_set_to_readonly &&
 	    within(address, (unsigned long)_text,
-		   (unsigned long)__end_rodata_hpage_align))
-		pgprot_val(forbidden) |= _PAGE_RW;
+		   (unsigned long)__end_rodata_hpage_align)) {
+		unsigned int level;
+
+		/*
+		 * Don't enforce the !RW mapping for the kernel text mapping,
+		 * if the current mapping is already using small page mapping.
+		 * No need to work hard to preserve large page mappings in this
+		 * case.
+		 *
+		 * This also fixes the Linux Xen paravirt guest boot failure
+		 * (because of unexpected read-only mappings for kernel identity
+		 * mappings). In this paravirt guest case, the kernel text
+		 * mapping and the kernel identity mapping share the same
+		 * page-table pages. Thus we can't really use different
+		 * protections for the kernel text and identity mappings. Also,
+		 * these shared mappings are made of small page mappings.
+		 * Thus this don't enforce !RW mapping for small page kernel
+		 * text mapping logic will help Linux Xen parvirt guest boot
+		 * aswell.
+		 */
+		if (lookup_address(address, &level) && (level != PG_LEVEL_4K))
+			pgprot_val(forbidden) |= _PAGE_RW;
+	}
 #endif
 
 	prot = __pgprot(pgprot_val(prot) & ~pgprot_val(forbidden));

[tip:x86/urgent] x86_64, cpa: Don't work hard in preserving kernel 2M mappings when using 4K already

[tip-bot for Suresh Siddha]

Commit-ID:  281ff33b7c1b1ba2a5f9b03425e5f692a94913fa
Gitweb:     http://git.kernel.org/tip/281ff33b7c1b1ba2a5f9b03425e5f692a94913fa
Author:     Suresh Siddha <suresh.b.siddha@intel.com>
AuthorDate: Thu, 18 Feb 2010 11:51:40 -0800
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Mon, 22 Feb 2010 15:09:31 -0800

x86_64, cpa: Don't work hard in preserving kernel 2M mappings when using 4K already

We currently enforce the !RW mapping for the kernel mapping that maps
holes between different text, rodata and data sections. However, kernel
identity mappings will have different RWX permissions to the pages mapping to
text and to the pages padding (which are freed) the text, rodata sections.
Hence kernel identity mappings will be broken to smaller pages. For 64-bit,
kernel text and kernel identity mappings are different, so we can enable
protection checks that come with CONFIG_DEBUG_RODATA, as well as retain 2MB
large page mappings for kernel text.

Konrad reported a boot failure with the Linux Xen paravirt guest because of
this. In this paravirt guest case, the kernel text mapping and the kernel
identity mapping share the same page-table pages. Thus forcing the !RW mapping
for some of the kernel mappings also cause the kernel identity mappings to be
read-only resulting in the boot failure. Linux Xen paravirt guest also
uses 4k mappings and don't use 2M mapping.

Fix this issue and retain large page performance advantage for native kernels
by not working hard and not enforcing !RW for the kernel text mapping,
if the current mapping is already using small page mapping.

Reported-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
LKML-Reference: <1266522700.2909.34.camel@sbs-t61.sc.intel.com>
Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: stable@kernel.org	[2.6.32, 2.6.33]
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/mm/pageattr.c |   25 +++++++++++++++++++++++--
 1 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 1d4eb93..cf07c26 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -291,8 +291,29 @@ static inline pgprot_t static_protections(pgprot_t prot, unsigned long address,
 	 */
 	if (kernel_set_to_readonly &&
 	    within(address, (unsigned long)_text,
-		   (unsigned long)__end_rodata_hpage_align))
-		pgprot_val(forbidden) |= _PAGE_RW;
+		   (unsigned long)__end_rodata_hpage_align)) {
+		unsigned int level;
+
+		/*
+		 * Don't enforce the !RW mapping for the kernel text mapping,
+		 * if the current mapping is already using small page mapping.
+		 * No need to work hard to preserve large page mappings in this
+		 * case.
+		 *
+		 * This also fixes the Linux Xen paravirt guest boot failure
+		 * (because of unexpected read-only mappings for kernel identity
+		 * mappings). In this paravirt guest case, the kernel text
+		 * mapping and the kernel identity mapping share the same
+		 * page-table pages. Thus we can't really use different
+		 * protections for the kernel text and identity mappings. Also,
+		 * these shared mappings are made of small page mappings.
+		 * Thus this don't enforce !RW mapping for small page kernel
+		 * text mapping logic will help Linux Xen parvirt guest boot
+		 * aswell.
+		 */
+		if (lookup_address(address, &level) && (level != PG_LEVEL_4K))
+			pgprot_val(forbidden) |= _PAGE_RW;
+	}
 #endif
 
 	prot = __pgprot(pgprot_val(prot) & ~pgprot_val(forbidden));