LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] [1/5] CPA: Split static_protections into required_static_prot and advised_static_prot
@ 2008-02-08 13:27 Andi Kleen
  2008-02-08 13:27 ` [PATCH] [2/5] Support range checking for required/advisory protections Andi Kleen
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Andi Kleen @ 2008-02-08 13:27 UTC (permalink / raw)
  To: mingo, tglx, linux-kernel


There is a big difference between NX and RO. NX absolutely has to be cleared
or the kernel will fail while RO just can be set, but does not need to.
And for a large page area not setting NX if there is a area below 
it that needs it is essential, while making it ro is optional again.

This is needed for a followup patch who uses requred_static_prot() for large 
pages where it is inconvenient to check all pages. 

No behaviour change in this patch.

[Lines > 80 characters are changed in followup patch]

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/x86/mm/pageattr.c |   24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

Index: linux/arch/x86/mm/pageattr.c
===================================================================
--- linux.orig/arch/x86/mm/pageattr.c
+++ linux/arch/x86/mm/pageattr.c
@@ -149,7 +149,7 @@ static unsigned long virt_to_highmap(voi
  * right (again, ioremap() on BIOS memory is not uncommon) so this function
  * checks and fixes these known static required protection bits.
  */
-static inline pgprot_t static_protections(pgprot_t prot, unsigned long address)
+static inline pgprot_t required_static_prot(pgprot_t prot, unsigned long address)
 {
 	pgprot_t forbidden = __pgprot(0);
 
@@ -173,21 +173,25 @@ static inline pgprot_t static_protection
 		pgprot_val(forbidden) |= _PAGE_NX;
 
 
+	prot = __pgprot(pgprot_val(prot) & ~pgprot_val(forbidden));
+
+	return prot;
+}
+
+static inline pgprot_t advised_static_prot(pgprot_t prot, unsigned long address)
+{
 #ifdef CONFIG_DEBUG_RODATA
 	/* The .rodata section needs to be read-only */
 	if (within(address, (unsigned long)__start_rodata,
 				(unsigned long)__end_rodata))
-		pgprot_val(forbidden) |= _PAGE_RW;
+		pgprot_val(prot) &= ~_PAGE_RW;
 	/*
 	 * Do the same for the x86-64 high kernel mapping
 	 */
 	if (within(address, virt_to_highmap(__start_rodata),
 				virt_to_highmap(__end_rodata)))
-		pgprot_val(forbidden) |= _PAGE_RW;
+		pgprot_val(prot) &= ~_PAGE_RW;
 #endif
-
-	prot = __pgprot(pgprot_val(prot) & ~pgprot_val(forbidden));
-
 	return prot;
 }
 
@@ -318,7 +322,8 @@ try_preserve_large_page(pte_t *kpte, uns
 
 	pgprot_val(new_prot) &= ~pgprot_val(cpa->mask_clr);
 	pgprot_val(new_prot) |= pgprot_val(cpa->mask_set);
-	new_prot = static_protections(new_prot, address);
+	new_prot = required_static_prot(new_prot, address);
+	new_prot = advised_static_prot(new_prot, address);
 
 	/*
 	 * If there are no changes, return. maxpages has been updated
@@ -456,7 +461,8 @@ repeat:
 		pgprot_val(new_prot) &= ~pgprot_val(cpa->mask_clr);
 		pgprot_val(new_prot) |= pgprot_val(cpa->mask_set);
 
-		new_prot = static_protections(new_prot, address);
+		new_prot = required_static_prot(new_prot, address);
+		new_prot = advised_static_prot(new_prot, address);
 
 		/*
 		 * We need to keep the pfn from the existing PTE,
@@ -546,7 +552,7 @@ static int change_page_attr_addr(struct 
 		 * for the non obvious details.
 		 *
 		 * Note that NX and other required permissions are
-		 * checked in static_protections().
+		 * checked in required_static_prot().
 		 */
 		address = phys_addr + HIGH_MAP_START - phys_base;
 

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

* [PATCH] [2/5] Support range checking for required/advisory protections
  2008-02-08 13:27 [PATCH] [1/5] CPA: Split static_protections into required_static_prot and advised_static_prot Andi Kleen
@ 2008-02-08 13:27 ` Andi Kleen
  2008-02-08 13:27 ` [PATCH] [3/5] CPA: Make advised protection check truly advisory Andi Kleen
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Andi Kleen @ 2008-02-08 13:27 UTC (permalink / raw)
  To: mingo, tglx, linux-kernel


Previously these checks would only check a single address, which is ok
for 4k pages, but not for large pages

Needed for followup patches

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/x86/mm/pageattr.c |   31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

Index: linux/arch/x86/mm/pageattr.c
===================================================================
--- linux.orig/arch/x86/mm/pageattr.c
+++ linux/arch/x86/mm/pageattr.c
@@ -35,6 +35,13 @@ within(unsigned long addr, unsigned long
 	return addr >= start && addr < end;
 }
 
+static inline int
+within_range(unsigned long addr_start, unsigned long addr_end,
+		unsigned long start, unsigned long end)
+{
+	return addr_end >= start && addr_start < end;
+}
+
 /*
  * Flushing functions
  */
@@ -149,7 +156,8 @@ static unsigned long virt_to_highmap(voi
  * right (again, ioremap() on BIOS memory is not uncommon) so this function
  * checks and fixes these known static required protection bits.
  */
-static inline pgprot_t required_static_prot(pgprot_t prot, unsigned long address)
+static inline pgprot_t
+required_static_prot(pgprot_t prot, unsigned long start, unsigned long end)
 {
 	pgprot_t forbidden = __pgprot(0);
 
@@ -157,19 +165,21 @@ static inline pgprot_t required_static_p
 	 * The BIOS area between 640k and 1Mb needs to be executable for
 	 * PCI BIOS based config access (CONFIG_PCI_GOBIOS) support.
 	 */
-	if (within(__pa(address), BIOS_BEGIN, BIOS_END))
+	if (within_range(__pa(start), __pa(end), BIOS_BEGIN, BIOS_END))
 		pgprot_val(forbidden) |= _PAGE_NX;
 
 	/*
 	 * The kernel text needs to be executable for obvious reasons
 	 * Does not cover __inittext since that is gone later on
 	 */
-	if (within(address, (unsigned long)_text, (unsigned long)_etext))
+	if (within_range(start, end,
+		(unsigned long)_text, (unsigned long)_etext))
 		pgprot_val(forbidden) |= _PAGE_NX;
 	/*
 	 * Do the same for the x86-64 high kernel mapping
 	 */
-	if (within(address, virt_to_highmap(_text), virt_to_highmap(_etext)))
+	if (within_range(start, end,
+			virt_to_highmap(_text), virt_to_highmap(_etext)))
 		pgprot_val(forbidden) |= _PAGE_NX;
 
 
@@ -178,17 +188,18 @@ static inline pgprot_t required_static_p
 	return prot;
 }
 
-static inline pgprot_t advised_static_prot(pgprot_t prot, unsigned long address)
+static inline pgprot_t
+advised_static_prot(pgprot_t prot, unsigned long start, unsigned long end)
 {
 #ifdef CONFIG_DEBUG_RODATA
 	/* The .rodata section needs to be read-only */
-	if (within(address, (unsigned long)__start_rodata,
+	if (within_range(start, end, (unsigned long)__start_rodata,
 				(unsigned long)__end_rodata))
 		pgprot_val(prot) &= ~_PAGE_RW;
 	/*
 	 * Do the same for the x86-64 high kernel mapping
 	 */
-	if (within(address, virt_to_highmap(__start_rodata),
+	if (within_range(start, end, virt_to_highmap(__start_rodata),
 				virt_to_highmap(__end_rodata)))
 		pgprot_val(prot) &= ~_PAGE_RW;
 #endif
@@ -322,8 +333,8 @@ try_preserve_large_page(pte_t *kpte, uns
 
 	pgprot_val(new_prot) &= ~pgprot_val(cpa->mask_clr);
 	pgprot_val(new_prot) |= pgprot_val(cpa->mask_set);
-	new_prot = required_static_prot(new_prot, address);
-	new_prot = advised_static_prot(new_prot, address);
+	new_prot = required_static_prot(new_prot, address, address + psize - 1);
+	new_prot = advised_static_prot(new_prot, address, address + psize - 1);
 
 	/*
 	 * If there are no changes, return. maxpages has been updated
@@ -447,6 +458,7 @@ repeat:
 	BUG_ON(PageCompound(kpte_page));
 
 	if (level == PG_LEVEL_4K) {
+		unsigned long end = address + PAGE_SIZE - 1;
 		pte_t new_pte, old_pte = *kpte;
 		pgprot_t new_prot = pte_pgprot(old_pte);
 
@@ -461,8 +473,8 @@ repeat:
 		pgprot_val(new_prot) &= ~pgprot_val(cpa->mask_clr);
 		pgprot_val(new_prot) |= pgprot_val(cpa->mask_set);
 
-		new_prot = required_static_prot(new_prot, address);
-		new_prot = advised_static_prot(new_prot, address);
+		new_prot = required_static_prot(new_prot, address, end);
+		new_prot = advised_static_prot(new_prot, address, end);
 
 		/*
 		 * We need to keep the pfn from the existing PTE,

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

* [PATCH] [3/5] CPA: Make advised protection check truly advisory
  2008-02-08 13:27 [PATCH] [1/5] CPA: Split static_protections into required_static_prot and advised_static_prot Andi Kleen
  2008-02-08 13:27 ` [PATCH] [2/5] Support range checking for required/advisory protections Andi Kleen
