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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.11.2026 12:05, George Dunlap wrote:
> 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.

Yet there's still the theoretical possibility of there being some other
mapping. In such a case imo we'd better avoid the freeing, accepting
the leak (with a log message perhaps).

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

Did I? (I don't seem to recall.)

Jan