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

[Alex Williamson <alex.williamson@redhat.com>]

On Sun, 30 Jan 2022 18:08:18 +0200
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>
> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
> ---
>  drivers/vfio/pci/vfio_pci.c      |  1 +
>  drivers/vfio/pci/vfio_pci_core.c | 90 ++++++++++++--------------------
>  drivers/vfio/vfio.c              | 46 ++++++++++++++--
>  include/linux/vfio.h             | 32 ++++++++++++
>  include/linux/vfio_pci_core.h    |  2 +
>  5 files changed, 109 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index a5ce92beb655..2b047469e02f 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -130,6 +130,7 @@ static const struct vfio_device_ops vfio_pci_ops = {
>  	.open_device	= vfio_pci_open_device,
>  	.close_device	= vfio_pci_core_close_device,
>  	.ioctl		= vfio_pci_core_ioctl,
> +	.device_feature = vfio_pci_core_ioctl_feature,
>  	.read		= vfio_pci_core_read,
>  	.write		= vfio_pci_core_write,
>  	.mmap		= vfio_pci_core_mmap,
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index f948e6cd2993..14a22ff20ef8 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -1114,70 +1114,44 @@ long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd,
>  
>  		return vfio_pci_ioeventfd(vdev, ioeventfd.offset,
>  					  ioeventfd.data, count, ioeventfd.fd);
> -	} else if (cmd == VFIO_DEVICE_FEATURE) {
> -		struct vfio_device_feature feature;
> -		uuid_t uuid;
> -
> -		minsz = offsetofend(struct vfio_device_feature, flags);
> -
> -		if (copy_from_user(&feature, (void __user *)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) {
> -		case VFIO_DEVICE_FEATURE_PCI_VF_TOKEN:
> -			if (!vdev->vf_token)
> -				return -ENOTTY;
> -
> -			/*
> -			 * We do not support GET of the VF Token UUID as this
> -			 * could expose the token of the previous device user.
> -			 */
> -			if (feature.flags & VFIO_DEVICE_FEATURE_GET)
> -				return -EINVAL;
> -
> -			if (feature.flags & VFIO_DEVICE_FEATURE_PROBE)
> -				return 0;
> -
> -			/* Don't SET unless told to do so */
> -			if (!(feature.flags & VFIO_DEVICE_FEATURE_SET))
> -				return -EINVAL;
> +	}
> +	return -ENOTTY;
> +}
> +EXPORT_SYMBOL_GPL(vfio_pci_core_ioctl);
>  
> -			if (feature.argsz < minsz + sizeof(uuid))
> -				return -EINVAL;
> +int vfio_pci_core_ioctl_feature(struct vfio_device *device, u32 flags,
> +				void __user *arg, size_t argsz)
> +{
> +	struct vfio_pci_core_device *vdev =
> +		container_of(device, struct vfio_pci_core_device, vdev);
> +	uuid_t uuid;
> +	int ret;

Nit, should uuid at least be scoped within the token code?  Or token
code pushed to a separate function?

>  
> -			if (copy_from_user(&uuid, (void __user *)(arg + minsz),
> -					   sizeof(uuid)))
> -				return -EFAULT;
> +	switch (flags & VFIO_DEVICE_FEATURE_MASK) {
> +	case VFIO_DEVICE_FEATURE_PCI_VF_TOKEN:
> +		if (!vdev->vf_token)
> +			return -ENOTTY;
> +		/*
> +		 * We do not support GET of the VF Token UUID as this could
> +		 * expose the token of the previous device user.
> +		 */
> +		ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_SET,
> +					sizeof(uuid));
> +		if (ret != 1)
> +			return ret;
>  
> -			mutex_lock(&vdev->vf_token->lock);
> -			uuid_copy(&vdev->vf_token->uuid, &uuid);
> -			mutex_unlock(&vdev->vf_token->lock);
> +		if (copy_from_user(&uuid, arg, sizeof(uuid)))
> +			return -EFAULT;
>  
> -			return 0;
> -		default:
> -			return -ENOTTY;
> -		}
> +		mutex_lock(&vdev->vf_token->lock);
> +		uuid_copy(&vdev->vf_token->uuid, &uuid);
> +		mutex_unlock(&vdev->vf_token->lock);
> +		return 0;
> +	default:
> +		return -ENOTTY;
>  	}
> -
> -	return -ENOTTY;
>  }
> -EXPORT_SYMBOL_GPL(vfio_pci_core_ioctl);
> +EXPORT_SYMBOL_GPL(vfio_pci_core_ioctl_feature);
...
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 76191d7abed1..ca69516f869d 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -55,6 +55,7 @@ struct vfio_device {
>   * @match: Optional device name match callback (return: 0 for no-match, >0 for
>   *         match, -errno for abort (ex. match with insufficient or incorrect
>   *         additional args)
> + * @device_feature: Fill in the VFIO_DEVICE_FEATURE ioctl
>   */
>  struct vfio_device_ops {
>  	char	*name;
> @@ -69,8 +70,39 @@ struct vfio_device_ops {
>  	int	(*mmap)(struct vfio_device *vdev, struct vm_area_struct *vma);
>  	void	(*request)(struct vfio_device *vdev, unsigned int count);
>  	int	(*match)(struct vfio_device *vdev, char *buf);
> +	int	(*device_feature)(struct vfio_device *device, u32 flags,
> +				  void __user *arg, size_t argsz);
>  };
>  
> +/**
> + * vfio_check_feature - Validate user input for the VFIO_DEVICE_FEATURE ioctl
> + * @flags: Arg from the device_feature op
> + * @argsz: Arg from the device_feature op
> + * @supported_ops: Combination of VFIO_DEVICE_FEATURE_GET and SET the driver
> + *                 supports
> + * @minsz: Minimum data size the driver accepts
> + *
> + * For use in a driver's device_feature op. Checks that the inputs to the
> + * VFIO_DEVICE_FEATURE ioctl are correct for the driver's feature. Returns 1 if
> + * the driver should execute the get or set, otherwise the relevant
> + * value should be returned.
> + */
> +static inline int vfio_check_feature(u32 flags, size_t argsz, u32 supported_ops,
> +				    size_t minsz)
> +{
> +	if ((flags & (VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_SET)) &
> +	    ~supported_ops)
> +		return -EINVAL;

These look like cases where it would be useful for userspace debugging
to differentiate errnos.

-EOPNOTSUPP?

> +	if (flags & VFIO_DEVICE_FEATURE_PROBE)
> +		return 0;
> +	/* Without PROBE one of GET or SET must be requested */
> +	if (!(flags & (VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_SET)))
> +		return -EINVAL;
> +	if (argsz < minsz)
> +		return -EINVAL;

-ENOSPC?

Thanks,
Alex