Re: [PATCH 2/2] xen/arm: p2m: Populate pages for GICv2 mapping in arch_domain_create()

[Julien Grall <julien@xxxxxxx>]

Hi Andrew,

On 19/10/2022 22:30, Andrew Cooper wrote:
> On 18/10/2022 00:01, Julien Grall wrote:
> > > > ... 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?
> Falling over a bad root pointer from an early construction exit.
You have been mentioning that several time now but I can't see how this
can happen. If you look at Henry's second patch, p2m_teardown() starts
with the following check:
if ( page_list_empty(&p2m->pages) )
   return;

Per the logic in p2m_init(), the root pages have to be allocated (note 
they are *not* allocated from the P2M pool) and the VMID as well before 
any pages could be added in the list.
> > 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.
> You have two totally different contexts with different safety
> requirements.  c/s 55914f7fc9 is a reasonably good and clean separation
> between preemptible and non-preemptible cleanup[1].
The part you mention in [1] was decided to be delayed post 4.17 for 
development cycle reasons.
> You've agreed that the introduction of the non-preemptible path to the
> preemptible path is a hack and layering violation, and will need undoing
> later.  Others have raised this concern too.
[...]

> Also realise that you've now split the helper between regular hypercall
> context, and RCU context, and recall what happened when we finally
> started asserting that memory couldn't be allocated in stop-machine context.
>
> How certain are you that the safety is the same on earlier versions of
> Xen?
I am pretty confident because the P2M code has not changed a lot.

> What is the likelihood that all of these actions will remain safe
> given future development?
Code always evolve and neither you (nor I) can claim that any work will 
stay safe forever. In your patch proposal, then the risk is a bug could 
be duplicated.
> Despite what is being claimed, the attempt to share cleanup logic is
> introducing fragility and risk, not removing it.
I find interesting you are saying that... If we were going to move 
p2m_teardown() in domain_teardown() then we would end up to share the code.
To me, this is not very different here because in one context it would 
be preemptible while the other it won't. At which point...
>   This is a bugfix for
> to a security fix issue which is totally dead on arrival; net safety,
> especially in older versions of the Xen, is *the highest priority*.
>
> These two different contexts don't share any common properties of how to
> clean up the pool, save freeing the frames back to the memory
> allocator.  In a proper design, this is the hint that they shouldn't
> share logic either
... why is your design better than what Henry's proposed?

> Given that you do expect someone to spend yet-more time&effort to undo
> the short term hack currently being proposed, how do you envisage the
> end result looking?
The end result will be p2m_teardown() & co to be called from 
domain_teardown().
Anyway, this discussion doesn't make us closer to come to an agreement 
on the correct approach. We have both very diverging opinion and so far 
I haven't seen any strong reasons that is showing yours is better.
So unless Bertrand or Stefano agree with you, then I will go ahead and 
merge Henry's patch tomorrow after a final review.
Cheers,

--
Julien Grall