The IOMMU group sysfs interface allows users to change the default
domain of a group. The current implementation uses device_lock() to make
sure that the devices in the group are not bound to any driver and won't
be bound during the process of changing the default domain. In order to
avoid a possible deadlock caused by lock order of device_lock and
group->mutex, it limits the functionality to singleton groups only.
The recently implemented DMA ownership framework can be applied here to
replace device_lock(). In addition, use group->mutex to ensure that the
iommu ops of the device are always valid during the process of changing
default domain.
With above replacement and enhancement, the device_lock() could be
removed and the singleton-group-only limitation could be removed.
This series is based on v6.3-rc3, and I also have it on github:
https://github.com/LuBaolu/intel-iommu/commits/iommu-sysfs-default-domain-extension-v4
Change log:
v4:
- Whole series reviewed by Jason.
- Robin suggested further simplification of using
arm_iommu_release_mapping() directly.
https://lore.kernel.org/linux-iommu/[email protected]/
- Rebase the whole series to v6.3-rc3.
v3:
- https://lore.kernel.org/linux-iommu/[email protected]/
- "arm_iommu_detach_device() is a noop" is not entirely right. It is
used to make the iommu driver stop using the domain that it is
about to free. It cannot be a NOP or it is a UAF. [Jason]
- Use Jason's new arm_iommu_release_device() proposal instead.
v2:
- https://lore.kernel.org/linux-iommu/[email protected]/
- Use group->mutex instead of an additional rw lock.
v1: initial post
- https://lore.kernel.org/linux-iommu/[email protected]/
Lu Baolu (6):
iommu/ipmmu-vmsa: Call arm_iommu_release_mapping() in release path
iommu: Split iommu_group_remove_device() into helpers
iommu: Same critical region for device release and removal
iommu: Move lock from iommu_change_dev_def_domain() to its caller
iommu: Replace device_lock() with group->mutex
iommu: Cleanup iommu_change_dev_def_domain()
drivers/iommu/iommu.c | 273 ++++++++----------
drivers/iommu/ipmmu-vmsa.c | 14 +-
.../ABI/testing/sysfs-kernel-iommu_groups | 1 -
3 files changed, 130 insertions(+), 158 deletions(-)
--
2.34.1
In the iommu driver's release_device operation, the driver should detach
the device from any attached domain and release the resources allocated
in the probe_device and probe_finalize paths.
Replace arm_iommu_detach_device() with arm_iommu_release_mapping() in the
release path of the ipmmu-vmsa driver. The device_release callback is
called in device_del(), this device is not coming back. Zeroing out
pointers and testing for a condition which cannot be true by construction
is simply a waste of time and code.
The bonus is that it also removes a obstacle of arm_iommu_detach_device()
re-entering the iommu core during release_device. With this removed, the
iommu core code could be simplified a lot.
Signed-off-by: Jason Gunthorpe <[email protected]>
Suggested-by: Robin Murphy <[email protected]>
Link: https://lore.kernel.org/linux-iommu/[email protected]/
Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
---
drivers/iommu/ipmmu-vmsa.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index bdf1a4e5eae0..bc135679523a 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -30,7 +30,6 @@
#define arm_iommu_create_mapping(...) NULL
#define arm_iommu_attach_device(...) -ENODEV
#define arm_iommu_release_mapping(...) do {} while (0)
-#define arm_iommu_detach_device(...) do {} while (0)
#endif
#define IPMMU_CTX_MAX 16U
@@ -820,7 +819,18 @@ static void ipmmu_probe_finalize(struct device *dev)
static void ipmmu_release_device(struct device *dev)
{
- arm_iommu_detach_device(dev);
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+ struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
+ unsigned int i;
+
+ for (i = 0; i < fwspec->num_ids; ++i) {
+ unsigned int utlb = fwspec->ids[i];
+
+ ipmmu_imuctr_write(mmu, utlb, 0);
+ mmu->utlb_ctx[utlb] = IPMMU_CTX_INVALID;
+ }
+
+ arm_iommu_release_mapping(mmu->mapping);
}
static struct iommu_group *ipmmu_find_group(struct device *dev)
--
2.34.1
So that code could be re-used by iommu_release_device() in the subsequent
change. No intention for functionality change.
Signed-off-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
---
drivers/iommu/iommu.c | 64 +++++++++++++++++++++++++++++--------------
1 file changed, 44 insertions(+), 20 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 10db680acaed..43db48323370 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -453,6 +453,46 @@ int iommu_probe_device(struct device *dev)
}
+/*
+ * Remove a device from a group's device list and return the group device
+ * if successful.
+ */
+static struct group_device *
+__iommu_group_remove_device(struct iommu_group *group, struct device *dev)
+{
+ struct group_device *device;
+
+ lockdep_assert_held(&group->mutex);
+ list_for_each_entry(device, &group->devices, list) {
+ if (device->dev == dev) {
+ list_del(&device->list);
+ return device;
+ }
+ }
+
+ return NULL;
+}
+
+/*
+ * Release a device from its group and decrements the iommu group reference
+ * count.
+ */
+static void __iommu_group_release_device(struct iommu_group *group,
+ struct group_device *grp_dev)
+{
+ struct device *dev = grp_dev->dev;
+
+ sysfs_remove_link(group->devices_kobj, grp_dev->name);
+ sysfs_remove_link(&dev->kobj, "iommu_group");
+
+ trace_remove_device_from_group(group->id, dev);
+
+ kfree(grp_dev->name);
+ kfree(grp_dev);
+ dev->iommu_group = NULL;
+ kobject_put(group->devices_kobj);
+}
+
void iommu_release_device(struct device *dev)
{
const struct iommu_ops *ops;
@@ -1068,7 +1108,7 @@ EXPORT_SYMBOL_GPL(iommu_group_add_device);
void iommu_group_remove_device(struct device *dev)
{
struct iommu_group *group = dev->iommu_group;
- struct group_device *tmp_device, *device = NULL;
+ struct group_device *device;
if (!group)
return;
@@ -1076,27 +1116,11 @@ void iommu_group_remove_device(struct device *dev)
dev_info(dev, "Removing from iommu group %d\n", group->id);
mutex_lock(&group->mutex);
- list_for_each_entry(tmp_device, &group->devices, list) {
- if (tmp_device->dev == dev) {
- device = tmp_device;
- list_del(&device->list);
- break;
- }
- }
+ device = __iommu_group_remove_device(group, dev);
mutex_unlock(&group->mutex);
- if (!device)
- return;
-
- sysfs_remove_link(group->devices_kobj, device->name);
- sysfs_remove_link(&dev->kobj, "iommu_group");
-
- trace_remove_device_from_group(group->id, dev);
-
- kfree(device->name);
- kfree(device);
- dev->iommu_group = NULL;
- kobject_put(group->devices_kobj);
+ if (device)
+ __iommu_group_release_device(group, device);
}
EXPORT_SYMBOL_GPL(iommu_group_remove_device);
--
2.34.1
In a non-driver context, it is crucial to ensure the consistency of a
device's iommu ops. Otherwise, it may result in a situation where a
device is released but it's iommu ops are still used.
Put the ops->release_device and __iommu_group_remove_device() in a same
group->mutext critical region, so that, as long as group->mutex is held
and the device is in its group's device list, its iommu ops are always
consistent. Add check of group ownership if the released device is the
last one.
Signed-off-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
---
drivers/iommu/iommu.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 43db48323370..6d27fd585e75 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -495,18 +495,44 @@ static void __iommu_group_release_device(struct iommu_group *group,
void iommu_release_device(struct device *dev)
{
+ struct iommu_group *group = dev->iommu_group;
+ struct group_device *device;
const struct iommu_ops *ops;
- if (!dev->iommu)
+ if (!dev->iommu || !group)
return;
iommu_device_unlink(dev->iommu->iommu_dev, dev);
+ mutex_lock(&group->mutex);
+ device = __iommu_group_remove_device(group, dev);
+
+ /*
+ * If the group has become empty then ownership must have been released,
+ * and the current domain must be set back to NULL or the default
+ * domain.
+ */
+ if (list_empty(&group->devices))
+ WARN_ON(group->owner_cnt ||
+ group->domain != group->default_domain);
+
+ /*
+ * release_device() must stop using any attached domain on the device.
+ * If there are still other devices in the group they are not effected
+ * by this callback.
+ *
+ * The IOMMU driver must set the device to either an identity or
+ * blocking translation and stop using any domain pointer, as it is
+ * going to be freed.
+ */
ops = dev_iommu_ops(dev);
if (ops->release_device)
ops->release_device(dev);
+ mutex_unlock(&group->mutex);
+
+ if (device)
+ __iommu_group_release_device(group, device);
- iommu_group_remove_device(dev);
module_put(ops->owner);
dev_iommu_free(dev);
}
--
2.34.1
The intention is to make it possible to put group ownership check and
default domain change in a same critical region protected by the group's
mutex lock. No intentional functional change.
Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
---
drivers/iommu/iommu.c | 29 ++++++++++++++---------------
1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 6d27fd585e75..54a5bd79d21b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2889,7 +2889,7 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
int ret, dev_def_dom;
struct device *dev;
- mutex_lock(&group->mutex);
+ lockdep_assert_held(&group->mutex);
if (group->default_domain != group->domain) {
dev_err_ratelimited(prev_dev, "Group not assigned to default domain\n");
@@ -2978,28 +2978,15 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
goto free_new_domain;
group->domain = group->default_domain;
-
- /*
- * Release the mutex here because ops->probe_finalize() call-back of
- * some vendor IOMMU drivers calls arm_iommu_attach_device() which
- * in-turn might call back into IOMMU core code, where it tries to take
- * group->mutex, resulting in a deadlock.
- */
- mutex_unlock(&group->mutex);
-
- /* Make sure dma_ops is appropriatley set */
- iommu_group_do_probe_finalize(dev, group->default_domain);
iommu_domain_free(prev_dom);
+
return 0;
free_new_domain:
iommu_domain_free(group->default_domain);
group->default_domain = prev_dom;
group->domain = prev_dom;
-
out:
- mutex_unlock(&group->mutex);
-
return ret;
}
@@ -3089,7 +3076,19 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
goto out;
}
+ mutex_lock(&group->mutex);
ret = iommu_change_dev_def_domain(group, dev, req_type);
+ /*
+ * Release the mutex here because ops->probe_finalize() call-back of
+ * some vendor IOMMU drivers calls arm_iommu_attach_device() which
+ * in-turn might call back into IOMMU core code, where it tries to take
+ * group->mutex, resulting in a deadlock.
+ */
+ mutex_unlock(&group->mutex);
+
+ /* Make sure dma_ops is appropriatley set */
+ if (!ret)
+ iommu_group_do_probe_finalize(dev, group->default_domain);
ret = ret ?: count;
out:
--
2.34.1
As the singleton group limitation has been removed, cleanup the code
in iommu_change_dev_def_domain() accordingly.
Documentation is also updated.
Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
---
drivers/iommu/iommu.c | 83 +++++--------------
.../ABI/testing/sysfs-kernel-iommu_groups | 1 -
2 files changed, 21 insertions(+), 63 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index b780cdddf415..a38b9f279c5d 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2867,11 +2867,10 @@ int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features feat)
EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
/*
- * Changes the default domain of an iommu group that has *only* one device
+ * Changes the default domain of an iommu group
*
* @group: The group for which the default domain should be changed
- * @prev_dev: The device in the group (this is used to make sure that the device
- * hasn't changed after the caller has called this function)
+ * @dev: The first device in the group
* @type: The type of the new default domain that gets associated with the group
*
* Returns 0 on success and error code on failure
@@ -2882,103 +2881,63 @@ EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
* Please take a closer look if intended to use for other purposes.
*/
static int iommu_change_dev_def_domain(struct iommu_group *group,
- struct device *prev_dev, int type)
+ struct device *dev, int type)
{
+ struct __group_domain_type gtype = {NULL, 0};
struct iommu_domain *prev_dom;
- struct group_device *grp_dev;
- int ret, dev_def_dom;
- struct device *dev;
+ int ret;
lockdep_assert_held(&group->mutex);
- if (group->default_domain != group->domain) {
- dev_err_ratelimited(prev_dev, "Group not assigned to default domain\n");
- ret = -EBUSY;
- goto out;
- }
-
- /*
- * iommu group wasn't locked while acquiring device lock in
- * iommu_group_store_type(). So, make sure that the device count hasn't
- * changed while acquiring device lock.
- *
- * Changing default domain of an iommu group with two or more devices
- * isn't supported because there could be a potential deadlock. Consider
- * the following scenario. T1 is trying to acquire device locks of all
- * the devices in the group and before it could acquire all of them,
- * there could be another thread T2 (from different sub-system and use
- * case) that has already acquired some of the device locks and might be
- * waiting for T1 to release other device locks.
- */
- if (iommu_group_device_count(group) != 1) {
- dev_err_ratelimited(prev_dev, "Cannot change default domain: Group has more than one device\n");
- ret = -EINVAL;
- goto out;
- }
-
- /* Since group has only one device */
- grp_dev = list_first_entry(&group->devices, struct group_device, list);
- dev = grp_dev->dev;
-
- if (prev_dev != dev) {
- dev_err_ratelimited(prev_dev, "Cannot change default domain: Device has been changed\n");
- ret = -EBUSY;
- goto out;
- }
-
prev_dom = group->default_domain;
- if (!prev_dom) {
- ret = -EINVAL;
- goto out;
- }
-
- dev_def_dom = iommu_get_def_domain_type(dev);
+ __iommu_group_for_each_dev(group, >ype,
+ probe_get_default_domain_type);
if (!type) {
/*
* If the user hasn't requested any specific type of domain and
* if the device supports both the domains, then default to the
* domain the device was booted with
*/
- type = dev_def_dom ? : iommu_def_domain_type;
- } else if (dev_def_dom && type != dev_def_dom) {
- dev_err_ratelimited(prev_dev, "Device cannot be in %s domain\n",
+ type = gtype.type ? : iommu_def_domain_type;
+ } else if (gtype.type && type != gtype.type) {
+ dev_err_ratelimited(dev, "Device cannot be in %s domain\n",
iommu_domain_type_str(type));
- ret = -EINVAL;
- goto out;
+ return -EINVAL;
}
/*
* Switch to a new domain only if the requested domain type is different
* from the existing default domain type
*/
- if (prev_dom->type == type) {
- ret = 0;
- goto out;
- }
+ if (prev_dom->type == type)
+ return 0;
+
+ group->default_domain = NULL;
+ group->domain = NULL;
/* Sets group->default_domain to the newly allocated domain */
ret = iommu_group_alloc_default_domain(dev->bus, group, type);
if (ret)
- goto out;
+ goto restore_old_domain;
- ret = iommu_create_device_direct_mappings(group, dev);
+ ret = iommu_group_create_direct_mappings(group);
if (ret)
goto free_new_domain;
- ret = __iommu_attach_device(group->default_domain, dev);
+ ret = __iommu_attach_group(group->default_domain, group);
if (ret)
goto free_new_domain;
- group->domain = group->default_domain;
iommu_domain_free(prev_dom);
return 0;
free_new_domain:
iommu_domain_free(group->default_domain);
+restore_old_domain:
group->default_domain = prev_dom;
group->domain = prev_dom;
-out:
+
return ret;
}
diff --git a/Documentation/ABI/testing/sysfs-kernel-iommu_groups b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
index b15af6a5bc08..a42d4383d999 100644
--- a/Documentation/ABI/testing/sysfs-kernel-iommu_groups
+++ b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
@@ -53,7 +53,6 @@ Description: /sys/kernel/iommu_groups/<grp_id>/type shows the type of default
The default domain type of a group may be modified only when
- - The group has only one device.
- The device in the group is not bound to any device driver.
So, the users must unbind the appropriate driver before
changing the default domain type.
--
2.34.1
device_lock() was used in iommu_group_store_type() to prevent the
devices in an iommu group from being attached by any device driver.
On the other hand, in order to avoid lock race between group->mutex
and device_lock(), it limited the usage scenario to the singleton
groups.
We already have the DMA ownership scheme to avoid driver attachment
and group->mutex ensures that device ops are always valid, there's
no need for device_lock() anymore. Remove device_lock() and the
singleton group limitation.
Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
---
drivers/iommu/iommu.c | 81 ++++++++++---------------------------------
1 file changed, 18 insertions(+), 63 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 54a5bd79d21b..b780cdddf415 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2956,14 +2956,6 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
goto out;
}
- /* We can bring up a flush queue without tearing down the domain */
- if (type == IOMMU_DOMAIN_DMA_FQ && prev_dom->type == IOMMU_DOMAIN_DMA) {
- ret = iommu_dma_init_fq(prev_dom);
- if (!ret)
- prev_dom->type = IOMMU_DOMAIN_DMA_FQ;
- goto out;
- }
-
/* Sets group->default_domain to the newly allocated domain */
ret = iommu_group_alloc_default_domain(dev->bus, group, type);
if (ret)
@@ -2996,7 +2988,7 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
* transition. Return failure if this isn't met.
*
* We need to consider the race between this and the device release path.
- * device_lock(dev) is used here to guarantee that the device release path
+ * group->mutex is used here to guarantee that the device release path
* will not be entered at the same time.
*/
static ssize_t iommu_group_store_type(struct iommu_group *group,
@@ -3023,61 +3015,29 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
else
return -EINVAL;
- /*
- * Lock/Unlock the group mutex here before device lock to
- * 1. Make sure that the iommu group has only one device (this is a
- * prerequisite for step 2)
- * 2. Get struct *dev which is needed to lock device
- */
mutex_lock(&group->mutex);
- if (iommu_group_device_count(group) != 1) {
+ /* We can bring up a flush queue without tearing down the domain. */
+ if (req_type == IOMMU_DOMAIN_DMA_FQ &&
+ group->default_domain->type == IOMMU_DOMAIN_DMA) {
+ ret = iommu_dma_init_fq(group->default_domain);
+ if (!ret)
+ group->default_domain->type = IOMMU_DOMAIN_DMA_FQ;
mutex_unlock(&group->mutex);
- pr_err_ratelimited("Cannot change default domain: Group has more than one device\n");
- return -EINVAL;
+
+ return ret ?: count;
+ }
+
+ /* Otherwise, ensure that device exists and no driver is bound. */
+ if (list_empty(&group->devices) || group->owner_cnt) {
+ mutex_unlock(&group->mutex);
+ return -EPERM;
}
- /* Since group has only one device */
grp_dev = list_first_entry(&group->devices, struct group_device, list);
dev = grp_dev->dev;
- get_device(dev);
- /*
- * Don't hold the group mutex because taking group mutex first and then
- * the device lock could potentially cause a deadlock as below. Assume
- * two threads T1 and T2. T1 is trying to change default domain of an
- * iommu group and T2 is trying to hot unplug a device or release [1] VF
- * of a PCIe device which is in the same iommu group. T1 takes group
- * mutex and before it could take device lock assume T2 has taken device
- * lock and is yet to take group mutex. Now, both the threads will be
- * waiting for the other thread to release lock. Below, lock order was
- * suggested.
- * device_lock(dev);
- * mutex_lock(&group->mutex);
- * iommu_change_dev_def_domain();
- * mutex_unlock(&group->mutex);
- * device_unlock(dev);
- *
- * [1] Typical device release path
- * device_lock() from device/driver core code
- * -> bus_notifier()
- * -> iommu_bus_notifier()
- * -> iommu_release_device()
- * -> ops->release_device() vendor driver calls back iommu core code
- * -> mutex_lock() from iommu core code
- */
- mutex_unlock(&group->mutex);
-
- /* Check if the device in the group still has a driver bound to it */
- device_lock(dev);
- if (device_is_bound(dev) && !(req_type == IOMMU_DOMAIN_DMA_FQ &&
- group->default_domain->type == IOMMU_DOMAIN_DMA)) {
- pr_err_ratelimited("Device is still bound to driver\n");
- ret = -EBUSY;
- goto out;
- }
-
- mutex_lock(&group->mutex);
ret = iommu_change_dev_def_domain(group, dev, req_type);
+
/*
* Release the mutex here because ops->probe_finalize() call-back of
* some vendor IOMMU drivers calls arm_iommu_attach_device() which
@@ -3088,14 +3048,9 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
/* Make sure dma_ops is appropriatley set */
if (!ret)
- iommu_group_do_probe_finalize(dev, group->default_domain);
- ret = ret ?: count;
-
-out:
- device_unlock(dev);
- put_device(dev);
+ __iommu_group_dma_finalize(group);
- return ret;
+ return ret ?: count;
}
static bool iommu_is_default_domain(struct iommu_group *group)
--
2.34.1
On Wed, Mar 22, 2023 at 02:49:50PM +0800, Lu Baolu wrote:
> Lu Baolu (6):
> iommu/ipmmu-vmsa: Call arm_iommu_release_mapping() in release path
> iommu: Split iommu_group_remove_device() into helpers
> iommu: Same critical region for device release and removal
> iommu: Move lock from iommu_change_dev_def_domain() to its caller
> iommu: Replace device_lock() with group->mutex
> iommu: Cleanup iommu_change_dev_def_domain()
>
> drivers/iommu/iommu.c | 273 ++++++++----------
> drivers/iommu/ipmmu-vmsa.c | 14 +-
> .../ABI/testing/sysfs-kernel-iommu_groups | 1 -
> 3 files changed, 130 insertions(+), 158 deletions(-)
Applied, thanks.