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

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

[Jan Beulich]

Since successful return from __cpa_process_fault() makes
__change_page_attr() exit early (and successfully), its caller needs to
be instructed to continue its iteration by adjusting ->numpages. While
this already happens on one of __cpa_process_fault()'s successful exit
paths, the other needs this done similarly. This was in particular a
problem when the top level caller passed zero for "checkalias"
(becoming the "primary" value for the other two mentioned functions),
as is the case in change_page_attr_set_clr() when the OR of "mask_set"
and "mask_clr" equals _PAGE_NX, as e.g. passed from set_memory_{,n}x().

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

--- 4.5-rc1/arch/x86/mm/pageattr.c
+++ 4.5-rc1-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

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

[Thomas Gleixner]

On Mon, 25 Jan 2016, Jan Beulich wrote:

Sorry, that changelog does not make any sense.

> Since successful return from __cpa_process_fault() makes
> __change_page_attr() exit early (and successfully), its caller needs to

That has nothing to do with a successful return from __cpa_process_fault().

__change_page_attr() always returns immediately after calling
__cpa_process_fault() no matter what the return code is.

> be instructed to continue its iteration by adjusting ->numpages.

And how is that instruction working?

> While this already happens on one of __cpa_process_fault()'s successful exit
> paths, the other needs this done similarly.

Why?

> This was in particular a problem when the top level caller passed zero for
> "checkalias" (becoming the "primary" value for the other two mentioned
> functions), as is the case in change_page_attr_set_clr() when the OR of
> "mask_set" and "mask_clr" equals _PAGE_NX, as e.g. passed from
> set_memory_{,n}x().

This is completely unparseable.

Can you please describe the failure and the solution in a way, which lets one
figure out what that means w/o studying the code in detail?

Thanks,

	tglx

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

[Jan Beulich]

>>> On 27.01.16 at 11:05, <tglx@linutronix.de> wrote:
> On Mon, 25 Jan 2016, Jan Beulich wrote:
> 
> Sorry, that changelog does not make any sense.
> 
>> Since successful return from __cpa_process_fault() makes
>> __change_page_attr() exit early (and successfully), its caller needs to
> 
> That has nothing to do with a successful return from __cpa_process_fault().

How does it not? When __change_page_attr() calls
__cpa_process_fault() it doesn't even loot at its return value, but
directly returns __cpa_process_fault()'s return value to its own
caller. I.e. success of __cpa_process_fault() means success of
__change_page_attr().

> __change_page_attr() always returns immediately after calling
> __cpa_process_fault() no matter what the return code is.
> 
>> be instructed to continue its iteration by adjusting ->numpages.
> 
> And how is that instruction working?

I think some understanding of the internal working of cpa can
be expected. Specifically here the fact that ->numpages upon
successful return indicates the number of processes pages. I.e.
if any party reporting success doesn't update the value, its
caller(s) will assume success for the entire requested range.
Hence instructing the caller to continue iterating is done as
described. I really how no idea how else I should express this.

>> While this already happens on one of __cpa_process_fault()'s successful exit
>> paths, the other needs this done similarly.
> 
> Why?

See above - to avoid misleading the caller.

>> This was in particular a problem when the top level caller passed zero for
>> "checkalias" (becoming the "primary" value for the other two mentioned
>> functions), as is the case in change_page_attr_set_clr() when the OR of
>> "mask_set" and "mask_clr" equals _PAGE_NX, as e.g. passed from
>> set_memory_{,n}x().
> 
> This is completely unparseable.
> 
> Can you please describe the failure and the solution in a way, which lets 
> one figure out what that means w/o studying the code in detail?

If the description doesn't suit you and I can't see any better way
to describe this - what do we do? Leave the bug unfixed? Treat the
patch as a bug report, for someone to fix in the indefinite future?
Had I been terse, that would be a problem. Now I've tried to be
verbose, yet that's a problem too.

The only adjustment I can see as being doable for me would be to
invert the ordering of the description, starting with describing the
failure scenario. That would nevertheless be with more or less the
same wording, so I'm quite uncertain it would help (and hence be
worth my time).

Jan

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

[Thomas Gleixner]

On Wed, 27 Jan 2016, Jan Beulich wrote:
> >>> On 27.01.16 at 11:05, <tglx@linutronix.de> wrote:
> > On Mon, 25 Jan 2016, Jan Beulich wrote:
> > 
> > Sorry, that changelog does not make any sense.
> > 
> >> Since successful return from __cpa_process_fault() makes
> >> __change_page_attr() exit early (and successfully), its caller needs to
> > 
> > That has nothing to do with a successful return from __cpa_process_fault().
> 
> How does it not? When __change_page_attr() calls
> __cpa_process_fault() it doesn't even loot at its return value, but
> directly returns __cpa_process_fault()'s return value to its own
> caller. I.e. success of __cpa_process_fault() means success of
> __change_page_attr().

You wrote:

