Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1330293 > unrolled thread

Re: [PATCH v2] x86/mm: avoid premature success when changing page attributes

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] x86/mm: avoid premature success when changing page  attributes Ingo Molnar <mingo@kernel.org> - 2016-02-09 15:40 +0100
    [PATCH v3] x86/mm: avoid premature success when changing page  attributes "Jan Beulich" <JBeulich@suse.com> - 2016-02-10 10:10 +0100

#1330293 — Re: [PATCH v2] x86/mm: avoid premature success when changing page attributes

* Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 28.01.16 at 09:42, <mingo@kernel.org> wrote:
> > Could we try a v3?
> 
> Okay, I withdraw the patch: Upon further consideration it is note really clear 
> what the intended behavior of set_memory_*() on address ranges with mapping 
> holes is supposed to be. The original issue was with set_memory_nx() (called 
> from mark_rodata_ro()) stumbling across an unmapped region (resulting from an 
> out of tree change completely unmapping the kernel mappings of address ranges 
> passed to free_init_pages()). [...]

So it still looks like a legitimate fix to me, even though your testcase was in an 
out of tree context:

> [...] I simply don't have the time to check whether the unmapping done with 
> CONFIG_DEBUG_PAGEALLOC would have a similar effect. The net result in any event 
> were pages (past the hole) reported as problematic when CONFIG_DEBUG_WX is 
> enabled.

Adding all the above information to the changelog addresses most of my complaints 
about it. You can also rephrase the DEBUG_PAGEALLOC bit to something like:

   I'm not completely sure about whether the unmapping done with
   CONFIG_DEBUG_PAGEALLOC would have a similar effect.

as it's perfectly fine to submit fixes you couldn't fully test.

Thanks,

	Ingo

[toc] | [next] | [standalone]

#1330957 — [PATCH v3] x86/mm: avoid premature success when changing page attributes

set_memory_nx() (and set_memory_x()) currently differ in behavior from
all other set_memory_*() functions when encountering a virtual address
space hole within the kernel address range: They stop processing at the
hole, but nevertheless report success (making the caller believe the
operation was carried out on the entire range). While observed to be a
problem - triggering the CONFIG_DEBUG_WX warning - only with out of
tree code, I suspect (but didn't check) that on x86-64 the
CONFIG_DEBUG_PAGEALLOC logic in free_init_pages() would, when called
from free_initmem(), have the same effect on the set_memory_nx() called
from mark_rodata_ro().

This unexpected behavior is a result of change_page_attr_set_clr()
special casing changes to only the NX bit, in that it passes "false" as
the "checkalias" argument to __change_page_attr_set_clr(). Since this
flag becomes the "primary" argument of both __change_page_attr() and
__cpa_process_fault(), the latter would so far return success without
adjusting cpa->numpages. Success to the higher level callers, however,
means that whatever cpa->numpages currently holds is the count of
successfully processed pages. The cases when __change_page_attr() calls
__cpa_process_fault(), otoh, don't generally mean the entire range got
processed (as can be seen from one of the two success return paths in
__cpa_process_fault() already adjusting ->numpages).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: Mostly re-written description.
v2: Completely re-written description.
---
 arch/x86/mm/pageattr.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--- 4.5-rc3/arch/x86/mm/pageattr.c
+++ 4.5-rc3-x86-cpa-non-primary/arch/x86/mm/pageattr.c
@@ -1122,8 +1122,10 @@ static int __cpa_process_fault(struct cp
 	/*
 	 * Ignore all non primary paths.
 	 */
-	if (!primary)
+	if (!primary) {
+		cpa->numpages = 1;
 		return 0;
+	}
 
 	/*
 	 * Ignore the NULL PTE for kernel identity mapping, as it is expected

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web