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

 -George