@ 2008-02-08 13:27 ` Andi Kleen
  2008-02-08 13:27 ` [PATCH] [4/5] Don't use inline for the protection checks Andi Kleen
  2008-02-08 13:27 ` [PATCH] [5/5] Switch i386 early boot page table initilization over to use required_static_prot() Andi Kleen
  3 siblings, 0 replies; 12+ messages in thread
From: Andi Kleen @ 2008-02-08 13:27 UTC (permalink / raw)
  To: mingo, tglx, linux-kernel


Only force RO in the advisory protection checks when all pages in the 
range are RO. Previously it would trigger when any page in the range
was ro.

I believe this will make try_preserve_large_page much safer to use.

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/x86/mm/pageattr.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Index: linux/arch/x86/mm/pageattr.c
===================================================================
--- linux.orig/arch/x86/mm/pageattr.c
+++ linux/arch/x86/mm/pageattr.c
@@ -191,14 +191,14 @@ advised_static_prot(pgprot_t prot, unsig
 {
 #ifdef CONFIG_DEBUG_RODATA
 	/* The .rodata section needs to be read-only */
-	if (within_range(start, end, (unsigned long)__start_rodata,
-				(unsigned long)__end_rodata))
+	if (within_range((unsigned long)__start_rodata,
+				(unsigned long)__end_rodata, start, end))
 		pgprot_val(prot) &= ~_PAGE_RW;
 	/*
 	 * Do the same for the x86-64 high kernel mapping
 	 */
-	if (within_range(start, end, virt_to_highmap(__start_rodata),
-				virt_to_highmap(__end_rodata)))
+	if (within_range(virt_to_highmap(__start_rodata),
+				virt_to_highmap(__end_rodata), start, end))
 		pgprot_val(prot) &= ~_PAGE_RW;
 #endif
 	return prot;

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

* [PATCH] [4/5] Don't use inline for the protection checks
  2008-02-08 13:27 [PATCH] [1/5] CPA: Split static_protections into required_static_prot and advised_static_prot Andi Kleen
  2008-02-08 13:27 ` [PATCH] [2/5] Support range checking for required/advisory protections Andi Kleen
  2008-02-08 13:27 ` [PATCH] [3/5] CPA: Make advised protection check truly advisory Andi Kleen
@ 2008-02-08 13:27 ` Andi Kleen
  2008-02-08 13:27 ` [PATCH] [5/5] Switch i386 early boot page table initilization over to use required_static_prot() Andi Kleen
  3 siblings, 0 replies; 12+ messages in thread
From: Andi Kleen @ 2008-02-08 13:27 UTC (permalink / raw)
  To: mingo, tglx, linux-kernel


There are multiple call sites and they are not time critical

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/x86/mm/pageattr.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux/arch/x86/mm/pageattr.c
===================================================================
--- linux.orig/arch/x86/mm/pageattr.c
+++ linux/arch/x86/mm/pageattr.c
@@ -156,7 +156,7 @@ static unsigned long virt_to_highmap(voi
  * right (again, ioremap() on BIOS memory is not uncommon) so this function
  * checks and fixes these known static required protection bits.
  */
-static inline pgprot_t
+static pgprot_t
 required_static_prot(pgprot_t prot, unsigned long start, unsigned long end)
 {
 	pgprot_t forbidden = __pgprot(0);
@@ -188,7 +188,7 @@ required_static_prot(pgprot_t prot, unsi
 	return prot;
 }
 
-static inline pgprot_t
+static pgprot_t
 advised_static_prot(pgprot_t prot, unsigned long start, unsigned long end)
 {
 #ifdef CONFIG_DEBUG_RODATA

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

* [PATCH] [5/5] Switch i386 early boot page table initilization over to use required_static_prot()
  2008-02-08 13:27 [PATCH] [1/5] CPA: Split static_protections into required_static_prot and advised_static_prot Andi Kleen
                   ` (2 preceding siblings ...)
  2008-02-08 13:27 ` [PATCH] [4/5] Don't use inline for the protection checks Andi Kleen
@ 2008-02-08 13:27 ` Andi Kleen
  3 siblings, 0 replies; 12+ messages in thread
