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

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

Jason,


On 6/10/2024 11:14 PM, Jason Gunthorpe wrote:
> On Wed, May 29, 2024 at 12:54:14PM +0530, Vasant Hegde wrote:
>> 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. 
> 
> The core code guarentees that the dev will not have racing
> attach_dev() ops, and we added the iommu_group_mutex_assert() as a way
> for drivers to document they are making use of that assumption.
> 
> I'm guessing the purpose of this patch is to allow the AMD driver to
> use iommu_group_mutex_assert() as it will fail in the deferred attach
> path.

Right. That's what I was thinking.

> 
>>> 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 :/
> 
> ops->attach_dev() must be called in a sleepable context, even
> the Intel driver will hit a GFP_KERNEL allocation in its attach_dev()
> op. It seems to be an issue in 795bbbb9b6f8 that it did not consider
> this.
> 
> I suppose in practice kdump using drivers are setting up DMA during
> their probe functions and don't get into this problem.

Looking into code path and AMD driver code, it does look like in kdump path
deferred attach gets called.

> 
> IMHO adding the mutex here is an appropriate thing, it should not be
> closing any race, but it is the right locking documentation to make
> iommu_group_mutex_assert() work.

It may be good thing to add mutex here.. But if any of the caller of these
deferred functions hold spin lock again we will hit the similar issue that I hit
with AMD driver.


-Vasant