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@xxxxxxxxx> > > 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
