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

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

[Roger Pau Monne]

If the new gfn matches the previous one (ie: gfn == old_gpfn)
xenmem_add_to_physmap_one will issue a duplicated call to
guest_physmap_remove_page with the same gfn, 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.

Fix this by adding a check that prevents a second call to
guest_physmap_remove_page if the previous one has already removed the
backing page from that gfn.

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>
---
 xen/arch/x86/mm/p2m.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 674a6f4fe9..2bd4d37286 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2813,7 +2813,7 @@ int xenmem_add_to_physmap_one(
     }
 
     /* Unmap from old location, if any. */
-    if ( !rc && old_gpfn != INVALID_M2P_ENTRY )
+    if ( !rc && old_gpfn != INVALID_M2P_ENTRY && !gfn_eq(_gfn(old_gpfn), gpfn) )
         rc = guest_physmap_remove_page(d, _gfn(old_gpfn), mfn, PAGE_ORDER_4K);
 
     /* Map at new location. */
-- 
2.33.0

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

[Jan Beulich]

On 14.09.2021 15:34, Roger Pau Monne wrote:
> If the new gfn matches the previous one (ie: gfn == old_gpfn)

It took me a while to realize that you probably mean "gpfn" here.

> xenmem_add_to_physmap_one will issue a duplicated call to
> guest_physmap_remove_page with the same gfn, because the

Considering the local variable of this name, may I suggest to
upper-case GFN in this case?

> 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.
> 
> Fix this by adding a check that prevents a second call to
> guest_physmap_remove_page if the previous one has already removed the
> backing page from that gfn.

Same here (if this remains; see below).

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2813,7 +2813,7 @@ int xenmem_add_to_physmap_one(
>      }
>  
>      /* Unmap from old location, if any. */
> -    if ( !rc && old_gpfn != INVALID_M2P_ENTRY )
> +    if ( !rc && old_gpfn != INVALID_M2P_ENTRY && !gfn_eq(_gfn(old_gpfn), gpfn) )
>          rc = guest_physmap_remove_page(d, _gfn(old_gpfn), mfn, PAGE_ORDER_4K);
>  
>      /* Map at new location. */
> 

It feels unbalanced to suppress the remove, but keep the add in
this case. Wouldn't we be better off skipping both, or short-
circuiting the effective no-op even earlier? I recall considering
to install a shortcut, but it didn't seem worth it. But now that
you've found actual breakage, perhaps this need reconsidering.

Jan

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

[Roger Pau Monné]

On Tue, Sep 14, 2021 at 04:32:53PM +0200, Jan Beulich wrote:
> On 14.09.2021 15:34, Roger Pau Monne wrote:
> > If the new gfn matches the previous one (ie: gfn == old_gpfn)
> 
> It took me a while to realize that you probably mean "gpfn" here.

Indeed. The variable names could probably do with some disambiguation.

> > xenmem_add_to_physmap_one will issue a duplicated call to
> > guest_physmap_remove_page with the same gfn, because the
> 
> Considering the local variable of this name, may I suggest to
> upper-case GFN in this case?
> 
> > 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.
> > 
> > Fix this by adding a check that prevents a second call to
> > guest_physmap_remove_page if the previous one has already removed the
> > backing page from that gfn.
> 
> Same here (if this remains; see below).
> 
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -2813,7 +2813,7 @@ int xenmem_add_to_physmap_one(
> >      }
> >  
> >      /* Unmap from old location, if any. */
> > -    if ( !rc && old_gpfn != INVALID_M2P_ENTRY )
> > +    if ( !rc && old_gpfn != INVALID_M2P_ENTRY && !gfn_eq(_gfn(old_gpfn), gpfn) )
> >          rc = guest_physmap_remove_page(d, _gfn(old_gpfn), mfn, PAGE_ORDER_4K);
> >  
> >      /* Map at new location. */
> > 
> 
> It feels unbalanced to suppress the remove, but keep the add in
> this case.

Well, there's a remove further up which is not skipped.

> Wouldn't we be better off skipping both, or short-
> circuiting the effective no-op even earlier? I recall considering
> to install a shortcut, but it didn't seem worth it. But now that
> you've found actual breakage, perhaps this need reconsidering.

Sure, just didn't think this corner case was worth adding a short
circuit. Let me send v2 with that approach if that's what you
prefer.

Thanks, Roger.