| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20230213074941.919324-4-baolu.lu@linux.intel.com>
Date: Mon, 13 Feb 2023 15:49:40 +0800
From: Lu Baolu <baolu.lu@...ux.intel.com>
To: iommu@...ts.linux.dev
Cc: Joerg Roedel <joro@...tes.org>, Jason Gunthorpe <jgg@...dia.com>,
Christoph Hellwig <hch@...radead.org>,
Kevin Tian <kevin.tian@...el.com>,
Will Deacon <will@...nel.org>,
Robin Murphy <robin.murphy@....com>,
linux-kernel@...r.kernel.org, Lu Baolu <baolu.lu@...ux.intel.com>
Subject: [PATCH 3/4] iommu: Remove unnecessary device_lock()
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 have used the DMA ownership framework to avoid driver attachment
and ensured that device ops are always valid, there's no need to
lock device anymore. Remove use of device_lock() and the singleton
group limitation.
Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
---
drivers/iommu/iommu.c | 78 ++++---------------------------------------
1 file changed, 6 insertions(+), 72 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 6547cb38480c..1e69def20a67 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2786,8 +2786,6 @@ EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
* Changes the default domain of an iommu group that has *only* one device
*
* @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)
* @type: The type of the new default domain that gets associated with the group
*
* Returns 0 on success and error code on failure
@@ -2797,8 +2795,7 @@ EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
* group's default domain type through /sys/kernel/iommu_groups/<grp_id>/type
* 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)
+static int iommu_change_dev_def_domain(struct iommu_group *group, int type)
{
struct iommu_domain *prev_dom;
struct group_device *grp_dev;
@@ -2821,7 +2818,7 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
* 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");
+ pr_err_ratelimited("Cannot change default domain: Group has more than one device\n");
ret = -EINVAL;
goto out;
}
@@ -2830,12 +2827,6 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
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;
@@ -2851,8 +2842,8 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
*/
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",
- iommu_domain_type_str(type));
+ pr_err_ratelimited("Device cannot be in %s domain\n",
+ iommu_domain_type_str(type));
ret = -EINVAL;
goto out;
}
@@ -2959,15 +2950,11 @@ static void iommu_group_unfreeze_dev_ops(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
- * will not be entered at the same time.
*/
static ssize_t iommu_group_store_type(struct iommu_group *group,
const char *buf, size_t count)
{
bool group_owner_claimed = false;
- struct group_device *grp_dev;
- struct device *dev;
int ret, req_type;
if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
@@ -2995,66 +2982,13 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
group_owner_claimed = true;
}
- /*
- * 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) {
- mutex_unlock(&group->mutex);
- if (group_owner_claimed)
- iommu_group_release_dma_owner(group);
- pr_err_ratelimited("Cannot change default domain: Group has more than one device\n");
- return -EINVAL;
- }
-
- /* 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);
-
iommu_group_freeze_dev_ops(group);
-
- device_lock(dev);
-
- ret = iommu_change_dev_def_domain(group, dev, req_type);
- ret = ret ?: count;
-
- device_unlock(dev);
+ ret = iommu_change_dev_def_domain(group, req_type);
iommu_group_unfreeze_dev_ops(group);
- put_device(dev);
if (group_owner_claimed)
iommu_group_release_dma_owner(group);
- return ret;
+ return ret ?: count;
}
static bool iommu_is_default_domain(struct iommu_group *group)
--
2.34.1
Powered by blists - more mailing lists