> From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> Sent: Friday, April 12, 2024 4:15 PM
>
> @@ -3332,11 +3333,34 @@ static int __iommu_set_group_pasid(struct
> iommu_domain *domain,
> err_revert:
> last_gdev = device;
> for_each_group_device(group, device) {
> - const struct iommu_ops *ops = dev_iommu_ops(device-
> >dev);
> + /*
> + * If no old domain, just undo all the devices/pasid that
> + * have attached to the new domain.
> + */
> + if (!old) {
> + const struct iommu_ops *ops =
> + dev_iommu_ops(device-
> >dev);
> +
> + if (device == last_gdev)
> + break;
> + ops = dev_iommu_ops(device->dev);
'ops' is already assigned
> + ops->remove_dev_pasid(device->dev, pasid, domain);
> + continue;
> + }
>
> - if (device == last_gdev)
> + /*
> + * Rollback the devices/pasid that have attached to the new
> + * domain. And it is a driver bug to fail attaching with a
> + * previously good domain.
> + */
> + if (device == last_gdev) {
> + WARN_ON(old->ops->set_dev_pasid(old, device-
> >dev,
> + pasid, NULL));
do we have a clear definition that @set_dev_pasid callback should
leave the device detached (as 'NULL' indicates) or we just don't
care the currently-attached domain at this point?
>
> +/**
> + * iommu_replace_device_pasid - replace the domain that a pasid is
> attached to
> + * @domain: new IOMMU domain to replace with
> + * @dev: the physical device
> + * @pasid: pasid that will be attached to the new domain
> + *
> + * This API allows the pasid to switch domains. Return 0 on success, or an
> + * error. The pasid will roll back to use the old domain if failure. The
> + * caller could call iommu_detach_device_pasid() before free the old
> domain
> + * in order to avoid use-after-free case.
I didn't get what the last sentence tries to convey. Do you mean that
the old domain cannot be freed even after the replace operation has
been completed successfully? why does it require a detach before
the free?
> + */
> +int iommu_replace_device_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t pasid)
> +{
> + /* Caller must be a probed driver on dev */
> + struct iommu_group *group = dev->iommu_group;
> + void *curr;
> + int ret;
> +
> + if (!domain)
> + return -EINVAL;
this check can be skipped. Accessing a null pointer will hit
a call trace already.
> +
> + if (!domain->ops->set_dev_pasid)
> + return -EOPNOTSUPP;
> +
> + if (!group)
> + return -ENODEV;
> +
> + if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain-
> >owner)
> + return -EINVAL;
and check it's not IOMMU_NO_PASID
> +
> + mutex_lock(&group->mutex);
> + curr = xa_store(&group->pasid_array, pasid, domain, GFP_KERNEL);
> + if (!curr) {
> + xa_erase(&group->pasid_array, pasid);
> + ret = -EINVAL;
> + goto out_unlock;
> + }
> +
> + ret = xa_err(curr);
> + if (ret)
> + goto out_unlock;
> +
> + if (curr == domain)
> + goto out_unlock;
emmm then 'ret' is used uninitialized here.
> +
> + ret = __iommu_set_group_pasid(domain, group, pasid, curr);
> + if (ret)
> + WARN_ON(xa_err(xa_store(&group->pasid_array, pasid,
> + curr, GFP_KERNEL)));
split the line. WARN_ON() as long as the return value doesn't match
'domain'.
> +out_unlock:
> + mutex_unlock(&group->mutex);
> + return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(iommu_replace_device_pasid,
> IOMMUFD_INTERNAL);
> +
> /*
> * iommu_detach_device_pasid() - Detach the domain from pasid of device
> * @domain: the iommu domain.
> --
> 2.34.1