From: Andi Kleen @ 2008-02-08 13:27 UTC (permalink / raw)
  To: mingo, tglx, linux-kernel


This makes it use the same tests for this as pageattr.

Does not check advisory protections yet because that is not needed yet.

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/x86/mm/init_32.c        |   15 +++------------
 arch/x86/mm/pageattr.c       |    2 +-
 include/asm-x86/cacheflush.h |    3 +++
 3 files changed, 7 insertions(+), 13 deletions(-)

Index: linux/arch/x86/mm/init_32.c
===================================================================
--- linux.orig/arch/x86/mm/init_32.c
+++ linux/arch/x86/mm/init_32.c
@@ -140,13 +140,6 @@ page_table_range_init(unsigned long star
 	}
 }
 
-static inline int is_kernel_text(unsigned long addr)
-{
-	if (addr >= PAGE_OFFSET && addr <= (unsigned long)__init_end)
-		return 1;
-	return 0;
-}
-
 /*
  * This maps the physical memory to kernel virtual address space, a total
  * of max_low_pfn pages, by creating page tables starting from address
@@ -189,9 +182,7 @@ static void __init kernel_physical_mappi
 				addr2 = (pfn + PTRS_PER_PTE-1) * PAGE_SIZE +
 					PAGE_OFFSET + PAGE_SIZE-1;
 
-				if (is_kernel_text(addr) ||
-				    is_kernel_text(addr2))
-					prot = PAGE_KERNEL_LARGE_EXEC;
+				prot = required_static_prot(prot, addr, addr2);
 
 				set_pmd(pmd, pfn_pmd(pfn, prot));
 
@@ -205,8 +196,8 @@ static void __init kernel_physical_mappi
 			     pte++, pfn++, pte_ofs++, addr += PAGE_SIZE) {
 				pgprot_t prot = PAGE_KERNEL;
 
-				if (is_kernel_text(addr))
-					prot = PAGE_KERNEL_EXEC;
+				prot = required_static_prot(prot, addr,
+							addr + PAGE_SIZE - 1);
 
 				set_pte(pte, pfn_pte(pfn, prot));
 			}
Index: linux/include/asm-x86/cacheflush.h
===================================================================
--- linux.orig/include/asm-x86/cacheflush.h
+++ linux/include/asm-x86/cacheflush.h
@@ -45,6 +45,9 @@ int set_memory_4k(unsigned long addr, in
 
 void clflush_cache_range(void *addr, unsigned int size);
 
+pgprot_t required_static_prot(pgprot_t prot, unsigned long start,
+				unsigned long end);
+
 #ifdef CONFIG_DEBUG_RODATA
 void mark_rodata_ro(void);
 #endif
Index: linux/arch/x86/mm/pageattr.c
===================================================================
--- linux.orig/arch/x86/mm/pageattr.c
+++ linux/arch/x86/mm/pageattr.c
@@ -156,7 +156,7 @@ static unsigned long virt_to_highmap(voi
  * right (again, ioremap() on BIOS memory is not uncommon) so this function
  * checks and fixes these known static required protection bits.
  */
