[PATCH rc 0/2] iommu/amd: Fix locking for domain->dev_list

[PATCH rc 0/2] iommu/amd: Fix locking for domain->dev_list

[Jason Gunthorpe]

Two small patches to fix locking regressions around this list. Found by
inspection.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Jason Gunthorpe (2):
  iommu/amd: Put list_add/del(dev_data) back under the domain->lock
  iommu/amd: Add lockdep asserts for domain->dev_list

 drivers/iommu/amd/iommu.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)


base-commit: 457730bc1f55c2b29d374e10cc125506f2b3cde9
-- 
2.43.0

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

[Jason Gunthorpe]

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

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

[Vasant Hegde]

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

[PATCH rc 2/2] iommu/amd: Add lockdep asserts for domain->dev_list

[Jason Gunthorpe]

Add an assertion to all the iteration points that don't obviously
have the lock held already. These all take the locker higher in their
call chains.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/iommu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 23a168e229b171..16f40b8000d798 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1415,6 +1415,7 @@ static int domain_flush_pages_v2(struct protection_domain *pdom,
 	struct iommu_cmd cmd;
 	int ret = 0;
 
+	lockdep_assert_held(&pdom->lock);
 	list_for_each_entry(dev_data, &pdom->dev_list, list) {
 		struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
 		u16 domid = dev_data->gcr3_info.domid;
@@ -1464,6 +1465,8 @@ static void __domain_flush_pages(struct protection_domain *domain,
 	ioasid_t pasid = IOMMU_NO_PASID;
 	bool gn = false;
 
+	lockdep_assert_held(&domain->lock);
+
 	if (pdom_is_v2_pgtbl_mode(domain)) {
 		gn = true;
 		ret = domain_flush_pages_v2(domain, address, size);
@@ -1585,6 +1588,8 @@ void amd_iommu_update_and_flush_device_table(struct protection_domain *domain)
 {
 	struct iommu_dev_data *dev_data;
 
+	lockdep_assert_held(&domain->lock);
+
 	list_for_each_entry(dev_data, &domain->dev_list, list) {
 		struct amd_iommu *iommu = rlookup_amd_iommu(dev_data->dev);
 
-- 
2.43.0

Re: [PATCH rc 2/2] iommu/amd: Add lockdep asserts for domain->dev_list

[Vasant Hegde]

On 12/6/2024 5:43 AM, Jason Gunthorpe wrote:
> Add an assertion to all the iteration points that don't obviously
> have the lock held already. These all take the locker higher in their
> call chains.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Thanks for the patch.

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

-Vasant

Re: [PATCH rc 0/2] iommu/amd: Fix locking for domain->dev_list

[Joerg Roedel]

On Thu, Dec 05, 2024 at 08:13:40PM -0400, Jason Gunthorpe wrote:
> Two small patches to fix locking regressions around this list. Found by
> inspection.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> Jason Gunthorpe (2):
>   iommu/amd: Put list_add/del(dev_data) back under the domain->lock
>   iommu/amd: Add lockdep asserts for domain->dev_list

Applied both to fixes branch, thanks.