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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.11.2026 14:52, George Dunlap wrote:
> On Tue, Nov 10, 2026 at 1:21 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> 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().
> 
> OK, I think I see.  Yes, I'd considered that.  I mean, to be honest,
> the whole xenheap allocation thing is janky; somehow we've taken a
> useful, well-understood primitive that should work simply and
> consistently (dropping the last ref causes a free) and added extra
> bits so that it's fragile once again.  (If shared_info were a domheap
> page, do_settime() would just grab a ref, map & access the page, then
> drop the ref, knowing the page wouldn't be freed while it held the
> ref, and *would* once it was dropped.)
> 
> Given that dropping a ref doesn't automatically / atomically free the
> page, it seems like the rule should be that you *never* grab a ref to
> a xenheap page, unless you've gone through the rest of the code and
> verified that the freeing path works correctly.
> 
> And given *that*, since nobody else is currently grabbing the ref, not
> checking the refcount should be safe.
> 
> I suppose you'll say, sure, but this is a non-hot path, and if we ever
> *do* free with an outstanding refcount it's a security issue, so why
> not be safe?  You have to draw the line on internal consistency checks
> *somewhere*; I'd leave the check off, but I can add it if you really
> want.

But is this only an _internal_ consistency check? Can't a toolstack or
DM map a domain's shared-info page (as soon as it exists)? IOW I didn't
think of the check as a consistency one at all, but rather as a guard
towards e.g. toolstack behavior changing.

>>> 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).
> 
> For clarity, I meant a comment on d->shared_info, saying not to retain
> pointers into it outside of a domain rcu_lock; and I guess now also
> adding not to grab an additional reference to it without coordinating
> free-on-last-reference properly.  But as I said, both should be a
> general rule, not something we need to add to each data structure
> affected.

Okay, looks like we agree here then that no new commentary needs adding.

Jan