1. Patchew
  2. linux
  3. [PATCH v3 0/5] Disable ATS via iommu during PCI resets
  4. View patch

[PATCH v3 1/5] iommu: Lock group->mutex in iommu_deferred_attach

Nicolin Chen posted 5 patches 1 month, 3 weeks ago
There is a newer version of this series
[PATCH v3 1/5] iommu: Lock group->mutex in iommu_deferred_attach
Posted by Nicolin Chen 1 month, 3 weeks ago 
The iommu_deferred_attach() is a runtime asynchronous function called by
iommu-dma function, which could race against other attach functions if it
accesses something in the dev->iommu_group.

So, grab the mutex to guard __iommu_attach_device() like other callers.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommu.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 060ebe330ee16..1e0116bce0762 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2144,10 +2144,17 @@ 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);
+	/*
+	 * This is called on the dma mapping fast path so avoid locking. This is
+	 * racy, but we have an expectation that the driver will setup its DMAs
+	 * inside probe while being single threaded to avoid racing.
+	 */
+	if (!dev->iommu || !dev->iommu->attach_deferred)
+		return 0;
 
-	return 0;
+	guard(mutex)(&dev->iommu_group->mutex);
+
+	return __iommu_attach_device(domain, dev);
 }
 
 void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
-- 
2.43.0
RE: [PATCH v3 1/5] iommu: Lock group->mutex in iommu_deferred_attach
Posted by Tian, Kevin 1 month, 2 weeks ago 
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Tuesday, August 12, 2025 6:59 AM
> 
> The iommu_deferred_attach() is a runtime asynchronous function called by
> iommu-dma function, which could race against other attach functions if it
> accesses something in the dev->iommu_group.

Is there a real racing scenario being observed or more theoretical?

If the former may need a Fix tag. 

> 
> So, grab the mutex to guard __iommu_attach_device() like other callers.
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/iommu.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 060ebe330ee16..1e0116bce0762 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2144,10 +2144,17 @@ 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);
> +	/*
> +	 * This is called on the dma mapping fast path so avoid locking. This is
> +	 * racy, but we have an expectation that the driver will setup its
> DMAs
> +	 * inside probe while being single threaded to avoid racing.
> +	 */
> +	if (!dev->iommu || !dev->iommu->attach_deferred)
> +		return 0;

Is there any way to detect a driver breaking the expectation?

> 
> -	return 0;
> +	guard(mutex)(&dev->iommu_group->mutex);
> +
> +	return __iommu_attach_device(domain, dev);
>  }
> 
>  void iommu_detach_device(struct iommu_domain *domain, struct device
> *dev)
> --
> 2.43.0
Re: [PATCH v3 1/5] iommu: Lock group->mutex in iommu_deferred_attach
Posted by Jason Gunthorpe 1 month, 2 weeks ago 
On Fri, Aug 15, 2025 at 08:24:57AM +0000, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Sent: Tuesday, August 12, 2025 6:59 AM
> > 
> > The iommu_deferred_attach() is a runtime asynchronous function called by
> > iommu-dma function, which could race against other attach functions if it
> > accesses something in the dev->iommu_group.
> 
> Is there a real racing scenario being observed or more theoretical?

I think the commit message should explain the actual reason this is
being done, which AFAICT because the new lockdeps added in following
patches will fail on this path otherwise.

Jason
Re: [PATCH v3 1/5] iommu: Lock group->mutex in iommu_deferred_attach
Posted by Nicolin Chen 1 month, 2 weeks ago 
On Mon, Aug 18, 2025 at 11:17:51AM -0300, Jason Gunthorpe wrote:
> On Fri, Aug 15, 2025 at 08:24:57AM +0000, Tian, Kevin wrote:
> > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > Sent: Tuesday, August 12, 2025 6:59 AM
> > > 
> > > The iommu_deferred_attach() is a runtime asynchronous function called by
> > > iommu-dma function, which could race against other attach functions if it
> > > accesses something in the dev->iommu_group.
> > 
> > Is there a real racing scenario being observed or more theoretical?
> 
> I think the commit message should explain the actual reason this is
> being done, which AFAICT because the new lockdeps added in following
> patches will fail on this path otherwise.

