An IOMMU driver that allocated a vIOMMU may want to revert the allocation,
if it encounters an internal error after the allocation. So, there needs a
destroy helper for drivers to use. For instance:

static my_viommu_alloc()
{
	...
	my_viommu = iommufd_viommu_alloc(viomm, struct my_viommu, core);
	...
	ret = init_my_viommu();
	if (ret) {
		/* Need to destroy the my_viommu->core */
		return ERR_PTR(ret);
	}
	return &my_viommu->core;
}

Move iommufd_object_abort() to the driver.c file and the public header, to
introduce common iommufd_struct_destroy() helper that will abort all kinds
of driver structures, not confined to iommufd_viommu but also the new ones
being added in the future.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommufd/iommufd_private.h |  1 -
 include/linux/iommufd.h                 | 16 ++++++++++++++++
 drivers/iommu/iommufd/driver.c          | 14 ++++++++++++++
 drivers/iommu/iommufd/main.c            | 13 -------------
 4 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 5c69ac05c029..8d96aa514033 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -180,7 +180,6 @@ static inline void iommufd_put_object(struct iommufd_ctx *ictx,
 		wake_up_interruptible_all(&ictx->destroy_wait);
 }
 
-void iommufd_object_abort(struct iommufd_ctx *ictx, struct iommufd_object *obj);
 void iommufd_object_abort_and_destroy(struct iommufd_ctx *ictx,
 				      struct iommufd_object *obj);
 void iommufd_object_finalize(struct iommufd_ctx *ictx,
diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
index dc6535e848df..f6f333eaaae3 100644
--- a/include/linux/iommufd.h
+++ b/include/linux/iommufd.h
@@ -213,6 +213,7 @@ static inline int iommufd_vfio_compat_set_no_iommu(struct iommufd_ctx *ictx)
 struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,
 					     size_t size,
 					     enum iommufd_object_type type);
+void iommufd_object_abort(struct iommufd_ctx *ictx, struct iommufd_object *obj);
 struct device *iommufd_viommu_find_dev(struct iommufd_viommu *viommu,
 				       unsigned long vdev_id);
 int iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu,
@@ -228,6 +229,11 @@ _iommufd_object_alloc(struct iommufd_ctx *ictx, size_t size,
 	return ERR_PTR(-EOPNOTSUPP);
 }
 
+static inline void iommufd_object_abort(struct iommufd_ctx *ictx,
+					struct iommufd_object *obj)
+{
+}
+
 static inline struct device *
 iommufd_viommu_find_dev(struct iommufd_viommu *viommu, unsigned long vdev_id)
 {
@@ -285,4 +291,14 @@ static inline int iommufd_viommu_report_event(struct iommufd_viommu *viommu,
 		}                                                              \
 		ret;                                                           \
 	})
+
+/* Helper for IOMMU driver to destroy structures created by allocators above */
+#define iommufd_struct_destroy(drv_struct, member)                             \
+	({                                                                     \
+		static_assert(__same_type(struct iommufd_object,               \
+					  drv_struct->member.obj));            \
+		static_assert(offsetof(typeof(*drv_struct), member.obj) == 0); \
+		iommufd_object_abort(drv_struct->member.ictx,                  \
+				     &drv_struct->member.obj);                 \
+	})
 #endif
diff --git a/drivers/iommu/iommufd/driver.c b/drivers/iommu/iommufd/driver.c
index 922cd1fe7ec2..7980a09761c2 100644
--- a/drivers/iommu/iommufd/driver.c
+++ b/drivers/iommu/iommufd/driver.c
@@ -36,6 +36,20 @@ struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,
 }
 EXPORT_SYMBOL_NS_GPL(_iommufd_object_alloc, "IOMMUFD");
 
