Re: [PATCH] iommu: fix double spin_lock_irqsave on `device_domain_lock'

[Iago Abal]

On Thu, Nov 3, 2016 at 9:51 PM, Joerg Roedel  wrote:
>
> No, you can't just release the lock to re-aquire it in
> dmar_remove_one_dev_info(). This introduces new races, as the list your
> are walking is no longer protected by the lock. The right solution is to
> call a variant of dmar_remove_one_dev_info() which does not take the
> lock. It turns out this function already exists, so the patch looks like
> below. Can you check if this is still correct and resubmit your patch?
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index a4407ea..3cadde2 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -1723,7 +1723,7 @@ static void disable_dmar_iommu(struct intel_iommu 
> *iommu)
>
> domain = info->domain;
>
> -   dmar_remove_one_dev_info(domain, info->dev);
> +   __dmar_remove_one_dev_info(info);
>
> if (!domain_type_is_vm_or_si(domain))
> domain_exit(domain);


That patch was actually my first attempt at fixing the problem, but I
ran the tool and I found a second possibility of deadlock:
`domain_exit' calls to `domain_remove_dev_info', which also spin_locks
on `device_domain_lock'.

Alternatively I could add another `__domain_exit' function that
doesn't take the lock.

Would that be fine?

-- iago
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH] iommu: fix double spin_lock_irqsave on `device_domain_lock'

[Iago Abal]

From: Iago Abal <m...@iagoabal.eu>

The EBA code analyzer (https://github.com/models-team/eba) reported
the following double lock:

1. In function `disable_dmar_iommu' defined at 1706;
2. the lock `device_domain_lock' is first taken in line 1714:

   // FIRST
   spin_lock_irqsave(_domain_lock, flags);

3. enter the `list_for_each_entry_safe' loop at 1715;
4. call function `dmar_remove_one_dev_info' (defined at 4851) in line 1726;
5. finally, the lock is taken a second time in line 4857:

   // SECOND: DOUBLE LOCK !!!
   spin_lock_irqsave(_domain_lock, flags);

In addition, within that same loop, there is also a call to `domain_exit', which
calls to `domain_remove_dev_info', which also spin_lock on `device_domain_lock'.

I fixed the potential deadlock by releasing the `device_domain_lock' during the
execution of the loop body. This seems to respect the locking assumptions made
by the rest of the code: both `dmar_remove_one_dev_info' and `domain_exit' will
(directly or indiretly) take that look, so they should not be called with it 
held.
Function `domain_type_is_vm_or_si' just checks `domain->flags' and there seem
to be no concurrent writes to this field.

Signed-off-by: Iago Abal <m...@iagoabal.eu>
---
 drivers/iommu/intel-iommu.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index a4407ea..05796a8 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1721,12 +1721,16 @@ static void disable_dmar_iommu(struct intel_iommu 
*iommu)
if (!info->dev || !info->domain)
continue;
 
+   spin_unlock_irqrestore(_domain_lock, flags);
+
domain = info->domain;
 
dmar_remove_one_dev_info(domain, info->dev);
 
if (!domain_type_is_vm_or_si(domain))
domain_exit(domain);
+
+   spin_lock_irqsave(_domain_lock, flags);
}
spin_unlock_irqrestore(_domain_lock, flags);
 
-- 
1.9.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Potential double spin_lock_irqsave BUG in Linux 4.7's drivers/iommu/intel-iommu.c

[Iago Abal]

Hi,

Using a static bug finder (EBA - https://github.com/models-team/eba) I
may have found a double spin_lock_irqsave bug in Linux 4.7's
drivers/iommu/intel-iommu.c.

The forward trace is as follows:

1. Starting in function `disable_dmar_iommu' defined at 1706;
   (see 
https://github.com/torvalds/linux/blob/v4.7/drivers/iommu/intel-iommu.c#L1706)
2. the lock `device_domain_lock' is first taken in line 1714:

   spin_lock_irqsave(_domain_lock, flags);

3. enter the `list_for_each_entry_safe' loop at 1715;
4. call function `dmar_remove_one_dev_info' (defined at 4850) in line 1726;
   (see 
https://github.com/torvalds/linux/blob/v4.7/drivers/iommu/intel-iommu.c#L4850)
5. finally, the lock is taken a second time in line 4856:

   spin_lock_irqsave(_domain_lock, flags);

If that's a bug then I am willing to help with a patch.

Thank you for your time,

Iago
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu