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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

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

No question that applying in-order will yield a good result. The question
is whether I need to wait with putting in patch 2 until patch 1 has been
acked.

Jan