+/* Undo _iommufd_object_alloc() if iommufd_object_finalize() was not called */
+void iommufd_object_abort(struct iommufd_ctx *ictx, struct iommufd_object *obj)
+{
+	XA_STATE(xas, &ictx->objects, obj->id);
+	void *old;
+
+	xa_lock(&ictx->objects);
+	old = xas_store(&xas, NULL);
+	xa_unlock(&ictx->objects);
+	WARN_ON(old != XA_ZERO_ENTRY);
+	kfree(obj);
+}
+EXPORT_SYMBOL_NS_GPL(iommufd_object_abort, "IOMMUFD");
+
 /* Caller should xa_lock(&viommu->vdevs) to protect the return value */
 struct device *iommufd_viommu_find_dev(struct iommufd_viommu *viommu,
 				       unsigned long vdev_id)
diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
index 3df468f64e7d..2b9ee9b4a424 100644
--- a/drivers/iommu/iommufd/main.c
+++ b/drivers/iommu/iommufd/main.c
@@ -51,19 +51,6 @@ void iommufd_object_finalize(struct iommufd_ctx *ictx,
 	WARN_ON(old != XA_ZERO_ENTRY);
 }
 
-/* Undo _iommufd_object_alloc() if iommufd_object_finalize() was not called */
-void iommufd_object_abort(struct iommufd_ctx *ictx, struct iommufd_object *obj)
-{
-	XA_STATE(xas, &ictx->objects, obj->id);
-	void *old;
-
-	xa_lock(&ictx->objects);
-	old = xas_store(&xas, NULL);
-	xa_unlock(&ictx->objects);
-	WARN_ON(old != XA_ZERO_ENTRY);
-	kfree(obj);
-}
-
 /*
  * Abort an object that has been fully initialized and needs destroy, but has
  * not been finalized.
-- 
2.43.0

On Thu, May 08, 2025 at 08:02:27PM -0700, Nicolin Chen wrote:
> An IOMMU driver that allocated a vIOMMU may want to revert the allocation,
> if it encounters an internal error after the allocation. So, there needs a
> destroy helper for drivers to use. For instance:
> 
> static my_viommu_alloc()
> {
> 	...
> 	my_viommu = iommufd_viommu_alloc(viomm, struct my_viommu, core);
> 	...
> 	ret = init_my_viommu();
> 	if (ret) {
> 		/* Need to destroy the my_viommu->core */
> 		return ERR_PTR(ret);
> 	}
> 	return &my_viommu->core;
> }
> 
> Move iommufd_object_abort() to the driver.c file and the public header, to
> introduce common iommufd_struct_destroy() helper that will abort all kinds
> of driver structures, not confined to iommufd_viommu but also the new ones
> being added in the future.
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/iommufd/iommufd_private.h |  1 -
>  include/linux/iommufd.h                 | 16 ++++++++++++++++
>  drivers/iommu/iommufd/driver.c          | 14 ++++++++++++++
>  drivers/iommu/iommufd/main.c            | 13 -------------
>  4 files changed, 30 insertions(+), 14 deletions(-)

One idea that struck me when I was looking at this was to copy the
technique from rdma.

When an object is allocated we keep track of it in the struct
iommufd_ucmd.

Then when the command is over the core code either aborts or finalizes
the objects in the iommufd_ucmd. This way you don't have to expose
abort and related to drivers.

Something like this:

diff --git a/drivers/iommu/iommufd/driver.c b/drivers/iommu/iommufd/driver.c
index 2d2695e2562d02..289dd19eec90f1 100644
--- a/drivers/iommu/iommufd/driver.c
+++ b/drivers/iommu/iommufd/driver.c
@@ -36,6 +36,16 @@ struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,
 }
 EXPORT_SYMBOL_NS_GPL(_iommufd_object_alloc, "IOMMUFD");
 