Hmm, I can mention that. But I think that's just a part of the
reason. It still doesn't seem correct to invoke an attach_dev
function without the lock since iommu_group_mutex_assert() may
be used in the path?

Thanks
Nicolin
Re: [PATCH v3 1/5] iommu: Lock group->mutex in iommu_deferred_attach
Posted by Jason Gunthorpe 1 month, 2 weeks ago 
On Mon, Aug 18, 2025 at 10:45:07AM -0700, Nicolin Chen wrote:
> On Mon, Aug 18, 2025 at 11:17:51AM -0300, Jason Gunthorpe wrote:
> > On Fri, Aug 15, 2025 at 08:24:57AM +0000, Tian, Kevin wrote:
> > > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > > Sent: Tuesday, August 12, 2025 6:59 AM
> > > > 
> > > > The iommu_deferred_attach() is a runtime asynchronous function called by
> > > > iommu-dma function, which could race against other attach functions if it
> > > > accesses something in the dev->iommu_group.
> > > 
> > > Is there a real racing scenario being observed or more theoretical?
> > 
> > I think the commit message should explain the actual reason this is
> > being done, which AFAICT because the new lockdeps added in following
> > patches will fail on this path otherwise.
> 
> Hmm, I can mention that. But I think that's just a part of the
> reason. It still doesn't seem correct to invoke an attach_dev
> function without the lock since iommu_group_mutex_assert() may
> be used in the path?

Last time this was brought up there was a bit of an argument that it
couldn't happen in parallel with anything anyhow so it doesn't
technically need locking. But I think we should not make such
arguments and be strict about our locking. It is too hard to
understand the system correctness otherwise.

Jason
Re: [PATCH v3 1/5] iommu: Lock group->mutex in iommu_deferred_attach
Posted by Nicolin Chen 1 month, 2 weeks ago 
On Mon, Aug 18, 2025 at 03:09:20PM -0300, Jason Gunthorpe wrote:
> On Mon, Aug 18, 2025 at 10:45:07AM -0700, Nicolin Chen wrote:
> > On Mon, Aug 18, 2025 at 11:17:51AM -0300, Jason Gunthorpe wrote:
> > > On Fri, Aug 15, 2025 at 08:24:57AM +0000, Tian, Kevin wrote:
> > > > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > > > Sent: Tuesday, August 12, 2025 6:59 AM
> > > > > 
> > > > > The iommu_deferred_attach() is a runtime asynchronous function called by
> > > > > iommu-dma function, which could race against other attach functions if it
> > > > > accesses something in the dev->iommu_group.
> > > > 
> > > > Is there a real racing scenario being observed or more theoretical?
> > > 
> > > I think the commit message should explain the actual reason this is
> > > being done, which AFAICT because the new lockdeps added in following
> > > patches will fail on this path otherwise.
> > 
> > Hmm, I can mention that. But I think that's just a part of the
> > reason. It still doesn't seem correct to invoke an attach_dev
> > function without the lock since iommu_group_mutex_assert() may
> > be used in the path?
> 
> Last time this was brought up there was a bit of an argument that it
> couldn't happen in parallel with anything anyhow so it doesn't
> technically need locking. But I think we should not make such
> arguments and be strict about our locking. It is too hard to
> understand the system correctness otherwise.

Yes. I will make the commit message clear that it isn't about a
bug fix, but to align with the locking policy at the attach_dev
callback and (more importantly) to fence gdev for the new flag.

