Re: [PATCH v6 12/24] vfio/pci: Allow passing zero-length fd array in VFIO_DEVICE_PCI_HOT_RESET

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Mar 20, 2023 at 04:52:17PM -0600, Alex Williamson wrote:

> > The APIs are well defined and userspace can always use them wrong. It
> > doesn't need to call RESET_INFO even today, it can just trivially pass
> > every group FD it owns to meet the security check.
> 
> That's not actually true, in order to avoid arbitrarily large buffers
> from the user, the ioctl won't accept an array greater than the number
> of devices affected by the reset.

Oh yuk!

> > It is much simpler if VFIO_DEVICE_PCI_HOT_RESET can pass the security
> > check without code marshalling fds, which is why we went this
> > direction.
> 
> I agree that nullifying the arg makes the ioctl easier to use, but my
> hesitation is whether it makes it more difficult to use correctly,
> which includes resetting devices unexpectedly.

I don't think it makes it harder to use correctly. It maybe makes it
easier to misuse, but IMHO not too much.

If the desire was to have an API that explicitly acknowledged the
reset scope then it should have taken in a list of device FDs and
optimally reset all of them or fail EPERM.

What is going to make this hard to use is the _INFO IOCTL, it returns
basically the BDF string, but I think we effectively get rid of this
in the new model. libvirt will know the BDF and open the cdev, then fd
pass the cdev to qemu. Qemu shouldn't also have to know the sysfs
path..

So we really want a new _INFO ioctl to make this easier to use..

> We can always blame the developer for using an interface incorrectly,
> but if we make it easier to use incorrectly in order to optimize
> something that doesn't need to be optimized, does that make it a good
> choice for the uAPI?

IMHO the API is designed around a security proof. Present some groups
and a subset of devices in those groups will be reset. You can't know
the subset unless you do the _INFO thing.

If we wanted it to be clearly linked to scope it should have taken in
a list of device FDs, and reset those devices FDs optimally or
returned -EPERM. Then the reset scope is very clearly connected to the
API.

Jason