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

[Jason Gunthorpe <jgg@ziepe.ca>]

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.

--- 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: