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