Thanks
Nicolin
Re: [PATCH v3 1/5] iommu: Lock group->mutex in iommu_deferred_attach
Posted by Nicolin Chen 1 month, 2 weeks ago 
On Fri, Aug 15, 2025 at 08:24:57AM +0000, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Sent: Tuesday, August 12, 2025 6:59 AM
> > 
> > The iommu_deferred_attach() is a runtime asynchronous function called by
> > iommu-dma function, which could race against other attach functions if it
> > accesses something in the dev->iommu_group.
> 
> Is there a real racing scenario being observed or more theoretical?
> 
> If the former may need a Fix tag. 

Theoretical. I will highlight that in next version.

> >  int iommu_deferred_attach(struct device *dev, struct iommu_domain
> > *domain)
> >  {
> > -	if (dev->iommu && dev->iommu->attach_deferred)
> > -		return __iommu_attach_device(domain, dev);
> > +	/*
> > +	 * This is called on the dma mapping fast path so avoid locking. This is
> > +	 * racy, but we have an expectation that the driver will setup its
> > DMAs
> > +	 * inside probe while being single threaded to avoid racing.
> > +	 */
> > +	if (!dev->iommu || !dev->iommu->attach_deferred)
> > +		return 0;
> 
> Is there any way to detect a driver breaking the expectation?

Hmm, I am not sure.. that would sound tricky if we really want to
have a 2nd flag to protect this one..

And this patch doesn't change the race situation, if it does ever
exist..

If we really want to play safe, moving the flag under the lock as
Baolu's suggestion is probably the answer?

Thanks
Nicolin
Re: [PATCH v3 1/5] iommu: Lock group->mutex in iommu_deferred_attach
Posted by Baolu Lu 1 month, 2 weeks ago 
On 8/12/25 06:59, Nicolin Chen wrote:
> The iommu_deferred_attach() is a runtime asynchronous function called by
> iommu-dma function, which could race against other attach functions if it
> accesses something in the dev->iommu_group.
> 
> So, grab the mutex to guard __iommu_attach_device() like other callers.
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>   drivers/iommu/iommu.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 060ebe330ee16..1e0116bce0762 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2144,10 +2144,17 @@ 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);
> +	/*
> +	 * This is called on the dma mapping fast path so avoid locking. This is
> +	 * racy, but we have an expectation that the driver will setup its DMAs

Why not making it like this,

int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
{
         /* Caller must be a probed driver on dev */
         if (!dev->iommu_group)
                 return 0;

         guard(mutex)(&dev->iommu_group->mutex);
         if (!dev->iommu->attach_deferred)
                 return 0;

         return __iommu_attach_device(domain, dev);
}

?

> +	 * inside probe while being single threaded to avoid racing.
> +	 */
> +	if (!dev->iommu || !dev->iommu->attach_deferred)
> +		return 0;
>   
> -	return 0;
> +	guard(mutex)(&dev->iommu_group->mutex);
> +
> +	return __iommu_attach_device(domain, dev);
>   }
>   
>   void iommu_detach_device(struct iommu_domain *domain, struct device *dev)

Thanks,
baolu
RE: [PATCH v3 1/5] iommu: Lock group->mutex in iommu_deferred_attach
Posted by Tian, Kevin 1 month, 2 weeks ago 
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Friday, August 15, 2025 1:10 PM
> 
> On 8/12/25 06:59, Nicolin Chen wrote:
> >
> > +	/*
> > +	 * This is called on the dma mapping fast path so avoid locking. This is
> > +	 * racy, but we have an expectation that the driver will setup its
> DMAs
> 
> Why not making it like this,
> 
> int iommu_deferred_attach(struct device *dev, struct iommu_domain
> *domain)
> {
>          /* Caller must be a probed driver on dev */
>          if (!dev->iommu_group)
>                  return 0;
> 
>          guard(mutex)(&dev->iommu_group->mutex);
>          if (!dev->iommu->attach_deferred)
>                  return 0;
> 
>          return __iommu_attach_device(domain, dev);
> }
> 

As the comment said it's the fast path so avoid locking. your way
implies that every map call needs to acquire the group mutex.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.