Re: [PATCH rc 2/3] iommufd: IOMMUFD_DESTROY should not increase the refcount

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Jul 27, 2023 at 05:25:33AM +0000, Tian, Kevin wrote:

> > It has the downside that if userspace races destroy with other operations
> > it will get an EBUSY instead of waiting, but this is kind of racing is
> > already dangerous.
> 
> it's not a new downside. Even old code also returns -EBUSY if
> iommufd_object_destroy_user() returns false.

It sort of is, previously you could race ioctls and destroy would wait
for the ioctl to finish, now it returns -EBUSY


> > +
> >  /*
> >   * The caller holds a users refcount and wants to destroy the object. Returns
> 
> s/users/user's/

'users' is the name of the variable
 
> > +	/*
> > +	 * If there is a bug and we couldn't destroy the object then we did put
> > +	 * back the callers refcount and will eventually try to free it again
> 
> s/callers/caller's/

Yep
 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> 
> btw,
> 
> > -	iommufd_ref_to_users(obj);
> > -	/* See iommufd_ref_to_users() */
> > -	if (!iommufd_object_destroy_user(ucmd->ictx, obj))
> > -		return -EBUSY;
> 
> I wonder whether there is any other reason to keep iommufd_ref_to_users().
> Now the only invocation is in iommufd_access_attach(), but it could be
> simply replaced by "get_object(); refcount_inc(); put_object()" as all other
> places are doing.

Hmm, yes, the replace series could probably be tweaked to comfortably
do this.

Jason