On Thu, 30 Mar 2023 19:44:55 -0300
Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> On Thu, Mar 30, 2023 at 12:48:03PM +0000, Liu, Yi L wrote:
> > + /*
> > + * If dev_id is needed, fill in the dev_id field, otherwise
> > + * fill in group_id.
> > + */
> > + if (fill->require_devid) {
> > + /*
> > + * Report the devices that are opened as cdev and have
> > + * the same iommufd with the fill->iommufd. Otherwise,
> > + * just fill in an IOMMUFD_INVALID_ID.
> > + */
> > + vdev = vfio_pci_find_device_in_devset(dev_set, pdev);
> > + if (vdev && !vfio_device_cdev_opened(vdev) &&
> > + fill->iommufd == vfio_iommufd_physical_ictx(vdev))
> > + vfio_iommufd_physical_devid(vdev, &fill->devices[fill->cur].dev_id);
> > + fill->devices[fill->cur].dev_id = IOMMUFD_INVALID_ID;
>
> This needs an else?
>
> I suggest to check for VFIO_PCI_HOT_RESET_FLAG_IOMMUFD_DEV_ID on input
> as well. I know the old kernels don't enforce this but at least we
> could start enforcing it going forward so that the group path would
> reject it to catch userspace bugs.
>
> May as well fix it up to fully validate the flags
Is this under the guise of "if nobody complains it's ok, otherwise
revert" plan? We report dev-id based on the nature of the device, not
the provided flags, so I'm not sure I follow how this protects the group
path, unless we've failed to clear the output flags on that path with
this change. Thanks,
Alex