+struct iommufd_object *_iommufd_object_alloc_ucmd(struct iommufd_ucmd *ucmd,
+						  size_t size,
+						  enum iommufd_object_type type)
+{
+	if (ucmd->alloced_obj)
+		return ERR_PTR(-EBUSY);
+	ucmd->alloced_obj = _iommufd_object_alloc(ucmd->ictx, size, type);
+	return ucmd->alloced_obj;
+}
+
 /* Undo _iommufd_object_alloc() if iommufd_object_finalize() was not called */
 void iommufd_object_abort(struct iommufd_ctx *ictx, struct iommufd_object *obj)
 {
diff --git a/drivers/iommu/iommufd/eventq.c b/drivers/iommu/iommufd/eventq.c
index f39cf079734769..f109948a81ac8c 100644
--- a/drivers/iommu/iommufd/eventq.c
+++ b/drivers/iommu/iommufd/eventq.c
@@ -473,7 +473,7 @@ int iommufd_fault_alloc(struct iommufd_ucmd *ucmd)
 	if (cmd->flags)
 		return -EOPNOTSUPP;
 
-	fault = __iommufd_object_alloc(ucmd->ictx, fault, IOMMUFD_OBJ_FAULT,
+	fault = __iommufd_object_alloc_ucmd(ucmd, fault, IOMMUFD_OBJ_FAULT,
 				       common.obj);
 	if (IS_ERR(fault))
 		return PTR_ERR(fault);
@@ -483,10 +483,8 @@ int iommufd_fault_alloc(struct iommufd_ucmd *ucmd)
 
 	fdno = iommufd_eventq_init(&fault->common, "[iommufd-pgfault]",
 				   ucmd->ictx, &iommufd_fault_fops);
-	if (fdno < 0) {
-		rc = fdno;
-		goto out_abort;
-	}
+	if (fdno < 0)
+		return fdno;
 
 	cmd->out_fault_id = fault->common.obj.id;
 	cmd->out_fault_fd = fdno;
@@ -494,7 +492,6 @@ int iommufd_fault_alloc(struct iommufd_ucmd *ucmd)
 	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
 	if (rc)
 		goto out_put_fdno;
-	iommufd_object_finalize(ucmd->ictx, &fault->common.obj);
 
 	fd_install(fdno, fault->common.filep);
 
@@ -502,9 +499,6 @@ int iommufd_fault_alloc(struct iommufd_ucmd *ucmd)
 out_put_fdno:
 	put_unused_fd(fdno);
 	fput(fault->common.filep);
-out_abort:
-	iommufd_object_abort_and_destroy(ucmd->ictx, &fault->common.obj);
-
 	return rc;
 }
 
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index c87326d7ecfccb..94cafae86d271c 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -152,6 +152,7 @@ struct iommufd_ucmd {
 	void __user *ubuffer;
 	u32 user_size;
 	void *cmd;
+	struct iommufd_object *alloced_obj;
 };
 
 int iommufd_vfio_ioctl(struct iommufd_ctx *ictx, unsigned int cmd,
@@ -258,6 +259,15 @@ iommufd_object_put_and_try_destroy(struct iommufd_ctx *ictx,
 #define iommufd_object_alloc(ictx, ptr, type) \
 	__iommufd_object_alloc(ictx, ptr, type, obj)
 
+#define __iommufd_object_alloc_uctx(uctx, ptr, type, obj)                      \
+	container_of(_iommufd_object_alloc_ucmd(                               \
+			     uctx,                                             \
+			     sizeof(*(ptr)) + BUILD_BUG_ON_ZERO(               \
+						      offsetof(typeof(*(ptr)), \
+							       obj) != 0),     \
+			     type),                                            \
+		     typeof(*(ptr)), obj)
+
 /*
  * The IO Address Space (IOAS) pagetable is a virtual page table backed by the
  * io_pagetable object. It is a user controlled mapping of IOVA -> PFNs. The
diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
index 1d7f3584aea0f7..0bc9c02f6bfc4f 100644
--- a/drivers/iommu/iommufd/main.c
+++ b/drivers/iommu/iommufd/main.c
@@ -408,6 +408,14 @@ static long iommufd_fops_ioctl(struct file *filp, unsigned int cmd,
 	if (ret)
 		return ret;
 	ret = op->execute(&ucmd);
+
+	if (ucmd.alloced_obj) {
+		if (ret)
+			iommufd_object_abort_and_destroy(ictx,
+							 ucmd.alloced_obj);
+		else
+			iommufd_object_finalize(ictx, ucmd.alloced_obj);
+	}
 	return ret;
 }

On Wed, May 14, 2025 at 03:26:00PM -0300, Jason Gunthorpe wrote:
> On Thu, May 08, 2025 at 08:02:27PM -0700, Nicolin Chen wrote:
> > An IOMMU driver that allocated a vIOMMU may want to revert the allocation,
> > if it encounters an internal error after the allocation. So, there needs a
> > destroy helper for drivers to use. For instance:
> > 
> > static my_viommu_alloc()
> > {
> > 	...
> > 	my_viommu = iommufd_viommu_alloc(viomm, struct my_viommu, core);
> > 	...
> > 	ret = init_my_viommu();
> > 	if (ret) {
> > 		/* Need to destroy the my_viommu->core */
> > 		return ERR_PTR(ret);
> > 	}
> > 	return &my_viommu->core;
> > }
> > 
> > Move iommufd_object_abort() to the driver.c file and the public header, to
> > introduce common iommufd_struct_destroy() helper that will abort all kinds
> > of driver structures, not confined to iommufd_viommu but also the new ones
> > being added in the future.
> > 
> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> > Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > ---
> >  drivers/iommu/iommufd/iommufd_private.h |  1 -
> >  include/linux/iommufd.h                 | 16 ++++++++++++++++
> >  drivers/iommu/iommufd/driver.c          | 14 ++++++++++++++
> >  drivers/iommu/iommufd/main.c            | 13 -------------
> >  4 files changed, 30 insertions(+), 14 deletions(-)
> 
> One idea that struck me when I was looking at this was to copy the
> technique from rdma.
> 
> When an object is allocated we keep track of it in the struct
> iommufd_ucmd.
> 
> Then when the command is over the core code either aborts or finalizes
> the objects in the iommufd_ucmd. This way you don't have to expose
> abort and related to drivers.

I see! Do you want this to apply to the all objects or just driver
allocated ones?

We would need a bigger preparatory series to roll out that to all
the allocators, and need to be careful at the existing abort() that
intertwines with other steps like an unlock().

Thanks
Nicolin

On Wed, May 14, 2025 at 12:21:37PM -0700, Nicolin Chen wrote:
> > Then when the command is over the core code either aborts or finalizes
> > the objects in the iommufd_ucmd. This way you don't have to expose
> > abort and related to drivers.
> 
> I see! Do you want this to apply to the all objects or just driver
> allocated ones?

I would do all the ones that can work that way easily

I think it would just be one patch, replace this patch with that one.

> We would need a bigger preparatory series to roll out that to all
> the allocators, and need to be careful at the existing abort() that
> intertwines with other steps like an unlock().

Those cases with special locking couldn't use it.

Jason

On Thu, May 15, 2025 at 09:49:11AM -0300, Jason Gunthorpe wrote:
> On Wed, May 14, 2025 at 12:21:37PM -0700, Nicolin Chen wrote:
> > > Then when the command is over the core code either aborts or finalizes
> > > the objects in the iommufd_ucmd. This way you don't have to expose
> > > abort and related to drivers.
> > 
> > I see! Do you want this to apply to the all objects or just driver
> > allocated ones?
> 
> I would do all the ones that can work that way easily
> 
> I think it would just be one patch, replace this patch with that one.
> 
> > We would need a bigger preparatory series to roll out that to all
> > the allocators, and need to be careful at the existing abort() that
> > intertwines with other steps like an unlock().
> 
> Those cases with special locking couldn't use it.

OK. I will add a patch in v5, replacing this one.

Thanks
Nicolin