Re: [PATCH v7 08/10] iommufd: Associate fault object with iommufd_hw_pgtable

[Baolu Lu <baolu.lu@linux.intel.com>]

On 2024/6/29 6:13, Jason Gunthorpe wrote:
> On Sun, Jun 16, 2024 at 02:11:53PM +0800, Lu Baolu wrote:
> 
>> @@ -308,13 +315,29 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd)
>>   		goto out_put_pt;
>>   	}
>>   
>> +	if (cmd->flags & IOMMU_HWPT_FAULT_ID_VALID) {
>> +		struct iommufd_fault *fault;
>> +
>> +		fault = iommufd_get_fault(ucmd, cmd->fault_id);
>> +		if (IS_ERR(fault)) {
>> +			rc = PTR_ERR(fault);
>> +			goto out_hwpt;
>> +		}
>> +		hwpt->fault = fault;
>> +		hwpt->domain->iopf_handler = iommufd_fault_iopf_handler;
>> +		hwpt->domain->fault_data = hwpt;
> 
> This is not the right refcounting for a longterm reference... The PT
> above shows the pattern:
> 
> 	pt_obj = iommufd_get_object(ucmd->ictx, cmd->pt_id, IOMMUFD_OBJ_ANY);
> 	hwpt_paging = iommufd_hwpt_paging_alloc()
>              	refcount_inc(&ioas->obj.users);
> 
> 	iommufd_put_object(ucmd->ictx, pt_obj);
> 
> Which is to say you need to incr users and then do the put object. And
> iommufd_object_abort_and_destroy() will always destroy the ref on the
> fault if the fault is non-null so the error handling will double free.
> 
> fail_nth is intended to catch this, but you have to add enough inputs
> to cover the new cases when you add them, it seems like that is
> missing in this series. ie add a fault object and hwpt alloc to a
> fail_nth test and see we execute the iommufd_ucmd_respond() failure
> path.

Yes. I will add below fail_nth case:

--- a/tools/testing/selftests/iommu/iommufd_utils.h
+++ b/tools/testing/selftests/iommu/iommufd_utils.h
@@ -201,6 +201,12 @@ static int _test_cmd_hwpt_alloc(int fd, __u32 
device_id, __u32 pt_id, __u32 ft_i
         ASSERT_EQ(0, _test_cmd_hwpt_alloc(self->fd, device_id, pt_id, 
fault_id, \
                                           flags, hwpt_id, data_type, 
data,      \
                                           data_len))
+#define test_err_hwpt_alloc_iopf(_errno, device_id, pt_id, fault_id, 
flags,     \
+                                hwpt_id, data_type, data, data_len) 
        \
+       EXPECT_ERRNO(_errno, 
        \
+                    _test_cmd_hwpt_alloc(self->fd, device_id, pt_id, 
fault_id, \
+                                         flags, hwpt_id, data_type, 
data,      \
+                                         data_len))

  #define test_cmd_hwpt_check_iotlb(hwpt_id, iotlb_id, expected) 
         \
         ({ 
        \
diff --git a/tools/testing/selftests/iommu/iommufd.c 
b/tools/testing/selftests/iommu/iommufd.c
index 5b0169875a4d..93634e53e95e 100644
--- a/tools/testing/selftests/iommu/iommufd.c
+++ b/tools/testing/selftests/iommu/iommufd.c
@@ -338,6 +338,10 @@ TEST_F(iommufd_ioas, alloc_hwpt_nested)
                                            &nested_hwpt_id[1],
                                            IOMMU_HWPT_DATA_SELFTEST, &data,
                                            sizeof(data));
+               test_err_hwpt_alloc_iopf(ENOENT, self->device_id, 
parent_hwpt_id,
+                                        UINT32_MAX, 
IOMMU_HWPT_FAULT_ID_VALID,
+                                        &iopf_hwpt_id, 
IOMMU_HWPT_DATA_SELFTEST,
+                                        &data, sizeof(data));
                 test_cmd_hwpt_alloc_iopf(self->device_id, 
parent_hwpt_id, fault_id,
                                          IOMMU_HWPT_FAULT_ID_VALID, 
&iopf_hwpt_id,
                                          IOMMU_HWPT_DATA_SELFTEST, &data,

> 
> --- a/drivers/iommu/iommufd/hw_pagetable.c
> +++ b/drivers/iommu/iommufd/hw_pagetable.c
> @@ -14,7 +14,7 @@ static void __iommufd_hwpt_destroy(struct iommufd_hw_pagetable *hwpt)
>                  iommu_domain_free(hwpt->domain);
>   
>          if (hwpt->fault)
> -               iommufd_put_object(hwpt->fault->ictx, &hwpt->fault->obj);
> +               refcount_dec(&hwpt->fault->obj.users);
>   }
>   
>   void iommufd_hwpt_paging_destroy(struct iommufd_object *obj)
> @@ -326,18 +326,17 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd)
>                  hwpt->fault = fault;
>                  hwpt->domain->iopf_handler = iommufd_fault_iopf_handler;
>                  hwpt->domain->fault_data = hwpt;
> +               refcount_inc(&fault->obj.users);
> +               iommufd_put_object(ucmd->ictx, &fault->obj);
>          }
>   
>          cmd->out_hwpt_id = hwpt->obj.id;
>          rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
>          if (rc)
> -               goto out_put_fault;
> +               goto out_hwpt;
>          iommufd_object_finalize(ucmd->ictx, &hwpt->obj);
>          goto out_unlock;
>   
> -out_put_fault:
> -       if (cmd->flags & IOMMU_HWPT_FAULT_ID_VALID)
> -               iommufd_put_object(ucmd->ictx, &hwpt->fault->obj);
>   out_hwpt:
>          iommufd_object_abort_and_destroy(ucmd->ictx, &hwpt->obj);
>   out_unlock:
> 

.. and merge above change to this patch.

Best regards,
baolu