x86/pv: free the shared_info page switch_compat() replaces

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.

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 <[email protected]>

Quoted 44 lines · George Dunlap

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.

Quoted 13 lines

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

Quoted 1 line

Suggested-by Jan Beulich <[email protected]>

Did I? (I don't seem to recall.)

Quoted 63 lines · Jan Beulich

Quoted 44 lines · George Dunlap

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.

Quoted 13 lines

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]

Quoted 3 lines

Quoted 1 line

Suggested-by Jan Beulich <[email protected]>

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

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.

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.

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

Quoted 5 lines

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

Quoted 4 lines · Jan Beulich

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.

Quoted 10 lines

Quoted 5 lines

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.

Quoted 5 lines

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.