An object allocator needs to call either iommufd_object_finalize() upon a
success or iommufd_object_abort_and_destroy() upon an error code.
To reduce duplication, store a new_obj in the struct iommufd_ucmd and call
iommufd_object_finalize/iommufd_object_abort_and_destroy() accordingly in
the main function.
Similar to iommufd_object_alloc() and __iommufd_object_alloc(), add a pair
of helpers: __iommufd_object_alloc_ucmd() and iommufd_object_alloc_ucmd().
Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
drivers/iommu/iommufd/iommufd_private.h | 26 +++++++++++++++++++++++++
drivers/iommu/iommufd/main.c | 25 ++++++++++++++++++++++++
2 files changed, 51 insertions(+)
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index ec5b499d139c..4f5e8cd99c96 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -135,6 +135,7 @@ struct iommufd_ucmd {
void __user *ubuffer;
u32 user_size;
void *cmd;
+ struct iommufd_object *new_obj;
};
int iommufd_vfio_ioctl(struct iommufd_ctx *ictx, unsigned int cmd,
@@ -230,6 +231,11 @@ iommufd_object_put_and_try_destroy(struct iommufd_ctx *ictx,
iommufd_object_remove(ictx, obj, obj->id, 0);
}
+/*
+ * Callers of these normal object allocators must call iommufd_object_finalize()
+ * to finalize the object, or call iommufd_object_abort_and_destroy() to revert
+ * the allocation.
+ */
struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,
size_t size,
enum iommufd_object_type type);
@@ -246,6 +252,26 @@ struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,
#define iommufd_object_alloc(ictx, ptr, type) \
__iommufd_object_alloc(ictx, ptr, type, obj)
+/*
+ * Callers of these _ucmd allocators should not call iommufd_object_finalize()
+ * or iommufd_object_abort_and_destroy(), as the core automatically does that.
+ */
+struct iommufd_object *
+_iommufd_object_alloc_ucmd(struct iommufd_ucmd *ucmd, size_t size,
+ enum iommufd_object_type type);
+
+#define __iommufd_object_alloc_ucmd(ucmd, ptr, type, obj) \
+ container_of(_iommufd_object_alloc_ucmd( \
+ ucmd, \
+ sizeof(*(ptr)) + BUILD_BUG_ON_ZERO( \
+ offsetof(typeof(*(ptr)), \
+ obj) != 0), \
+ type), \
+ typeof(*(ptr)), obj)
+
+#define iommufd_object_alloc_ucmd(ucmd, ptr, type) \
+ __iommufd_object_alloc_ucmd(ucmd, ptr, type, 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 85ad2853da0b..778694d7c207 100644
--- a/drivers/iommu/iommufd/main.c
+++ b/drivers/iommu/iommufd/main.c
@@ -61,6 +61,24 @@ struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,
return ERR_PTR(rc);
}
+struct iommufd_object *_iommufd_object_alloc_ucmd(struct iommufd_ucmd *ucmd,
+ size_t size,
+ enum iommufd_object_type type)
+{
+ struct iommufd_object *new_obj;
+
+ /* Something is coded wrong if this is hit */
+ if (WARN_ON(ucmd->new_obj))
+ return ERR_PTR(-EBUSY);
+
+ new_obj = _iommufd_object_alloc(ucmd->ictx, size, type);
+ if (IS_ERR(new_obj))
+ return new_obj;
+
+ ucmd->new_obj = new_obj;
+ return new_obj;
+}
+
/*
* Allow concurrent access to the object.
*
@@ -448,6 +466,13 @@ static long iommufd_fops_ioctl(struct file *filp, unsigned int cmd,
if (ret)
return ret;
ret = op->execute(&ucmd);
+
+ if (ucmd.new_obj) {
+ if (ret)
+ iommufd_object_abort_and_destroy(ictx, ucmd.new_obj);
+ else
+ iommufd_object_finalize(ictx, ucmd.new_obj);
+ }
return ret;
}
--
2.43.0> @@ -61,6 +61,24 @@ struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,
> return ERR_PTR(rc);
> }
>
> +struct iommufd_object *_iommufd_object_alloc_ucmd(struct iommufd_ucmd *ucmd,
> + size_t size,
> + enum iommufd_object_type type)
> +{
> + struct iommufd_object *new_obj;
> +
> + /* Something is coded wrong if this is hit */
> + if (WARN_ON(ucmd->new_obj))
> + return ERR_PTR(-EBUSY);
> +
> + new_obj = _iommufd_object_alloc(ucmd->ictx, size, type);
> + if (IS_ERR(new_obj))
> + return new_obj;
> +
> + ucmd->new_obj = new_obj;
> + return new_obj;
> +}
> +
> /*
> * Allow concurrent access to the object.
> *
> @@ -448,6 +466,13 @@ static long iommufd_fops_ioctl(struct file *filp, unsigned int cmd,
> if (ret)
> return ret;
> ret = op->execute(&ucmd);
> +
> + if (ucmd.new_obj) {
> + if (ret)
> + iommufd_object_abort_and_destroy(ictx, ucmd.new_obj);
Sorry I didn't follow this thread before and maybe missed something.
According to 70eadc7fc7ef, abort op is for the object that can assume
the caller is holding the lock. But here is for no locking, so calling
iommufd_object_abort_and_destroy() is quite confusing.
Is it better we change to:
if (ret) {
iommufd_object_ops[obj->type].destroy(obj);
iommufd_object_abort(ictx, obj);
}
Also explicitely assert iommufd_object_alloc_ucmd() and abort can't be
used at the same time.
in _iommufd_object_alloc_ucmd():
if (WARN_ON(iommufd_object_ops[type].abort))
return ERR_PTR(-EFAULT);
Thanks,
Yilun
> + else
> + iommufd_object_finalize(ictx, ucmd.new_obj);
> + }
> return ret;
> }
>
> --
> 2.43.0
>
>
> From: Xu Yilun <yilun.xu@linux.intel.com>
> Sent: Wednesday, July 9, 2025 1:32 PM
>
> > @@ -61,6 +61,24 @@ struct iommufd_object
> *_iommufd_object_alloc(struct iommufd_ctx *ictx,
> > return ERR_PTR(rc);
> > }
> >
> > +struct iommufd_object *_iommufd_object_alloc_ucmd(struct
> iommufd_ucmd *ucmd,
> > + size_t size,
> > + enum iommufd_object_type
> type)
> > +{
> > + struct iommufd_object *new_obj;
> > +
> > + /* Something is coded wrong if this is hit */
> > + if (WARN_ON(ucmd->new_obj))
> > + return ERR_PTR(-EBUSY);
> > +
> > + new_obj = _iommufd_object_alloc(ucmd->ictx, size, type);
> > + if (IS_ERR(new_obj))
> > + return new_obj;
> > +
> > + ucmd->new_obj = new_obj;
> > + return new_obj;
> > +}
> > +
> > /*
> > * Allow concurrent access to the object.
> > *
> > @@ -448,6 +466,13 @@ static long iommufd_fops_ioctl(struct file *filp,
> unsigned int cmd,
> > if (ret)
> > return ret;
> > ret = op->execute(&ucmd);
> > +
> > + if (ucmd.new_obj) {
> > + if (ret)
> > + iommufd_object_abort_and_destroy(ictx,
> ucmd.new_obj);
>
> Sorry I didn't follow this thread before and maybe missed something.
>
> According to 70eadc7fc7ef, abort op is for the object that can assume
> the caller is holding the lock. But here is for no locking, so calling
> iommufd_object_abort_and_destroy() is quite confusing.
>
> Is it better we change to:
>
> if (ret) {
> iommufd_object_ops[obj->type].destroy(obj);
> iommufd_object_abort(ictx, obj);
> }
I'd keep the original way. The function name describes what to do,
not what to be called exactly inside. Lacking of the abort method
doesn't change the meaning of the function which is about abort
and destroy (just like how it's called before introducing @abort).
>
> Also explicitely assert iommufd_object_alloc_ucmd() and abort can't be
> used at the same time.
>
> in _iommufd_object_alloc_ucmd():
>
> if (WARN_ON(iommufd_object_ops[type].abort))
> return ERR_PTR(-EFAULT);
>
but this check sounds necessary.
On Thu, Jul 10, 2025 at 05:32:13AM +0000, Tian, Kevin wrote: > > From: Xu Yilun <yilun.xu@linux.intel.com> > > Also explicitely assert iommufd_object_alloc_ucmd() and abort can't be > > used at the same time. > > > > in _iommufd_object_alloc_ucmd(): > > > > if (WARN_ON(iommufd_object_ops[type].abort)) > > return ERR_PTR(-EFAULT); > > > > but this check sounds necessary. Let me send a patch for this. Thanks Nicolin
On Fri, Jun 13, 2025 at 11:35:25PM -0700, Nicolin Chen wrote: > An object allocator needs to call either iommufd_object_finalize() upon a > success or iommufd_object_abort_and_destroy() upon an error code. > > To reduce duplication, store a new_obj in the struct iommufd_ucmd and call > iommufd_object_finalize/iommufd_object_abort_and_destroy() accordingly in > the main function. > > Similar to iommufd_object_alloc() and __iommufd_object_alloc(), add a pair > of helpers: __iommufd_object_alloc_ucmd() and iommufd_object_alloc_ucmd(). > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > drivers/iommu/iommufd/iommufd_private.h | 26 +++++++++++++++++++++++++ > drivers/iommu/iommufd/main.c | 25 ++++++++++++++++++++++++ > 2 files changed, 51 insertions(+) > > -- Acked-by: Pranjal Shrivastava <praan@google.com> > 2.43.0 >
On 6/14/25 14:35, Nicolin Chen wrote: > An object allocator needs to call either iommufd_object_finalize() upon a > success or iommufd_object_abort_and_destroy() upon an error code. > > To reduce duplication, store a new_obj in the struct iommufd_ucmd and call > iommufd_object_finalize/iommufd_object_abort_and_destroy() accordingly in > the main function. > > Similar to iommufd_object_alloc() and __iommufd_object_alloc(), add a pair > of helpers: __iommufd_object_alloc_ucmd() and iommufd_object_alloc_ucmd(). > > Suggested-by: Jason Gunthorpe<jgg@nvidia.com> > Reviewed-by: Kevin Tian<kevin.tian@intel.com> > Reviewed-by: Jason Gunthorpe<jgg@nvidia.com> > Signed-off-by: Nicolin Chen<nicolinc@nvidia.com> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
© 2016 - 2025 Red Hat, Inc.