On Thu, Jun 04, 2020 at 06:32:06PM -0700, Sai Praneeth Prakhya wrote:
> +static int iommu_change_dev_def_domain(struct iommu_group *group, int type)
> +{
> + struct iommu_domain *prev_dom;
> + struct group_device *grp_dev;
> + const struct iommu_ops *ops;
> + int ret, dev_def_dom;
> + struct device *dev;
> +
> + if (!group)
> + return -EINVAL;
> +
> + mutex_lock(&group->mutex);
> +
> + if (group->default_domain != group->domain) {
> + pr_err_ratelimited("Group assigned to user level for direct
> access\n");
Make this message: "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) {
> + pr_err_ratelimited("Cannot change default domain of a group
> with two or more devices\n");
"Can not change default domain: Group has more than one device\n"
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + /* Since group has only one device */
> + list_for_each_entry(grp_dev, &group->devices, list)
> + dev = grp_dev->dev;
> +
> + prev_dom = group->default_domain;
> + if (!prev_dom || !prev_dom->ops || !prev_dom->ops->def_domain_type) {
> + pr_err_ratelimited("'def_domain_type' call back isn't
> registered\n");
This message isn't needed.
> + ret = __iommu_attach_device(group->default_domain, dev);
> + if (ret)
> + goto free_new_domain;
> +
> + group->domain = group->default_domain;
> +
> + ret = iommu_create_device_direct_mappings(group, dev);
> + if (ret)
> + goto free_new_domain;
You need to create the direct mappings before you attach the device to
the new domain. Otherwise there might be a short time-window where RMRR
regions are not mapped.
> +static ssize_t iommu_group_store_type(struct iommu_group *group,
> + const char *buf, size_t count)
> +{
> + struct group_device *grp_dev;
> + struct device *dev;
> + int ret, req_type;
> +
> + if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
> + return -EACCES;
> +
> + 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, "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
> + * 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 of a group
> with two or more devices\n");
> + return -EINVAL;
> + }
> +
> + /* Since group has only one device */
> + list_for_each_entry(grp_dev, &group->devices, list)
> + dev = grp_dev->dev;
Please use list_first_entry().
You also need to take a reference with get_device() and then drop the
group->mutex.
After device_lock() you need to verify that the device is still in the
same group and that the group has still only one device in it. Then you
can call down to iommu_change_dev_def_domain() which does not need to
take the group-mutex by itself.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
