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

[George Dunlap <gwd@xxxxxxxxxxxxxx>]

On Thu, Nov 12, 2026 at 9:46 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 12.11.2026 10:30, George Dunlap wrote:
> > On Thu, Nov 12, 2026 at 8:28 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>
> >> On 11.11.2026 18:14, 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.  If something holds one all the same (a toolstack that
> >>> mapped the page before the switch, for instance), leave the page alone
> >>> and leak it, rather than free a page that is still in use.
> >>>
> >>> Freeing the page relies on "tools/libs/guest: re-read dominfo after
> >>> changing a PV guest's width on restore", without which restoring a
> >>> 32-bit PV guest maps the replaced page by the frame number it read
> >>> before the switch.  Once the page is freed, that frame can come back as
> >>> one of the restored domain's own pages, which restore would then write
> >>> the saved shared_info into.
> >>>
> >>> 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>
> >>> ---
> >>> Changes in v2:
> >>> - If the page is still referenced once its allocation reference has
> >>>   been dropped, leak it rather than free it (Jan).
> >>> - Depends on a new patch, "tools/libs/guest: re-read dominfo after
> >>>   changing a PV guest's width on restore".
> >>
> >> Actually, is there a dependency here as in "must be applied in this
> >> order"? It looks to me as if the 2nd patch going in first wouldn't
> >> make the situation worse.
> >
> > Before patch 2, the stale frame will always have a refcount of 0, so
> > the map will always fail, causing the restore to fail.  (And in the
> > case of a live migrate, causing the attempted sending domain to
> > resume.)
> >
> > After patch 2, the stale frame may be in one of three states:
> >
> > a) Free, refcount of zero.  libxc's map will fail the same way,
> > failing the restore.
> >
> > b) Allocated to a different domain.  This will also fail, because it
> > will have the wrong domain, failing the restore.
> >
> > c) Allocated to the same domain.  In this case, the map will succeed:
> > libxc will restore the contents into the wrong page, and then write a
> > truncated frame into the start_info that it passes to the guest.  This
> > is a lot worse than the failed restore.
> >
> > In Claude's experiments, on an idle system, the page gets re-allocated
> > to the same domain 10/10 times.
>
> Hmm, all of this to me reads like an analysis of what happens if only
> patch 1 went in. But the question was whether putting in patch 2 without
> patch 1 would cause things to be worse than before.

I'm a bit confused.  Are we talking about the same patch 1 and 2?  If
we put in the current patch we're discussing, "[PATCH v2 2/2] x86/pv:
free the shared_info page switch_compat() replaces", without 1/2, then
we'll have the issue described above until patch 1 is applied.  1/2
fixes the stale-shared_info-on-restore problem before 2/2 enables the
old shared_info to actually be freed and reused.

I don't see how putting patch 1/2 in without patch 2/2 would cause the
issues described.

 -George