| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170519103352.7befec0b@w520.home>
Date: Fri, 19 May 2017 10:33:52 -0600
From: Alex Williamson <alex.williamson@...hat.com>
To: Xiaoguang Chen <xiaoguang.chen@...el.com>
Cc: kraxel@...hat.com, intel-gfx@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, zhenyuw@...ux.intel.com,
zhiyuan.lv@...el.com, intel-gvt-dev@...ts.freedesktop.org,
zhi.a.wang@...el.com, kevin.tian@...el.com
Subject: Re: [PATCH v2 5/5] drm/i915/gvt: Adding interface so user space can
get the dma-buf
On Thu, 18 May 2017 17:50:05 +0800
Xiaoguang Chen <xiaoguang.chen@...el.com> wrote:
> User space will try to create a management fd for the dma-buf operation.
> Using this management fd user can query the plane information and create
> a dma-buf fd if necessary.
> GVT-g will handle the life cycle of the management fd and will align the
> life cycle of the fd with the vfio device.
> User space should handle the life cycle of the created dma-buf fd close
> the dma-buf fd timely when finishing use.
>
> Signed-off-by: Xiaoguang Chen <xiaoguang.chen@...el.com>
> ---
> drivers/gpu/drm/i915/gvt/gvt.c | 2 +
> drivers/gpu/drm/i915/gvt/gvt.h | 3 ++
> drivers/gpu/drm/i915/gvt/kvmgt.c | 89 ++++++++++++++++++++++++++++++++++++++++
> include/uapi/drm/i915_drm.h | 2 +
> include/uapi/linux/vfio.h | 12 ++++++
> 5 files changed, 108 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
> index 2032917..48e04e6 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.c
> +++ b/drivers/gpu/drm/i915/gvt/gvt.c
> @@ -54,6 +54,8 @@
> .vgpu_reset = intel_gvt_reset_vgpu,
> .vgpu_activate = intel_gvt_activate_vgpu,
> .vgpu_deactivate = intel_gvt_deactivate_vgpu,
> + .vgpu_query_dmabuf = intel_vgpu_query_dmabuf,
> + .vgpu_create_dmabuf = intel_vgpu_create_dmabuf,
> };
>
> /**
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> index a553120..b7fdfd5 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.h
> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> @@ -185,6 +185,7 @@ struct intel_vgpu {
> struct kvm *kvm;
> struct work_struct release_work;
> atomic_t released;
> + struct vfio_device *vfio_device;
> } vdev;
> #endif
> struct intel_vgpu_plane_info *plane_info;
> @@ -469,6 +470,8 @@ struct intel_gvt_ops {
> void (*vgpu_reset)(struct intel_vgpu *);
> void (*vgpu_activate)(struct intel_vgpu *);
> void (*vgpu_deactivate)(struct intel_vgpu *);
> + int (*vgpu_query_dmabuf)(struct intel_vgpu *, void *);
> + int (*vgpu_create_dmabuf)(struct intel_vgpu *, void *);
> };
>
>
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 389f072..9a663df 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -41,6 +41,7 @@
> #include <linux/kvm_host.h>
> #include <linux/vfio.h>
> #include <linux/mdev.h>
> +#include <linux/anon_inodes.h>
>
> #include "i915_drv.h"
> #include "gvt.h"
> @@ -524,6 +525,66 @@ static int intel_vgpu_reg_init_opregion(struct intel_vgpu *vgpu)
> return ret;
> }
>
> +static int intel_vgpu_dmabuf_mgr_fd_mmap(struct file *file,
> + struct vm_area_struct *vma)
> +{
> + return -EPERM;
> +}
> +
> +static int intel_vgpu_dmabuf_mgr_fd_release(struct inode *inode,
> + struct file *filp)
> +{
> + struct intel_vgpu *vgpu = filp->private_data;
> +
> + if (vgpu->vdev.vfio_device != NULL)
> + vfio_device_put(vgpu->vdev.vfio_device);
> + else
> + gvt_vgpu_err("intel vgpu dmabuf mgr fd is in a wrong state\n");
You could do:
if (WARN_ON(!vgpu->vdev.vfio_device))
return -EINVAL;
> +
> + return 0;
> +}
> +
> +static long intel_vgpu_dmabuf_mgr_fd_ioctl(struct file *filp,
> + unsigned int ioctl, unsigned long arg)
> +{
> + struct intel_vgpu *vgpu = filp->private_data;
> + int minsz;
> + struct intel_vgpu_dmabuf dmabuf;
> + int ret;
> + struct fd f;
> +
> + minsz = offsetofend(struct intel_vgpu_dmabuf, tiled);
> + if (copy_from_user(&dmabuf, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + f = fdget(dmabuf.fd);
> +
> + if (ioctl == INTEL_VGPU_QUERY_DMABUF)
> + ret = intel_gvt_ops->vgpu_query_dmabuf(vgpu, &dmabuf);
> + else if (ioctl == INTEL_VGPU_GENERATE_DMABUF)
> + ret = intel_gvt_ops->vgpu_create_dmabuf(vgpu, &dmabuf);
Why are these still Intel specific?
> + else {
> + fdput(f);
> + gvt_vgpu_err("unsupported dmabuf operation\n");
> + return -EINVAL;
> + }
> +
> + if (ret != 0) {
> + fdput(f);
> + gvt_vgpu_err("gvt-g get dmabuf failed:%d\n", ret);
> + return -EINVAL;
> + }
> + fdput(f);
> +
> + return copy_to_user((void __user *)arg, &dmabuf, minsz) ? -EFAULT : 0;
> +}
> +
> +static const struct file_operations intel_vgpu_dmabuf_mgr_fd_ops = {
> + .release = intel_vgpu_dmabuf_mgr_fd_release,
> + .unlocked_ioctl = intel_vgpu_dmabuf_mgr_fd_ioctl,
> + .mmap = intel_vgpu_dmabuf_mgr_fd_mmap,
> + .llseek = noop_llseek,
> +};
> static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
> {
> struct intel_vgpu *vgpu = NULL;
> @@ -1259,6 +1320,34 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
> } else if (cmd == VFIO_DEVICE_RESET) {
> intel_gvt_ops->vgpu_reset(vgpu);
> return 0;
> + } else if (cmd == VFIO_DEVICE_GET_FD) {
> + int fd;
> + u32 type;
> + struct vfio_device *device;
> +
> + if (copy_from_user(&type, (void __user *)arg, sizeof(type)))
> + return -EINVAL;
> + if (type != INTEL_VGPU_DMABUF_MGR_FD) {
Yet more unnecessary Intel specific code. Userspace doesn't want to
handle a dmabuf differently between Intel, AMD, and NVIDIA, so if we
have no reason to need vendor specific ioctl and parameters, then don't.
> + gvt_vgpu_err("not supported fd type:%d\n", type);
Just return error, users can abuse any logging they can get to.
> + return -EINVAL;
> + }
> +
> + fd = anon_inode_getfd("intel-vgpu-dmabuf-mgr-fd",
> + &intel_vgpu_dmabuf_mgr_fd_ops,
> + vgpu, O_RDWR | O_CLOEXEC);
And this ordering is still wrong, do this last so you don't need to
worry about getting the vfio_device reference below failing and calling
release unnecessarily (though it seems this fd is leaked in that case
anyway).
> + if (fd < 0) {
> + gvt_vgpu_err("create dmabuf mgr fd failed\n");
> + return -EINVAL;
> + }
> +
> + device = vfio_device_get_from_dev(mdev_dev(mdev));
> + if (device == NULL) {
> + gvt_vgpu_err("kvmgt: vfio device is null\n");
> + return -EINVAL;
> + }
> + vgpu->vdev.vfio_device = device;
> +
> + return fd;
> }
>
> return 0;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index cde4f8e..9d28433 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1466,6 +1466,8 @@ struct intel_vgpu_dmabuf {
> __u32 tiled;
> };
>
> +#define INTEL_VGPU_DMABUF_MGR_FD 0
> +
> #if defined(__cplusplus)
> }
> #endif
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index ae46105..c81500b 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -502,6 +502,18 @@ struct vfio_pci_hot_reset {
>
> #define VFIO_DEVICE_PCI_HOT_RESET _IO(VFIO_TYPE, VFIO_BASE + 13)
>
> +/**
> + * VFIO_DEVICE_GET_FD - _IO(VFIO_TYPE, VFIO_BASE + 14, __u32)
> + *
> + * Create a fd for a vfio device based on the input type
> + * Vendor driver should handle this ioctl to create a fd and manage the
> + * life cycle of this fd.
> + *
> + * Return: a fd if vendor support that type, -errno if not supported
> + */
> +
> +#define VFIO_DEVICE_GET_FD _IO(VFIO_TYPE, VFIO_BASE + 14)
> +
The known input types would need to be defined here too. Definition of
the ioctls available on that type should also be documented here.
> /* -------- API for Type1 VFIO IOMMU -------- */
>
> /**
Powered by blists - more mailing lists