[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] xen/arm: p2m: Populate pages for GICv2 mapping in arch_domain_create()
Hi Andrew,
On 17/10/2022 22:50, Andrew Cooper wrote:
On 17/10/2022 21:36, Julien Grall wrote:
Hi Andrew,
On 17/10/2022 20:12, Andrew Cooper wrote:
From: Henry Wang <Henry.Wang@xxxxxxx>
The XSA-409 fixes discovered that the GICv2 path tries to create P2M
mappings
in the domain_create() path. This fails, as the P2M pool is empty
before a
XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION hypercall.
As a stopgap, automatically give domains 16 pages of P2M memory.
This is
large enough to allow the GICv2 case to work, but small enough to not
introduce a continuation worry.
A consequence is that, for later error paths domain_create(), we end
up in
p2m_final_teardown() with a nonzero P2M pool. Such a domain has no
vCPUs, and
has never been scheduled, so free the memory directly.
Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the
P2M pool")
Suggested-by: Julien Grall <jgrall@xxxxxxxxxx>
This is not really in the spirit of my original suggestion anymore
Ok, I have dropped it.
... In fact, you drop all the explanations regarding how the code is
fragile (e.g. we are relying on early mapping to not take any extra
reference). Maybe you don't care, but I do as Henry and I spent ages
to figure out all the corner cases.
I presume you're referring to the todo? If so, that's an statement, not
an explanation of what is suddenly different about it.
What has XSA-409 changed in this regard? Because it looks like the
answer is nothing and the GICv2 path was similarly fragile beforehand.
In which case, why it is appropriate content for a security patch?
This is explaining why the current logic (and the one you add) is still
OK. It is not entirely related to XSA-409, but relevant to the fix
itself (and why the issue is now "properly" closed).
Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
CC: Julien Grall <julien@xxxxxxx>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
CC: Henry Wang <Henry.Wang@xxxxxxx>
---
xen/arch/arm/p2m.c | 43 +++++++++++++++++++++++++++++++++++++++++--
1 file changed, 41 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 6826f6315080..76a0e31c6c8c 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1736,8 +1736,36 @@ void p2m_final_teardown(struct domain *d)
if ( !p2m->domain )
return;
- ASSERT(page_list_empty(&p2m->pages));
- ASSERT(page_list_empty(&d->arch.paging.p2m_freelist));
+ /*
+ * On the domain_create() error path only, we can end up here
with a
+ * non-zero P2M pool.
+ *
+ * At present, this is a maximum of 16 pages, spread between
p2m->pages
+ * and the free list. The domain has never been scheduled (it
has no
+ * vcpus), so there is TLB maintenance to perform; just free
everything.
+ */
+ if ( !page_list_empty(&p2m->pages) ||
+ !page_list_empty(&d->arch.paging.p2m_freelist) )
+ {
+ struct page_info *pg;
+
+ /*
+ * There's no sensible "in the domain_create() error path"
predicate,
+ * so simply sanity check that we don't have unexpected work
to do.
+ */
+ ASSERT(d->arch.paging.p2m_total_pages <= 16);
+
+ spin_lock(&d->arch.paging.lock);
+
+ while ( (pg = page_list_remove_head(&p2m->pages)) )
+ free_domheap_page(pg);
+ while ( (pg =
page_list_remove_head(&d->arch.paging.p2m_freelist)) )
+ free_domheap_page(pg);
+
+ d->arch.paging.p2m_total_pages = 0;
+
+ spin_unlock(&d->arch.paging.lock);
+ }
... you are hardcoding both p2m_teardown() and p2m_set_allocation().
IMO this is not an improvement at all. It is just making the code more
complex than necessary and lack all the explanation on the assumptions.
So while I am fine with your patch #1 (already reviewed it), there is
a better patch from Henry on the ML. So we should take his (rebased)
instead of yours.
If by better, you mean something that still has errors, then sure.
There's a really good reason why you cannot safely repurpose
p2m_teardown(). It's written expecting a fully constructed domain -
which is fine because that's how it is used. It doesn't cope safely
with an partially constructed domain.
It is not 100% clear what is the issue you are referring to as the VMID
is valid at this point. So what part would be wrong?
But if there are part of p2m_teardown() that are not safe for partially
constructed domain, then we should split the code. This would be much
better that the duplication you are proposing.
Cheers,
--
Julien Grall
|
