[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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

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 )

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


Julien Grall



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.