> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> 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.
