[PATCH 0/8] x86: XSA-40{1,2,8} follow-up

[PATCH 0/8] x86: XSA-40{1,2,8} follow-up

[Jan Beulich]

Perhaps not entirely unexpected the work there has turned up a few
further items which want dealing with. Most if not all of these
aren't interdependent, so could probably be looked at (and go in)
in (about) any order.

There's one more change I'd like to make, but I didn't get around
to making (trying to make) yet: PGC_page_table being a shadow-only
concept, it can apparently simply be 0 when !SHADOW_PAGING.

1: shadow: drop shadow_prepare_page_type_change()'s 3rd parameter
2: shadow: properly handle get_page() failing
3: mm: enforce return value checking on get_page()
4: shadow: exclude HVM-only code from sh_remove_shadows() when !HVM
5: shadow: don't open-code shadow_remove_all_shadows()
6: shadow: drop CONFIG_HVM conditionals from sh_update_cr3()
7: mm: adjust type check around _get_page_type()'s TLB flush
8: mm: re-arrange type check around _get_page_type()'s TLB flush

Jan

[PATCH 1/8] x86/shadow: drop shadow_prepare_page_type_change()'s 3rd parameter

[Jan Beulich]

As of 8cc5036bc385 ("x86/pv: Fix ABAC cmpxchg() race in
_get_page_type()") this no longer needs passing separately - the type
can now be read from struct page_info, as the call now happens after its
writing.

While there also constify the 2nd parameter.

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

--- a/xen/arch/x86/include/asm/shadow.h
+++ b/xen/arch/x86/include/asm/shadow.h
@@ -84,8 +84,8 @@ void shadow_final_teardown(struct domain
 void sh_remove_shadows(struct domain *d, mfn_t gmfn, int fast, int all);
 
 /* Adjust shadows ready for a guest page to change its type. */
-void shadow_prepare_page_type_change(struct domain *d, struct page_info *page,
-                                     unsigned long new_type);
+void shadow_prepare_page_type_change(struct domain *d,
+                                     const struct page_info *page);
 
 /* Discard _all_ mappings from the domain's shadows. */
 void shadow_blow_tables_per_domain(struct domain *d);
@@ -113,8 +113,7 @@ static inline void sh_remove_shadows(str
                                      int fast, int all) {}
 
 static inline void shadow_prepare_page_type_change(struct domain *d,
-                                                   struct page_info *page,
-                                                   unsigned long new_type) {}
+                                                   const struct page_info *page) {}
 
 static inline void shadow_blow_tables_per_domain(struct domain *d) {}
 
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3018,7 +3018,7 @@ static int _get_page_type(struct page_in
         struct domain *d = page_get_owner(page);
 
         if ( d && shadow_mode_enabled(d) )
-            shadow_prepare_page_type_change(d, page, type);
+            shadow_prepare_page_type_change(d, page);
 
         if ( (x & PGT_type_mask) != type )
         {
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -2265,8 +2265,8 @@ void sh_remove_shadows(struct domain *d,
     paging_unlock(d);
 }
 
-void shadow_prepare_page_type_change(struct domain *d, struct page_info *page,
-                                     unsigned long new_type)
+void shadow_prepare_page_type_change(struct domain *d,
+                                     const struct page_info *page)
 {
     if ( !(page->count_info & PGC_page_table) )
         return;
@@ -2278,7 +2278,7 @@ void shadow_prepare_page_type_change(str
      * pages are allowed to become writeable.
      */
     if ( (page->shadow_flags & SHF_oos_may_write) &&
-         new_type == PGT_writable_page )
+         (page->u.inuse.type_info & PGT_type_mask) == PGT_writable_page )
         return;
 #endif
 

[PATCH 2/8] x86/shadow: properly handle get_page() failing

[Jan Beulich]

We should not blindly (in a release build) insert the new entry in the
hash if a reference to the guest page cannot be obtained, or else an
excess reference would be put when removing the hash entry again. Crash
the domain in that case instead. The sole caller doesn't further care
about the state of the guest page: All it does is return the
corresponding shadow page (which was obtained successfully before) to
its caller.

To compensate we further need to adjust hash removal: Since the shadow
page already has had its backlink set, domain cleanup code would try to
destroy the shadow, and hence still cause a put_page() without
corresponding get_page(). Leverage that the failed get_page() leads to
no hash insertion, making shadow_hash_delete() no longer assume it will
find the requested entry. Instead return back whether the entry was
found. This way delete_shadow_status() can avoid calling put_page() in
the problem scenario.

For the other caller of shadow_hash_delete() simply reinstate the
otherwise dropped assertion at the call site.

While touching the conditionals in {set,delete}_shadow_status() anyway,
also switch around their two pre-existing parts, to have the cheap one
first (frequently allowing to avoid evaluation of the expensive - due to
evaluate_nospec() - one altogether).

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

--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -1581,7 +1581,7 @@ void shadow_hash_insert(struct domain *d
     sh_hash_audit_bucket(d, key);
 }
 
-void shadow_hash_delete(struct domain *d, unsigned long n, unsigned int t,
+bool shadow_hash_delete(struct domain *d, unsigned long n, unsigned int t,
                         mfn_t smfn)
 /* Excise the mapping (n,t)->smfn from the hash table */
 {
@@ -1606,10 +1606,8 @@ void shadow_hash_delete(struct domain *d
     {
         /* Need to search for the one we want */
         x = d->arch.paging.shadow.hash_table[key];
-        while ( 1 )
+        while ( x )
         {
-            ASSERT(x); /* We can't have hit the end, since our target is
-                        * still in the chain somehwere... */
             if ( next_shadow(x) == sp )
             {
                 x->next_shadow = sp->next_shadow;
@@ -1617,10 +1615,14 @@ void shadow_hash_delete(struct domain *d
             }
             x = next_shadow(x);
         }
+        if ( !x )
+            return false;
     }
     set_next_shadow(sp, NULL);
 
     sh_hash_audit_bucket(d, key);
+
+    return true;
 }
 
 typedef int (*hash_vcpu_callback_t)(struct vcpu *v, mfn_t smfn, mfn_t other_mfn);
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -132,7 +132,8 @@ delete_fl1_shadow_status(struct domain *
     SHADOW_PRINTK("gfn=%"SH_PRI_gfn", type=%08x, smfn=%"PRI_mfn"\n",
                    gfn_x(gfn), SH_type_fl1_shadow, mfn_x(smfn));
     ASSERT(mfn_to_page(smfn)->u.sh.head);
-    shadow_hash_delete(d, gfn_x(gfn), SH_type_fl1_shadow, smfn);
+    if ( !shadow_hash_delete(d, gfn_x(gfn), SH_type_fl1_shadow, smfn) )
+        ASSERT_UNREACHABLE();
 }
 
 
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -375,7 +375,7 @@ shadow_size(unsigned int shadow_type)
 mfn_t shadow_hash_lookup(struct domain *d, unsigned long n, unsigned int t);
 void  shadow_hash_insert(struct domain *d,
                          unsigned long n, unsigned int t, mfn_t smfn);
-void  shadow_hash_delete(struct domain *d,
+bool  shadow_hash_delete(struct domain *d,
                          unsigned long n, unsigned int t, mfn_t smfn);
 
 /* shadow promotion */
@@ -773,18 +773,19 @@ static inline void
 set_shadow_status(struct domain *d, mfn_t gmfn, u32 shadow_type, mfn_t smfn)
 /* Put a shadow into the hash table */
 {
-    int res;
-
     SHADOW_PRINTK("d%d gmfn=%lx, type=%08x, smfn=%lx\n",
                   d->domain_id, mfn_x(gmfn), shadow_type, mfn_x(smfn));
 
     ASSERT(mfn_to_page(smfn)->u.sh.head);
 
     /* 32-bit PV guests don't own their l4 pages so can't get_page them */
-    if ( !is_pv_32bit_domain(d) || shadow_type != SH_type_l4_64_shadow )
+    if ( (shadow_type != SH_type_l4_64_shadow || !is_pv_32bit_domain(d)) &&
+         !get_page(mfn_to_page(gmfn), d) )
     {
-        res = get_page(mfn_to_page(gmfn), d);
-        ASSERT(res == 1);
+        printk(XENLOG_G_ERR "%pd: cannot get page for MFN %" PRI_mfn "\n",
+               d, mfn_x(gmfn));
+        domain_crash(d);
+        return;
     }
 
     shadow_hash_insert(d, mfn_x(gmfn), shadow_type, smfn);
@@ -797,9 +798,9 @@ delete_shadow_status(struct domain *d, m
     SHADOW_PRINTK("d%d gmfn=%"PRI_mfn", type=%08x, smfn=%"PRI_mfn"\n",
                   d->domain_id, mfn_x(gmfn), shadow_type, mfn_x(smfn));
     ASSERT(mfn_to_page(smfn)->u.sh.head);
-    shadow_hash_delete(d, mfn_x(gmfn), shadow_type, smfn);
-    /* 32-bit PV guests don't own their l4 pages; see set_shadow_status */
-    if ( !is_pv_32bit_domain(d) || shadow_type != SH_type_l4_64_shadow )
+    if ( shadow_hash_delete(d, mfn_x(gmfn), shadow_type, smfn) &&
+         /* 32-bit PV guests don't own their l4 pages; see set_shadow_status */
+         (shadow_type != SH_type_l4_64_shadow || !is_pv_32bit_domain(d)) )
         put_page(mfn_to_page(gmfn));
 }
 

[PATCH 3/8] mm: enforce return value checking on get_page()

[Jan Beulich]

It's hard to imagine a case where an error may legitimately be ignored
here. It's bad enough that in at least one case (set_shadow_status())
the return value was checked only by way of ASSERT()ing.

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

--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -61,7 +61,7 @@
 struct page_info;
 
 void put_page(struct page_info *);
-bool get_page(struct page_info *, const struct domain *);
+bool __must_check get_page(struct page_info *, const struct domain *);
 struct domain *__must_check page_get_owner_and_reference(struct page_info *);
 
 /* Boot-time allocator. Turns into generic allocator after bootstrap. */

[PATCH 4/8] x86/shadow: exclude HVM-only code from sh_remove_shadows() when !HVM

[Jan Beulich]

In my (debug) build this amounts to well over 500 bytes of dead code.

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

--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -2240,10 +2240,12 @@ void sh_remove_shadows(struct domain *d,
     }                                                                   \
 } while (0)
 
+#ifdef CONFIG_HVM
     DO_UNSHADOW(SH_type_l2_32_shadow);
     DO_UNSHADOW(SH_type_l1_32_shadow);
     DO_UNSHADOW(SH_type_l2_pae_shadow);
     DO_UNSHADOW(SH_type_l1_pae_shadow);
+#endif
     DO_UNSHADOW(SH_type_l4_64_shadow);
     DO_UNSHADOW(SH_type_l3_64_shadow);
     DO_UNSHADOW(SH_type_l2h_64_shadow);

[PATCH 5/8] x86/shadow: don't open-code shadow_remove_all_shadows()

[Jan Beulich]

Let's use the existing inline wrapper instead of repeating respective
commentary at every site.

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

--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -389,7 +389,7 @@ static int oos_remove_write_access(struc
          * the page.  If that doesn't work either, the guest is granting
          * his pagetables and must be killed after all.
          * This will flush the tlb, so we can return with no worries. */
-        sh_remove_shadows(d, gmfn, 0 /* Be thorough */, 1 /* Must succeed */);
+        shadow_remove_all_shadows(d, gmfn);
         return 1;
     }
 
--- a/xen/arch/x86/mm/shadow/hvm.c
+++ b/xen/arch/x86/mm/shadow/hvm.c
@@ -783,7 +783,7 @@ sh_remove_all_shadows_and_parents(struct
 /* Even harsher: this is a HVM page that we thing is no longer a pagetable.
  * Unshadow it, and recursively unshadow pages that reference it. */
 {
-    sh_remove_shadows(d, gmfn, 0, 1);
+    shadow_remove_all_shadows(d, gmfn);
     /* XXX TODO:
      * Rework this hashtable walker to return a linked-list of all
      * the shadows it modified, then do breadth-first recursion
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -2632,7 +2632,7 @@ static int cf_check sh_page_fault(
         SHADOW_PRINTK("user-mode fault to PT, unshadowing mfn %#lx\n",
                       mfn_x(gmfn));
         perfc_incr(shadow_fault_emulate_failed);
-        sh_remove_shadows(d, gmfn, 0 /* thorough */, 1 /* must succeed */);
+        shadow_remove_all_shadows(d, gmfn);
         trace_shadow_emulate_other(TRC_SHADOW_EMULATE_UNSHADOW_USER,
                                       va, gfn);
         goto done;
@@ -2718,7 +2718,7 @@ static int cf_check sh_page_fault(
             v->arch.paging.last_write_emul_ok = 0;
         }
 #endif
-        sh_remove_shadows(d, gmfn, 0 /* thorough */, 1 /* must succeed */);
+        shadow_remove_all_shadows(d, gmfn);
         trace_shadow_emulate_other(TRC_SHADOW_EMULATE_UNSHADOW_EVTINJ,
                                    va, gfn);
         return EXCRET_fault_fixed;

[PATCH 6/8] x86/shadow: drop CONFIG_HVM conditionals from sh_update_cr3()

[Jan Beulich]

Now that we're not building multi.c anymore for 2 and 3 guest levels
when !HVM, there's no point in having these conditionals anymore. (As
somewhat a special case, the last of the removed conditionals really
builds on shadow_mode_external() always returning false when !HVM.) This
way the code becomes a tiny bit more readable.

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

--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -3228,7 +3228,7 @@ static void cf_check sh_update_cr3(struc
 {
     struct domain *d = v->domain;
     mfn_t gmfn;
-#if GUEST_PAGING_LEVELS == 3 && defined(CONFIG_HVM)
+#if GUEST_PAGING_LEVELS == 3
     const guest_l3e_t *gl3e;
     unsigned int i, guest_idx;
 #endif
@@ -3279,7 +3279,7 @@ static void cf_check sh_update_cr3(struc
 #endif
         gmfn = pagetable_get_mfn(v->arch.guest_table);
 
-#if GUEST_PAGING_LEVELS == 3 && defined(CONFIG_HVM)
+#if GUEST_PAGING_LEVELS == 3
     /*
      * On PAE guests we don't use a mapping of the guest's own top-level
      * table.  We cache the current state of that table and shadow that,
@@ -3321,8 +3321,6 @@ static void cf_check sh_update_cr3(struc
                   !VM_ASSIST(d, m2p_strict) )
             fill_ro_mpt(smfn);
     }
-#elif !defined(CONFIG_HVM)
-    ASSERT_UNREACHABLE();
 #elif GUEST_PAGING_LEVELS == 3
     /* PAE guests have four shadow_table entries, based on the
      * current values of the guest's four l3es. */
@@ -3373,8 +3371,6 @@ static void cf_check sh_update_cr3(struc
 #error This should never happen
 #endif
 
-
-#ifdef CONFIG_HVM
     ///
     /// v->arch.paging.shadow.l3table
     ///
@@ -3400,7 +3396,6 @@ static void cf_check sh_update_cr3(struc
             }
         }
 #endif /* SHADOW_PAGING_LEVELS == 3 */
-#endif /* CONFIG_HVM */
 
     ///
     /// v->arch.cr3
@@ -3419,8 +3414,6 @@ static void cf_check sh_update_cr3(struc
     }
 #endif
 
-
-#ifdef CONFIG_HVM
     ///
     /// v->arch.hvm.hw_cr[3]
     ///
@@ -3437,7 +3430,6 @@ static void cf_check sh_update_cr3(struc
 #endif
         hvm_update_guest_cr3(v, noflush);
     }
-#endif /* CONFIG_HVM */
 
     /* Fix up the linear pagetable mappings */
     sh_update_linear_entries(v);

[PATCH 7/8] x86/mm: adjust type check around _get_page_type()'s TLB flush

[Jan Beulich]

While "type" can include PGT_pae_xen_l2, "x" can't, as the bit is
cleared upon de-validation (see also the respective assertion earlier in
the function).

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

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3020,7 +3020,7 @@ static int _get_page_type(struct page_in
         if ( d && shadow_mode_enabled(d) )
             shadow_prepare_page_type_change(d, page);
 
-        if ( (x & PGT_type_mask) != type )
+        if ( (x ^ type) & PGT_type_mask )
         {
             /*
              * On type change we check to flush stale TLB entries. It is

[PATCH 8/8] x86/mm: re-arrange type check around _get_page_type()'s TLB flush

[Jan Beulich]

Checks dependent on only d and x can be pulled out, thus allowing to
skip the flush mask calculation.

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

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3020,7 +3020,10 @@ static int _get_page_type(struct page_in
         if ( d && shadow_mode_enabled(d) )
             shadow_prepare_page_type_change(d, page);
 
-        if ( (x ^ type) & PGT_type_mask )
+        if ( ((x ^ type) & PGT_type_mask) &&
+             /* Shadow mode: track only writable pages. */
+             (!shadow_mode_enabled(d) ||
+              ((x & PGT_type_mask) == PGT_writable_page)) )
         {
             /*
              * On type change we check to flush stale TLB entries. It is
@@ -3035,10 +3038,7 @@ static int _get_page_type(struct page_in
             /* Don't flush if the timestamp is old enough */
             tlbflush_filter(mask, page->tlbflush_timestamp);
 
-            if ( unlikely(!cpumask_empty(mask)) &&
-                 /* Shadow mode: track only writable pages. */
-                 (!shadow_mode_enabled(d) ||
-                  ((x & PGT_type_mask) == PGT_writable_page)) )
+            if ( unlikely(!cpumask_empty(mask)) )
             {
                 perfc_incr(need_flush_tlb_flush);
                 /*

Re: [PATCH 3/8] mm: enforce return value checking on get_page()

[Julien Grall]

Hi Jan,

On 26/07/2022 17:04, Jan Beulich wrote:
> It's hard to imagine a case where an error may legitimately be ignored
> here. It's bad enough that in at least one case (set_shadow_status())
> the return value was checked only by way of ASSERT()ing.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH 1/8] x86/shadow: drop shadow_prepare_page_type_change()'s 3rd parameter

[Andrew Cooper]

On 26/07/2022 17:03, Jan Beulich wrote:
> As of 8cc5036bc385 ("x86/pv: Fix ABAC cmpxchg() race in
> _get_page_type()") this no longer needs passing separately - the type
> can now be read from struct page_info, as the call now happens after its
> writing.
>
> While there also constify the 2nd parameter.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Re: [PATCH 2/8] x86/shadow: properly handle get_page() failing

[Andrew Cooper]

On 26/07/2022 17:04, Jan Beulich wrote:
> We should not blindly (in a release build) insert the new entry in the
> hash if a reference to the guest page cannot be obtained, or else an
> excess reference would be put when removing the hash entry again. Crash
> the domain in that case instead. The sole caller doesn't further care
> about the state of the guest page: All it does is return the
> corresponding shadow page (which was obtained successfully before) to
> its caller.
>
> To compensate we further need to adjust hash removal: Since the shadow
> page already has had its backlink set, domain cleanup code would try to
> destroy the shadow, and hence still cause a put_page() without
> corresponding get_page(). Leverage that the failed get_page() leads to
> no hash insertion, making shadow_hash_delete() no longer assume it will
> find the requested entry. Instead return back whether the entry was
> found. This way delete_shadow_status() can avoid calling put_page() in
> the problem scenario.
>
> For the other caller of shadow_hash_delete() simply reinstate the
> otherwise dropped assertion at the call site.
>
> While touching the conditionals in {set,delete}_shadow_status() anyway,
> also switch around their two pre-existing parts, to have the cheap one
> first (frequently allowing to avoid evaluation of the expensive - due to
> evaluate_nospec() - one altogether).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although with
some observations and a suggestion.

>
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -1581,7 +1581,7 @@ void shadow_hash_insert(struct domain *d
>      sh_hash_audit_bucket(d, key);
>  }
>  
> -void shadow_hash_delete(struct domain *d, unsigned long n, unsigned int t,
> +bool shadow_hash_delete(struct domain *d, unsigned long n, unsigned int t,
>                          mfn_t smfn)
>  /* Excise the mapping (n,t)->smfn from the hash table */
>  {
> @@ -1606,10 +1606,8 @@ void shadow_hash_delete(struct domain *d
>      {
>          /* Need to search for the one we want */
>          x = d->arch.paging.shadow.hash_table[key];
> -        while ( 1 )
> +        while ( x )
>          {
> -            ASSERT(x); /* We can't have hit the end, since our target is
> -                        * still in the chain somehwere... */
>              if ( next_shadow(x) == sp )
>              {
>                  x->next_shadow = sp->next_shadow;
> @@ -1617,10 +1615,14 @@ void shadow_hash_delete(struct domain *d
>              }
>              x = next_shadow(x);
>          }
> +        if ( !x )
> +            return false;
>      }

This entire if/else block is "delete matching element from single linked
list", opencoded in a very confusing way to follow.  I can't help but
feel there's a way to simplify it.  (Not in this patch, but for future
clean-up.)

>      set_next_shadow(sp, NULL);
>  
>      sh_hash_audit_bucket(d, key);
> +
> +    return true;
>  }
>  
>  typedef int (*hash_vcpu_callback_t)(struct vcpu *v, mfn_t smfn, mfn_t other_mfn);
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -132,7 +132,8 @@ delete_fl1_shadow_status(struct domain *
>      SHADOW_PRINTK("gfn=%"SH_PRI_gfn", type=%08x, smfn=%"PRI_mfn"\n",
>                     gfn_x(gfn), SH_type_fl1_shadow, mfn_x(smfn));
>      ASSERT(mfn_to_page(smfn)->u.sh.head);
> -    shadow_hash_delete(d, gfn_x(gfn), SH_type_fl1_shadow, smfn);
> +    if ( !shadow_hash_delete(d, gfn_x(gfn), SH_type_fl1_shadow, smfn) )
> +        ASSERT_UNREACHABLE();

I'd recommend crashing the domain here too.  I know the we've already
got the return value we want, but this represents corruption in the
shadow pagetable metadata, and I highly doubt it is safe to let the
guest continue to execute in such circumstances.

>  }
>  
>  
> --- a/xen/arch/x86/mm/shadow/private.h
> +++ b/xen/arch/x86/mm/shadow/private.h
> @@ -375,7 +375,7 @@ shadow_size(unsigned int shadow_type)
>  mfn_t shadow_hash_lookup(struct domain *d, unsigned long n, unsigned int t);
>  void  shadow_hash_insert(struct domain *d,
>                           unsigned long n, unsigned int t, mfn_t smfn);
> -void  shadow_hash_delete(struct domain *d,
> +bool  shadow_hash_delete(struct domain *d,
>                           unsigned long n, unsigned int t, mfn_t smfn);
>  
>  /* shadow promotion */
> @@ -773,18 +773,19 @@ static inline void
>  set_shadow_status(struct domain *d, mfn_t gmfn, u32 shadow_type, mfn_t smfn)
>  /* Put a shadow into the hash table */
>  {
> -    int res;
> -
>      SHADOW_PRINTK("d%d gmfn=%lx, type=%08x, smfn=%lx\n",
>                    d->domain_id, mfn_x(gmfn), shadow_type, mfn_x(smfn));
>  
>      ASSERT(mfn_to_page(smfn)->u.sh.head);
>  
>      /* 32-bit PV guests don't own their l4 pages so can't get_page them */
> -    if ( !is_pv_32bit_domain(d) || shadow_type != SH_type_l4_64_shadow )
> +    if ( (shadow_type != SH_type_l4_64_shadow || !is_pv_32bit_domain(d)) &&

This is the sensible way around anyway, but this is also a great example
of a predicate which really doesn't want to be eval_nospec().

We've got a growing pile of such usecases, so really do need to find a
predicate we can use which indicates "safely not speculation relevant".

~Andrew

Re: [PATCH 2/8] x86/shadow: properly handle get_page() failing

[Jan Beulich]

On 27.07.2022 14:46, Andrew Cooper wrote:
> On 26/07/2022 17:04, Jan Beulich wrote:
>> We should not blindly (in a release build) insert the new entry in the
>> hash if a reference to the guest page cannot be obtained, or else an
>> excess reference would be put when removing the hash entry again. Crash
>> the domain in that case instead. The sole caller doesn't further care
>> about the state of the guest page: All it does is return the
>> corresponding shadow page (which was obtained successfully before) to
>> its caller.
>>
>> To compensate we further need to adjust hash removal: Since the shadow
>> page already has had its backlink set, domain cleanup code would try to
>> destroy the shadow, and hence still cause a put_page() without
>> corresponding get_page(). Leverage that the failed get_page() leads to
>> no hash insertion, making shadow_hash_delete() no longer assume it will
>> find the requested entry. Instead return back whether the entry was
>> found. This way delete_shadow_status() can avoid calling put_page() in
>> the problem scenario.
>>
>> For the other caller of shadow_hash_delete() simply reinstate the
>> otherwise dropped assertion at the call site.
>>
>> While touching the conditionals in {set,delete}_shadow_status() anyway,
>> also switch around their two pre-existing parts, to have the cheap one
>> first (frequently allowing to avoid evaluation of the expensive - due to
>> evaluate_nospec() - one altogether).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although with
> some observations and a suggestion.

Thanks.

>> --- a/xen/arch/x86/mm/shadow/multi.c
>> +++ b/xen/arch/x86/mm/shadow/multi.c
>> @@ -132,7 +132,8 @@ delete_fl1_shadow_status(struct domain *
>>      SHADOW_PRINTK("gfn=%"SH_PRI_gfn", type=%08x, smfn=%"PRI_mfn"\n",
>>                     gfn_x(gfn), SH_type_fl1_shadow, mfn_x(smfn));
>>      ASSERT(mfn_to_page(smfn)->u.sh.head);
>> -    shadow_hash_delete(d, gfn_x(gfn), SH_type_fl1_shadow, smfn);
>> +    if ( !shadow_hash_delete(d, gfn_x(gfn), SH_type_fl1_shadow, smfn) )
>> +        ASSERT_UNREACHABLE();
> 
> I'd recommend crashing the domain here too.  I know the we've already
> got the return value we want, but this represents corruption in the
> shadow pagetable metadata, and I highly doubt it is safe to let the
> guest continue to execute in such circumstances.

I'm happy to add or convert, but please clarify: DYM in addition to
the assertion or in place of it?

Jan

Re: [PATCH 4/8] x86/shadow: exclude HVM-only code from sh_remove_shadows() when !HVM

[Andrew Cooper]

On 26/07/2022 17:05, Jan Beulich wrote:
> In my (debug) build this amounts to well over 500 bytes of dead code.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Re: [PATCH 5/8] x86/shadow: don't open-code shadow_remove_all_shadows()

[Andrew Cooper]

On 26/07/2022 17:05, Jan Beulich wrote:
> Let's use the existing inline wrapper instead of repeating respective
> commentary at every site.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Re: [PATCH 6/8] x86/shadow: drop CONFIG_HVM conditionals from sh_update_cr3()

[Andrew Cooper]

On 26/07/2022 17:06, Jan Beulich wrote:
> Now that we're not building multi.c anymore for 2 and 3 guest levels
> when !HVM, there's no point in having these conditionals anymore. (As
> somewhat a special case, the last of the removed conditionals really
> builds on shadow_mode_external() always returning false when !HVM.) This
> way the code becomes a tiny bit more readable.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Re: [PATCH 7/8] x86/mm: adjust type check around _get_page_type()'s TLB flush

[Andrew Cooper]

On 26/07/2022 17:06, Jan Beulich wrote:
> While "type" can include PGT_pae_xen_l2, "x" can't, as the bit is
> cleared upon de-validation (see also the respective assertion earlier in
> the function).

While this statement is true, it doesn't really explain why this is
relevant (or not) to TLB flushing.

As far as the change goes, it's safe on 64bit builds of Xen (I think),
but would not be on 32bit builds when this logic was first written.

~Andrew

Re: [PATCH 8/8] x86/mm: re-arrange type check around _get_page_type()'s TLB flush

[Andrew Cooper]

On 26/07/2022 17:07, Jan Beulich wrote:
> Checks dependent on only d and x can be pulled out, thus allowing to
> skip the flush mask calculation.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Do I get a Suggested-by seeing as this was on the very long list of
improvements ?

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

Re: [PATCH 2/8] x86/shadow: properly handle get_page() failing

[Andrew Cooper]

On 27/07/2022 13:53, Jan Beulich wrote:
> On 27.07.2022 14:46, Andrew Cooper wrote:
>> On 26/07/2022 17:04, Jan Beulich wrote:
>>> --- a/xen/arch/x86/mm/shadow/multi.c
>>> +++ b/xen/arch/x86/mm/shadow/multi.c
>>> @@ -132,7 +132,8 @@ delete_fl1_shadow_status(struct domain *
>>>      SHADOW_PRINTK("gfn=%"SH_PRI_gfn", type=%08x, smfn=%"PRI_mfn"\n",
>>>                     gfn_x(gfn), SH_type_fl1_shadow, mfn_x(smfn));
>>>      ASSERT(mfn_to_page(smfn)->u.sh.head);
>>> -    shadow_hash_delete(d, gfn_x(gfn), SH_type_fl1_shadow, smfn);
>>> +    if ( !shadow_hash_delete(d, gfn_x(gfn), SH_type_fl1_shadow, smfn) )
>>> +        ASSERT_UNREACHABLE();
>> I'd recommend crashing the domain here too.  I know the we've already
>> got the return value we want, but this represents corruption in the
>> shadow pagetable metadata, and I highly doubt it is safe to let the
>> guest continue to execute in such circumstances.
> I'm happy to add or convert, but please clarify: DYM in addition to
> the assertion or in place of it?

Erm pass.

We still don't have a sensible construct for ASSERT_OR_DOMAIN_CRASH(),
or a well-thought-through piece of guidance.

Probably keep the ASSERT() ?  Almost all state checking in the shadow
code is via asserts.


Mostly what I'm concerned by is it feeling weird to have one path which
only domain crashes, and one path which only asserts.

~Andrew

Re: [PATCH 2/8] x86/shadow: properly handle get_page() failing

[Jan Beulich]

On 27.07.2022 21:06, Andrew Cooper wrote:
> On 27/07/2022 13:53, Jan Beulich wrote:
>> On 27.07.2022 14:46, Andrew Cooper wrote:
>>> On 26/07/2022 17:04, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/mm/shadow/multi.c
>>>> +++ b/xen/arch/x86/mm/shadow/multi.c
>>>> @@ -132,7 +132,8 @@ delete_fl1_shadow_status(struct domain *
>>>>      SHADOW_PRINTK("gfn=%"SH_PRI_gfn", type=%08x, smfn=%"PRI_mfn"\n",
>>>>                     gfn_x(gfn), SH_type_fl1_shadow, mfn_x(smfn));
>>>>      ASSERT(mfn_to_page(smfn)->u.sh.head);
>>>> -    shadow_hash_delete(d, gfn_x(gfn), SH_type_fl1_shadow, smfn);
>>>> +    if ( !shadow_hash_delete(d, gfn_x(gfn), SH_type_fl1_shadow, smfn) )
>>>> +        ASSERT_UNREACHABLE();
>>> I'd recommend crashing the domain here too.  I know the we've already
>>> got the return value we want, but this represents corruption in the
>>> shadow pagetable metadata, and I highly doubt it is safe to let the
>>> guest continue to execute in such circumstances.
>> I'm happy to add or convert, but please clarify: DYM in addition to
>> the assertion or in place of it?
> 
> Erm pass.
> 
> We still don't have a sensible construct for ASSERT_OR_DOMAIN_CRASH(),
> or a well-thought-through piece of guidance.
> 
> Probably keep the ASSERT() ?  Almost all state checking in the shadow
> code is via asserts.

Meanwhile I was actually leaning towards dropping the assertion. The
goal is to catch attention when things go wrong. A suddenly dying
domain surely ought to be as noticable as an assertion triggering.

> Mostly what I'm concerned by is it feeling weird to have one path which
> only domain crashes, and one path which only asserts.

I'm afraid I've not been successful in determining which other path it
is you're referring to. If I knew, I might be up for converting that
as well (not in this same patch, perhaps). I don't think you mean
set_shadow_status(), since there we can't validly ASSERT(), as the
condition is (in principle) guest controllable.

Jan

Re: [PATCH 7/8] x86/mm: adjust type check around _get_page_type()'s TLB flush

[Jan Beulich]

On 27.07.2022 20:25, Andrew Cooper wrote:
> On 26/07/2022 17:06, Jan Beulich wrote:
>> While "type" can include PGT_pae_xen_l2, "x" can't, as the bit is
>> cleared upon de-validation (see also the respective assertion earlier in
>> the function).
> 
> While this statement is true, it doesn't really explain why this is
> relevant (or not) to TLB flushing.
> 
> As far as the change goes, it's safe on 64bit builds of Xen (I think),
> but would not be on 32bit builds when this logic was first written.

How that? The flush is needed if the type changes. If (for the purposes
here) PGT_pae_xen_l2 was considered part of the type, then the bit being
lost upon de-validation would mean we're flushing too little.

Jan

Re: [PATCH 8/8] x86/mm: re-arrange type check around _get_page_type()'s TLB flush

[Jan Beulich]

On 27.07.2022 20:31, Andrew Cooper wrote:
> On 26/07/2022 17:07, Jan Beulich wrote:
>> Checks dependent on only d and x can be pulled out, thus allowing to
>> skip the flush mask calculation.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Do I get a Suggested-by seeing as this was on the very long list of
> improvements ?

I can add one, but you weren't the only one to notice.

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

Thanks.

Jan

Re: [PATCH 0/8] x86: XSA-40{1,2,8} follow-up

[Tim Deegan]

At 17:58 +0200 on 26 Jul (1658858332), Jan Beulich wrote:
> Perhaps not entirely unexpected the work there has turned up a few
> further items which want dealing with. Most if not all of these
> aren't interdependent, so could probably be looked at (and go in)
> in (about) any order.

Shadow parts Acked-by: Tim Deegan <tim@xen.org>

Cheers,

Tim.

Re: [PATCH 7/8] x86/mm: adjust type check around _get_page_type()'s TLB flush

[Jan Beulich]

On 27.07.2022 20:25, Andrew Cooper wrote:
> On 26/07/2022 17:06, Jan Beulich wrote:
>> While "type" can include PGT_pae_xen_l2, "x" can't, as the bit is
>> cleared upon de-validation (see also the respective assertion earlier in
>> the function).
> 
> While this statement is true, it doesn't really explain why this is
> relevant (or not) to TLB flushing.
> 
> As far as the change goes, it's safe on 64bit builds of Xen (I think),
> but would not be on 32bit builds when this logic was first written.

Actually no, I don't think it's safe, and I therefore withdraw the
patch. (I'll re-base the subsequent one accordingly, which you did
give R-b for already.) Whatever content may have been in an L2 table
which is to become a PGT_pae_xen_l2 one may still be in TLBs / paging
structure caches, yet we'll replace those entries without further
flushing.

The opposite direction (previously PGT_pae_xen_l2 trying to become an
ordinary L2) is "fine": Due to the Xen entries still there, its
validation will simply fail.

Jan