On 6/5/24 4:02 PM, Tian, Kevin wrote:
From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
Sent: Monday, May 27, 2024 12:05 PM
@@ -99,7 +99,9 @@ struct iommu_sva *iommu_sva_bind_device(struct device
*dev, struct mm_struct *mm
/* Search for an existing domain. */
list_for_each_entry(domain, &mm->iommu_mm->sva_domains, next) {
- ret = iommu_attach_device_pasid(domain, dev, iommu_mm-
pasid);
+ handle->handle.domain = domain;
+ ret = iommu_attach_device_pasid(domain, dev, iommu_mm-
pasid,
+ &handle->handle);
move the setting of handle.domain into the helper?
Yes.
@@ -3382,11 +3383,9 @@ int iommu_attach_device_pasid(struct
iommu_domain *domain,
}
}
- curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain,
GFP_KERNEL);
- if (curr) {
- ret = xa_err(curr) ? : -EBUSY;
+ ret = xa_insert(&group->pasid_array, pasid, handle, GFP_KERNEL);
+ if (ret)
goto out_unlock;
- }
this leads to a slightly different implication comparing to the old code.
the domain pointer was always tracked in the old code but now the handle
is optional.
if iommu core only needs to know whether a pasid has been attached (as
in this helper), it still works as xa_insert() will mark the entry as reserved
if handle==NULL and next xa_insert() at the same index will fail due to
the entry being reserved.
But if certain path (other than iopf) in the iommu core needs to know
the exact domain pointer then this change breaks it.
The iommu core should not fetch the domain pointer in paths other than
attach/detach/replace. There is currently no reference counter for an
iommu domain, hence fetching the domain for other purposes will
potentially lead to a use-after-free issue.
Anyway some explanation is welcomed here for why this change is safe.
@@ -3414,7 +3413,7 @@ void iommu_detach_device_pasid(struct
iommu_domain *domain, struct device *dev,
mutex_lock(&group->mutex);
__iommu_remove_group_pasid(group, pasid, domain);
- WARN_ON(xa_erase(&group->pasid_array, pasid) != domain);
+ xa_erase(&group->pasid_array, pasid);
if the entry is valid do we want to keep the WARN_ON() upon handle->domain?
The domain pointer has already been passed to the iommu driver in
__iommu_remove_group_pasid(). Therefore, the check for the pointer's
validity should be performed before calling
__iommu_remove_group_pasid(). Hence, in my view, using WARN_ON() around
xa_erase() is not very useful.
Best regards,
baolu
