Re: [PATCH for-4.15 1/4] xen/iommu: Check if the IOMMU was initialized before tearing down

[Julien Grall <julien@xxxxxxx>]

Hi Paul,

On 04/01/2021 09:28, Paul Durrant wrote:
> > -----Original Message-----
> > From: Julien Grall <julien@xxxxxxx>
> > Sent: 22 December 2020 15:44
> > To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: hongyxia@xxxxxxxxxxxx; Julien Grall <jgrall@xxxxxxxxxx>; Jan Beulich 
> > <jbeulich@xxxxxxxx>; Paul
> > Durrant <paul@xxxxxxx>
> > Subject: [PATCH for-4.15 1/4] xen/iommu: Check if the IOMMU was initialized 
> > before tearing down
> >
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > is_iommu_enabled() will return true even if the IOMMU has not been
> > initialized (e.g. the ops are not set).
> >
> > In the case of an early failure in arch_domain_init(), the function
> > iommu_destroy_domain() will be called even if the IOMMU is initialized.
> >
> > This will result to dereference the ops which will be NULL and an host
> > crash.
> >
> > Fix the issue by checking that ops has been set before accessing it.
> > Note that we are assuming that arch_iommu_domain_init() will cleanup an
> > intermediate failure if it failed.
> >
> > Fixes: 71e617a6b8f6 ("use is_iommu_enabled() where appropriate...")
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> > ---
> >   xen/drivers/passthrough/iommu.c | 10 +++++++++-
> >   1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> > index 2358b6eb09f4..f976d5a0b0a5 100644
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -226,7 +226,15 @@ static void iommu_teardown(struct domain *d)
> >
> >   void iommu_domain_destroy(struct domain *d)
> >   {
> > -    if ( !is_iommu_enabled(d) )
> > +    struct domain_iommu *hd = dom_iommu(d);
> > +
> > +    /*
> > +     * In case of failure during the domain construction, it would be
> > +     * possible to reach this function with the IOMMU enabled but not
> > +     * yet initialized. We assume that hd->platforms will be non-NULL as
> > +     * soon as we start to initialize the IOMMU.
> > +     */
> > +    if ( !is_iommu_enabled(d) || !hd->platform_ops )
> >           return;
> TBH, this seems like the wrong way to fix it. The ops dereference is done in 
> iommu_teardown() so that ought to be doing the check,
> but given it is single line function then it could be inlined at the same time. 
> That said, I think arch_iommu_domain_destroy() needs
> to be call unconditionally as it arch_iommu_domain_init() is called before the 
> ops pointer is set.
I originally resolved this way to avoid arch_iommu_domain_init() and 
iommu_teardown() to be called when the failure may happen before 
iommu_domain_init() is called. IOW, the lists/locks would be unitialized.
After discussing with Jan, it turns out that we could check whether the 
list was initialized or not. So I have reworked the code to now call 
arch_iommu_domain_init() unconditionally and move the ops check in 
iommu_teardown().
Cheers,

--
Julien Grall