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

Groups > linux.kernel > #1317030 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] x86/mm: avoid premature success when changing page  attributes "Jan Beulich" <JBeulich@suse.com> - 2016-01-25 18:00 +0100
    Re: [PATCH] x86/mm: avoid premature success when changing page  attributes Thomas Gleixner <tglx@linutronix.de> - 2016-01-27 11:10 +0100
      Re: [PATCH] x86/mm: avoid premature success when changing page  attributes "Jan Beulich" <JBeulich@suse.com> - 2016-01-27 11:50 +0100
        Re: [PATCH] x86/mm: avoid premature success when changing page  attributes Thomas Gleixner <tglx@linutronix.de> - 2016-01-27 12:00 +0100
          Re: [PATCH] x86/mm: avoid premature success when changing page  attributes "Jan Beulich" <JBeulich@suse.com> - 2016-01-27 12:20 +0100
          [PATCH v2] x86/mm: avoid premature success when changing page  attributes "Jan Beulich" <JBeulich@suse.com> - 2016-01-27 12:30 +0100

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

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

[toc] | [next] | [standalone]

#1318815

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

[toc] | [prev] | [next] | [standalone]

#1318845

>>> 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

[toc] | [prev] | [next] | [standalone]

#1318852

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

[toc] | [prev] | [next] | [standalone]

#1318861

>>> 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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web