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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.11.2026 13:24, George Dunlap wrote:
> On Tue, Nov 10, 2026 at 11:33 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 10.11.2026 12:05, 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.
>>
>> Yet there's still the theoretical possibility of there being some other
>> mapping. In such a case imo we'd better avoid the freeing, accepting
>> the leak (with a log message perhaps).
> 
> [snip]
> 
>>> Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> Did I? (I don't seem to recall.)
> 
> I sent an earlier version of the patch to the security team due to the
> theoretical underflow (which was in the end deemed not a security
> issue); that version kept the page and freed it when the domain was
> destroyed.  In response, you wrote:
> 
> 8<---
> 
> As to the patch: I understand the need for deferred freeing of the
> page, but does it really need deferring as much? Wouldn't RCU-based
> freeing suffice (there can't be any other references to this page
> without the domain having been launched, or without the domain
> having at least some memory assigned to it, so it could have page
> tables referencing the page)?
> 
> --->8

Oh, I see.

> Which, naturally, required me to become familiar with how Xen's RCU
> system worked enough to get it implemented.  (Claude did the writing,
> but I need to understand things well enough to direct it and review
> the results.)  I don't think it's unreasonable for me to be annoyed
> that you're now saying you want it back the way I wrote it originally.

But I didn't ask for that. Now that I read your reply, I think I see
how you may have extended my reply to arrive there, though.

What I asked for here is that we free the page if its refcount is zero
(the common case), exactly as you do (via RCU). Yet we would skip the
freeing (and leak the page) in the unexpected case of the refcount not
being zero after the put_page_alloc_ref().

> Now that we have the RCU version implemented and tested, I think we
> should keep it.  It's safe now, and we can keep it safe in the future.
> If you want we could add a comment to the field warning that no
> references should be taken to it because it may change; but honestly
> that seems like overkill to me.

For the field I'm not concerned - it's purpose is clear, and anyone
trying to add obtaining of a ref using it as input will hopefully be
caught the latest during review. It's a ref existing ahead of the
put_page_alloc_ref() which worries me (some).

Jan