> >> Since successful return from __cpa_process_fault() makes
> >> __change_page_attr() exit early

This is wrong. Because it implies that a non successful return from
__cpa_process_fault() is not resulting in an early exit of
__change_page_attr().

> > __change_page_attr() always returns immediately after calling
> > __cpa_process_fault() no matter what the return code is.
> > 
> >> be instructed to continue its iteration by adjusting ->numpages.
> > 
> > And how is that instruction working?
> 
> I think some understanding of the internal working of cpa can
> be expected.

I very much know how it works as I wrote large parts of it. Still your
changelog makes no sense to me.

> The only adjustment I can see as being doable for me would be to
> invert the ordering of the description, starting with describing the
> failure scenario. That would nevertheless be with more or less the
> same wording, so I'm quite uncertain it would help (and hence be
> worth my time).

Documentation/SubmittingPatches:: 2) Describe your changes.

does apply to everyone including you. It's well worth the time because badly
written changelogs waste the time of reviewers, maintainers and people who
need to consult changelogs later on.

Thanks,

	tglx

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

[Jan Beulich]

>>> On 27.01.16 at 11:53, <tglx@linutronix.de> wrote:
> Documentation/SubmittingPatches:: 2) Describe your changes.

So what is it that you think I've done?

Anyway, I'll make a second try, and if that's still not understandable
likely give up (and retain the bug fix in my own trees) unless given
more clear guidance on what is acceptable.

Jan

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

[Jan Beulich]

When __change_page_attr() finds it necessary to call
__cpa_process_fault(), it passes its return value directly up to its
own caller, even if this indicates success. Success to the callers,
however, means that whatever ->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() adjusting ->numpages).

When a top level caller, like in the case of change_page_attr_set_clr()
only meaning to alter _PAGE_NX, wants to suppress alias processing, the
boolean value to indicate so results in __cpa_process_fault() taking
its other successful exit path. Since ->numpages so far didn't get
adjusted there, hitting either of the conditions that cause
__cpa_process_fault() to get called meant early termination of the
processing without having processed the entire range, yet still
reporting success.

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

--- 4.5-rc1/arch/x86/mm/pageattr.c
+++ 4.5-rc1-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

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

[Ingo Molnar]

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

> When __change_page_attr() finds it necessary to call
> __cpa_process_fault(), it passes its return value directly up to its
> own caller, even if this indicates success. Success to the callers,
> however, means that whatever ->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() adjusting ->numpages).
> 
> When a top level caller, like in the case of change_page_attr_set_clr()
> only meaning to alter _PAGE_NX, wants to suppress alias processing, the
> boolean value to indicate so results in __cpa_process_fault() taking
> its other successful exit path. Since ->numpages so far didn't get
> adjusted there, hitting either of the conditions that cause
> __cpa_process_fault() to get called meant early termination of the
> processing without having processed the entire range, yet still
> reporting success.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Completely re-written description.

So maybe it's just me, but I'm still quite unhappy about this changelog, it's hard 
to parse and doesn't really do what a good changelog should do :-/

First I'd like to quote from a mail of Andrew Morton:

 "Please update the changelog to describe the current behavior.

  Please also describe why you think that behavior should be changed.
  ie: what's the reason for this patch.

  Please update Documentation/ for this feature.  Probably that's
  kernel-parameters.txt for the boot option and sysctl/kernel.txt for the
  procfs addition."

Alternatively:

 1- first describe the symptoms of the bug - how does a user notice? 

 2- then describe how the code behaves today and how that is causing the bug

 3- and then only describe how it's fixed.

Or:

  " Current code does (A), this has a problem when (B).
    We can improve this doing (C), because (D)."

This changelog concentrates excessively on implementational details, without 
providing context and without touching upon the practical effects - nor does it do 
a clear before/after description.

I.e. what you describe in the changelog is 90% of what a developer intimate with 
this code finds interesting about the patch - but that's not what good changelogs 
are about!

Could we try a v3?

Thanks,

        Ingo

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

[Jan Beulich]

>>> 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()). 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.

Jan

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

[Ingo Molnar]

* 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

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

[Jan Beulich]

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

[tip:x86/mm] x86/mm: Avoid premature success when changing page attributes

[tip-bot for Jan Beulich]

Commit-ID:  405e1133d00e0271cedef75c17ecb773ff3e2732
Gitweb:     http://git.kernel.org/tip/405e1133d00e0271cedef75c17ecb773ff3e2732
Author:     Jan Beulich <JBeulich@suse.com>
AuthorDate: Wed, 10 Feb 2016 02:03:00 -0700
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 25 Feb 2016 10:41:43 +0100

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>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/56BB0AD402000078000D05BF@prv-mh.provo.novell.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/mm/pageattr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 2440814..3dd6afd 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -1122,8 +1122,10 @@ static int __cpa_process_fault(struct cpa_data *cpa, unsigned long vaddr,
 	/*
 	 * 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