[PATCH 0/4] x86/mm: address observations made while working on XSA-388

[PATCH 0/4] x86/mm: address observations made while working on XSA-388

[Jan Beulich]

1: PoD: simplify / improve p2m_pod_cache_add()
2: PoD: HVM guests can't pin their pages
3: altp2m: p2m_altp2m_get_or_propagate() should honor present page order
4: altp2m: p2m_altp2m_propagate_change() should honor present page order

The last one is RFC, as there is an aspect I can't make sense of.
See there.

Jan

[PATCH 1/4] x86/PoD: simplify / improve p2m_pod_cache_add()

[Jan Beulich]

Avoid recurring MFN -> page or page -> MFN translations. Drop the pretty
pointless local variable "p". Make sure the MFN logged in a debugging
error message is actually the offending one. Adjust style.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -58,14 +58,10 @@ p2m_pod_cache_add(struct p2m_domain *p2m
                   unsigned int order)
 {
     unsigned long i;
-    struct page_info *p;
     struct domain *d = p2m->domain;
+    mfn_t mfn = page_to_mfn(page);
 
 #ifndef NDEBUG
-    mfn_t mfn;
-
-    mfn = page_to_mfn(page);
-
     /* Check to make sure this is a contiguous region */
     if ( mfn_x(mfn) & ((1UL << order) - 1) )
     {
@@ -74,17 +70,14 @@ p2m_pod_cache_add(struct p2m_domain *p2m
         return -1;
     }
 
-    for ( i = 0; i < 1UL << order ; i++)
+    for ( i = 0; i < (1UL << order); i++)
     {
-        struct domain * od;
+        const struct domain *od = page_get_owner(page + i);
 
-        p = mfn_to_page(mfn_add(mfn, i));
-        od = page_get_owner(p);
         if ( od != d )
         {
-            printk("%s: mfn %lx expected owner d%d, got owner d%d!\n",
-                   __func__, mfn_x(mfn), d->domain_id,
-                   od ? od->domain_id : -1);
+            printk("%s: mfn %lx owner: expected %pd, got %pd!\n",
+                   __func__, mfn_x(mfn) + i, d, od);
             return -1;
         }
     }
@@ -98,16 +91,12 @@ p2m_pod_cache_add(struct p2m_domain *p2m
      * promise to provide zero pages. So we scrub pages before using.
      */
     for ( i = 0; i < (1UL << order); i++ )
-        clear_domain_page(mfn_add(page_to_mfn(page), i));
+        clear_domain_page(mfn_add(mfn, i));
 
     /* First, take all pages off the domain list */
     lock_page_alloc(p2m);
-    for ( i = 0; i < 1UL << order ; i++ )
-    {
-        p = page + i;
-        page_list_del(p, &d->page_list);
-    }
-
+    for ( i = 0; i < (1UL << order); i++ )
+        page_list_del(page + i, &d->page_list);
     unlock_page_alloc(p2m);
 
     /* Then add to the appropriate populate-on-demand list. */

Re: [PATCH 1/4] x86/PoD: simplify / improve p2m_pod_cache_add()

[Andrew Cooper]

On 01/12/2021 10:53, Jan Beulich wrote:
> Avoid recurring MFN -> page or page -> MFN translations. Drop the pretty
> pointless local variable "p". Make sure the MFN logged in a debugging
> error message is actually the offending one. Adjust style.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/mm/p2m-pod.c
> +++ b/xen/arch/x86/mm/p2m-pod.c
> @@ -58,14 +58,10 @@ p2m_pod_cache_add(struct p2m_domain *p2m
>                    unsigned int order)
>  {
>      unsigned long i;
> -    struct page_info *p;
>      struct domain *d = p2m->domain;
> +    mfn_t mfn = page_to_mfn(page);
>  
>  #ifndef NDEBUG
> -    mfn_t mfn;
> -
> -    mfn = page_to_mfn(page);
> -
>      /* Check to make sure this is a contiguous region */
>      if ( mfn_x(mfn) & ((1UL << order) - 1) )
>      {
> @@ -74,17 +70,14 @@ p2m_pod_cache_add(struct p2m_domain *p2m
>          return -1;
>      }
>  
> -    for ( i = 0; i < 1UL << order ; i++)
> +    for ( i = 0; i < (1UL << order); i++)
>      {
> -        struct domain * od;
> +        const struct domain *od = page_get_owner(page + i);
>  
> -        p = mfn_to_page(mfn_add(mfn, i));
> -        od = page_get_owner(p);
>          if ( od != d )
>          {
> -            printk("%s: mfn %lx expected owner d%d, got owner d%d!\n",
> -                   __func__, mfn_x(mfn), d->domain_id,
> -                   od ? od->domain_id : -1);
> +            printk("%s: mfn %lx owner: expected %pd, got %pd!\n",
> +                   __func__, mfn_x(mfn) + i, d, od);

Take the opportunity to drop the superfluous punctuation?


Looking through this code, no callers check for any errors, and the only
failure paths are in a debug region.  The function really ought to
become void, or at the very least, switch to -EINVAL to avoid aliasing
-EPERM.

Furthermore, in all(?) cases that it fails, we'll leak the domain
allocated page, which at the very best means the VM is going to hit
memory limit problems.  i.e. nothing good can come.

Both failures are internal memory handling errors in Xen, so the least
invasive option is probably to switch to ASSERT() (for the alignment
check), and ASSERT_UNREACHABLE() here.  That also addresses the issue
that these printk()'s aren't ratelimited, and used within several loops.

>              return -1;
>          }
>      }
> @@ -98,16 +91,12 @@ p2m_pod_cache_add(struct p2m_domain *p2m
>       * promise to provide zero pages. So we scrub pages before using.
>       */
>      for ( i = 0; i < (1UL << order); i++ )
> -        clear_domain_page(mfn_add(page_to_mfn(page), i));
> +        clear_domain_page(mfn_add(mfn, i));

(For a future change,) this is double scrubbing in most cases.

~Andrew

>  
>      /* First, take all pages off the domain list */
>      lock_page_alloc(p2m);
> -    for ( i = 0; i < 1UL << order ; i++ )
> -    {
> -        p = page + i;
> -        page_list_del(p, &d->page_list);
> -    }
> -
> +    for ( i = 0; i < (1UL << order); i++ )
> +        page_list_del(page + i, &d->page_list);
>      unlock_page_alloc(p2m);
>  
>      /* Then add to the appropriate populate-on-demand list. */
>
>

Re: [PATCH 1/4] x86/PoD: simplify / improve p2m_pod_cache_add()

[Jan Beulich]

On 01.12.2021 13:01, Andrew Cooper wrote:
> On 01/12/2021 10:53, Jan Beulich wrote:
>> --- a/xen/arch/x86/mm/p2m-pod.c
>> +++ b/xen/arch/x86/mm/p2m-pod.c
>> @@ -58,14 +58,10 @@ p2m_pod_cache_add(struct p2m_domain *p2m
>>                    unsigned int order)
>>  {
>>      unsigned long i;
>> -    struct page_info *p;
>>      struct domain *d = p2m->domain;
>> +    mfn_t mfn = page_to_mfn(page);
>>  
>>  #ifndef NDEBUG
>> -    mfn_t mfn;
>> -
>> -    mfn = page_to_mfn(page);
>> -
>>      /* Check to make sure this is a contiguous region */
>>      if ( mfn_x(mfn) & ((1UL << order) - 1) )
>>      {
>> @@ -74,17 +70,14 @@ p2m_pod_cache_add(struct p2m_domain *p2m
>>          return -1;
>>      }
>>  
>> -    for ( i = 0; i < 1UL << order ; i++)
>> +    for ( i = 0; i < (1UL << order); i++)
>>      {
>> -        struct domain * od;
>> +        const struct domain *od = page_get_owner(page + i);
>>  
>> -        p = mfn_to_page(mfn_add(mfn, i));
>> -        od = page_get_owner(p);
>>          if ( od != d )
>>          {
>> -            printk("%s: mfn %lx expected owner d%d, got owner d%d!\n",
>> -                   __func__, mfn_x(mfn), d->domain_id,
>> -                   od ? od->domain_id : -1);
>> +            printk("%s: mfn %lx owner: expected %pd, got %pd!\n",
>> +                   __func__, mfn_x(mfn) + i, d, od);
> 
> Take the opportunity to drop the superfluous punctuation?

Fine with me; means just the exclamation mark though, unless you tell
me what else you would see sensibly dropped. I'd certainly prefer to
keep both colons (the latter of which I'm actually adding here).

> Looking through this code, no callers check for any errors, and the only
> failure paths are in a debug region.  The function really ought to
> become void, or at the very least, switch to -EINVAL to avoid aliasing
> -EPERM.

I'd be okay making this change (I may prefer to avoid EINVAL if I can
find a better match), but I wouldn't want to switch the function to
void - callers would better care about the return value.

> Furthermore, in all(?) cases that it fails, we'll leak the domain
> allocated page, which at the very best means the VM is going to hit
> memory limit problems.  i.e. nothing good can come.
> 
> Both failures are internal memory handling errors in Xen, so the least
> invasive option is probably to switch to ASSERT() (for the alignment
> check), and ASSERT_UNREACHABLE() here.  That also addresses the issue
> that these printk()'s aren't ratelimited, and used within several loops.

Doing this right here, otoh, feels like going too far with a single
change. That's not the least because I would question the value of
doing these checks in debug builds only or tying them to NDEBUG
rather than CONFIG_DEBUG. After all the alignment check could have
triggered prior to the XSA-388 fixes.

Jan

[PATCH 2/4] x86/PoD: HVM guests can't pin their pages

[Jan Beulich]

Pinning is a PV concept, used there only for page table pages.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I'm actually inclined to hide _PGT_pinned in !HVM builds; the downside
of doing so is some new #ifdef-ary which would need adding.

--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -260,9 +260,6 @@ p2m_pod_set_cache_target(struct p2m_doma
                 goto out;
             }
 
-            if ( test_and_clear_bit(_PGT_pinned, &(page+i)->u.inuse.type_info) )
-                put_page_and_type(page + i);
-
             put_page_alloc_ref(page + i);
             put_page(page + i);
 

Re: [PATCH 2/4] x86/PoD: HVM guests can't pin their pages

[Andrew Cooper]

On 01/12/2021 10:53, Jan Beulich wrote:
> Pinning is a PV concept, used there only for page table pages.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Perhaps this is a leftover from autotranslate mode?  That was doing some
very HVM-like things for PV guests.

> ---
> I'm actually inclined to hide _PGT_pinned in !HVM builds; the downside
> of doing so is some new #ifdef-ary which would need adding.

Judging by the current users, I doubt it is worth it, although folding
this delta wouldn't go amiss.

diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index cb9052749963..e5f63daa1a71 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -36,7 +36,7 @@
  /* Page is locked? */
 #define _PGT_locked       PG_shift(4)
 #define PGT_locked        PG_mask(1, 4)
- /* Owning guest has pinned this page to its current type? */
+ /* Owning guest has pinned this page to its current type? PV only */
 #define _PGT_pinned       PG_shift(5)
 #define PGT_pinned        PG_mask(1, 5)
  /* Has this page been validated for use as its current type? */

~Andrew

[PATCH 3/4] x86/altp2m: p2m_altp2m_get_or_propagate() should honor present page order

[Jan Beulich]

Prior to XSA-304 the only caller merely happened to not use any further
the order value that it passes into the function. Already then this was
a latent issue: The function really should, in the "get" case, hand back
the order the underlying mapping actually uses (or actually the smaller
of the two), such that (going forward) there wouldn't be any action on
unrelated mappings (in particular ones which did already diverge from
the host P2M).

Similarly in the "propagate" case only the smaller of the two orders
should actually get used for creating the new entry, again to avoid
altering mappings which did already diverge from the host P2M.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1826,7 +1826,7 @@ int hvm_hap_nested_page_fault(paddr_t gp
          * altp2m.
          */
         if ( p2m_altp2m_get_or_propagate(p2m, gfn, &mfn, &p2mt,
-                                         &p2ma, page_order) )
+                                         &p2ma, &page_order) )
         {
             /* Entry was copied from host -- retry fault */
             rc = 1;
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2198,10 +2198,11 @@ bool_t p2m_switch_vcpu_altp2m_by_id(stru
  */
 bool p2m_altp2m_get_or_propagate(struct p2m_domain *ap2m, unsigned long gfn_l,
                                  mfn_t *mfn, p2m_type_t *p2mt,
-                                 p2m_access_t *p2ma, unsigned int page_order)
+                                 p2m_access_t *p2ma, unsigned int *page_order)
 {
     p2m_type_t ap2mt;
     p2m_access_t ap2ma;
+    unsigned int cur_order;
     unsigned long mask;
     gfn_t gfn;
     mfn_t amfn;
@@ -2214,7 +2215,10 @@ bool p2m_altp2m_get_or_propagate(struct
      */
     p2m_lock(ap2m);
 
-    amfn = get_gfn_type_access(ap2m, gfn_l, &ap2mt, &ap2ma, 0, NULL);
+    amfn = get_gfn_type_access(ap2m, gfn_l, &ap2mt, &ap2ma, 0, &cur_order);
+
+    if ( cur_order > *page_order )
+        cur_order = *page_order;
 
     if ( !mfn_eq(amfn, INVALID_MFN) )
     {
@@ -2222,6 +2226,7 @@ bool p2m_altp2m_get_or_propagate(struct
         *mfn  = amfn;
         *p2mt = ap2mt;
         *p2ma = ap2ma;
+        *page_order = cur_order;
         return false;
     }
 
@@ -2229,6 +2234,7 @@ bool p2m_altp2m_get_or_propagate(struct
     if ( mfn_eq(*mfn, INVALID_MFN) )
     {
         p2m_unlock(ap2m);
+        *page_order = cur_order;
         return false;
     }
 
@@ -2237,11 +2243,11 @@ bool p2m_altp2m_get_or_propagate(struct
      * to the start of the superpage.  NB that we repupose `amfn`
      * here.
      */
-    mask = ~((1UL << page_order) - 1);
+    mask = ~((1UL << cur_order) - 1);
     amfn = _mfn(mfn_x(*mfn) & mask);
     gfn = _gfn(gfn_l & mask);
 
-    rc = p2m_set_entry(ap2m, gfn, amfn, page_order, *p2mt, *p2ma);
+    rc = p2m_set_entry(ap2m, gfn, amfn, cur_order, *p2mt, *p2ma);
     p2m_unlock(ap2m);
 
     if ( rc )
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -852,7 +852,7 @@ void p2m_flush_altp2m(struct domain *d);
 /* Alternate p2m paging */
 bool p2m_altp2m_get_or_propagate(struct p2m_domain *ap2m, unsigned long gfn_l,
                                  mfn_t *mfn, p2m_type_t *p2mt,
-                                 p2m_access_t *p2ma, unsigned int page_order);
+                                 p2m_access_t *p2ma, unsigned int *page_order);
 
 /* Make a specific alternate p2m valid */
 int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx);

Re: [PATCH 3/4] x86/altp2m: p2m_altp2m_get_or_propagate() should honor present page order

[Andrew Cooper]

On 01/12/2021 10:54, Jan Beulich wrote:
> @@ -2237,11 +2243,11 @@ bool p2m_altp2m_get_or_propagate(struct
>       * to the start of the superpage.  NB that we repupose `amfn`
>       * here.
>       */
> -    mask = ~((1UL << page_order) - 1);
> +    mask = ~((1UL << cur_order) - 1);
>      amfn = _mfn(mfn_x(*mfn) & mask);
>      gfn = _gfn(gfn_l & mask);
>  
> -    rc = p2m_set_entry(ap2m, gfn, amfn, page_order, *p2mt, *p2ma);
> +    rc = p2m_set_entry(ap2m, gfn, amfn, cur_order, *p2mt, *p2ma);
>      p2m_unlock(ap2m);

While I agree with the problem you've identified, this function has some
very broken return semantics.

Logically, it is taking some hostp2m properties for gfn, and replacing
them with ap2m properties for the same gfn.


It cannot be correct to only update the caller state on the error
paths.  At a minimum, the

    if ( paged )
        p2m_mem_paging_populate(currd, _gfn(gfn));

path in the success case is wrong when we've adjusted gfn down.

~Andrew

Re: [PATCH 3/4] x86/altp2m: p2m_altp2m_get_or_propagate() should honor present page order

[Jan Beulich]

On 01.12.2021 13:44, Andrew Cooper wrote:
> On 01/12/2021 10:54, Jan Beulich wrote:
>> @@ -2237,11 +2243,11 @@ bool p2m_altp2m_get_or_propagate(struct
>>       * to the start of the superpage.  NB that we repupose `amfn`
>>       * here.
>>       */
>> -    mask = ~((1UL << page_order) - 1);
>> +    mask = ~((1UL << cur_order) - 1);
>>      amfn = _mfn(mfn_x(*mfn) & mask);
>>      gfn = _gfn(gfn_l & mask);
>>  
>> -    rc = p2m_set_entry(ap2m, gfn, amfn, page_order, *p2mt, *p2ma);
>> +    rc = p2m_set_entry(ap2m, gfn, amfn, cur_order, *p2mt, *p2ma);
>>      p2m_unlock(ap2m);
> 
> While I agree with the problem you've identified, this function has some
> very broken return semantics.
> 
> Logically, it is taking some hostp2m properties for gfn, and replacing
> them with ap2m properties for the same gfn.
> 
> 
> It cannot be correct to only update the caller state on the error
> paths.  At a minimum, the
> 
>     if ( paged )
>         p2m_mem_paging_populate(currd, _gfn(gfn));
> 
> path in the success case is wrong when we've adjusted gfn down.

I wonder which of the exit paths you consider to be "error" ones. The
first one returning "false" pretty clearly isn't, for example. And the
path returning "true" is after a p2m_set_entry(), which means (if that
one succeeded) that caller values are now in sync with the P2M and
hence doen't need updating afaics.

And anyway - how does what you say relate to the patch at hand? I don't
think you mean to request that I fix further problems elsewhere, right
here?

Jan

[PATCH RFC 4/4] x86/altp2m: p2m_altp2m_propagate_change() should honor present page order

[Jan Beulich]

For higher order mappings the comparison against p2m->min_remapped_gfn
needs to take the upper bound of the covered GFN range into account, not
just the base GFN.

Otherwise, i.e. when dropping a mapping and not overlapping the remapped
range, XXX.

Note that there's no need to call get_gfn_type_access() ahead of the
check against the remapped range boundaries: None of its outputs are
needed earlier. Local variables get moved into the more narrow scope to
demonstrate this.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I may be entirely wrong and hence that part of the change may also be
wrong, but I'm having trouble seeing why the original
"!mfn_eq(m, INVALID_MFN)" wasn't "mfn_eq(m, INVALID_MFN)". Isn't the
goal there to pre-fill entries that were previously invalid, instead of
undoing prior intentional divergence from the host P2M? (I have
intentionally not reflected this aspect in the description yet; I can't
really write a description of this without understanding what's going on
in case the original code was correct.)

When cur_order is below the passed in page_order, the p2m_set_entry() is
of course not covering the full range. This could be put in a loop, but
locking looks to be a little problematic: If a set of lower order pages
gets re-combined to a large one while already having progressed into the
range, we'd need to carefully back off. Alternatively the full incoming
GFN range could be held locked for the entire loop; this would likely
mean relying on gfn_lock() actually resolving to p2m_lock(). But perhaps
that's not a big problem, considering that core functions like
p2m_get_gfn_type_access() or __put_gfn() assume so, too (because of
not taking the order into account at all)?

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2532,9 +2532,6 @@ int p2m_altp2m_propagate_change(struct d
                                 p2m_type_t p2mt, p2m_access_t p2ma)
 {
     struct p2m_domain *p2m;
-    p2m_access_t a;
-    p2m_type_t t;
-    mfn_t m;
     unsigned int i;
     unsigned int reset_count = 0;
     unsigned int last_reset_idx = ~0;
@@ -2551,12 +2548,30 @@ int p2m_altp2m_propagate_change(struct d
             continue;
 
         p2m = d->arch.altp2m_p2m[i];
-        m = get_gfn_type_access(p2m, gfn_x(gfn), &t, &a, 0, NULL);
 
+        if ( !mfn_eq(mfn, INVALID_MFN) )
+        {
+            p2m_access_t a;
+            p2m_type_t t;
+            unsigned int cur_order;
+
+            if ( mfn_eq(get_gfn_type_access(p2m, gfn_x(gfn), &t, &a, 0,
+                                            &cur_order),
+                        INVALID_MFN) )
+            {
+                int rc = p2m_set_entry(p2m, gfn, mfn, min(cur_order, page_order),
+                                       p2mt, p2ma);
+
+                /* Best effort: Don't bail on error. */
+                if ( !ret )
+                    ret = rc;
+            }
+
+            __put_gfn(p2m, gfn_x(gfn));
+        }
         /* Check for a dropped page that may impact this altp2m */
-        if ( mfn_eq(mfn, INVALID_MFN) &&
-             gfn_x(gfn) >= p2m->min_remapped_gfn &&
-             gfn_x(gfn) <= p2m->max_remapped_gfn )
+        else if ( gfn_x(gfn) + (1UL << page_order) > p2m->min_remapped_gfn &&
+                  gfn_x(gfn) <= p2m->max_remapped_gfn )
         {
             if ( !reset_count++ )
             {
@@ -2566,8 +2581,6 @@ int p2m_altp2m_propagate_change(struct d
             else
             {
                 /* At least 2 altp2m's impacted, so reset everything */
-                __put_gfn(p2m, gfn_x(gfn));
-
                 for ( i = 0; i < MAX_ALTP2M; i++ )
                 {
                     if ( i == last_reset_idx ||
@@ -2581,16 +2594,6 @@ int p2m_altp2m_propagate_change(struct d
                 break;
             }
         }
-        else if ( !mfn_eq(m, INVALID_MFN) )
-        {
-            int rc = p2m_set_entry(p2m, gfn, mfn, page_order, p2mt, p2ma);
-
-            /* Best effort: Don't bail on error. */
-            if ( !ret )
-                ret = rc;
-        }
-
-        __put_gfn(p2m, gfn_x(gfn));
     }
 
     altp2m_list_unlock(d);