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 11:47 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 12.11.2026 11:49, George Dunlap wrote:
> > 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.
>
> Right, but is this any worse than the situation without either patch
> applied?

Yes?  To repeat myself: before patch 2, the bug in patch 1 always
gives you a clean failure.  After patch 2, the bug in patch 1 usually
results in non-clean failure, and may result in silent corruption
instead.

 -George