Re: [PATCH v2] x86/p2m: fix xenmem_add_to_physmap_one double page removal

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.09.2021 10:44, Roger Pau Monné wrote:
> On Wed, Sep 15, 2021 at 10:16:41AM +0200, Jan Beulich wrote:
>> On 15.09.2021 10:03, Roger Pau Monne wrote:
>>> If the new gfn matches the previous one (ie: gpfn == old_gpfn)
>>> xenmem_add_to_physmap_one will issue a duplicated call to
>>> guest_physmap_remove_page with the same guest frame number, because
>>> the get_gpfn_from_mfn call has been moved by commit f8582da041 to be
>>> performed before the original page is removed. This leads to the
>>> second guest_physmap_remove_page failing, which was not the case
>>> before commit f8582da041.
>>>
>>> Add a shortcut to skip modifying the p2m if the mapping is already as
>>> requested.
>>
>> I've meanwhile had further thoughts here - not sure in how far they
>> affect what to do (including whether to go back to v1, with the
>> description's 1st paragraph adjusted as per above):
>>
>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -2799,6 +2799,13 @@ int xenmem_add_to_physmap_one(
>>>          goto put_all;
>>>      }
>>>  
>>> +    if ( gfn_eq(_gfn(old_gpfn), gpfn) )
>>> +    {
>>> +        /* Nothing to do, mapping is already as requested. */
>>> +        ASSERT(mfn_eq(prev_mfn, mfn));
>>> +        goto put_all;
>>> +    }
>>
>> The mapping may not be "already as requested" because of p2m type or
>> p2m access. Thoughts? (At the very least the new check would likely
>> want to move after the p2m_mmio_direct one.)
> 
> Is the type really relevant for the caller? If the mapping is already
> setup as requested, I don't think it makes sense to return -EPERM if
> the type is mmio. I'm also unsure whether we can get into that state
> in the first place.

mmio perhaps indeed can't occur (because of the earlier
get_page_from_mfn()), but in general the type might very well be
relevant: The seemingly benign change might e.g. change logdirty
to ram_rw. Whether that's for good or bad is a different matter.

> I'm unsure regarding the access, I would say changing the access type
> should be done by other means rather that replying on
> xenmem_add_to_physmap.

It _should_, yes, but there might be callers relying on it
changing as a side effect here. (There might also be callers
abusing the call for this purpose.)

> v1 was indeed more similar to the previous behavior IMO, so it's
> likely a safer approach.

Okay, I'll commit v1 with the slightly adjusted description then.

Jan