|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [PATCH v2 2/2] 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. 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".
Tested on an Intel box (debug build, PV dom0), with restore's steps
replayed from dom0 in the order libxenguest takes them without
"tools/libs/guest: re-read dominfo after changing a PV guest's width on
restore" (no 32-bit PV guest here can suspend). Once the domain's
memory is populated, the replaced page comes back as its pfn 0 every
time, and maps. With a mapping of the old page held across the switch,
the page is leaked with the new message, and the unmap is clean.
---
xen/arch/x86/include/asm/domain.h | 3 +++
xen/arch/x86/pv/domain.c | 43 ++++++++++++++++++++++++++-----
2 files changed, 40 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/include/asm/domain.h
b/xen/arch/x86/include/asm/domain.h
index 634d0710bf..906078641c 100644
--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -302,6 +302,9 @@ struct pv_domain
/* Maximum physical-address bitwidth supported by this guest. */
unsigned int physaddr_bitsize;
+
+ /* shared_info page switch_compat() replaced, until it is freed. */
+ void *old_shared_info;
#endif
/* map_domain_page() mapping cache. */
diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
index 0c42ae58aa..3bd6b6f6d7 100644
--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -231,6 +231,14 @@ unsigned long pv_make_cr4(const struct vcpu *v)
}
#ifdef CONFIG_PV32
+static void cf_check _free_old_shared_info(struct rcu_head *head)
+{
+ struct domain *d = container_of(head, struct domain, rcu);
+
+ FREE_XENHEAP_PAGE(d->arch.pv.old_shared_info);
+ put_domain(d);
+}
+
int switch_compat(struct domain *d)
{
struct vcpu *v;
@@ -256,16 +264,17 @@ int switch_compat(struct domain *d)
if ( virt_to_maddr(d->shared_info) >> 32 )
{
shared_info_t *prev = d->shared_info;
+ shared_info_t *next = alloc_xenheap_pages(0, MEMF_bits(32));
+ struct page_info *prev_pg = virt_to_page(prev);
- d->shared_info = alloc_xenheap_pages(0, MEMF_bits(32));
- if ( !d->shared_info )
+ if ( !next )
{
- d->shared_info = prev;
rc = -ENOMEM;
goto undo_and_fail;
}
- clear_page(d->shared_info);
- share_xen_page_with_guest(virt_to_page(d->shared_info), d, SHARE_rw);
+ clear_page(next);
+ share_xen_page_with_guest(virt_to_page(next), d, SHARE_rw);
+ d->shared_info = next;
/*
* Ensure all pointers to the old shared_info page are replaced. vCPUs
* below XEN_LEGACY_MAX_VCPUS may have stashed a pointer to
@@ -273,7 +282,29 @@ int switch_compat(struct domain *d)
*/
for_each_vcpu ( d, v )
vcpu_info_reset(v);
- put_page(virt_to_page(prev));
+
+ /*
+ * Drop the allocation reference, which takes the page off the
+ * domain's xenheap list unless something else still refers to it.
+ * If something does, leak the page rather than free it while it is
+ * in use. Otherwise free it once an RCU grace period has passed:
+ * do_settime() may still be writing through a pointer to it. The
+ * reference taken on the domain keeps it alive until then, and
+ * keeps domain_destroy(), the only other user of d->rcu, from
+ * queueing it while the callback is pending.
+ */
+ put_page_alloc_ref(prev_pg);
+ if ( !(prev_pg->count_info & PGC_count_mask) )
+ {
+ ASSERT(!d->arch.pv.old_shared_info);
+ d->arch.pv.old_shared_info = prev;
+ get_knownalive_domain(d);
+ call_rcu(&d->rcu, _free_old_shared_info);
+ }
+ else
+ printk(XENLOG_WARNING
+ "%pd: leaking replaced shared_info page %"PRI_mfn": still
in use\n",
+ d, mfn_x(page_to_mfn(prev_pg)));
}
for_each_vcpu( d, v )
--
2.56.0
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |