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

[Jason Gunthorpe <jgg@ziepe.ca>]

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.

> > 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.

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.

Jason