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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan