PCIe permits a device to ignore ATS invalidation TLPs, while processing a
reset. This creates a problem visible to the OS where an ATS invalidation
command will time out: e.g. an SVA domain will have no coordination with a
reset event and can racily issue ATS invalidations to a resetting device.

The OS should do something to mitigate this as we do not want production
systems to be reporting critical ATS failures, especially in a hypervisor
environment. Broadly, OS could arrange to ignore the timeouts, block page
table mutations to prevent invalidations, or disable and block ATS.

The PCIe spec in sec 10.3.1 IMPLEMENTATION NOTE recommends to disable and
block ATS before initiating a Function Level Reset. It also mentions that
other reset methods could have the same vulnerability as well.

Provide a callback from the PCI subsystem that will enclose the reset and
have the iommu core temporarily change all the attached domain to BLOCKED.
After attaching a BLOCKED domain, IOMMU drivers should fence any incoming
ATS queries, synchronously stop issuing new ATS invalidations, and wait
for all ATS invalidations to complete. This can avoid any ATS invaliation
timeouts.

However, if there is a domain attachment/replacement happening during an
ongoing reset, the ATS might be re-enabled between the two function calls.
Introduce a new pending_reset flag in group_device to defer an attachment
during a reset, allowing iommu core to cache the target domains in the SW
level but bypassing the driver. The iommu_dev_reset_done() will re-attach
these soft-attached domains via __iommu_attach_device/set_group_pasid().

Notes:
 - This only works for IOMMU drivers that implemented ops->blocked_domain
   correctly with pci_disable_ats().
 - This only works for IOMMU drivers that will not issue ATS invalidation
   requests to the device, after it's docked at ops->blocked_domain.
Driver should fix itself to align with the aforementioned notes.

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 include/linux/iommu.h |  12 ++++
 drivers/iommu/iommu.c | 158 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 170 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 156732807994..a17161b8625a 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1123,6 +1123,9 @@ void dev_iommu_priv_set(struct device *dev, void *priv);
 extern struct mutex iommu_probe_device_lock;
 int iommu_probe_device(struct device *dev);
 
+int iommu_dev_reset_prepare(struct device *dev);
+void iommu_dev_reset_done(struct device *dev);
+
 int iommu_device_use_default_domain(struct device *dev);
 void iommu_device_unuse_default_domain(struct device *dev);
 
@@ -1407,6 +1410,15 @@ static inline int iommu_fwspec_add_ids(struct device *dev, u32 *ids,
 	return -ENODEV;
 }
 
+static inline int iommu_dev_reset_prepare(struct device *dev)
+{
+	return 0;
+}
+
+static inline void iommu_dev_reset_done(struct device *dev)
+{
+}
+
 static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct device *dev)
 {
 	return NULL;
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index bd3deedcd2de..14bfeaa9ac29 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -71,12 +71,29 @@ struct group_device {
 	struct list_head list;
 	struct device *dev;
 	char *name;
+	bool pending_reset : 1;
 };
 
 /* Iterate over each struct group_device in a struct iommu_group */
 #define for_each_group_device(group, pos) \
 	list_for_each_entry(pos, &(group)->devices, list)
 
+/* Callers must hold the dev->iommu_group->mutex */
+static struct group_device *device_to_group_device(struct device *dev)
+{
+	struct iommu_group *group = dev->iommu_group;
+	struct group_device *gdev;
+
+	lockdep_assert_held(&group->mutex);
+
+	/* gdev must be in the list */
+	for_each_group_device(group, gdev) {
+		if (gdev->dev == dev)
+			break;
+	}
+	return gdev;
+}
+
 struct iommu_group_attribute {
 	struct attribute attr;
 	ssize_t (*show)(struct iommu_group *group, char *buf);
@@ -2155,8 +2172,17 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
 	int ret = 0;
 
 	mutex_lock(&group->mutex);
+
+	/*
+	 * There is a racy attach while the device is resetting. Defer it until
+	 * the iommu_dev_reset_done() that attaches the device to group->domain.
+	 */
+	if (device_to_group_device(dev)->pending_reset)
+		goto unlock;
+
 	if (dev->iommu && dev->iommu->attach_deferred)
 		ret = __iommu_attach_device(domain, dev);
+unlock:
 	mutex_unlock(&group->mutex);
 	return ret;
 }
@@ -2295,6 +2321,13 @@ static int __iommu_device_set_domain(struct iommu_group *group,
 		dev->iommu->attach_deferred = 0;
 	}
 
+	/*
+	 * There is a racy attach while the device is resetting. Defer it until
+	 * the iommu_dev_reset_done() that attaches the device to group->domain.
+	 */
+	if (gdev->pending_reset)
+		return 0;
+
 	ret = __iommu_attach_device(new_domain, dev);
 	if (ret) {
 		/*
@@ -3378,6 +3411,13 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain,
 	int ret;
 
 	for_each_group_device(group, device) {
+		/*
+		 * There is a racy attach while the device is resetting. Defer
+		 * it until the iommu_dev_reset_done() that attaches the device
+		 * to group->domain.
+		 */
+		if (device->pending_reset)
+			continue;
 		if (device->dev->iommu->max_pasids > 0) {
 			ret = domain->ops->set_dev_pasid(domain, device->dev,
 							 pasid, old);
@@ -3799,6 +3839,124 @@ int iommu_replace_group_handle(struct iommu_group *group,
 }
 EXPORT_SYMBOL_NS_GPL(iommu_replace_group_handle, "IOMMUFD_INTERNAL");
 
+/*
+ * Caller must use iommu_dev_reset_prepare() and iommu_dev_reset_done() together
+ * before/after the core-level reset routine, to unclear the pending_reset flag
+ * and to put the iommu_group reference.
+ *
+ * These two functions are designed to be used by PCI reset functions that would
+ * not invoke any racy iommu_release_device() since PCI sysfs node gets removed
+ * before it notifies with a BUS_NOTIFY_REMOVED_DEVICE. When using them in other
+ * case, callers must ensure there will be no racy iommu_release_device() call,
+ * which otherwise would UAF the dev->iommu_group pointer.
+ */
+int iommu_dev_reset_prepare(struct device *dev)
+{
+	const struct iommu_ops *ops;
+	struct iommu_group *group;
+	unsigned long pasid;
+	void *entry;
+	int ret = 0;
+
+	if (!dev_has_iommu(dev))
+		return 0;
+
+	if (dev->iommu->require_direct) {
+		dev_warn(
+			dev,
+			"Firmware has requested this device have a 1:1 IOMMU mapping, rejecting configuring the device without a 1:1 mapping. Contact your platform vendor.\n");
+		return -EINVAL;
+	}
+
+	/* group will be put in iommu_dev_reset_done() */
+	group = iommu_group_get(dev);
+
+	/* Caller ensures no racy iommu_release_device(), so this won't UAF */
+	mutex_lock(&group->mutex);
+
+	ops = dev_iommu_ops(dev);
+	if (!ops->blocked_domain) {
+		dev_warn(dev,
+			 "IOMMU driver doesn't support IOMMU_DOMAIN_BLOCKED\n");
+		ret = -EOPNOTSUPP;
+		goto unlock;
+	}
+
+	device_to_group_device(dev)->pending_reset = true;
+
+	/* Device is already attached to the blocked_domain. Nothing to do */
+	if (group->domain->type == IOMMU_DOMAIN_BLOCKED)
+		goto unlock;
+
+	/* Dock RID domain to blocked_domain while retaining group->domain */
+	ret = __iommu_attach_device(ops->blocked_domain, dev);
+	if (ret)
+		goto unlock;
+
+	/* Dock PASID domains to blocked_domain while retaining pasid_array */
+	xa_lock(&group->pasid_array);
+	xa_for_each_start(&group->pasid_array, pasid, entry, 1)
+		iommu_remove_dev_pasid(dev, pasid,
+				       pasid_array_entry_to_domain(entry));
+	xa_unlock(&group->pasid_array);
+
+unlock:
+	mutex_unlock(&group->mutex);
+	if (ret)
+		iommu_group_put(group);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_dev_reset_prepare);
+
+/*
+ * Pair with a previous iommu_dev_reset_prepare() that was successfully returned
+ *
+ * Note that, although unlikely, there is a risk that re-attaching domains might
+ * fail due to some unexpected happening like OOM.
+ */
+void iommu_dev_reset_done(struct device *dev)
+{
+	struct iommu_group *group = dev->iommu_group;
+	const struct iommu_ops *ops;
+	struct group_device *gdev;
+	unsigned long pasid;
+	void *entry;
+
+	if (!dev_has_iommu(dev))
+		return;
+
+	mutex_lock(&group->mutex);
+
+	gdev = device_to_group_device(dev);
+
+	ops = dev_iommu_ops(dev);
+	/* iommu_dev_reset_prepare() was not successfully called */
+	if (WARN_ON(!ops->blocked_domain || !gdev->pending_reset)) {
+		mutex_unlock(&group->mutex);
+		return;
+	}
+
+	if (group->domain->type == IOMMU_DOMAIN_BLOCKED)
+		goto done;
+
+	/* Shift RID domain back to group->domain */
+	WARN_ON(__iommu_attach_device(group->domain, dev));
+
+	/* Shift PASID domains back to domains retained in pasid_array */
+	xa_lock(&group->pasid_array);
+	xa_for_each_start(&group->pasid_array, pasid, entry, 1)
+		WARN_ON(__iommu_set_group_pasid(
+			pasid_array_entry_to_domain(entry), group, pasid,
+			ops->blocked_domain));
+	xa_unlock(&group->pasid_array);
+
+done:
+	gdev->pending_reset = false;
+	mutex_unlock(&group->mutex);
+	iommu_group_put(group);
+}
+EXPORT_SYMBOL_GPL(iommu_dev_reset_done);
+
 #if IS_ENABLED(CONFIG_IRQ_MSI_IOMMU)
 /**
  * iommu_dma_prepare_msi() - Map the MSI page in the IOMMU domain
-- 
2.43.0

On Sat, Jun 28, 2025 at 12:42:41AM -0700, Nicolin Chen wrote:

>  - This only works for IOMMU drivers that implemented ops->blocked_domain
>    correctly with pci_disable_ats().

As was in the thread, it works for everyone. Even if we install an
empty paging domain for blocking that still will stop the ATS
invalidations from being issued. ATS remains on but this is not a
problem.

>  - This only works for IOMMU drivers that will not issue ATS invalidation
>    requests to the device, after it's docked at ops->blocked_domain.

Which should be everyone.. It would be broken and racy with release to
do otherwise.

> @@ -2155,8 +2172,17 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
>  	int ret = 0;
>  
>  	mutex_lock(&group->mutex);
> +
> +	/*
> +	 * There is a racy attach while the device is resetting. Defer it until
> +	 * the iommu_dev_reset_done() that attaches the device to group->domain.
> +	 */
> +	if (device_to_group_device(dev)->pending_reset)
> +		goto unlock;
> +
>  	if (dev->iommu && dev->iommu->attach_deferred)
>  		ret = __iommu_attach_device(domain, dev);
> +unlock:
>  	mutex_unlock(&group->mutex);

Actually looking at this some more maybe write it like:

/*
 * 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 still single threaded to avoid racing.
 */
if (dev->iommu && !READ_ONCE(dev->iommu->attach_deferred))
   return 0;

guard(mutex)(&group->mutex);
if (device_to_group_device(dev)->pending_reset)
    return 0;

if (!dev->iommu->attach_deferred)
   return 0;
return __iommu_attach_device(domain, dev);

And of course it is already quite crazy to be doing FLR during a
device probe so this is not a realistic scenario.

> @@ -2295,6 +2321,13 @@ static int __iommu_device_set_domain(struct iommu_group *group,
>  		dev->iommu->attach_deferred = 0;
>  	}
>  
> +	/*
> +	 * There is a racy attach while the device is resetting. Defer it until
> +	 * the iommu_dev_reset_done() that attaches the device to group->domain.

"There is a concurrent attach" here and other places

> +int iommu_dev_reset_prepare(struct device *dev)
> +{
> +	const struct iommu_ops *ops;
> +	struct iommu_group *group;
> +	unsigned long pasid;
> +	void *entry;
> +	int ret = 0;
> +
> +	if (!dev_has_iommu(dev))
> +		return 0;
> +
> +	if (dev->iommu->require_direct) {
> +		dev_warn(
> +			dev,
> +			"Firmware has requested this device have a 1:1 IOMMU mapping, rejecting configuring the device without a 1:1 mapping. Contact your platform vendor.\n");
> +		return -EINVAL;
> +	}

I don't think we can do this. eg on ARM all devices have RMRs inside
VMs so this will completely break FLR inside a vm???

Either ignore this condition with the rational that we are about to
reset it so it doesn't matter, or we need to establish a new paging
domain for isolation purposes that has the RMR setup.

> +	/* group will be put in iommu_dev_reset_done() */
> +	group = iommu_group_get(dev);

Probably don't need this. If we are already requiring no
iommu_release_device() then we can keep with that.

> +	/* Caller ensures no racy iommu_release_device(), so this won't UAF */
> +	mutex_lock(&group->mutex);

It is the group_get above that won't UAF, this is fine once we have a
refcount.

> +	ops = dev_iommu_ops(dev);
> +	if (!ops->blocked_domain) {
> +		dev_warn(dev,
> +			 "IOMMU driver doesn't support IOMMU_DOMAIN_BLOCKED\n");
> +		ret = -EOPNOTSUPP;
> +		goto unlock;

Not necessary, just use the existing flow to allocate an empty paging
domain to group->blocking_domain

> +	device_to_group_device(dev)->pending_reset = true;
> +
> +	/* Device is already attached to the blocked_domain. Nothing to do */
> +	if (group->domain->type == IOMMU_DOMAIN_BLOCKED)
> +		goto unlock;

Should be group->domain == group->blocking_domain

> +	/* Dock RID domain to blocked_domain while retaining group->domain */
> +	ret = __iommu_attach_device(ops->blocked_domain, dev);

group->blocking_domain

> +	if (ret)
> +		goto unlock;
> +
> +	/* Dock PASID domains to blocked_domain while retaining pasid_array */
> +	xa_lock(&group->pasid_array);

Not sure we need this lock? The group mutex already prevents mutation
of the xa list and I dont' think it is allowed to call
iommu_remove_dev_pasid() in an atomic context.

Jason

Sorry for a huge delay. I've addressed all, following your remarks.

Some feedbacks inline.

On Fri, Jul 04, 2025 at 12:43:42PM -0300, Jason Gunthorpe wrote:
> On Sat, Jun 28, 2025 at 12:42:41AM -0700, Nicolin Chen wrote:
> 
> >  - This only works for IOMMU drivers that implemented ops->blocked_domain
> >    correctly with pci_disable_ats().
> 
> As was in the thread, it works for everyone. Even if we install an
> empty paging domain for blocking that still will stop the ATS
> invalidations from being issued. ATS remains on but this is not a
> problem.

OK. And I am dropping this validation in the PCI patch:

	/* Something wrong with the iommu driver that failed to disable ATS */
	if (dev->ats_enabled)
		pci_err(dev, "failed to stop ATS. ATS invalidation may time out\n");

> > @@ -2155,8 +2172,17 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
> >  	int ret = 0;
> >  
> >  	mutex_lock(&group->mutex);
> > +
> > +	/*
> > +	 * There is a racy attach while the device is resetting. Defer it until
> > +	 * the iommu_dev_reset_done() that attaches the device to group->domain.
> > +	 */
> > +	if (device_to_group_device(dev)->pending_reset)
> > +		goto unlock;
> > +
> >  	if (dev->iommu && dev->iommu->attach_deferred)
> >  		ret = __iommu_attach_device(domain, dev);
> > +unlock:
> >  	mutex_unlock(&group->mutex);
> 
> Actually looking at this some more maybe write it like:
> 
> /*
>  * 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 still single threaded to avoid racing.
>  */
> if (dev->iommu && !READ_ONCE(dev->iommu->attach_deferred))

This triggers a build error as attach_deferred is a bit-field. So I
am changing it from "u32 attach_deferred:1" to "bool" for this.

And, to keep the original logic, I think it should be:
	if (!dev->iommu || !READ_ONCE(dev->iommu->attach_deferred))

>    return 0;
> 
> guard(mutex)(&group->mutex);

I recall Baolu mentioned that Joerg might not like the guard style
so I am keeping mutex_lock/unlock().

> if (device_to_group_device(dev)->pending_reset)
>     return 0;
> 
> if (!dev->iommu->attach_deferred)
>    return 0;

I think this is redundant since the fast path checked.

> return __iommu_attach_device(domain, dev);
> 
> And of course it is already quite crazy to be doing FLR during a
> device probe so this is not a realistic scenario.

Hmm, I am not sure about that, as I see iommu_deferred_attach() get
mostly invoked by a dma_alloc() or even a dma_map(). So, this might
not be confined to a device probe?

> > +	if (dev->iommu->require_direct) {
> > +		dev_warn(
> > +			dev,
> > +			"Firmware has requested this device have a 1:1 IOMMU mapping, rejecting configuring the device without a 1:1 mapping. Contact your platform vendor.\n");
> > +		return -EINVAL;
> > +	}
> 
> I don't think we can do this. eg on ARM all devices have RMRs inside
> VMs so this will completely break FLR inside a vm???
> 
> Either ignore this condition with the rational that we are about to
> reset it so it doesn't matter, or we need to establish a new paging
> domain for isolation purposes that has the RMR setup.

Ah, you are right. ARM MSI in a VM uses RMR and sets this.

But does it also raise a question that a VM having RMR can't use
the blocked_domain, as __iommu_device_set_domain() has the exact
same check rejecting blocked_domain? Not sure if there would be
some unintended consequnce though...

> > +	if (ret)
> > +		goto unlock;
> > +
> > +	/* Dock PASID domains to blocked_domain while retaining pasid_array */
> > +	xa_lock(&group->pasid_array);
> 
> Not sure we need this lock? The group mutex already prevents mutation
> of the xa list and I dont' think it is allowed to call
> iommu_remove_dev_pasid() in an atomic context.

I see only iommu_attach_handle_get() doesn't use group->mutex. And
it's a reader. So I think it's safe to drop the xa_lock.

I added this:

	/*                                                                                                                                                                                                                                                                                                                                              |||     iommu_map_sg
	 * Dock PASID domains to blocking_domain while retaining pasid_array.
	 *
	 * The pasid_array is mostly fenced by group->mutex, except one reader
	 * in iommu_attach_handle_get(), so it's safe to read without xa_lock.
	 */

Thanks!
Nicolin

On Tue, Jul 22, 2025 at 02:58:21PM -0700, 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 inside probe while still single threaded to avoid racing.
> >  */
> > if (dev->iommu && !READ_ONCE(dev->iommu->attach_deferred))
> 
> This triggers a build error as attach_deferred is a bit-field. So I
> am changing it from "u32 attach_deferred:1" to "bool" for this.

Bleck, that seems undesirable.

> And, to keep the original logic, I think it should be:
> 	if (!dev->iommu || !READ_ONCE(dev->iommu->attach_deferred))

That doesn't seem right, if there is no iommu by the time a driver is
probed there never will be an iommu and this device should be running
in direct mode only.

> > And of course it is already quite crazy to be doing FLR during a
> > device probe so this is not a realistic scenario.
> 
> Hmm, I am not sure about that, as I see iommu_deferred_attach() get
> mostly invoked by a dma_alloc() or even a dma_map(). So, this might
> not be confined to a device probe?

Once you do deferred_attach the first time it is done and won't have
any further impact. So long as the dev->iommu->attach_deferred guards
any changes to domains it is unlikely to be racing with FLR.

> > Either ignore this condition with the rational that we are about to
> > reset it so it doesn't matter, or we need to establish a new paging
> > domain for isolation purposes that has the RMR setup.
> 
> Ah, you are right. ARM MSI in a VM uses RMR and sets this.
> 
> But does it also raise a question that a VM having RMR can't use
> the blocked_domain, as __iommu_device_set_domain() has the exact
> same check rejecting blocked_domain? Not sure if there would be
> some unintended consequnce though...

Sounds like it needs some sorting out.. For the purposes of FLR I
think the blocked domain is OK, so maybe just move some of those
checks around?

Jason

On Sun, Jul 27, 2025 at 01:25:01PM -0300, Jason Gunthorpe wrote:
> On Tue, Jul 22, 2025 at 02:58:21PM -0700, 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 inside probe while still single threaded to avoid racing.
> > >  */
> > > if (dev->iommu && !READ_ONCE(dev->iommu->attach_deferred))
> > 
> > This triggers a build error as attach_deferred is a bit-field. So I
> > am changing it from "u32 attach_deferred:1" to "bool" for this.
> 
> Bleck, that seems undesirable.

But inevitable for READ_ONCE :(

> > And, to keep the original logic, I think it should be:
> > 	if (!dev->iommu || !READ_ONCE(dev->iommu->attach_deferred))
> 
> That doesn't seem right, if there is no iommu by the time a driver is
> probed there never will be an iommu and this device should be running
> in direct mode only.

Well, the current function does:
	if (dev->iommu && dev->iommu->attach_deferred)
		return __iommu_attach_device(domain, dev);
	return 0;

So, matching to that logic, it would be:
	if (!dev->iommu || !dev->iommu->attach_deferred)
		return 0;
	return __iommu_attach_device(domain, dev);
then add guard(mutex).

I do see your point. Yet, given that it is an exported function,
I think it'd be safer to have a check. Perhaps it should give a
WARN_ON(!dev->iommu).

> > > And of course it is already quite crazy to be doing FLR during a
> > > device probe so this is not a realistic scenario.
> > 
> > Hmm, I am not sure about that, as I see iommu_deferred_attach() get
> > mostly invoked by a dma_alloc() or even a dma_map(). So, this might
> > not be confined to a device probe?
> 
> Once you do deferred_attach the first time it is done and won't have
> any further impact. So long as the dev->iommu->attach_deferred guards
> any changes to domains it is unlikely to be racing with FLR.

I see. The existing callers are all in dma-iommu.c. So, we can
assume that iommu_deferred_attach() is already done, when a PCI
driver calls any function from dma-iommu.c.

> > > Either ignore this condition with the rational that we are about to
> > > reset it so it doesn't matter, or we need to establish a new paging
> > > domain for isolation purposes that has the RMR setup.
> > 
> > Ah, you are right. ARM MSI in a VM uses RMR and sets this.
> > 
> > But does it also raise a question that a VM having RMR can't use
> > the blocked_domain, as __iommu_device_set_domain() has the exact
> > same check rejecting blocked_domain? Not sure if there would be
> > some unintended consequnce though...
> 
> Sounds like it needs some sorting out.. For the purposes of FLR I
> think the blocked domain is OK, so maybe just move some of those
> checks around?

These two new APIs call the lower-level __iommu_attach_device()
that does not check require_direct. So, we are fine, so long as
we don't check it in the new API as you previously pointed out.

I'm worried about using blocked domains in general.

Thanks
Nicolin

On Mon, Jul 28, 2025 at 12:07:59PM -0700, Nicolin Chen wrote:
> On Sun, Jul 27, 2025 at 01:25:01PM -0300, Jason Gunthorpe wrote:
> > On Tue, Jul 22, 2025 at 02:58:21PM -0700, 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 inside probe while still single threaded to avoid racing.
> > > >  */
> > > > if (dev->iommu && !READ_ONCE(dev->iommu->attach_deferred))
> > > 
> > > This triggers a build error as attach_deferred is a bit-field. So I
> > > am changing it from "u32 attach_deferred:1" to "bool" for this.
> > 
> > Bleck, that seems undesirable.
> 
> But inevitable for READ_ONCE :(

I guess drop the READ_ONCE change
 
> > > And, to keep the original logic, I think it should be:
> > > 	if (!dev->iommu || !READ_ONCE(dev->iommu->attach_deferred))
> > 
> > That doesn't seem right, if there is no iommu by the time a driver is
> > probed there never will be an iommu and this device should be running
> > in direct mode only.
> 
> Well, the current function does:
> 	if (dev->iommu && dev->iommu->attach_deferred)
> 		return __iommu_attach_device(domain, dev);
> 	return 0;
> 
> So, matching to that logic, it would be:
> 	if (!dev->iommu || !dev->iommu->attach_deferred)
> 		return 0;
> 	return __iommu_attach_device(domain, dev);
> then add guard(mutex).

Yeah Ok

Jason

On 7/23/25 05:58, Nicolin Chen wrote:
>>     return 0;
>>
>> guard(mutex)(&group->mutex);
> I recall Baolu mentioned that Joerg might not like the guard style
> so I am keeping mutex_lock/unlock().

You may be misremembering or mixing something up. I didn't see Joerg
express that opinion. :-)

My understanding is that cleanup.h could be used in new or refactored
code, but people don't like converting existing lock/unlock mechanisms
for no real benefit.

Thanks,
baolu

On Wed, Jul 23, 2025 at 10:21:41AM +0800, Baolu Lu wrote:
> On 7/23/25 05:58, Nicolin Chen wrote:
> > >     return 0;
> > > 
> > > guard(mutex)(&group->mutex);
> > I recall Baolu mentioned that Joerg might not like the guard style
> > so I am keeping mutex_lock/unlock().
> 
> You may be misremembering or mixing something up. I didn't see Joerg
> express that opinion. :-)
> 
> My understanding is that cleanup.h could be used in new or refactored
> code, but people don't like converting existing lock/unlock mechanisms
> for no real benefit.

Ah, thanks for clarifying. Let's do the guard() way then :)

Nicolin

On 6/28/2025 3:42 PM, Nicolin Chen wrote:
> PCIe permits a device to ignore ATS invalidation TLPs, while processing a
> reset. This creates a problem visible to the OS where an ATS invalidation
> command will time out: e.g. an SVA domain will have no coordination with a
> reset event and can racily issue ATS invalidations to a resetting device.
> 
> The OS should do something to mitigate this as we do not want production
> systems to be reporting critical ATS failures, especially in a hypervisor
> environment. Broadly, OS could arrange to ignore the timeouts, block page
> table mutations to prevent invalidations, or disable and block ATS.
> 
> The PCIe spec in sec 10.3.1 IMPLEMENTATION NOTE recommends to disable and
> block ATS before initiating a Function Level Reset. It also mentions that
> other reset methods could have the same vulnerability as well.
> 
> Provide a callback from the PCI subsystem that will enclose the reset and
> have the iommu core temporarily change all the attached domain to BLOCKED.
> After attaching a BLOCKED domain, IOMMU drivers should fence any incoming
> ATS queries, synchronously stop issuing new ATS invalidations, and wait
> for all ATS invalidations to complete. This can avoid any ATS invaliation
> timeouts.
> 
> However, if there is a domain attachment/replacement happening during an
> ongoing reset, the ATS might be re-enabled between the two function calls.
> Introduce a new pending_reset flag in group_device to defer an attachment
> during a reset, allowing iommu core to cache the target domains in the SW
> level but bypassing the driver. The iommu_dev_reset_done() will re-attach
> these soft-attached domains via __iommu_attach_device/set_group_pasid().
> 
> Notes:
>   - This only works for IOMMU drivers that implemented ops->blocked_domain
>     correctly with pci_disable_ats().

Does this mean the IOMMU driver should disable ATS when ops-
 >blocked_domain is used? This might not be feasible because ops-
 >blocked_domain might possibly be attached to a PASID of a device, while
other PASIDs still use ATS for functionality.

>   - This only works for IOMMU drivers that will not issue ATS invalidation
>     requests to the device, after it's docked at ops->blocked_domain.
> Driver should fix itself to align with the aforementioned notes.

My understanding of the requirements for the iommu drivers is: when all
PASIDs are docked in the blocking DMA state, the IOMMU driver should:

- Flush all outstanding ATS invalidation requests;
- Stop issuing any further ATS invalidations;
- Configure the hardware to reject further ATS translation requests.

> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>   include/linux/iommu.h |  12 ++++
>   drivers/iommu/iommu.c | 158 ++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 170 insertions(+)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 156732807994..a17161b8625a 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -1123,6 +1123,9 @@ void dev_iommu_priv_set(struct device *dev, void *priv);
>   extern struct mutex iommu_probe_device_lock;
>   int iommu_probe_device(struct device *dev);
>   
> +int iommu_dev_reset_prepare(struct device *dev);
> +void iommu_dev_reset_done(struct device *dev);
> +
>   int iommu_device_use_default_domain(struct device *dev);
>   void iommu_device_unuse_default_domain(struct device *dev);
>   
> @@ -1407,6 +1410,15 @@ static inline int iommu_fwspec_add_ids(struct device *dev, u32 *ids,
>   	return -ENODEV;
>   }
>   
> +static inline int iommu_dev_reset_prepare(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static inline void iommu_dev_reset_done(struct device *dev)
> +{
> +}
> +
>   static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct device *dev)
>   {
>   	return NULL;
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index bd3deedcd2de..14bfeaa9ac29 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -71,12 +71,29 @@ struct group_device {
>   	struct list_head list;
>   	struct device *dev;
>   	char *name;
> +	bool pending_reset : 1;
>   };
>   
>   /* Iterate over each struct group_device in a struct iommu_group */
>   #define for_each_group_device(group, pos) \
>   	list_for_each_entry(pos, &(group)->devices, list)
>   
> +/* Callers must hold the dev->iommu_group->mutex */
> +static struct group_device *device_to_group_device(struct device *dev)
> +{
> +	struct iommu_group *group = dev->iommu_group;
> +	struct group_device *gdev;
> +
> +	lockdep_assert_held(&group->mutex);
> +
> +	/* gdev must be in the list */
> +	for_each_group_device(group, gdev) {
> +		if (gdev->dev == dev)
> +			break;
> +	}
> +	return gdev;
> +}
> +
>   struct iommu_group_attribute {
>   	struct attribute attr;
>   	ssize_t (*show)(struct iommu_group *group, char *buf);
> @@ -2155,8 +2172,17 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
>   	int ret = 0;
>   
>   	mutex_lock(&group->mutex);
> +
> +	/*
> +	 * There is a racy attach while the device is resetting. Defer it until
> +	 * the iommu_dev_reset_done() that attaches the device to group->domain.
> +	 */
> +	if (device_to_group_device(dev)->pending_reset)
> +		goto unlock;
> +
>   	if (dev->iommu && dev->iommu->attach_deferred)
>   		ret = __iommu_attach_device(domain, dev);
> +unlock:
>   	mutex_unlock(&group->mutex);
>   	return ret;
>   }
> @@ -2295,6 +2321,13 @@ static int __iommu_device_set_domain(struct iommu_group *group,
>   		dev->iommu->attach_deferred = 0;
>   	}
>   
> +	/*
> +	 * There is a racy attach while the device is resetting. Defer it until
> +	 * the iommu_dev_reset_done() that attaches the device to group->domain.
> +	 */
> +	if (gdev->pending_reset)
> +		return 0;
> +
>   	ret = __iommu_attach_device(new_domain, dev);
>   	if (ret) {
>   		/*
> @@ -3378,6 +3411,13 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain,
>   	int ret;
>   
>   	for_each_group_device(group, device) {
> +		/*
> +		 * There is a racy attach while the device is resetting. Defer
> +		 * it until the iommu_dev_reset_done() that attaches the device
> +		 * to group->domain.
> +		 */
> +		if (device->pending_reset)
> +			continue;
>   		if (device->dev->iommu->max_pasids > 0) {
>   			ret = domain->ops->set_dev_pasid(domain, device->dev,
>   							 pasid, old);
> @@ -3799,6 +3839,124 @@ int iommu_replace_group_handle(struct iommu_group *group,
>   }
>   EXPORT_SYMBOL_NS_GPL(iommu_replace_group_handle, "IOMMUFD_INTERNAL");
>   
> +/*
> + * Caller must use iommu_dev_reset_prepare() and iommu_dev_reset_done() together
> + * before/after the core-level reset routine, to unclear the pending_reset flag
> + * and to put the iommu_group reference.
> + *
> + * These two functions are designed to be used by PCI reset functions that would
> + * not invoke any racy iommu_release_device() since PCI sysfs node gets removed
> + * before it notifies with a BUS_NOTIFY_REMOVED_DEVICE. When using them in other
> + * case, callers must ensure there will be no racy iommu_release_device() call,
> + * which otherwise would UAF the dev->iommu_group pointer.
> + */

Use kdoc style comments.

> +int iommu_dev_reset_prepare(struct device *dev)
> +{
> +	const struct iommu_ops *ops;
> +	struct iommu_group *group;
> +	unsigned long pasid;
> +	void *entry;
> +	int ret = 0;
> +
> +	if (!dev_has_iommu(dev))
> +		return 0;
> +
> +	if (dev->iommu->require_direct) {
> +		dev_warn(
> +			dev,
> +			"Firmware has requested this device have a 1:1 IOMMU mapping, rejecting configuring the device without a 1:1 mapping. Contact your platform vendor.\n");
> +		return -EINVAL;
> +	}
> +
> +	/* group will be put in iommu_dev_reset_done() */
> +	group = iommu_group_get(dev);
> +
> +	/* Caller ensures no racy iommu_release_device(), so this won't UAF */
> +	mutex_lock(&group->mutex);
> +
> +	ops = dev_iommu_ops(dev);
> +	if (!ops->blocked_domain) {
> +		dev_warn(dev,
> +			 "IOMMU driver doesn't support IOMMU_DOMAIN_BLOCKED\n");
> +		ret = -EOPNOTSUPP;
> +		goto unlock;
> +	}
> +
> +	device_to_group_device(dev)->pending_reset = true;
> +
> +	/* Device is already attached to the blocked_domain. Nothing to do */
> +	if (group->domain->type == IOMMU_DOMAIN_BLOCKED)
> +		goto unlock;

"group->domain->type == IOMMU_DOMAIN_BLOCKED" means that IOMMU_NO_PASID
is docked in the blocking DMA state, but it doesn't imply that other
PASIDs are also in the blocking DMA state. Therefore, we might still
need the following lines to handle other PASIDs.

On the other hand, perhaps we should use "group->domain == ops-
 >blocked_domain" instead of "group->domain->type ==
IOMMU_DOMAIN_BLOCKED" to make the code consistent with the commit
message.

> +
> +	/* Dock RID domain to blocked_domain while retaining group->domain */
> +	ret = __iommu_attach_device(ops->blocked_domain, dev);
> +	if (ret)
> +		goto unlock;
> +
> +	/* Dock PASID domains to blocked_domain while retaining pasid_array */
> +	xa_lock(&group->pasid_array);
> +	xa_for_each_start(&group->pasid_array, pasid, entry, 1)
> +		iommu_remove_dev_pasid(dev, pasid,
> +				       pasid_array_entry_to_domain(entry));
> +	xa_unlock(&group->pasid_array);
> +
> +unlock:
> +	mutex_unlock(&group->mutex);
> +	if (ret)
> +		iommu_group_put(group);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(iommu_dev_reset_prepare);
> +
> +/*
> + * Pair with a previous iommu_dev_reset_prepare() that was successfully returned
> + *
> + * Note that, although unlikely, there is a risk that re-attaching domains might
> + * fail due to some unexpected happening like OOM.
> + */
> +void iommu_dev_reset_done(struct device *dev)
> +{
> +	struct iommu_group *group = dev->iommu_group;
> +	const struct iommu_ops *ops;
> +	struct group_device *gdev;
> +	unsigned long pasid;
> +	void *entry;
> +
> +	if (!dev_has_iommu(dev))
> +		return;
> +
> +	mutex_lock(&group->mutex);
> +
> +	gdev = device_to_group_device(dev);
> +
> +	ops = dev_iommu_ops(dev);
> +	/* iommu_dev_reset_prepare() was not successfully called */
> +	if (WARN_ON(!ops->blocked_domain || !gdev->pending_reset)) {
> +		mutex_unlock(&group->mutex);
> +		return;
> +	}
> +
> +	if (group->domain->type == IOMMU_DOMAIN_BLOCKED)
> +		goto done;

The same here?

> +
> +	/* Shift RID domain back to group->domain */
> +	WARN_ON(__iommu_attach_device(group->domain, dev));
> +
> +	/* Shift PASID domains back to domains retained in pasid_array */
> +	xa_lock(&group->pasid_array);
> +	xa_for_each_start(&group->pasid_array, pasid, entry, 1)
> +		WARN_ON(__iommu_set_group_pasid(
> +			pasid_array_entry_to_domain(entry), group, pasid,
> +			ops->blocked_domain));
> +	xa_unlock(&group->pasid_array);
> +
> +done:
> +	gdev->pending_reset = false;
> +	mutex_unlock(&group->mutex);
> +	iommu_group_put(group);
> +}
> +EXPORT_SYMBOL_GPL(iommu_dev_reset_done);
> +
>   #if IS_ENABLED(CONFIG_IRQ_MSI_IOMMU)
>   /**
>    * iommu_dma_prepare_msi() - Map the MSI page in the IOMMU domain

Thanks,
baolu

On Sat, Jun 28, 2025 at 09:28:12PM +0800, Baolu Lu wrote:
 
> Does this mean the IOMMU driver should disable ATS when ops-
> >blocked_domain is used? This might not be feasible because ops-
> >blocked_domain might possibly be attached to a PASID of a device,
> while other PASIDs still use ATS for functionality.

No.. The above should be setting everything, including PASIDs to the
blocked domain.

The driver doesn't have to disable ATS at the device, but ARM does.

It does have to stop issuing invalidations, which is part of the
definition of blocked in the first place.

> >   - This only works for IOMMU drivers that will not issue ATS invalidation
> >     requests to the device, after it's docked at ops->blocked_domain.
> > Driver should fix itself to align with the aforementioned notes.
> 
> My understanding of the requirements for the iommu drivers is: when all
> PASIDs are docked in the blocking DMA state, the IOMMU driver should:
> 
> - Flush all outstanding ATS invalidation requests;

Arugably driver needs to have serialized ATS invalidation
synchronously during the change to the blocked domain. The prior
paging domain could be immediately freed so lingering invalidations
are probably an existing bug.

> - Stop issuing any further ATS invalidations;

Yes

> - Configure the hardware to reject further ATS translation requests.

Not required. Blocked domain inherently responds to all ATS
translation requests with no-present which is not allowed to be
cached.

> > +int iommu_dev_reset_prepare(struct device *dev)
> > +{
> > +	const struct iommu_ops *ops;
> > +	struct iommu_group *group;
> > +	unsigned long pasid;
> > +	void *entry;
> > +	int ret = 0;
> > +
> > +	if (!dev_has_iommu(dev))
> > +		return 0;
> > +
> > +	if (dev->iommu->require_direct) {
> > +		dev_warn(
> > +			dev,
> > +			"Firmware has requested this device have a 1:1 IOMMU mapping, rejecting configuring the device without a 1:1 mapping. Contact your platform vendor.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* group will be put in iommu_dev_reset_done() */
> > +	group = iommu_group_get(dev);
> > +
> > +	/* Caller ensures no racy iommu_release_device(), so this won't UAF */
> > +	mutex_lock(&group->mutex);
> > +
> > +	ops = dev_iommu_ops(dev);
> > +	if (!ops->blocked_domain) {
> > +		dev_warn(dev,
> > +			 "IOMMU driver doesn't support IOMMU_DOMAIN_BLOCKED\n");
> > +		ret = -EOPNOTSUPP;
> > +		goto unlock;
> > +	}
> > +
> > +	device_to_group_device(dev)->pending_reset = true;
> > +
> > +	/* Device is already attached to the blocked_domain. Nothing to do */
> > +	if (group->domain->type == IOMMU_DOMAIN_BLOCKED)
> > +		goto unlock;
> 
> "group->domain->type == IOMMU_DOMAIN_BLOCKED" means that IOMMU_NO_PASID
> is docked in the blocking DMA state, but it doesn't imply that other
> PASIDs are also in the blocking DMA state. Therefore, we might still
> need the following lines to handle other PASIDs.

Yes, we always have to check the xarray.

> On the other hand, perhaps we should use "group->domain == ops-
> >blocked_domain" instead of "group->domain->type ==
> IOMMU_DOMAIN_BLOCKED" to make the code consistent with the commit
> message.

ops->blocked_domain is not good, we support devices without static
blocking domain. But yes, using DOMAIN_BLOCKED is not greap, there is
a group->blocked_domain that should be used and will dynamicaly create
an empty paging domain if needed.

Jason

On Mon, Jun 30, 2025 at 09:38:14AM -0300, Jason Gunthorpe wrote:
> On Sat, Jun 28, 2025 at 09:28:12PM +0800, Baolu Lu wrote:
>  
> > Does this mean the IOMMU driver should disable ATS when ops-
> > >blocked_domain is used? This might not be feasible because ops-
> > >blocked_domain might possibly be attached to a PASID of a device,
> > while other PASIDs still use ATS for functionality.
> 
> No.. The above should be setting everything, including PASIDs to the
> blocked domain.
> 
> The driver doesn't have to disable ATS at the device, but ARM does.

Oh, the code is expecting a pci_disable_ats() call, as the next
patch will check if ats is disabled on the PCI side..

If that's the case, we'd have to leave the ATS enabled but only
trust that iommu driver won't issue any new ATS invalidation?

Or should we ask driver to be "must" v.s. "doesn't have to"?

> > > +	/* Device is already attached to the blocked_domain. Nothing to do */
> > > +	if (group->domain->type == IOMMU_DOMAIN_BLOCKED)
> > > +		goto unlock;
> > 
> > "group->domain->type == IOMMU_DOMAIN_BLOCKED" means that IOMMU_NO_PASID
> > is docked in the blocking DMA state, but it doesn't imply that other
> > PASIDs are also in the blocking DMA state. Therefore, we might still
> > need the following lines to handle other PASIDs.
> 
> Yes, we always have to check the xarray.

OK. This check should apply to the RID domain attach only then.

> > On the other hand, perhaps we should use "group->domain == ops-
> > >blocked_domain" instead of "group->domain->type ==
> > IOMMU_DOMAIN_BLOCKED" to make the code consistent with the commit
> > message.
> 
> ops->blocked_domain is not good, we support devices without static
> blocking domain. But yes, using DOMAIN_BLOCKED is not greap, there is
> a group->blocked_domain that should be used and will dynamicaly create
> an empty paging domain if needed.

You mean we should use the group->blocking_domain, even if it was
allocated to be a paging domain as the driver doesn't understand
a IOMMU_DOMAIN_BLOCKED yet?

Thanks
Nicolin

On Mon, Jun 30, 2025 at 10:29:12AM -0700, Nicolin Chen wrote:
> On Mon, Jun 30, 2025 at 09:38:14AM -0300, Jason Gunthorpe wrote:
> > On Sat, Jun 28, 2025 at 09:28:12PM +0800, Baolu Lu wrote:
> >  
> > > Does this mean the IOMMU driver should disable ATS when ops-
> > > >blocked_domain is used? This might not be feasible because ops-
> > > >blocked_domain might possibly be attached to a PASID of a device,
> > > while other PASIDs still use ATS for functionality.
> > 
> > No.. The above should be setting everything, including PASIDs to the
> > blocked domain.
> > 
> > The driver doesn't have to disable ATS at the device, but ARM does.
> 
> Oh, the code is expecting a pci_disable_ats() call, as the next
> patch will check if ats is disabled on the PCI side..

I would not bother, it is alot more work to fix AMD and Intel iommu
drivers and I don't think it really buys us anything..

> If that's the case, we'd have to leave the ATS enabled but only
> trust that iommu driver won't issue any new ATS invalidation?

Yes.

> > ops->blocked_domain is not good, we support devices without static
> > blocking domain. But yes, using DOMAIN_BLOCKED is not greap, there is
> > a group->blocked_domain that should be used and will dynamicaly create
> > an empty paging domain if needed.
> 
> You mean we should use the group->blocking_domain, even if it was
> allocated to be a paging domain as the driver doesn't understand
> a IOMMU_DOMAIN_BLOCKED yet?

Yes, and you just get a group->blocking_domain to assign for the same reason.

Jason