Re: [PATCH v3 03/15] vfio: Accept vfio device file in the driver facing kAPI

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Feb 15, 2023 at 02:43:20PM +0000, Liu, Yi L wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Wednesday, February 15, 2023 8:39 PM
> > 
> > On Tue, Feb 14, 2023 at 02:02:37AM +0000, Liu, Yi L wrote:
> > > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > > Sent: Tuesday, February 14, 2023 7:44 AM
> > > >
> > > > On Mon, Feb 13, 2023 at 07:13:36AM -0800, Yi Liu wrote:
> > > > > +static struct vfio_device *vfio_device_from_file(struct file *file)
> > > > > +{
> > > > > +	struct vfio_device_file *df = file->private_data;
> > > > > +
> > > > > +	if (file->f_op != &vfio_device_fops)
> > > > > +		return NULL;
> > > > > +	return df->device;
> > > > > +}
> > > > > +
> > > > >  /**
> > > > >   * vfio_file_is_valid - True if the file is usable with VFIO APIS
> > > > >   * @file: VFIO group file or VFIO device file
> > > > >   */
> > > > >  bool vfio_file_is_valid(struct file *file)
> > > > >  {
> > > > > -	return vfio_group_from_file(file);
> > > > > +	return vfio_group_from_file(file) ||
> > > > > +	       vfio_device_from_file(file);
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(vfio_file_is_valid);
> > > >
> > > > This can only succeed on a device cdev that has been fully opened.
> > >
> > > Actually, we cannot. This is used in the kvm-vfio code to see if the
> > > user-provided fd is vfio fds in the SET_KVM path. And we don't
> > > have the device cdev fully opened until BIND_IOMMUFD. But we do
> > > need to invoke SET_KVM before issuing BIND_IOMMUFD as the device
> > > open needs kvm pointer. So if we cannot apply fully opened limit to this
> > > interface. Maybe an updated function comment is needed.
> > 
> > This also seems sketchy, KVM is using the VFIO fd as a "proof" to
> > enable the wbinvd stuff. A half opened cdev should not be used as that
> > proof.
> 
> From this angle, the group path seems has the same concern. Device is not
> opened until VFIO_GROUP_GET_DEVICE_FD. 

Well, classically the device was DMA ownership claimed at least.

> But group path has one advantage, which make it ok. Group can only be
> opened by one application. So once it is opened, the devices within the
> group are somehow obtained by the application until group fd close.

It depends on what do we want the KVM proof to actually mean.

Is simply having permissions on the cdev node sufficient proof for
wbinvd?

I admit I poorly understand the threat model for this in kvm beyond
that kvm doesn't want everyone to use wbinvd.

> > Regardless it needs to be fixed for the pci usage.
> 
> For the pci usage, does my below reply make any sense?
> 
> https://lore.kernel.org/kvm/DS0PR11MB7529CFCE99E8A77AAC76DC7CC3A39@DS0PR11MB7529.namprd11.prod.outlook.com/T/#m7c00ae5dcae15f42b6dc0b3767c7037b99f53a56

You basically end up with two APIs that test two different levels of
openeness (I have permissions vs I actually am the driver owning this device)

Document it carefully at least

Jason