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;
}
Jason
