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

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

On 2024/12/20 11:31, Baolu Lu wrote:
> On 12/19/24 21:27, 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          | 62 +++++++++++++++++++++++++
>>   drivers/iommu/iommufd/fault.c           | 56 +---------------------
>>   drivers/iommu/iommufd/iommufd_private.h |  8 ++++
>>   3 files changed, 72 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
>> index dfd0898fb6c1..0e1baf84e887 100644
>> --- a/drivers/iommu/iommufd/device.c
>> +++ b/drivers/iommu/iommufd/device.c
>> @@ -293,6 +293,68 @@ u32 iommufd_device_to_id(struct iommufd_device *idev)
>>   }
>>   EXPORT_SYMBOL_NS_GPL(iommufd_device_to_id, "IOMMUFD");
>> +/**
>> + * iommufd_device_get_attach_handle - Return the attach handle for the RID
>> + *
>> + * @idev: The device to get attach_handle
>> + *
>> + * Currently there is no locking to synchronize threads that access the
>> + * returned handle with those attaching or replacing the domain which might
>> + * change the handle. It's caller's duty to guarantee no use-after-free.
> 
> It's better to make "It's caller's duty to guarantee no use-after-free"
> more specific. Something like, the caller is responsible for ensuring
> that the returned pointer is not used after the domain is removed from
> the device's RID.

ok.

>> + *
>> + * Return valid attach_handle if there is, otherwise NULL.
>> + */
>> +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;
>> +}
> 
> Where will the old handle be freed? It seems unreasonable to allocate
> the handle in these helper functions, only to have it freed by callers
> in other files.

yes, it's in the caller side. See the below snippet in patch 02. Maybe
it's an over work to split this patch with patch 04. Merging them may
be helpful. Nic has a proposed patch as well for such purpose.

@@ -196,13 +187,24 @@ int iommufd_fault_domain_replace_dev(struct 
iommufd_device *idev,
  			return ret;
  	}

-	ret = __fault_domain_replace_dev(idev, hwpt, old);
+	curr = iommufd_device_get_attach_handle(idev);
+
+	if (hwpt->fault)
+		ret = __fault_domain_replace_dev(idev, hwpt, old);
+	else
+		ret = iommu_replace_group_handle(idev->igroup->group,
+						 hwpt->domain, NULL);
  	if (ret) {
  		if (iopf_on)
  			iommufd_fault_iopf_disable(idev);
  		return ret;
  	}

+	if (curr) {
+		iommufd_auto_response_faults(old, curr);
+		kfree(curr);
+	}
+


-- 
Regards,
Yi Liu