On Sun, 30 Jan 2022 18:08:18 +0200
Yishai Hadas <yishaih@xxxxxxxxxx> wrote:
> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Yishai Hadas <yishaih@xxxxxxxxxx>
> ---
> 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
