Re: [PATCH 1/2] iommu: Take group lock before attaching device in iommu_deferred_attach()

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

Hi Robin,


On 5/28/2024 11:36 PM, Robin Murphy wrote:
> On 2024-05-28 5:39 pm, Vasant Hegde wrote:
>> Commit 3ab657291638 ("iommu: use the __iommu_attach_device() directly for
>> deferred attach")
>> replaced iommu_attach_device() call with __iommu_attach_device(). But
>> missed to take group lock. Take lock before attaching device to domain.
> 
> Why? Nothing here is even touching the group. If we've reached a deferred attach
> then we know the domain is the default domain already initialised and set as the
> current of the device's group, and the device is already otherwise added and
> holding a reference to that group, and a driver is bound to the device in order
> to make the DMA API call we're inside, so nothing should be at risk of
> disappearing under our feet. Please clarify what purpose this locking actually
> serves.

I am sorry. I should have written better description. I was trying to see if its
safe to use  iommu_group_mutex_assert() in our driver. I did audit the iommu.c
code and saw this one place it was missed. I just looked into git history and
tried to fix it. As you explained below I should have looked into the caller of
this function in detail!

> 
> But then there's also the fact that the initial DMA mapping operation which
> triggers deferred attach could legitimately be called under a spinlock or in an
> IRQ handler, so if anything it was a bug in 795bbbb9b6f8 ("iommu/dma-iommu:
> Handle deferred devices") to ever use iommu_attach_device() (and thus involve
> the mutex) in the first place :/

Thanks for the detailed explanation. I will drop this patch in next version.

-Vasant


> 
> Thanks,
> Robin.
> 
>> Fixes: 3ab657291638 ("iommu: use the __iommu_attach_device() directly for
>> deferred attach")
>> Cc: Lianbo Jiang <lijiang@redhat.com>
>> Cc: Robin Murphy <robin.murphy@arm.com>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>   drivers/iommu/iommu.c | 15 ++++++++++++---
>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 9df7cc75c1bc..1743dba023b6 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -2114,10 +2114,19 @@ EXPORT_SYMBOL_GPL(iommu_attach_device);
>>     int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
>>   {
>> -    if (dev->iommu && dev->iommu->attach_deferred)
>> -        return __iommu_attach_device(domain, dev);
>> +    int ret = 0;
>> +    struct iommu_group *group = dev->iommu_group;
>>   -    return 0;
>> +    if (!group)
>> +        return -EINVAL;
>> +
>> +    if (dev->iommu && dev->iommu->attach_deferred) {
>> +        mutex_lock(&group->mutex);
>> +        ret = __iommu_attach_device(domain, dev);
>> +        mutex_unlock(&group->mutex);
>> +    }
>> +
>> +    return ret;
>>   }
>>     void iommu_detach_device(struct iommu_domain *domain, struct device *dev)