[Intel-gfx] [PATCH v4 09/19] vfio/pci: Accept device fd for hot reset

[Tian, Kevin]

> From: Jason Gunthorpe <jgg at nvidia.com>
> Sent: Thursday, February 23, 2023 9:22 PM
> 
> On Thu, Feb 23, 2023 at 07:55:21AM +0000, Tian, Kevin wrote:
> > > From: Jason Gunthorpe
> > > Sent: Thursday, February 23, 2023 1:18 AM
> > >
> > > > > static bool vfio_dev_in_groups(struct vfio_pci_core_device *vdev,
> > > > >                                struct vfio_pci_group_info *groups)
> > > > > {
> > > > >  	unsigned int i;
> > > > >
> > > > > 	for (i = 0; i < groups->count; i++)
> > > > > 		if (vfio_file_has_dev(groups->files[i], &vdev->vdev))
> > > > > 			return true;
> > > > > 	return false;
> > > > > }
> > > > >
> > > > > Presumably when cdev fd is provided above should compare iommu
> > > > > group of the fd and that of the vdev. Otherwise it expects the user
> > > > > to have full access to every device in the set which is impractical.
> > >
> > > No, it should check the dev's directly, userspace has to provide every
> > > dev in the dev set to do a reset. We should not allow userspace to
> > > take a shortcut based on hidden group stuff.
> > >
> > > The dev set is already unrelated to the groups, and userspace cannot
> > > discover the devset, so nothing has changed.
> >
> > Agree. But I envision there might be a user-visible impact.
> >
> > Say a scenario where group happens to overlap with devset. Let's say
> > two devices in the group/devset.
> >
> > An existing deployment assigns only dev1 to Qemu. In this case dev1
> > is resettable via group fd given dev2 cannot be opened by another
> > user.
> 
> Oh, that is just because we took a shortcut in this logic and assumed
> that if the group is open then all the devices are opened by the same
> security domain.
> 
> But we can also more clearly state that any closed device is
> acceptable for reset and doesn't need to be presented.
> 
> So, like this:
> 
> 		if (cur_vma->vdev.open_count &&
> 		    !vfio_dev_in_groups(cur_vma, groups) &&
> 		    !iommufd_ctx_has_device(iommufd_ctx, &cur_vma-
> >pdev->dev)) {
> 			ret = -EINVAL;
> 			goto err_undo;
> 		}
> 

Yes, this makes sense.

Yi, while you are incorporating this change please also update the
uapi header. Rename 'group_fds[]' to 'fds[]' and add comment to
explain that it could be an array of group fds or a single iommufd.