[PATCH] x86/pv: free the shared_info page switch_compat() replaces

[George Dunlap <gwd@xxxxxxxxxxxxxx>]

For a 32-bit PV guest, switch_compat() replaces a shared_info page
above 4GiB with one below, and drops the old page with a bare
put_page().

Unfortunately, this violates the expected
xenheap-page-shared-with-guest freeing discipline.  Xenheap pages
don't start with a reference count, and aren't put back into the
allocation pool after their reference count drops to zero; they're
removed from the domain's xenheap list, but someone needs to
explicitly call free_xenheap_page() on them for them to return to
their pool.

d->shared_info here only has a non-zero reference count because it was
shared with a guest; and the thing that holds that reference is the
PGC_allocated bit.  Dropping the reference without having been the one
to clear the PGC_allocated bit puts the page in an invalid state.

If there are no outstanding references to d->shared_info when
switch_compat() happens, the result for pages moved this way is merely
a memory leak: the page is removed from the xenheap list, and nothing
ever calls free_xenheap_page() on it.  So whenever this path is
triggered, a page is leaked.

Theoretically, if there *is* an outstanding reference to
d->shared_info, *and* that reference is still held when the domain is
destroyed, then the result is a reference count underflow: Because the
reference count never drops to zero, the page stays on the xenheap
list; then during domain destruction, relinquish_memory() walks the
list, sees the PGC_allocated bit, and calls put_page() again --
potentially dropping the page count to zero.  When the other reference
holder drops their reference, the count underflows.  (put_page() only
ASSERT()s that the count is non-zero, so this wouldn't cause a crash
in a release build; and the page is never freed, so other potential
hazards of general ref count underflow are avoided.)  Fortunately this
is impossible with the in-tree toolstack, which only switches a domain
while building or restoring it, and doesn't have a mapping of
shared_info that it holds across that call.

Release the page the way domain destruction releases d->shared_info:
drop the allocation reference with put_page_alloc_ref(), as
relinquish_memory() does, which clears PGC_allocated and takes the page
off the xenheap list once nothing else refers to it, and free it with
free_xenheap_page() once an RCU grace period has passed.

The free waits for the grace period because do_settime() writes every
domain's wallclock holding only wc_lock and the domain list's RCU read
lock, neither of which excludes switch_compat(), through a pointer it
may have read before the switch.  The RCU callback uses the domain's
own RCU head, d->rcu, and holds a reference to the domain.  That
reference keeps the domain from being destroyed, and its struct freed,
before the callback has run.  It also keeps d->rcu's only other user,
domain_destroy(), from queueing the head in the meantime; if the
callback drops the last reference, domain_destroy() queues d->rcu
again, which RCU has finished with by then.  No other reference to the
page is expected: with no memory, the domain has no page tables that
could map it, and the in-tree toolstack holds no mapping of it across
the switch.

Also allocate the new page into a local variable rather than straight
into d->shared_info: if the allocation failed, a concurrent
do_settime() could dereference the NULL pointer before the old value
was restored.

Fixes: 8a9255405ca6 ("x86/domain: adjust limitation on shared_info allocation 
below 4G")
Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
Assisted-by: Claude Code:claude-opus-5-5
Signed-off-by: George Dunlap <gwd@xxxxxxxxxxxxxx>
---
 xen/arch/x86/include/asm/domain.h |  3 +++
 xen/arch/x86/pv/domain.c          | 33 +++++++++++++++++++++++++------
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/include/asm/domain.h 
b/xen/arch/x86/include/asm/domain.h
index 634d0710bf..906078641c 100644
--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -302,6 +302,9 @@ struct pv_domain
 
     /* Maximum physical-address bitwidth supported by this guest. */
     unsigned int physaddr_bitsize;
+
+    /* shared_info page switch_compat() replaced, until it is freed. */
+    void *old_shared_info;
 #endif
 
     /* map_domain_page() mapping cache. */
diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
index 0c42ae58aa..dc481bcc07 100644
--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -231,6 +231,14 @@ unsigned long pv_make_cr4(const struct vcpu *v)
 }
 
 #ifdef CONFIG_PV32
+static void cf_check _free_old_shared_info(struct rcu_head *head)
+{
+    struct domain *d = container_of(head, struct domain, rcu);
+
+    FREE_XENHEAP_PAGE(d->arch.pv.old_shared_info);
+    put_domain(d);
+}
+
 int switch_compat(struct domain *d)
 {
     struct vcpu *v;
@@ -256,16 +264,16 @@ int switch_compat(struct domain *d)
     if ( virt_to_maddr(d->shared_info) >> 32 )
     {
         shared_info_t *prev = d->shared_info;
+        shared_info_t *next = alloc_xenheap_pages(0, MEMF_bits(32));
 
-        d->shared_info = alloc_xenheap_pages(0, MEMF_bits(32));
-        if ( !d->shared_info )
+        if ( !next )
         {
-            d->shared_info = prev;
             rc = -ENOMEM;
             goto undo_and_fail;
         }
-        clear_page(d->shared_info);
-        share_xen_page_with_guest(virt_to_page(d->shared_info), d, SHARE_rw);
+        clear_page(next);
+        share_xen_page_with_guest(virt_to_page(next), d, SHARE_rw);
+        d->shared_info = next;
         /*
          * Ensure all pointers to the old shared_info page are replaced.  vCPUs
          * below XEN_LEGACY_MAX_VCPUS may have stashed a pointer to
@@ -273,7 +281,20 @@ int switch_compat(struct domain *d)
          */
         for_each_vcpu ( d, v )
             vcpu_info_reset(v);
-        put_page(virt_to_page(prev));
+
+        /*
+         * Drop the allocation reference, which takes the page off the
+         * domain's xenheap list, and free the page once an RCU grace period
+         * has passed: do_settime() may still be writing through a pointer to
+         * it.  The reference taken on the domain keeps it alive until then,
+         * and keeps domain_destroy(), the only other user of d->rcu, from
+         * queueing it while the callback is pending.
+         */
+        put_page_alloc_ref(virt_to_page(prev));
+        ASSERT(!d->arch.pv.old_shared_info);
+        d->arch.pv.old_shared_info = prev;
+        get_knownalive_domain(d);
+        call_rcu(&d->rcu, _free_old_shared_info);
     }
 
     for_each_vcpu( d, v )
-- 
2.56.0