-static pgprot_t 
+pgprot_t
 required_static_prot(pgprot_t prot, unsigned long start, unsigned long end)
 {
 	pgprot_t forbidden = __pgprot(0);

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

* Re: [PATCH] [1/5] CPA: Split static_protections into required_static_prot and advised_static_prot
  2008-02-09 17:09           ` Thomas Gleixner
@ 2008-02-10  9:39             ` Andi Kleen
  0 siblings, 0 replies; 12+ messages in thread
From: Andi Kleen @ 2008-02-10  9:39 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Andi Kleen, Andi Kleen, mingo, linux-kernel

On Sat, Feb 09, 2008 at 06:09:23PM +0100, Thomas Gleixner wrote:
> On Sat, 9 Feb 2008, Andi Kleen wrote:
> > > > Note the behaviour for pageattr and thus DEBUG_RODATA / debugging
> > > > sitations where you don't care about your TLB this
> > > > does not change, this makes only a difference for the initial init_32
> > > > direct mapping setup.
> > > 
> > > Your patches do change the behaviour. The range checking breaks the
> > > enforcement of some restrictions for the sake of keeping the large
> > > page intact.
> > 
> > You mean in try_preserve_large_page()?
> > 
> > No actually they were not completely enforced previously at all, because
> > it did only check the restrictions of the first page.
> 
> Right, you poked my nose to it. I did not think about it when I coded
> it. It is wrong and needs to be fixed, but not by the range check you
> introduced.

Well I need the range check for a different piece of code (init_memory_mapping())
For that a range check is definitely needed and the existing code there
also does an (although quite fishy) range check. The DEBUG_RODATA case
 is also handled correctly there because DEBUG_RODATA is applied explicitely 
using pageattr later. 

You have not commented on that at all so I assume it's ok for you.

> > On the end of my patch series the enforcement is actually stricter
> > than it was before, although not 100%.
> 
> As far as I can tell it is more relaxed, as it will make overlapping
> regions of rodata and rwdata completely rw instead of splitting it up.

In try_preserve_large_page()? No because it only checks the first page.

In all other cases (in the existing code; my patchkit adds a new case 
in mm/init_32.c) it always only checks single 4K pages so the only
overlap case would be sub 4K. For that there can be no split up.

-Andi

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

* Re: [PATCH] [1/5] CPA: Split static_protections into required_static_prot and advised_static_prot
  2008-02-09 16:39         ` Andi Kleen
@ 2008-02-09 17:09           ` Thomas Gleixner
  2008-02-10  9:39             ` Andi Kleen
  0 siblings, 1 reply; 12+ messages in thread
From: Thomas Gleixner @ 2008-02-09 17:09 UTC (permalink / raw)
  To: Andi Kleen; +Cc: Andi Kleen, mingo, linux-kernel

On Sat, 9 Feb 2008, Andi Kleen wrote:
> > > Note the behaviour for pageattr and thus DEBUG_RODATA / debugging
> > > sitations where you don't care about your TLB this
> > > does not change, this makes only a difference for the initial init_32
> > > direct mapping setup.
> > 
> > Your patches do change the behaviour. The range checking breaks the
> > enforcement of some restrictions for the sake of keeping the large
> > page intact.
> 
> You mean in try_preserve_large_page()?
> 
> No actually they were not completely enforced previously at all, because
> it did only check the restrictions of the first page.

Right, you poked my nose to it. I did not think about it when I coded
it. It is wrong and needs to be fixed, but not by the range check you
introduced.
 
> On the end of my patch series the enforcement is actually stricter
> than it was before, although not 100%.

As far as I can tell it is more relaxed, as it will make overlapping
regions of rodata and rwdata completely rw instead of splitting it up.

Thanks,
	tglx

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

* Re: [PATCH] [1/5] CPA: Split static_protections into required_static_prot and advised_static_prot
  2008-02-09 15:50       ` Thomas Gleixner
@ 2008-02-09 16:39         ` Andi Kleen
  2008-02-09 17:09           ` Thomas Gleixner
  0 siblings, 1 reply; 12+ messages in thread
From: Andi Kleen @ 2008-02-09 16:39 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Andi Kleen, Andi Kleen, mingo, linux-kernel

On Sat, Feb 09, 2008 at 04:50:14PM +0100, Thomas Gleixner wrote:
> On Sat, 9 Feb 2008, Andi Kleen wrote:
> > On Sat, Feb 09, 2008 at 03:56:02PM +0100, Thomas Gleixner wrote:
> > > On Fri, 8 Feb 2008, Andi Kleen wrote:
> > > > There is a big difference between NX and RO. NX absolutely has to be cleared
> > > > or the kernel will fail while RO just can be set, but does not need to.
> > > > And for a large page area not setting NX if there is a area below 
> > > > it that needs it is essential, while making it ro is optional again.
> > 
> > Optional as in it doesn't need to be forced.
> > 
> > > 
> > > No, it's not optional. Making the PMD RO will write protect all 4k
> > > PTEs below independent of their setting. So there is the same
> > > restriction as we have with NX.
> > 
> > If there is a boundary between a RO area
> > and a RW area and you want to map it with 2MB pages then NX is required,
> > but RO is optional on the page and if you prefer TLB use minimalization
> > over debugging it is optional. Is it clear now? 
> 
> No, it is not clear at all. Why is there a requirement for NX, when I

Sorry the requirement is no NX, not NX. I left out the "no"
in the second sentence.

> have an overlapping area of RO and RW in a large page mapping? If I
> want to preserve the large page mapping in such a case, then its
> required to make the mapping RW and omit the protection of the RO
> area, nothing else.

Yes that is what my patch (or rather a followup) patch implements.

> > Note the behaviour for pageattr and thus DEBUG_RODATA / debugging
> > sitations where you don't care about your TLB this
> > does not change, this makes only a difference for the initial init_32
> > direct mapping setup.
> 
> Your patches do change the behaviour. The range checking breaks the
> enforcement of some restrictions for the sake of keeping the large
> page intact.

You mean in try_preserve_large_page()?

No actually they were not completely enforced previously at all, because
it did only check the restrictions of the first page.

On the end of my patch series the enforcement is actually stricter
than it was before, although not 100%.

-Andi


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

* Re: [PATCH] [1/5] CPA: Split static_protections into required_static_prot and advised_static_prot
  2008-02-09 15:13     ` Andi Kleen
@ 2008-02-09 15:50       ` Thomas Gleixner
  2008-02-09 16:39         ` Andi Kleen
  0 siblings, 1 reply; 12+ messages in thread
From: Thomas Gleixner @ 2008-02-09 15:50 UTC (permalink / raw)
  To: Andi Kleen; +Cc: Andi Kleen, mingo, linux-kernel

On Sat, 9 Feb 2008, Andi Kleen wrote:
> On Sat, Feb 09, 2008 at 03:56:02PM +0100, Thomas Gleixner wrote:
> > On Fri, 8 Feb 2008, Andi Kleen wrote:
> > > There is a big difference between NX and RO. NX absolutely has to be cleared
> > > or the kernel will fail while RO just can be set, but does not need to.
> > > And for a large page area not setting NX if there is a area below 
> > > it that needs it is essential, while making it ro is optional again.
> 
> Optional as in it doesn't need to be forced.
> 
> > 
> > No, it's not optional. Making the PMD RO will write protect all 4k
> > PTEs below independent of their setting. So there is the same
> > restriction as we have with NX.
> 
> If there is a boundary between a RO area
> and a RW area and you want to map it with 2MB pages then NX is required,
> but RO is optional on the page and if you prefer TLB use minimalization
> over debugging it is optional. Is it clear now? 

No, it is not clear at all. Why is there a requirement for NX, when I
have an overlapping area of RO and RW in a large page mapping? If I
want to preserve the large page mapping in such a case, then its
required to make the mapping RW and omit the protection of the RO
area, nothing else.

> Note the behaviour for pageattr and thus DEBUG_RODATA / debugging
> sitations where you don't care about your TLB this
> does not change, this makes only a difference for the initial init_32
> direct mapping setup.

Your patches do change the behaviour. The range checking breaks the
enforcement of some restrictions for the sake of keeping the large
page intact.

Thanks,
	tglx

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

* Re: [PATCH] [1/5] CPA: Split static_protections into required_static_prot and advised_static_prot
  2008-02-09 14:56   ` Thomas Gleixner
@ 2008-02-09 15:13     ` Andi Kleen
  2008-02-09 15:50       ` Thomas Gleixner
  0 siblings, 1 reply; 12+ messages in thread
From: Andi Kleen @ 2008-02-09 15:13 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Andi Kleen, mingo, linux-kernel

On Sat, Feb 09, 2008 at 03:56:02PM +0100, Thomas Gleixner wrote:
> On Fri, 8 Feb 2008, Andi Kleen wrote:
> > There is a big difference between NX and RO. NX absolutely has to be cleared
> > or the kernel will fail while RO just can be set, but does not need to.
> > And for a large page area not setting NX if there is a area below 
> > it that needs it is essential, while making it ro is optional again.

Optional as in it doesn't need to be forced.

> 
> No, it's not optional. Making the PMD RO will write protect all 4k
> PTEs below independent of their setting. So there is the same
> restriction as we have with NX.

If there is a boundary between a RO area
and a RW area and you want to map it with 2MB pages then NX is required,
but RO is optional on the page and if you prefer TLB use minimalization
over debugging it is optional. Is it clear now? 

Note the behaviour for pageattr and thus DEBUG_RODATA / debugging
sitations where you don't care about your TLB this
does not change, this makes only a difference for the initial init_32
direct mapping setup.

-Andi

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

* Re: [PATCH] [1/5] CPA: Split static_protections into required_static_prot and advised_static_prot
  2008-02-08 16:36 ` [PATCH] [1/5] CPA: Split static_protections into required_static_prot and advised_static_prot Andi Kleen
@ 2008-02-09 14:56   ` Thomas Gleixner
  2008-02-09 15:13     ` Andi Kleen
  0 siblings, 1 reply; 12+ messages in thread
From: Thomas Gleixner @ 2008-02-09 14:56 UTC (permalink / raw)
  To: Andi Kleen; +Cc: mingo, linux-kernel

On Fri, 8 Feb 2008, Andi Kleen wrote:
> There is a big difference between NX and RO. NX absolutely has to be cleared
> or the kernel will fail while RO just can be set, but does not need to.
> And for a large page area not setting NX if there is a area below 
> it that needs it is essential, while making it ro is optional again.

No, it's not optional. Making the PMD RO will write protect all 4k
PTEs below independent of their setting. So there is the same
restriction as we have with NX.

> This is needed for a followup patch who uses requred_static_prot() for large 
> pages where it is inconvenient to check all pages. 

> No behaviour change in this patch.
> 
> [Lines > 80 characters are changed in followup patch]

ROTFL.

	tglx

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

* [PATCH] [1/5] CPA: Split static_protections into required_static_prot and advised_static_prot
  2008-02-08 16:36 [PATCH] [0/5] pageattr protection patchkit v2 for the latest kernel Andi Kleen
@ 2008-02-08 16:36 ` Andi Kleen
  2008-02-09 14:56   ` Thomas Gleixner
  0 siblings, 1 reply; 12+ messages in thread
From: Andi Kleen @ 2008-02-08 16:36 UTC (permalink / raw)
  To: mingo, tglx, linux-kernel


There is a big difference between NX and RO. NX absolutely has to be cleared
or the kernel will fail while RO just can be set, but does not need to.
And for a large page area not setting NX if there is a area below 
it that needs it is essential, while making it ro is optional again.

This is needed for a followup patch who uses requred_static_prot() for large 
pages where it is inconvenient to check all pages. 

No behaviour change in this patch.

[Lines > 80 characters are changed in followup patch]

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/x86/mm/pageattr.c |   19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

Index: linux/arch/x86/mm/pageattr.c
===================================================================
--- linux.orig/arch/x86/mm/pageattr.c
+++ linux/arch/x86/mm/pageattr.c
@@ -149,7 +149,7 @@ static unsigned long virt_to_highmap(voi
  * right (again, ioremap() on BIOS memory is not uncommon) so this function
  * checks and fixes these known static required protection bits.
  */
-static inline pgprot_t static_protections(pgprot_t prot, unsigned long address)
+static inline pgprot_t required_static_prot(pgprot_t prot, unsigned long address)
 {
 	pgprot_t forbidden = __pgprot(0);
 
@@ -172,6 +172,15 @@ static inline pgprot_t static_protection
 	if (within(address, virt_to_highmap(_text), virt_to_highmap(_etext)))
 		pgprot_val(forbidden) |= _PAGE_NX;
 
+	prot = __pgprot(pgprot_val(prot) & ~pgprot_val(forbidden));
+
+	return prot;
+}
+
+static inline pgprot_t advised_static_prot(pgprot_t prot, unsigned long address)
+{
+	pgprot_t forbidden = __pgprot(0);
+
 	/* The .rodata section needs to be read-only */
 	if (within(address, (unsigned long)__start_rodata,
 				(unsigned long)__end_rodata))
@@ -304,7 +313,8 @@ try_preserve_large_page(pte_t *kpte, uns
 
 	pgprot_val(new_prot) &= ~pgprot_val(cpa->mask_clr);
 	pgprot_val(new_prot) |= pgprot_val(cpa->mask_set);
-	new_prot = static_protections(new_prot, address);
+	new_prot = required_static_prot(new_prot, address);
+	new_prot = advised_static_prot(new_prot, address);
 
 	/*
 	 * If there are no changes, return. maxpages has been updated
@@ -442,7 +452,8 @@ repeat:
 		pgprot_val(new_prot) &= ~pgprot_val(cpa->mask_clr);
 		pgprot_val(new_prot) |= pgprot_val(cpa->mask_set);
 
-		new_prot = static_protections(new_prot, address);
+		new_prot = required_static_prot(new_prot, address);
+		new_prot = advised_static_prot(new_prot, address);
 
 		/*
 		 * We need to keep the pfn from the existing PTE,
@@ -532,7 +543,7 @@ static int change_page_attr_addr(struct 
 		 * for the non obvious details.
 		 *
 		 * Note that NX and other required permissions are
-		 * checked in static_protections().
+		 * checked in required_static_prot().
 		 */
 		address = phys_addr + HIGH_MAP_START - phys_base;
 

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

end of thread, other threads:[~2008-02-10  9:04 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-02-08 13:27 [PATCH] [1/5] CPA: Split static_protections into required_static_prot and advised_static_prot Andi Kleen
2008-02-08 13:27 ` [PATCH] [2/5] Support range checking for required/advisory protections Andi Kleen
2008-02-08 13:27 ` [PATCH] [3/5] CPA: Make advised protection check truly advisory Andi Kleen
2008-02-08 13:27 ` [PATCH] [4/5] Don't use inline for the protection checks Andi Kleen
2008-02-08 13:27 ` [PATCH] [5/5] Switch i386 early boot page table initilization over to use required_static_prot() Andi Kleen
2008-02-08 16:36 [PATCH] [0/5] pageattr protection patchkit v2 for the latest kernel Andi Kleen
2008-02-08 16:36 ` [PATCH] [1/5] CPA: Split static_protections into required_static_prot and advised_static_prot Andi Kleen
2008-02-09 14:56   ` Thomas Gleixner
2008-02-09 15:13     ` Andi Kleen
2008-02-09 15:50       ` Thomas Gleixner
2008-02-09 16:39         ` Andi Kleen
2008-02-09 17:09           ` Thomas Gleixner
2008-02-10  9:39             ` Andi Kleen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).