> From: Zhao, Yan Y <yan.y.zhao@xxxxxxxxx>
> Sent: Thursday, February 16, 2023 5:23 PM
>
> On Thu, Feb 16, 2023 at 05:10:06PM +0800, Liu, Yi L wrote:
> > > From: Zhao, Yan Y <yan.y.zhao@xxxxxxxxx>
> > > Sent: Thursday, February 16, 2023 4:24 PM
> > >
> > > On Mon, Feb 13, 2023 at 07:13:47AM -0800, Yi Liu wrote:
> > > ...
> > >
> > > > +long vfio_device_ioctl_bind_iommufd(struct vfio_device_file *df,
> > > > + unsigned long arg)
> > > > +{
> > > > + struct vfio_device *device = df->device;
> > > > + struct vfio_device_bind_iommufd bind;
> > > > + struct iommufd_ctx *iommufd = NULL;
> > > > + struct fd f;
> > > > + unsigned long minsz;
> > > > + int ret;
> > > > +
> > > > + minsz = offsetofend(struct vfio_device_bind_iommufd, out_devid);
> > > > +
> > > > + if (copy_from_user(&bind, (void __user *)arg, minsz))
> > > > + return -EFAULT;
> > > > +
> > > > + if (bind.argsz < minsz || bind.flags)
> > > > + return -EINVAL;
> > > > +
> > > > + if (!device->ops->bind_iommufd)
> > > > + return -ENODEV;
> > > > +
> > > > + ret = vfio_device_claim_group(device);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + mutex_lock(&device->dev_set->lock);
> > > > + /*
> > > > + * If already been bound to an iommufd, or already set noiommu
> > > > + * then fail it.
> > > > + */
> > > > + if (df->iommufd || df->noiommu) {
> > > > + ret = -EINVAL;
> > > > + goto out_unlock;
> > > > + }
> > > > +
> > > > + /* iommufd < 0 means noiommu mode */
> > > > + if (bind.iommufd < 0) {
> > > > + if (!capable(CAP_SYS_RAWIO)) {
> > > > + ret = -EPERM;
> > > > + goto out_unlock;
> > > > + }
> > > > + df->noiommu = true;
> > > > + } else {
> > > > + f = fdget(bind.iommufd);
> > > Here, the iommufd file count + 1,
> > >
> > > > + if (!f.file) {
> > > > + ret = -EBADF;
> > > > + goto out_unlock;
> > > > + }
> > > > + iommufd = iommufd_ctx_from_file(f.file);
> > > iommufd file count + 1, again
> > >
> > > > + if (IS_ERR(iommufd)) {
> > > > + ret = PTR_ERR(iommufd);
> > > > + goto out_put_file;
> > > > + }
> > > > + }
> > > > +
> > > > + /*
> > > > + * Before the device open, get the KVM pointer currently
> > > > + * associated with the device file (if there is) and obtain a
> > > > + * reference. This reference is held until device closed. Save
> > > > + * the pointer in the device for use by drivers.
> > > > + */
> > > > + vfio_device_get_kvm_safe(df);
> > > > +
> > > > + df->iommufd = iommufd;
> > > > + ret = vfio_device_open(df, &bind.out_devid, NULL);
> > > iommufd file count + 1 in iommufd_device_bind for first open.
> > >
> > > > + if (ret)
> > > > + goto out_put_kvm;
> > > > +
> > > > + ret = copy_to_user((void __user *)arg +
> > > > + offsetofend(struct vfio_device_bind_iommufd,
> > > iommufd),
> > > > + &bind.out_devid,
> > > > + sizeof(bind.out_devid)) ? -EFAULT : 0;
> > > > + if (ret)
> > > > + goto out_close_device;
> > > > +
> > > > + if (iommufd)
> > > > + fdput(f);
> > > But, only one file count is put.
> >
> > Good catch! Yes it is missed. And needs to call iommufd_ctx_put()
> > in vfio_device_cdev_close() as well.
> If I read correctly, iommufd_device_bind() in the first open will
> get the reference through iommufd_ctx_get(ictx) and
> iommufd_device_destroy() in the last close will do the iommufd_ctx_put().
Aha, functionally no problem. Even storing iommufd_ctx in df without
an explicit get for iommufd_ctx is fine since iommufd_device_bind()
has an implicitly get for iommufd_ctx. But it appears to be better to
have an explicit get.
However, I need to admit, your fix can reduce the reference of iommufd file.
This may avoid file reference counting overflow if there are multiple devices
assigned to an application. I'm not sure how possible it is though. 😊 I'll see
if Alex or Jason have any preference.
Regards,
Yi Liu
> As vfio_device_ioctl_bind_iommufd() isn't paring with
> vfio_device_cdev_close(), I think the fix below is simpler :)
>
> > > Need a paring iommufd_ctx_put() after a successful
> > > iommufd_ctx_from_file()
> > > above to avoid iommufd_fops_release() never being called.
> > >
> > > e.g.
> > >
> > > @@ -1222,11 +1226,13 @@ static long
> > > vfio_device_ioctl_bind_iommufd(struct vfio_device_file *df,
> > > ret = -EBADF;
> > > goto out_unlock;
> > > }
> > > iommufd = iommufd_ctx_from_file(f.file);
> > > if (IS_ERR(iommufd)) {
> > > ret = PTR_ERR(iommufd);
> > > goto out_put_file;
> > > }
> > > + iommufd_ctx_put(iommufd);
> >
> > Since iommufd is recorded in df, so needs to hold refcount till
> > df->iommufd=NULL;
> >
> >
> > > }
> > >
> > > /* df->kvm is supposed to be set in vfio_device_file_set_kvm() */
> > >
> > > > + else if (df->noiommu)
> > > > + dev_warn(device->dev, "vfio-noiommu device used by user
> > > "
> > > > + "(%s:%d)\n", current->comm,
> > > task_pid_nr(current));
> > > > + mutex_unlock(&device->dev_set->lock);
> > > > + return 0;
> > > > +
> > > > +out_close_device:
> > > > + vfio_device_close(df);
> > > > +out_put_kvm:
> > > > + df->iommufd = NULL;
> > > > + df->noiommu = false;
> > > > + vfio_device_put_kvm(device);
> > > > +out_put_file:
> > > > + if (iommufd)
> > > > + fdput(f);
> > > > +out_unlock:
> > > > + mutex_unlock(&device->dev_set->lock);
> > > > + vfio_device_release_group(device);
> > > > + return ret;
> > > > +}
> > > > +
