Re: [PATCH v5 03/12] iommufd: Move the iommufd_handle helpers to device.c

[Yi Liu <yi.l.liu@intel.com>]

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.

-- 
Regards,
Yi Liu