Re: [PATCH V8 mlx5-next 08/15] vfio: Have the core code decode the VFIO_DEVICE_FEATURE ioctl

[Cornelia Huck <cohuck@redhat.com>]

On Sun, Feb 20 2022, Yishai Hadas <yishaih@nvidia.com> wrote:

> From: Jason Gunthorpe <jgg@nvidia.com>
>
> Invoke a new device op 'device_feature' to handle just the data array
> portion of the command. This lifts the ioctl validation to the core code
> and makes it simpler for either the core code, or layered drivers, to
> implement their own feature values.
>
> Provide vfio_check_feature() to consolidate checking the flags/etc against
> what the driver supports.
>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
> ---
>  drivers/vfio/pci/vfio_pci.c      |  1 +
>  drivers/vfio/pci/vfio_pci_core.c | 94 +++++++++++++-------------------
>  drivers/vfio/vfio.c              | 46 ++++++++++++++--
>  include/linux/vfio.h             | 32 +++++++++++
>  include/linux/vfio_pci_core.h    |  2 +
>  5 files changed, 114 insertions(+), 61 deletions(-)
>

(...)

> +static int vfio_ioctl_device_feature(struct vfio_device *device,
> +				     struct vfio_device_feature __user *arg)
> +{
> +	size_t minsz = offsetofend(struct vfio_device_feature, flags);
> +	struct vfio_device_feature feature;
> +
> +	if (copy_from_user(&feature, arg, minsz))
> +		return -EFAULT;
> +
> +	if (feature.argsz < minsz)
> +		return -EINVAL;
> +
> +	/* Check unknown flags */
> +	if (feature.flags &
> +	    ~(VFIO_DEVICE_FEATURE_MASK | VFIO_DEVICE_FEATURE_SET |
> +	      VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_PROBE))
> +		return -EINVAL;
> +
> +	/* GET & SET are mutually exclusive except with PROBE */
> +	if (!(feature.flags & VFIO_DEVICE_FEATURE_PROBE) &&
> +	    (feature.flags & VFIO_DEVICE_FEATURE_SET) &&
> +	    (feature.flags & VFIO_DEVICE_FEATURE_GET))
> +		return -EINVAL;
> +
> +	switch (feature.flags & VFIO_DEVICE_FEATURE_MASK) {
> +	default:
> +		if (unlikely(!device->ops->device_feature))
> +			return -EINVAL;
> +		return device->ops->device_feature(device, feature.flags,
> +						   arg->data,
> +						   feature.argsz - minsz);
> +	}
> +}
> +
>  static long vfio_device_fops_unl_ioctl(struct file *filep,
>  				       unsigned int cmd, unsigned long arg)
>  {
>  	struct vfio_device *device = filep->private_data;
>  
> -	if (unlikely(!device->ops->ioctl))
> -		return -EINVAL;
> -
> -	return device->ops->ioctl(device, cmd, arg);
> +	switch (cmd) {
> +	case VFIO_DEVICE_FEATURE:
> +		return vfio_ioctl_device_feature(device, (void __user *)arg);
> +	default:
> +		if (unlikely(!device->ops->ioctl))
> +			return -EINVAL;
> +		return device->ops->ioctl(device, cmd, arg);
> +	}
>  }

One not-that-obvious change this is making is how VFIO_DEVICE_* ioctls
are processed. With this patch, VFIO_DEVICE_FEATURE is handled a bit
differently to other ioctl commands that are passed directly to the
device; here we have the common handling first, then control is passed
to the device. When I read in Documentation/driver-api/vfio.rst

"The ioctl interface provides a direct pass through for VFIO_DEVICE_*
ioctls."

I would not really expect that behaviour. No objection to introducing
it, but I think that needs a note in the doc, as you only see that if
you actually read the implementation (and not just the header and the
docs).