| Message ID | 20241104132513.15890-4-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iommufd support pasid attach/replace | expand |
On 11/4/24 21:25, Yi Liu wrote: > The iommu_attach_handle is now only passed when attaching iopf-capable > domain, while it is not convenient for the iommu core to track the > attached domain of pasids. To address it, the iommu_attach_handle will > be passed to iommu core for non-fault-able domain as well. Hence the > iommufd_handle related helpers are no longer fault specific, it makes > more sense to move it out of fault.c. > > Signed-off-by: Yi Liu<yi.l.liu@intel.com> > --- > drivers/iommu/iommufd/device.c | 51 ++++++++++++++++++++++ > drivers/iommu/iommufd/fault.c | 56 +------------------------ > drivers/iommu/iommufd/iommufd_private.h | 8 ++++ > 3 files changed, 61 insertions(+), 54 deletions(-) > > diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c > index 5fd3dd420290..823c81145214 100644 > --- a/drivers/iommu/iommufd/device.c > +++ b/drivers/iommu/iommufd/device.c > @@ -293,6 +293,57 @@ u32 iommufd_device_to_id(struct iommufd_device *idev) > } > EXPORT_SYMBOL_NS_GPL(iommufd_device_to_id, IOMMUFD); > > +struct iommufd_attach_handle * > +iommufd_device_get_attach_handle(struct iommufd_device *idev) > +{ > + struct iommu_attach_handle *handle; > + > + handle = iommu_attach_handle_get(idev->igroup->group, IOMMU_NO_PASID, 0); > + if (IS_ERR(handle)) > + return NULL; > + > + return to_iommufd_handle(handle); > +} I would suggest placing this helper closer to where it is used. Because, there is currently no locking mechanism to synchronize threads that access the returned handle with those attaching or replacing the domain. This lack of synchronization could potentially lead to use-after-free issue. By placing the helper near its callers and perhaps adding comments explaining this limitation, we can improve maintainability and prevent misuse in the future. -- baolu
On 2024/11/5 13:21, Baolu Lu wrote: > On 11/4/24 21:25, Yi Liu wrote: >> The iommu_attach_handle is now only passed when attaching iopf-capable >> domain, while it is not convenient for the iommu core to track the >> attached domain of pasids. To address it, the iommu_attach_handle will >> be passed to iommu core for non-fault-able domain as well. Hence the >> iommufd_handle related helpers are no longer fault specific, it makes >> more sense to move it out of fault.c. >> >> Signed-off-by: Yi Liu<yi.l.liu@intel.com> >> --- >> drivers/iommu/iommufd/device.c | 51 ++++++++++++++++++++++ >> drivers/iommu/iommufd/fault.c | 56 +------------------------ >> drivers/iommu/iommufd/iommufd_private.h | 8 ++++ >> 3 files changed, 61 insertions(+), 54 deletions(-) >> >> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c >> index 5fd3dd420290..823c81145214 100644 >> --- a/drivers/iommu/iommufd/device.c >> +++ b/drivers/iommu/iommufd/device.c >> @@ -293,6 +293,57 @@ u32 iommufd_device_to_id(struct iommufd_device *idev) >> } >> EXPORT_SYMBOL_NS_GPL(iommufd_device_to_id, IOMMUFD); >> +struct iommufd_attach_handle * >> +iommufd_device_get_attach_handle(struct iommufd_device *idev) >> +{ >> + struct iommu_attach_handle *handle; >> + >> + handle = iommu_attach_handle_get(idev->igroup->group, >> IOMMU_NO_PASID, 0); >> + if (IS_ERR(handle)) >> + return NULL; >> + >> + return to_iommufd_handle(handle); >> +} > > I would suggest placing this helper closer to where it is used. Because, > there is currently no locking mechanism to synchronize threads that > access the returned handle with those attaching or replacing the domain. > This lack of synchronization could potentially lead to use-after-free > issue. > > By placing the helper near its callers and perhaps adding comments > explaining this limitation, we can improve maintainability and prevent > misuse in the future. with this comment, it seems better to put it in the header file. There are two files that has referred this helper. the fault.c and iommufd_private.h.
On Tue, Nov 05, 2024 at 04:01:24PM +0800, Yi Liu wrote: > > By placing the helper near its callers and perhaps adding comments > > explaining this limitation, we can improve maintainability and prevent > > misuse in the future. > > with this comment, it seems better to put it in the header file. There are > two files that has referred this helper. the fault.c and iommufd_private.h. Put kdoc comments at the function body in the .c file please Jason
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index 5fd3dd420290..823c81145214 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -293,6 +293,57 @@ u32 iommufd_device_to_id(struct iommufd_device *idev) } EXPORT_SYMBOL_NS_GPL(iommufd_device_to_id, IOMMUFD); +struct iommufd_attach_handle * +iommufd_device_get_attach_handle(struct iommufd_device *idev) +{ + struct iommu_attach_handle *handle; + + handle = iommu_attach_handle_get(idev->igroup->group, IOMMU_NO_PASID, 0); + if (IS_ERR(handle)) + return NULL; + + return to_iommufd_handle(handle); +} + +int iommufd_dev_attach_handle(struct iommufd_hw_pagetable *hwpt, + struct iommufd_device *idev) +{ + struct iommufd_attach_handle *handle; + int ret; + + handle = kzalloc(sizeof(*handle), GFP_KERNEL); + if (!handle) + return -ENOMEM; + + handle->idev = idev; + ret = iommu_attach_group_handle(hwpt->domain, idev->igroup->group, + &handle->handle); + if (ret) + kfree(handle); + + return ret; +} + +int iommufd_dev_replace_handle(struct iommufd_device *idev, + struct iommufd_hw_pagetable *hwpt, + struct iommufd_hw_pagetable *old) +{ + struct iommufd_attach_handle *handle; + int ret; + + handle = kzalloc(sizeof(*handle), GFP_KERNEL); + if (!handle) + return -ENOMEM; + + handle->idev = idev; + ret = iommu_replace_group_handle(idev->igroup->group, + hwpt->domain, &handle->handle); + if (ret) + kfree(handle); + + return ret; +} + static int iommufd_group_setup_msi(struct iommufd_group *igroup, struct iommufd_hwpt_paging *hwpt_paging) { diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c index 230d37c17102..add94b044dc6 100644 --- a/drivers/iommu/iommufd/fault.c +++ b/drivers/iommu/iommufd/fault.c @@ -55,25 +55,6 @@ static void iommufd_fault_iopf_disable(struct iommufd_device *idev) mutex_unlock(&idev->iopf_lock); } -static int __fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt, - struct iommufd_device *idev) -{ - struct iommufd_attach_handle *handle; - int ret; - - handle = kzalloc(sizeof(*handle), GFP_KERNEL); - if (!handle) - return -ENOMEM; - - handle->idev = idev; - ret = iommu_attach_group_handle(hwpt->domain, idev->igroup->group, - &handle->handle); - if (ret) - kfree(handle); - - return ret; -} - int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt, struct iommufd_device *idev) { @@ -86,7 +67,7 @@ int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt, if (ret) return ret; - ret = __fault_domain_attach_dev(hwpt, idev); + ret = iommufd_dev_attach_handle(hwpt, idev); if (ret) iommufd_fault_iopf_disable(idev); @@ -122,18 +103,6 @@ static void iommufd_auto_response_faults(struct iommufd_hw_pagetable *hwpt, mutex_unlock(&fault->mutex); } -static struct iommufd_attach_handle * -iommufd_device_get_attach_handle(struct iommufd_device *idev) -{ - struct iommu_attach_handle *handle; - - handle = iommu_attach_handle_get(idev->igroup->group, IOMMU_NO_PASID, 0); - if (IS_ERR(handle)) - return NULL; - - return to_iommufd_handle(handle); -} - void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt, struct iommufd_device *idev) { @@ -146,27 +115,6 @@ void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt, kfree(handle); } -static int -__fault_domain_replace_dev(struct iommufd_device *idev, - struct iommufd_hw_pagetable *hwpt, - struct iommufd_hw_pagetable *old) -{ - struct iommufd_attach_handle *handle; - int ret; - - handle = kzalloc(sizeof(*handle), GFP_KERNEL); - if (!handle) - return -ENOMEM; - - handle->idev = idev; - ret = iommu_replace_group_handle(idev->igroup->group, - hwpt->domain, &handle->handle); - if (ret) - kfree(handle); - - return ret; -} - int iommufd_fault_domain_replace_dev(struct iommufd_device *idev, struct iommufd_hw_pagetable *hwpt, struct iommufd_hw_pagetable *old) @@ -185,7 +133,7 @@ int iommufd_fault_domain_replace_dev(struct iommufd_device *idev, curr = iommufd_device_get_attach_handle(idev); if (hwpt->fault) - ret = __fault_domain_replace_dev(idev, hwpt, old); + ret = iommufd_dev_replace_handle(idev, hwpt, old); else ret = iommu_replace_group_handle(idev->igroup->group, hwpt->domain, NULL); diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index f1d865e6fab6..66eb95063068 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -478,6 +478,14 @@ struct iommufd_attach_handle { /* Convert an iommu attach handle to iommufd handle. */ #define to_iommufd_handle(hdl) container_of(hdl, struct iommufd_attach_handle, handle) +struct iommufd_attach_handle * +iommufd_device_get_attach_handle(struct iommufd_device *idev); +int iommufd_dev_attach_handle(struct iommufd_hw_pagetable *hwpt, + struct iommufd_device *idev); +int iommufd_dev_replace_handle(struct iommufd_device *idev, + struct iommufd_hw_pagetable *hwpt, + struct iommufd_hw_pagetable *old); + static inline struct iommufd_fault * iommufd_get_fault(struct iommufd_ucmd *ucmd, u32 id) {
The iommu_attach_handle is now only passed when attaching iopf-capable domain, while it is not convenient for the iommu core to track the attached domain of pasids. To address it, the iommu_attach_handle will be passed to iommu core for non-fault-able domain as well. Hence the iommufd_handle related helpers are no longer fault specific, it makes more sense to move it out of fault.c. Signed-off-by: Yi Liu <yi.l.liu@intel.com> --- drivers/iommu/iommufd/device.c | 51 ++++++++++++++++++++++ drivers/iommu/iommufd/fault.c | 56 +------------------------ drivers/iommu/iommufd/iommufd_private.h | 8 ++++ 3 files changed, 61 insertions(+), 54 deletions(-)