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

[Jan Beulich]

>>> On 27.01.16 at 11:05, <tglx@xxxxxxxxxxxxx> 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