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

[Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>]

On Mon, Oct 17, 2016 at 10:54:40AM +0200, Iago Abal wrote:
> 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(&device_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(&device_domain_lock, flags);
>  	}
>  	spin_unlock_irqrestore(&device_domain_lock, flags);

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);