diff mbox series

[v5,03/13] iommufd: Add iommufd_verify_unfinalized_object

Message ID e99946ea0128370349373c94339d465397946137.1729897352.git.nicolinc@nvidia.com
State New
Headers show
Series iommufd: Add vIOMMU infrastructure (Part-1) | expand

Commit Message

Nicolin Chen Oct. 25, 2024, 11:49 p.m. UTC 
To support driver-allocated vIOMMU objects, it's suggested to call the
allocator helper in IOMMU dirvers. However, there is no guarantee that
drivers will all use it and allocate objects properly.

Add a helper for iommufd core to verify if an unfinalized object is at
least reserved in the ictx.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommufd/iommufd_private.h |  3 +++
 drivers/iommu/iommufd/main.c            | 20 ++++++++++++++++++++
 2 files changed, 23 insertions(+)

Comments

Jason Gunthorpe Oct. 29, 2024, 6:55 p.m. UTC | #1 
On Tue, Oct 29, 2024 at 09:18:05AM -0700, Nicolin Chen wrote:
> On Tue, Oct 29, 2024 at 11:49:07AM -0300, Jason Gunthorpe wrote:
> > On Fri, Oct 25, 2024 at 04:49:43PM -0700, Nicolin Chen wrote:
> > > To support driver-allocated vIOMMU objects, it's suggested to call the
> > > allocator helper in IOMMU dirvers. However, there is no guarantee that
> > > drivers will all use it and allocate objects properly.
> > > 
> > > Add a helper for iommufd core to verify if an unfinalized object is at
> > > least reserved in the ictx.
> > 
> > I don't think we need this..
> > 
> > iommufd_object_finalize() already does:
> > 
> > 	old = xa_store(&ictx->objects, obj->id, obj, GFP_KERNEL);
> > 	/* obj->id was returned from xa_alloc() so the xa_store() cannot fail */
> > 	WARN_ON(old);
> 
> It feels unsafe to carry on the iommufd_viommu_alloc_ioctl() until
> iommufd_object_finalize() as the function would touch the returned
> faulty viommu pointer? E.g. what if the viommu has an even smaller
> size than struct iommufd_viommu?

This is Linux just because the output came from a driver doesn't mean
we have to validate it somehow. It is reasonable to be helpful and
detect driver bugs, but if the driver is buggy it is still OK to
crash.

So you don't *have* to check any of this, if the driver didn't use the
right function to allocate the memory then it will go bad pretty fast.

Improving the xa_store() is something that will detect more kinds of
bugs everywhere, so seems more worthwhile

> I think we'd need the same change in iommufd_object_abort() too.

Makes sense

Jason
diff mbox series

Patch

diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 5bd41257f2ef..d53c1ca75532 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -152,6 +152,9 @@  static inline void iommufd_put_object(struct iommufd_ctx *ictx,
 		wake_up_interruptible_all(&ictx->destroy_wait);
 }
 
+int iommufd_verify_unfinalized_object(struct iommufd_ctx *ictx,
+				      struct iommufd_object *to_verify);
+
 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);
diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
index 92bd075108e5..e244fed1b7ab 100644
--- a/drivers/iommu/iommufd/main.c
+++ b/drivers/iommu/iommufd/main.c
@@ -89,6 +89,26 @@  struct iommufd_object *iommufd_get_object(struct iommufd_ctx *ictx, u32 id,
 	return obj;
 }
 
+int iommufd_verify_unfinalized_object(struct iommufd_ctx *ictx,
+				      struct iommufd_object *to_verify)
+{
+	XA_STATE(xas, &ictx->objects, 0);
+	struct iommufd_object *obj;
+	int rc = 0;
+
+	if (!to_verify || !to_verify->id)
+		return -EINVAL;
+	xas.xa_index = to_verify->id;
+
+	xa_lock(&ictx->objects);
+	obj = xas_load(&xas);
+	/* Being an unfinalized object, the loaded obj is a reserved space */
+	if (obj != XA_ZERO_ENTRY)
+		rc = -ENOENT;
+	xa_unlock(&ictx->objects);
+	return rc;
+}
+
 static int iommufd_object_dec_wait_shortterm(struct iommufd_ctx *ictx,
 					     struct iommufd_object *to_destroy)
 {