Re: [PATCH v5 06/19] iommufd: File descriptor, context, kconfig and makefiles

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Nov 18, 2022 at 05:27:35PM +0100, Eric Auger wrote:
> > +config IOMMUFD
> > +	tristate "IOMMU Userspace API"
> > +	select INTERVAL_TREE
> > +	select INTERVAL_TREE_SPAN_ITER
> > +	select IOMMU_API
> > +	default n
> > +	help
> > +	  Provides /dev/iommu the user API to control the IOMMU subsystem as
> > +	  it relates to managing IO page tables that point at user space memory.

> nit: missing ',' after /dev/iommu or Provides /dev/iommu user API

Done

> > +/**
> > + * iommufd_ref_to_users() - Switch from destroy_rwsem to users refcount
> > + *        protection
> > + * @obj - Object to release
> > + *
> > + * Objects have two refcount protections (destroy_rwsem and the refcount_t
> > + * users). Holding either of these will prevent the object from being destroyed.
> > + *
> > + * Depending on the use case, one protection or the other is appropriate.  In
> > + * most cases references are being protected by the destroy_rwsem. This allows
> > + * orderly destruction of the object because iommufd_object_destroy_user() will
> > + * wait for it to become unlocked. However, as a rwsem, it cannot be held across
> > + * a system call return. So cases that have longer term needs must switch
> > + * to the weaker users refcount_t.
> > + *
> > + * With users protection iommufd_object_destroy_user() will return -EBUSY to
> 
> iommufd_object_destroy_user() returns false and iommufd_destroy
>  retruns -EBUSY.

""
 * With users protection iommufd_object_destroy_user() will return false,
 * refusing to destroy the object, causing -EBUSY to userspace.
 */
""

> 
> > + * userspace and refuse to destroy the object.
> > + */
> > +static inline void iommufd_ref_to_users(struct iommufd_object *obj)
> > +{
> > +	up_read(&obj->destroy_rwsem);
> > +	/* iommufd_lock_obj() obtains users as well */
> Do you have a way to check that put() is done in accordance, ie. we are
> not going to try up_read() the rwsem if this latter is not used anymore?

If put becomes unbalanced then fd closure will WARN_ON

If someone misuses the rwsem (eg double up_reading it) then lockdep
will fire

> > +static int iommufd_fops_release(struct inode *inode, struct file *filp)
> > +{
> > +	struct iommufd_ctx *ictx = filp->private_data;
> > +	struct iommufd_object *obj;
> > +
> > +	/* Destroy the graph from depth first */
> I would suggest: destroy the leaf objects first thanks to the
> hierarchical user ref counting? or something alike

"depth first" is a technical term when working with graphs..

How about replacing both comments with this:

	/*
	 * The objects in the xarray form a graph of "users" counts, and we have
	 * to destroy them in a depth first manner. Leaf objects will reduce the
	 * users count of interior objects when they are destroyed.
	 *
	 * Repeatedly destroying all the "1 users" leaf objects will progress
	 * until the entire list is destroyed. If this can't progress then there
	 * is some bug related to object refcounting.
	 */

> > +	while (!xa_empty(&ictx->objects)) {
> > +		unsigned int destroyed = 0;
> > +		unsigned long index;
> > +
> > +		xa_for_each(&ictx->objects, index, obj) {
> > +			/*
> > +			 * Since we are in release elevated users must come from
> > +			 * other objects holding the users. We will eventually
> the sentense sounds a bit cryptic to me.
> > +			 * destroy the object that holds this one and the next
> > +			 * pass will progress it.
> > +			 */
> > +			if (!refcount_dec_if_one(&obj->users))
> > +				continue;
> > +			destroyed++;
> > +			xa_erase(&ictx->objects, index);
> > +			iommufd_object_ops[obj->type].destroy(obj);
> > +			kfree(obj);
> 
> Use iommufd_object_abort_and_destroy(obj) instead of the above 3 lines?

Ah, they are not quite the same things, the order is different and
abort has a protective assertion that the xa_array hasn't been messed
with. It would be messy to merge them

It is also very similar to iommufd_object_destroy_user() except we
shortcut some unncessary locking.

> > +/**
> > + * DOC: General ioctl format
> > + *
> > + * The ioctl interface follows a general format to allow for extensibility. Each
> > + * ioctl is passed in a structure pointer as the argument providing the size of
> > + * the structure in the first u32. The kernel checks that any structure space
> > + * beyond what it understands is 0. This allows userspace to use the backward
> > + * compatible portion while consistently using the newer, larger, structures.
> > + *
> > + * ioctls use a standard meaning for common errnos:
> > + *
> > + *  - ENOTTY: The IOCTL number itself is not supported at all
> > + *  - E2BIG: The IOCTL number is supported, but the provided structure has
> > + *    non-zero in a part the kernel does not understand.
> > + *  - EOPNOTSUPP: The IOCTL number is supported, and the structure is
> > + *    understood, however a known field has a value the kernel does not
> > + *    understand or support.
> > + *  - EINVAL: Everything about the IOCTL was understood, but a field is not
> > + *    correct.
> > + *  - ENOENT: An ID or IOVA provided does not exist.
> > + *  - ENOMEM: Out of memory.
> > + *  - EOVERFLOW: Mathematics oveflowed.
> overflowed

Done

Thanks,
Jason