Here's the patch to implement render nodes as discussed in the "DRM2" talk at XDC:
http://wiki.x.org/wiki/Events/XDC2012/Proceedings#DRM2
The core problem is that DRM security is compromised in the face of VT switching and multiple DRM masters. Any local user can access all shared buffers from within any X server on the system, even when that user doesn't have access to any of those X servers.
The fix for this is to use dmabuf/prime and fd passing for buffer sharing. That infrastructure is already in place and we need to start using that in user space. Once we're passing buffers between display servers and clients in a point-to-point fashion, we no longer need to authenticate clients. We just need to make sure they can only render and import/export buffers to fds. That's what this patch does, by creating a new type of drm device node. Accessing this node doesn't require authentication (and as such can be used without a master, ie headless), but will only expose the safe, modern (DRI2ish) rendering ioctls.
Once userspace is sharing buffers through fd passing, the legacy card0 node can be locked down by unix permissions, for example in a drm-master group, so that only setgid binaries (X, weston, other KMS apps) can access it.
Kristian
See also:
http://wiki.x.org/wiki/Events/XDC2012/Proceedings#Graphics_stack_security
We got the minor number base right, but limit is too big and causes the minor numer ranges for the control and render nodes to overlap.
Signed-off-by: Kristian Høgsberg krh@bitplanet.net --- drivers/gpu/drm/drm_stub.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index 21bcd4a..d6d5160 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -107,10 +107,10 @@ static int drm_minor_get_id(struct drm_device *dev, int type)
if (type == DRM_MINOR_CONTROL) { base += 64; - limit = base + 127; + limit = base + 64; } else if (type == DRM_MINOR_RENDER) { base += 128; - limit = base + 255; + limit = base + 64; }
again:
On Fri, 28 Sep 2012, Kristian Høgsberg wrote:
if (type == DRM_MINOR_CONTROL) { base += 64; - limit = base + 127; + limit = base + 64; } else if (type == DRM_MINOR_RENDER) { base += 128; - limit = base + 255; + limit = base + 64;
If my first grade arithmetics serves me well, shouldn't limit in the first clause be base + 63 and in the second clause, base + 127. The render node starts at 128 and spans to 255, so there are total of 128 render nodes, 64 card (legacy) nodes and 64 control nodes allowed.
Actually, this construction of limit relative to the base does not achieve anything. The code would be much more readable if it were something like this:
if (type == DRM_MINOR_CONTROL) { base = 64; limit = 127; } else if (type == DRM_MINOR_RENDER) { base = 128; limit = 255; }
-- Ilija
This patch introduces a new kind of drm device node: the render node. A render node exposes a safe subset of the legacy drm device ioctls and can only be used for rendering. The legacy node supports modesetting, DRI1 and global buffer sharing, while the render node only supports rendering and limited buffer sharing. A render node can either export a buffer using the gem flink mechanism, or it can import and export buffers using the prime fd passing mechanism.
A render node is not associated with any drm master and does not require the legacy authentication step. Render nodes can be used without a master, potentially in headless setups such as video-transcoding or GPGPU work.
Signed-off-by: Kristian Høgsberg krh@bitplanet.net --- drivers/gpu/drm/drm_drv.c | 13 +++++++------ drivers/gpu/drm/drm_fops.c | 9 ++++++--- drivers/gpu/drm/drm_pci.c | 9 +++++++++ include/drm/drmP.h | 3 +++ 4 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 9238de4..2876e17 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -131,14 +131,14 @@ static struct drm_ioctl_desc drm_ioctls[] = {
DRM_IOCTL_DEF(DRM_IOCTL_UPDATE_DRAW, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
- DRM_IOCTL_DEF(DRM_IOCTL_GEM_CLOSE, drm_gem_close_ioctl, DRM_UNLOCKED), - DRM_IOCTL_DEF(DRM_IOCTL_GEM_FLINK, drm_gem_flink_ioctl, DRM_AUTH|DRM_UNLOCKED), + DRM_IOCTL_DEF(DRM_IOCTL_GEM_CLOSE, drm_gem_close_ioctl, DRM_UNLOCKED | DRM_RENDER_ALLOW), + DRM_IOCTL_DEF(DRM_IOCTL_GEM_FLINK, drm_gem_flink_ioctl, DRM_AUTH|DRM_UNLOCKED | DRM_RENDER_ALLOW), DRM_IOCTL_DEF(DRM_IOCTL_GEM_OPEN, drm_gem_open_ioctl, DRM_AUTH|DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
- DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_AUTH|DRM_UNLOCKED), - DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_AUTH|DRM_UNLOCKED), + DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, DRM_CONTROL_ALLOW|DRM_UNLOCKED), @@ -427,9 +427,10 @@ long drm_ioctl(struct file *filp, DRM_DEBUG("no function\n"); retcode = -EINVAL; } else if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) || - ((ioctl->flags & DRM_AUTH) && !file_priv->authenticated) || + ((ioctl->flags & DRM_AUTH) && (file_priv->minor->type != DRM_MINOR_RENDER) && !file_priv->authenticated) || ((ioctl->flags & DRM_MASTER) && !file_priv->is_master) || - (!(ioctl->flags & DRM_CONTROL_ALLOW) && (file_priv->minor->type == DRM_MINOR_CONTROL))) { + (!(ioctl->flags & DRM_CONTROL_ALLOW) && (file_priv->minor->type == DRM_MINOR_CONTROL)) || + (!(ioctl->flags & DRM_RENDER_ALLOW) && (file_priv->minor->type == DRM_MINOR_RENDER))) { retcode = -EACCES; } else { if (cmd & (IOC_IN | IOC_OUT)) { diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 5062eec..bee4244 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -279,7 +279,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
/* if there is no current master make this fd it */ mutex_lock(&dev->struct_mutex); - if (!priv->minor->master) { + if (!priv->minor->master && priv->minor->type != DRM_MINOR_RENDER) { /* create a new master */ priv->minor->master = drm_master_create(priv->minor); if (!priv->minor->master) { @@ -318,10 +318,12 @@ static int drm_open_helper(struct inode *inode, struct file *filp, } } mutex_unlock(&dev->struct_mutex); - } else { + } else if (priv->minor->type != DRM_MINOR_RENDER) { /* get a reference to the master */ priv->master = drm_master_get(priv->minor->master); mutex_unlock(&dev->struct_mutex); + } else { + mutex_unlock(&dev->struct_mutex); }
mutex_lock(&dev->struct_mutex); @@ -513,7 +515,8 @@ int drm_release(struct inode *inode, struct file *filp) iput(container_of(dev->dev_mapping, struct inode, i_data));
/* drop the reference held my the file priv */ - drm_master_put(&file_priv->master); + if (file_priv->master) + drm_master_put(&file_priv->master); file_priv->is_master = 0; list_del(&file_priv->lhead); mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index 5320364..ff53f9c 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -348,6 +348,12 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, goto err_g2; }
+ if (drm_core_check_feature(dev, DRIVER_RENDER)) { + ret = drm_get_minor(dev, &dev->render, DRM_MINOR_RENDER); + if (ret) + goto err_g21; + } + if ((ret = drm_get_minor(dev, &dev->primary, DRM_MINOR_LEGACY))) goto err_g3;
@@ -377,6 +383,9 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, err_g4: drm_put_minor(&dev->primary); err_g3: + if (drm_core_check_feature(dev, DRIVER_RENDER)) + drm_put_minor(&dev->render); +err_g21: if (drm_core_check_feature(dev, DRIVER_MODESET)) drm_put_minor(&dev->control); err_g2: diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d6b67bb..a9ffc81 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -152,6 +152,7 @@ int drm_err(const char *func, const char *format, ...); #define DRIVER_GEM 0x1000 #define DRIVER_MODESET 0x2000 #define DRIVER_PRIME 0x4000 +#define DRIVER_RENDER 0x8000
#define DRIVER_BUS_PCI 0x1 #define DRIVER_BUS_PLATFORM 0x2 @@ -306,6 +307,7 @@ typedef int drm_ioctl_compat_t(struct file *filp, unsigned int cmd, #define DRM_ROOT_ONLY 0x4 #define DRM_CONTROL_ALLOW 0x8 #define DRM_UNLOCKED 0x10 +#define DRM_RENDER_ALLOW 0x20
struct drm_ioctl_desc { unsigned int cmd; @@ -1185,6 +1187,7 @@ struct drm_device { struct drm_local_map *agp_buffer_map; unsigned int agp_buffer_token; struct drm_minor *control; /**< Control node for card */ + struct drm_minor *render; /**< Render node for card */ struct drm_minor *primary; /**< render type primary screen head */
struct drm_mode_config mode_config; /**< Current mode config */
Enable support for drm render nodes for i915 by flagging the ioctls that are safe and just needed for rendering.
Signed-off-by: Kristian Høgsberg krh@bitplanet.net --- drivers/gpu/drm/i915/i915_dma.c | 36 ++++++++++++++++++------------------ drivers/gpu/drm/i915/i915_drv.c | 3 ++- 2 files changed, 20 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 914c0df..d253d71 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1819,7 +1819,7 @@ struct drm_ioctl_desc i915_ioctls[] = { DRM_IOCTL_DEF_DRV(I915_BATCHBUFFER, i915_batchbuffer, DRM_AUTH), DRM_IOCTL_DEF_DRV(I915_IRQ_EMIT, i915_irq_emit, DRM_AUTH), DRM_IOCTL_DEF_DRV(I915_IRQ_WAIT, i915_irq_wait, DRM_AUTH), - DRM_IOCTL_DEF_DRV(I915_GETPARAM, i915_getparam, DRM_AUTH), + DRM_IOCTL_DEF_DRV(I915_GETPARAM, i915_getparam, DRM_AUTH|DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(I915_SETPARAM, i915_setparam, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), DRM_IOCTL_DEF_DRV(I915_ALLOC, drm_noop, DRM_AUTH), DRM_IOCTL_DEF_DRV(I915_FREE, drm_noop, DRM_AUTH), @@ -1832,32 +1832,32 @@ struct drm_ioctl_desc i915_ioctls[] = { DRM_IOCTL_DEF_DRV(I915_HWS_ADDR, i915_set_status_page, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), DRM_IOCTL_DEF_DRV(I915_GEM_INIT, i915_gem_init_ioctl, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY|DRM_UNLOCKED), DRM_IOCTL_DEF_DRV(I915_GEM_EXECBUFFER, i915_gem_execbuffer, DRM_AUTH|DRM_UNLOCKED), - DRM_IOCTL_DEF_DRV(I915_GEM_EXECBUFFER2, i915_gem_execbuffer2, DRM_AUTH|DRM_UNLOCKED), + DRM_IOCTL_DEF_DRV(I915_GEM_EXECBUFFER2, i915_gem_execbuffer2, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(I915_GEM_PIN, i915_gem_pin_ioctl, DRM_AUTH|DRM_ROOT_ONLY|DRM_UNLOCKED), DRM_IOCTL_DEF_DRV(I915_GEM_UNPIN, i915_gem_unpin_ioctl, DRM_AUTH|DRM_ROOT_ONLY|DRM_UNLOCKED), - DRM_IOCTL_DEF_DRV(I915_GEM_BUSY, i915_gem_busy_ioctl, DRM_AUTH|DRM_UNLOCKED), - DRM_IOCTL_DEF_DRV(I915_GEM_THROTTLE, i915_gem_throttle_ioctl, DRM_AUTH|DRM_UNLOCKED), + DRM_IOCTL_DEF_DRV(I915_GEM_BUSY, i915_gem_busy_ioctl, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF_DRV(I915_GEM_THROTTLE, i915_gem_throttle_ioctl, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(I915_GEM_ENTERVT, i915_gem_entervt_ioctl, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY|DRM_UNLOCKED), DRM_IOCTL_DEF_DRV(I915_GEM_LEAVEVT, i915_gem_leavevt_ioctl, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY|DRM_UNLOCKED), - DRM_IOCTL_DEF_DRV(I915_GEM_CREATE, i915_gem_create_ioctl, DRM_UNLOCKED), - DRM_IOCTL_DEF_DRV(I915_GEM_PREAD, i915_gem_pread_ioctl, DRM_UNLOCKED), - DRM_IOCTL_DEF_DRV(I915_GEM_PWRITE, i915_gem_pwrite_ioctl, DRM_UNLOCKED), - DRM_IOCTL_DEF_DRV(I915_GEM_MMAP, i915_gem_mmap_ioctl, DRM_UNLOCKED), - DRM_IOCTL_DEF_DRV(I915_GEM_MMAP_GTT, i915_gem_mmap_gtt_ioctl, DRM_UNLOCKED), - DRM_IOCTL_DEF_DRV(I915_GEM_SET_DOMAIN, i915_gem_set_domain_ioctl, DRM_UNLOCKED), - DRM_IOCTL_DEF_DRV(I915_GEM_SW_FINISH, i915_gem_sw_finish_ioctl, DRM_UNLOCKED), - DRM_IOCTL_DEF_DRV(I915_GEM_SET_TILING, i915_gem_set_tiling, DRM_UNLOCKED), - DRM_IOCTL_DEF_DRV(I915_GEM_GET_TILING, i915_gem_get_tiling, DRM_UNLOCKED), - DRM_IOCTL_DEF_DRV(I915_GEM_GET_APERTURE, i915_gem_get_aperture_ioctl, DRM_UNLOCKED), + DRM_IOCTL_DEF_DRV(I915_GEM_CREATE, i915_gem_create_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF_DRV(I915_GEM_PREAD, i915_gem_pread_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF_DRV(I915_GEM_PWRITE, i915_gem_pwrite_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF_DRV(I915_GEM_MMAP, i915_gem_mmap_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF_DRV(I915_GEM_MMAP_GTT, i915_gem_mmap_gtt_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF_DRV(I915_GEM_SET_DOMAIN, i915_gem_set_domain_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF_DRV(I915_GEM_SW_FINISH, i915_gem_sw_finish_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF_DRV(I915_GEM_SET_TILING, i915_gem_set_tiling, DRM_UNLOCKED|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF_DRV(I915_GEM_GET_TILING, i915_gem_get_tiling, DRM_UNLOCKED|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF_DRV(I915_GEM_GET_APERTURE, i915_gem_get_aperture_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(I915_GET_PIPE_FROM_CRTC_ID, intel_get_pipe_from_crtc_id, DRM_UNLOCKED), - DRM_IOCTL_DEF_DRV(I915_GEM_MADVISE, i915_gem_madvise_ioctl, DRM_UNLOCKED), + DRM_IOCTL_DEF_DRV(I915_GEM_MADVISE, i915_gem_madvise_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(I915_OVERLAY_PUT_IMAGE, intel_overlay_put_image, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF_DRV(I915_OVERLAY_ATTRS, intel_overlay_attrs, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF_DRV(I915_SET_SPRITE_COLORKEY, intel_sprite_set_colorkey, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF_DRV(I915_GET_SPRITE_COLORKEY, intel_sprite_get_colorkey, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), - DRM_IOCTL_DEF_DRV(I915_GEM_WAIT, i915_gem_wait_ioctl, DRM_AUTH|DRM_UNLOCKED), - DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE, i915_gem_context_create_ioctl, DRM_UNLOCKED), - DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_UNLOCKED), + DRM_IOCTL_DEF_DRV(I915_GEM_WAIT, i915_gem_wait_ioctl, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE, i915_gem_context_create_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW), };
int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls); diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index a24ffbe..6a92a12 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -956,7 +956,8 @@ static struct drm_driver driver = { */ .driver_features = DRIVER_USE_AGP | DRIVER_REQUIRE_AGP | /* DRIVER_USE_MTRR |*/ - DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM | DRIVER_PRIME, + DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM | DRIVER_PRIME | + DRIVER_RENDER, .load = i915_driver_load, .unload = i915_driver_unload, .open = i915_driver_open,
On Fri, Sep 28, 2012 at 12:35 PM, Kristian Høgsberg krh@bitplanet.net wrote:
Here's the patch to implement render nodes as discussed in the "DRM2" talk at XDC:
http://wiki.x.org/wiki/Events/XDC2012/Proceedings#DRM2
The core problem is that DRM security is compromised in the face of VT switching and multiple DRM masters. Any local user can access all shared buffers from within any X server on the system, even when that user doesn't have access to any of those X servers.
The fix for this is to use dmabuf/prime and fd passing for buffer sharing. That infrastructure is already in place and we need to start using that in user space. Once we're passing buffers between display servers and clients in a point-to-point fashion, we no longer need to authenticate clients. We just need to make sure they can only render and import/export buffers to fds. That's what this patch does, by creating a new type of drm device node. Accessing this node doesn't require authentication (and as such can be used without a master, ie headless), but will only expose the safe, modern (DRI2ish) rendering ioctls.
Once userspace is sharing buffers through fd passing, the legacy card0 node can be locked down by unix permissions, for example in a drm-master group, so that only setgid binaries (X, weston, other KMS apps) can access it.
I haven't read through your patches yet, but Dave and Ilija already did something similar a while back:
http://lists.freedesktop.org/archives/dri-devel/2012-March/020765.html
Alex
Kristian
See also:
http://wiki.x.org/wiki/Events/XDC2012/Proceedings#Graphics_stack_security
https://lwn.net/Articles/517375/
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, 28 Sep 2012, Alex Deucher wrote:
I haven't read through your patches yet, but Dave and Ilija already did something similar a while back:
http://lists.freedesktop.org/archives/dri-devel/2012-March/020765.html
Actually, there is a, a newer series of patches here (applied few comments I got after first series)
http://lists.freedesktop.org/archives/dri-devel/2012-April/021326.html
I have the v3 series (applied more comments I got after v2 series, plus some more cleanup) that I have never sent out, because my perception was that there was low interest in this work. I can send the v3 out, if there is a revived interest in this work.
The original work is by Dave and I did some major cleanup and built more on the top of it.
Kristian's patches at the first glance (I have not looked them in detail) appear more like prep. work (like which ioctl can a render node use and which can't, etc.), but my impression is that they still require the work cited above (Dave's original work and/or my followup work) to actually be able to create and use the render node.
BTW, the third patch in Kristian's series is for Intel only and we'll probably need equivalent patches for radeon and nouveau.
-- Ilija
On Fri, Sep 28, 2012 at 7:59 PM, Ilija Hadzic ihadzic@research.bell-labs.com wrote:
On Fri, 28 Sep 2012, Alex Deucher wrote:
I haven't read through your patches yet, but Dave and Ilija already did something similar a while back:
http://lists.freedesktop.org/archives/dri-devel/2012-March/020765.html
Actually, there is a, a newer series of patches here (applied few comments I got after first series)
http://lists.freedesktop.org/archives/dri-devel/2012-April/021326.html
I have the v3 series (applied more comments I got after v2 series, plus some more cleanup) that I have never sent out, because my perception was that there was low interest in this work. I can send the v3 out, if there is a revived interest in this work.
The original work is by Dave and I did some major cleanup and built more on the top of it.
Kristian's patches at the first glance (I have not looked them in detail) appear more like prep. work (like which ioctl can a render node use and which can't, etc.), but my impression is that they still require the work cited above (Dave's original work and/or my followup work) to actually be able to create and use the render node.
BTW, the third patch in Kristian's series is for Intel only and we'll probably need equivalent patches for radeon and nouveau.
On a quick look the rendernode Kristian propose and your work seem to attack slightly different issues. Your/Dave's patch series seems to put large efforts into (dynamically) splitting up the resources of a drm device, including the modeset stuff. Kristians proposal here is much more modest, with just enabling a way for to do the same for render clients. All the modeset (and flink open stuff) would still be only done through the legacy drm node.
One thing though that Kristians patches miss is properly splitting out mmap support such that each open file of the rendernode only has access to it's file-private gem object and not all of them. Since linux has the mmap interface at the struct file level, that shouldn't be a big hassle to get off the ground: We just need to add an mmap_offset table to file_prive and then add the offset lookup at the right place (and do the lookup with the right table), depending upon whether it's a legacy or a new render node doing the offset create (or mmap syscall). -Daniel
On Fri, 28 Sep 2012, Daniel Vetter wrote:
On a quick look the rendernode Kristian propose and your work seem to attack slightly different issues. Your/Dave's patch series seems to put large efforts into (dynamically) splitting up the resources of a drm device, including the modeset stuff.
Correct, the goal is to be able to run multiseat while sharing a GPU. Actually, with my variant of render nodes, I even got multiple desktops residing in different LXC containers to share the GPU, which is kind of cool.
Kristians proposal here is much more modest, with just enabling a way for to do the same for render clients. All the modeset (and flink open stuff) would still be only done through the legacy drm node.
OK I see. From what I can tell from the second patch, drm_get_pci_dev will create one (and I guess only one, right ?) render node if the underlying driver has DRIVER_RENDER node feature. The third patch (among other things) adds that feature to Intel driver.
So if I boot up a system with these patches and with Intel GPU, I will automagically get one more /dev/dri/renderD128 node, right ? The intent is that the render client opens and uses that render node. The /dev/dri/controlDNN node still remains an unused "orphan", right ?
So would you entertain the possibility that the render node is created from user space on demand using an ioctl into the control node ? If that's a possiblity for you, then my set of patches is a superset of what Kristian needs. If you just need a render client, you can create a node with no display resources and you would get something quite close to what these 3 patches try to do..... unless I am missing something.
-- Ilija
On Fri, Sep 28, 2012 at 8:42 PM, Ilija Hadzic ihadzic@research.bell-labs.com wrote:
On Fri, 28 Sep 2012, Daniel Vetter wrote:
On a quick look the rendernode Kristian propose and your work seem to attack slightly different issues. Your/Dave's patch series seems to put large efforts into (dynamically) splitting up the resources of a drm device, including the modeset stuff.
Correct, the goal is to be able to run multiseat while sharing a GPU. Actually, with my variant of render nodes, I even got multiple desktops residing in different LXC containers to share the GPU, which is kind of cool.
Kristians proposal here is much more modest, with just enabling a way for to do the same for render clients. All the modeset (and flink open stuff) would still be only done through the legacy drm node.
OK I see. From what I can tell from the second patch, drm_get_pci_dev will create one (and I guess only one, right ?) render node if the underlying driver has DRIVER_RENDER node feature. The third patch (among other things) adds that feature to Intel driver.
So if I boot up a system with these patches and with Intel GPU, I will automagically get one more /dev/dri/renderD128 node, right ? The intent is that the render client opens and uses that render node. The /dev/dri/controlDNN node still remains an unused "orphan", right ?
Yeah, the plan is to just have one single render node and ensure insulation by not allowing any open file of that render node to access any other buffer not associated to the file_prive. Like I've said, the current patches have a little hole wrt mmap handling there ;-)
The only way to share buffers is via dma_buf (which is fd based, so we could attach full selinux contexts if required) or flink (but not opening an flink name, that requires master rights on the legacy node).
So would you entertain the possibility that the render node is created from user space on demand using an ioctl into the control node ? If that's a possiblity for you, then my set of patches is a superset of what Kristian needs. If you just need a render client, you can create a node with no display resources and you would get something quite close to what these 3 patches try to do..... unless I am missing something.
Well, dynamically creating render nodes is not required just for insulating different render clients. The goal is very much to allow background/headless usage of the gpu, e.g. for opencl and video encode/decode. So first asking a central deamon to span another render node just to transcode another video isn't that great. Obviously the security separation only works if the gpu actually supports different vm address spaces for each node ...
The multi-seat issue is imo orthogonal to that and I don't think we should mangle (like you've noticed, ppl seem to get scared about it and not push those patches too much). And with new stuff like atomic modeset and gpus having a lot of shared resources in the display hw (plls, memory bw, shared links between pipes, ...) I don't think we could even statically split up the modeset resurces, like your patch would allow. Imho a better solution for the mutliseat use-case would be to have a (priviledge) system-compositor that handles the resource sharing between the different seats. Display servers would then simply be yet another render node client (and doing modeset changes through a protocol to the system compositor). The system-compositor could be very well something that would aweful closely resemble wayland ;-)
Cheers, Daniel
Hi Kristian,
Could you please update Documentation/DocBook/drm.tmpl with render nodes documentation ?
On Friday 28 September 2012 12:35:56 Kristian Høgsberg wrote:
Here's the patch to implement render nodes as discussed in the "DRM2" talk at XDC:
http://wiki.x.org/wiki/Events/XDC2012/Proceedings#DRM2
The core problem is that DRM security is compromised in the face of VT switching and multiple DRM masters. Any local user can access all shared buffers from within any X server on the system, even when that user doesn't have access to any of those X servers.
The fix for this is to use dmabuf/prime and fd passing for buffer sharing. That infrastructure is already in place and we need to start using that in user space. Once we're passing buffers between display servers and clients in a point-to-point fashion, we no longer need to authenticate clients. We just need to make sure they can only render and import/export buffers to fds. That's what this patch does, by creating a new type of drm device node. Accessing this node doesn't require authentication (and as such can be used without a master, ie headless), but will only expose the safe, modern (DRI2ish) rendering ioctls.
Once userspace is sharing buffers through fd passing, the legacy card0 node can be locked down by unix permissions, for example in a drm-master group, so that only setgid binaries (X, weston, other KMS apps) can access it.
Kristian
See also:
http://wiki.x.org/wiki/Events/XDC2012/Proceedings#Graphics_stack_security
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Daniel Vetter -
Ilija Hadzic -
Kristian Høgsberg -
Laurent Pinchart