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

[George Dunlap <gwd@xxxxxxxxxxxxxx>]

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.

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

 -George