> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Sent: Monday, February 27, 2023 7:40 AM
> On Sun, Feb 26, 2023 at 08:59:01AM +0000, Liu, Yi L wrote:
> > > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > > Sent: Friday, February 24, 2023 10:36 AM
> > >
> > > On Fri, Feb 24, 2023 at 02:21:33AM +0000, Tian, Kevin wrote:
> > >
> > > > 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.
> > >
> > > Upon reflection we can probably make it even simpler and just have a 0
> > > length fd array mean to use the iommufd the vfio_device is already
> > > associated with
> > >
> > > And the check for correctness can be simplified to simply see if each
> > > vfio_device in the dev_set is attached to the same iommufd ctx
> > > pointer instead of searching the xarray.
> >
> > Sorry, it appears to me the below concern is not solved as above logic
> > still requires userspace to open and bind devices to the same iommufd.
> >
> > "
> > > 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.
> > "
>
> You solve this by checking for a 0 open count as already discussed.
Ok. this scenario is solved. so if open_count is non-zero, it should be
either bound with iommufd or should be within groups as your below
suggestion.
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;
}
Btw. In cdev path, open_count++ and iommufd bound are done in a
single dev_set->lock lock and unlock pair, so if cur_vma->vdev has
iommufd_ctx, then its open_count is non-zero. I have another scenario
that dev1 and dev2 are from different groups but happen to be in
the same dev_set, and userspace only opens dev1, this logic will allow
userspace to reset dev1, but dev2 may be opened by another userspace.
This seems to be a problem in my prior thinking. However, dev_set->lock
Is held in the reset path, so other userspace cannot open and bind
cur_vma->vdev to iommufd during reset. 😊
Regards,
Yi Liu
