Re: [PATCH rc 1/2] iommu/amd: Put list_add/del(dev_data) back under the domain->lock

[Vasant Hegde <vasant.hegde@amd.com>]

On 12/6/2024 5:43 AM, Jason Gunthorpe wrote:
> The list domain->dev_list is protected by the domain->lock spinlock.
> Any iteration, addition or removal must be under the lock.
> 
> Move the list_del() up into the critical section. pdom_is_sva_capable(),
> and destroy_gcr3_table() do not interact with the list element.
> 
> Wrap the list_add() in a lock, it would make more sense if this was under
> the same critical section as adjusting the refcounts earlier, but that
> requires more complications.
> 
> Fixes: d6b47dec3684 ("iommu/amd: Reduce domain lock scope in attach device path")
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Thanks for the fix. Patch looks good.

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>


-Vasant


> ---
>  drivers/iommu/amd/iommu.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 3f691e1fd22ce4..23a168e229b171 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2073,6 +2073,7 @@ static int attach_device(struct device *dev,
>  	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>  	struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
>  	struct pci_dev *pdev;
> +	unsigned long flags;
>  	int ret = 0;
>  
>  	mutex_lock(&dev_data->mutex);
> @@ -2113,7 +2114,9 @@ static int attach_device(struct device *dev,
>  
>  	/* Update data structures */
>  	dev_data->domain = domain;
> +	spin_lock_irqsave(&domain->lock, flags);
>  	list_add(&dev_data->list, &domain->dev_list);
> +	spin_unlock_irqrestore(&domain->lock, flags);
>  
>  	/* Update device table */
>  	dev_update_dte(dev_data, true);
> @@ -2160,6 +2163,7 @@ static void detach_device(struct device *dev)
>  	/* Flush IOTLB and wait for the flushes to finish */
>  	spin_lock_irqsave(&domain->lock, flags);
>  	amd_iommu_domain_flush_all(domain);
> +	list_del(&dev_data->list);
>  	spin_unlock_irqrestore(&domain->lock, flags);
>  
>  	/* Clear GCR3 table */
> @@ -2168,7 +2172,6 @@ static void detach_device(struct device *dev)
>  
>  	/* Update data structures */
>  	dev_data->domain = NULL;
> -	list_del(&dev_data->list);
>  
>  	/* decrease reference counters - needs to happen after the flushes */
>  	pdom_detach_iommu(iommu, domain);