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

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

[Roger Pau Monne]

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.

Fixes: f8582da041 ('x86/mm: pull a sanity check earlier in xenmem_add_to_physmap_one()')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Short circuit and skip modifying the p2m.
---
 xen/arch/x86/mm/p2m.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 674a6f4fe9..bcdc5c7014 100644
--- 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;
+    }
+
     /* Remove previously mapped page if it was present. */
     if ( p2mt == p2m_mmio_direct )
         rc = -EPERM;
-- 
2.33.0

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

[Jan Beulich]

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

I've also meanwhile realized that it was a different form of short
circuiting that I had been thinking about before: XENMAPSPACE_gmfn's
idx == gpfn.

Jan

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

[Roger Pau Monné]

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.

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.

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

Thanks, Roger.

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

[Jan Beulich]

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