| 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
| ||
|
Date: Thu, 7 Apr 2022 15:40:14 +0800
From: "Leizhen (ThunderTown)" <thunder.leizhen@...wei.com>
To: John Garry <john.garry@...wei.com>, <joro@...tes.org>,
<will@...nel.org>, <robin.murphy@....com>
CC: <mst@...hat.com>, <jasowang@...hat.com>,
<iommu@...ts.linux-foundation.org>, <linux-kernel@...r.kernel.org>,
<virtualization@...ts.linux-foundation.org>,
<chenxiang66@...ilicon.com>, <jean-philippe@...aro.org>,
<linuxarm@...wei.com>
Subject: Re: [PATCH RESEND v5 1/5] iommu: Refactor iommu_group_store_type()
Reviewed-by: Zhen Lei <thunder.leizhen@...wei.com>
On 2022/4/4 19:27, John Garry wrote:
> Function iommu_group_store_type() supports changing the default domain
> of an IOMMU group.
>
> Many conditions need to be satisfied and steps taken for this action to be
> successful.
>
> Satisfying these conditions and steps will be required for setting other
> IOMMU group attributes, so factor into a common part and a part specific
> to update the IOMMU group attribute.
>
> No functional change intended.
>
> Some code comments are tidied up also.
>
> Signed-off-by: John Garry <john.garry@...wei.com>
> ---
> drivers/iommu/iommu.c | 96 ++++++++++++++++++++++++++++---------------
> 1 file changed, 62 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index f2c45b85b9fc..0dd766030baf 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3000,21 +3000,57 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
> return ret;
> }
>
> +enum iommu_group_op {
> + CHANGE_GROUP_TYPE,
> +};
> +
> +static int __iommu_group_store_type(const char *buf, struct iommu_group *group,
> + struct device *dev)
> +{
> + int type;
> +
> + if (sysfs_streq(buf, "identity"))
> + type = IOMMU_DOMAIN_IDENTITY;
> + else if (sysfs_streq(buf, "DMA"))
> + type = IOMMU_DOMAIN_DMA;
> + else if (sysfs_streq(buf, "DMA-FQ"))
> + type = IOMMU_DOMAIN_DMA_FQ;
> + else if (sysfs_streq(buf, "auto"))
> + type = 0;
> + else
> + return -EINVAL;
> +
> + /*
> + * Check if the only device in the group still has a driver bound or
> + * we're transistioning from DMA -> DMA-FQ
> + */
> + if (device_is_bound(dev) && !(type == IOMMU_DOMAIN_DMA_FQ &&
> + group->default_domain->type == IOMMU_DOMAIN_DMA)) {
> + pr_err_ratelimited("Device is still bound to driver\n");
> + return -EINVAL;
> + }
> +
> + return iommu_change_dev_def_domain(group, dev, type);
> +}
> +
> /*
> * Changing the default domain through sysfs requires the users to unbind the
> * drivers from the devices in the iommu group, except for a DMA -> DMA-FQ
> - * transition. Return failure if this isn't met.
> + * transition. Changing or any other IOMMU group attribute still requires the
> + * user to unbind the drivers from the devices in the iommu group. Return
> + * failure if these conditions are not 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)
> +static ssize_t iommu_group_store_common(struct iommu_group *group,
> + enum iommu_group_op op,
> + const char *buf, size_t count)
> {
> struct group_device *grp_dev;
> struct device *dev;
> - int ret, req_type;
> + int ret;
>
> if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
> return -EACCES;
> @@ -3022,27 +3058,16 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
> if (WARN_ON(!group))
> return -EINVAL;
>
> - if (sysfs_streq(buf, "identity"))
> - req_type = IOMMU_DOMAIN_IDENTITY;
> - else if (sysfs_streq(buf, "DMA"))
> - req_type = IOMMU_DOMAIN_DMA;
> - else if (sysfs_streq(buf, "DMA-FQ"))
> - req_type = IOMMU_DOMAIN_DMA_FQ;
> - else if (sysfs_streq(buf, "auto"))
> - req_type = 0;
> - 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
> + * 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);
> - pr_err_ratelimited("Cannot change default domain: Group has more than one device\n");
> + pr_err_ratelimited("Cannot change IOMMU group default domain attribute: Group has more than one device\n");
> return -EINVAL;
> }
>
> @@ -3054,16 +3079,16 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
> /*
> * 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.
> + * two threads, T1 and T2. T1 is trying to change default domain
> + * attribute 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 the group mutex and before it could take device lock T2 may
> + * have 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();
> + * cb->iommu_change_dev_def_domain(); [example cb]
> * mutex_unlock(&group->mutex);
> * device_unlock(dev);
> *
> @@ -3077,21 +3102,24 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
> */
> 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;
> + switch (op) {
> + case CHANGE_GROUP_TYPE:
> + ret = __iommu_group_store_type(buf, group, dev);
> + break;
> + default:
> + ret = -EINVAL;
> }
> -
> - ret = iommu_change_dev_def_domain(group, dev, req_type);
> ret = ret ?: count;
>
> -out:
> device_unlock(dev);
> put_device(dev);
>
> return ret;
> }
> +
> +static ssize_t iommu_group_store_type(struct iommu_group *group,
> + const char *buf, size_t count)
> +{
> + return iommu_group_store_common(group, CHANGE_GROUP_TYPE, buf, count);
> +}
>
--
Regards,
Zhen Lei
Powered by blists - more mailing lists