* [PATCH 2/2] drm: Remove the minor master list
2014-02-19 13:40 [PATCH 1/2] drm: Make control nodes master-less Thomas Hellstrom
@ 2014-02-19 13:40 ` Thomas Hellstrom
2014-03-05 14:46 ` David Herrmann
2014-02-28 13:31 ` [PATCH 1/2] drm: Make control nodes master-less Thomas Hellstrom
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Thomas Hellstrom @ 2014-02-19 13:40 UTC (permalink / raw)
To: airlied, dri-devel; +Cc: Thomas Hellstrom, linux-graphics-maintainer
It doesn't appear to be used anywhere.
Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
drivers/gpu/drm/drm_stub.c | 5 -----
include/drm/drmP.h | 2 --
2 files changed, 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 98a33c580..4f17c79 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -152,8 +152,6 @@ struct drm_master *drm_master_create(struct drm_minor *minor)
INIT_LIST_HEAD(&master->magicfree);
master->minor = minor;
- list_add_tail(&master->head, &minor->master_list);
-
return master;
}
@@ -171,8 +169,6 @@ static void drm_master_destroy(struct kref *kref)
struct drm_device *dev = master->minor->dev;
struct drm_map_list *r_list, *list_temp;
- list_del(&master->head);
-
if (dev->driver->master_destroy)
dev->driver->master_destroy(dev, master);
@@ -296,7 +292,6 @@ static int drm_get_minor(struct drm_device *dev, struct drm_minor **minor,
new_minor->device = MKDEV(DRM_MAJOR, minor_id);
new_minor->dev = dev;
new_minor->index = minor_id;
- INIT_LIST_HEAD(&new_minor->master_list);
idr_replace(&drm_minors_idr, new_minor, minor_id);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index ff68e26..56c519f 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -718,7 +718,6 @@ struct drm_master {
struct kref refcount; /* refcount for this master */
- struct list_head head; /**< each minor contains a list of masters */
struct drm_minor *minor; /**< link back to minor we are a master for */
char *unique; /**< Unique identifier: e.g., busid */
@@ -1050,7 +1049,6 @@ struct drm_minor {
struct mutex debugfs_lock; /* Protects debugfs_list. */
struct drm_master *master; /* currently active master for this node */
- struct list_head master_list;
struct drm_mode_group mode_group;
};
--
1.7.10.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 2/2] drm: Remove the minor master list
2014-02-19 13:40 ` [PATCH 2/2] drm: Remove the minor master list Thomas Hellstrom
@ 2014-03-05 14:46 ` David Herrmann
0 siblings, 0 replies; 11+ messages in thread
From: David Herrmann @ 2014-03-05 14:46 UTC (permalink / raw)
To: Thomas Hellstrom
Cc: Dave Airlie, linux-graphics-maintainer,
dri-devel@lists.freedesktop.org
Hi
On Wed, Feb 19, 2014 at 2:40 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
> It doesn't appear to be used anywhere.
Looks good.
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> ---
> drivers/gpu/drm/drm_stub.c | 5 -----
> include/drm/drmP.h | 2 --
> 2 files changed, 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> index 98a33c580..4f17c79 100644
> --- a/drivers/gpu/drm/drm_stub.c
> +++ b/drivers/gpu/drm/drm_stub.c
> @@ -152,8 +152,6 @@ struct drm_master *drm_master_create(struct drm_minor *minor)
> INIT_LIST_HEAD(&master->magicfree);
> master->minor = minor;
>
> - list_add_tail(&master->head, &minor->master_list);
> -
> return master;
> }
>
> @@ -171,8 +169,6 @@ static void drm_master_destroy(struct kref *kref)
> struct drm_device *dev = master->minor->dev;
> struct drm_map_list *r_list, *list_temp;
>
> - list_del(&master->head);
> -
> if (dev->driver->master_destroy)
> dev->driver->master_destroy(dev, master);
>
> @@ -296,7 +292,6 @@ static int drm_get_minor(struct drm_device *dev, struct drm_minor **minor,
> new_minor->device = MKDEV(DRM_MAJOR, minor_id);
> new_minor->dev = dev;
> new_minor->index = minor_id;
> - INIT_LIST_HEAD(&new_minor->master_list);
>
> idr_replace(&drm_minors_idr, new_minor, minor_id);
>
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index ff68e26..56c519f 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -718,7 +718,6 @@ struct drm_master {
>
> struct kref refcount; /* refcount for this master */
>
> - struct list_head head; /**< each minor contains a list of masters */
> struct drm_minor *minor; /**< link back to minor we are a master for */
>
> char *unique; /**< Unique identifier: e.g., busid */
> @@ -1050,7 +1049,6 @@ struct drm_minor {
> struct mutex debugfs_lock; /* Protects debugfs_list. */
>
> struct drm_master *master; /* currently active master for this node */
> - struct list_head master_list;
> struct drm_mode_group mode_group;
> };
>
> --
> 1.7.10.4
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm: Make control nodes master-less
2014-02-19 13:40 [PATCH 1/2] drm: Make control nodes master-less Thomas Hellstrom
2014-02-19 13:40 ` [PATCH 2/2] drm: Remove the minor master list Thomas Hellstrom
@ 2014-02-28 13:31 ` Thomas Hellstrom
2014-03-04 8:27 ` Daniel Vetter
2014-03-05 14:32 ` David Herrmann
3 siblings, 0 replies; 11+ messages in thread
From: Thomas Hellstrom @ 2014-02-28 13:31 UTC (permalink / raw)
To: Thomas Hellstrom; +Cc: airlied, linux-graphics-maintainer, dri-devel
Hi!
Could someone please review these two patches?
Thanks,
Thomas
On 02/19/2014 02:40 PM, Thomas Hellstrom wrote:
> Like for render-nodes, there is no point in maintaining the master concept
> for control nodes, so set the struct drm_file::master pointer to NULL.
>
> At the same time, make sure DRM_MASTER | DRM_CONTROL_ALLOW ioctls are always
> allowed when called through the control node. Previously the caller also
> needed to be master.
>
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> ---
> drivers/gpu/drm/drm_drv.c | 5 +++--
> drivers/gpu/drm/drm_fops.c | 5 +++--
> include/drm/drmP.h | 5 +++++
> 3 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 345be03..42af8bd 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -355,8 +355,9 @@ long drm_ioctl(struct file *filp,
> retcode = -EINVAL;
> } else if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) ||
> ((ioctl->flags & DRM_AUTH) && !drm_is_render_client(file_priv) && !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_MASTER) && !file_priv->is_master) &&
> + !(drm_is_control(file_priv) && (ioctl->flags & DRM_CONTROL_ALLOW))) ||
> + (!(ioctl->flags & DRM_CONTROL_ALLOW) && drm_is_control(file_priv)) ||
> (!(ioctl->flags & DRM_RENDER_ALLOW) && drm_is_render_client(file_priv))) {
> retcode = -EACCES;
> } else {
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 7f2af9a..08a3196 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -259,7 +259,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
> /* if there is no current master make this fd it, but do not create
> * any master object for render clients */
> mutex_lock(&dev->struct_mutex);
> - if (!priv->minor->master && !drm_is_render_client(priv)) {
> + if (!priv->minor->master && !drm_is_render_client(priv) &&
> + !drm_is_control(priv)) {
> /* create a new master */
> priv->minor->master = drm_master_create(priv->minor);
> if (!priv->minor->master) {
> @@ -297,7 +298,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
> goto out_close;
> }
> }
> - } else if (!drm_is_render_client(priv)) {
> + } else if (!drm_is_render_client(priv) && !drm_is_control(priv)) {
> /* get a reference to the master */
> priv->master = drm_master_get(priv->minor->master);
> }
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 04a7f31..ff68e26 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1246,6 +1246,11 @@ static inline bool drm_is_render_client(struct drm_file *file_priv)
> return file_priv->minor->type == DRM_MINOR_RENDER;
> }
>
> +static inline bool drm_is_control(struct drm_file *file_priv)
> +{
> + return file_priv->minor->type == DRM_MINOR_CONTROL;
> +}
> +
> /******************************************************************/
> /** \name Internal function definitions */
> /*@{*/
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/2] drm: Make control nodes master-less
2014-02-19 13:40 [PATCH 1/2] drm: Make control nodes master-less Thomas Hellstrom
2014-02-19 13:40 ` [PATCH 2/2] drm: Remove the minor master list Thomas Hellstrom
2014-02-28 13:31 ` [PATCH 1/2] drm: Make control nodes master-less Thomas Hellstrom
@ 2014-03-04 8:27 ` Daniel Vetter
2014-03-05 14:32 ` David Herrmann
3 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2014-03-04 8:27 UTC (permalink / raw)
To: Thomas Hellstrom; +Cc: airlied, linux-graphics-maintainer, dri-devel
On Wed, Feb 19, 2014 at 02:40:41PM +0100, Thomas Hellstrom wrote:
> Like for render-nodes, there is no point in maintaining the master concept
> for control nodes, so set the struct drm_file::master pointer to NULL.
>
> At the same time, make sure DRM_MASTER | DRM_CONTROL_ALLOW ioctls are always
> allowed when called through the control node. Previously the caller also
> needed to be master.
>
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> ---
> drivers/gpu/drm/drm_drv.c | 5 +++--
> drivers/gpu/drm/drm_fops.c | 5 +++--
> include/drm/drmP.h | 5 +++++
> 3 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 345be03..42af8bd 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -355,8 +355,9 @@ long drm_ioctl(struct file *filp,
> retcode = -EINVAL;
> } else if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) ||
> ((ioctl->flags & DRM_AUTH) && !drm_is_render_client(file_priv) && !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_MASTER) && !file_priv->is_master) &&
> + !(drm_is_control(file_priv) && (ioctl->flags & DRM_CONTROL_ALLOW))) ||
> + (!(ioctl->flags & DRM_CONTROL_ALLOW) && drm_is_control(file_priv)) ||
> (!(ioctl->flags & DRM_RENDER_ALLOW) && drm_is_render_client(file_priv))) {
This is hideous to review ;-) I think it would be really good to extract
this entire condition into a drm_check_ioctl_acces(ioctl, file_priv)
helper and untangle all the different cases a bit by splitting it up into
if checks with individual return false/true; statements.
With that bit of polish for the next reviewer's sanity applied both
patches are Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
I guess with this change we could move the master pointer from drm_minor
to drm_device, which would make it really clear that there's only ever one
master per device. But that's one giant sed job, so meh ;-)
One thing I'm unsure about is whether we want/need to have the master
concept on the control node, too. logind uses set/dropmaster as a
kms-specific revoke support, so if we ever want to switch to using control
nodes for display servers we'd need to shuffle this a bit again.
Otoh no one is using control nodes for real afaik, so we likely need some
interface polishing anyway. And for non-root display servers we can block
out all the awful legacy drm ioctls easily, so just keeping on using
legacy nodes isn't a security issue for that use-case.
Cheers, Daniel
> retcode = -EACCES;
> } else {
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 7f2af9a..08a3196 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -259,7 +259,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
> /* if there is no current master make this fd it, but do not create
> * any master object for render clients */
> mutex_lock(&dev->struct_mutex);
> - if (!priv->minor->master && !drm_is_render_client(priv)) {
> + if (!priv->minor->master && !drm_is_render_client(priv) &&
> + !drm_is_control(priv)) {
> /* create a new master */
> priv->minor->master = drm_master_create(priv->minor);
> if (!priv->minor->master) {
> @@ -297,7 +298,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
> goto out_close;
> }
> }
> - } else if (!drm_is_render_client(priv)) {
> + } else if (!drm_is_render_client(priv) && !drm_is_control(priv)) {
> /* get a reference to the master */
> priv->master = drm_master_get(priv->minor->master);
> }
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 04a7f31..ff68e26 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1246,6 +1246,11 @@ static inline bool drm_is_render_client(struct drm_file *file_priv)
> return file_priv->minor->type == DRM_MINOR_RENDER;
> }
>
> +static inline bool drm_is_control(struct drm_file *file_priv)
> +{
> + return file_priv->minor->type == DRM_MINOR_CONTROL;
> +}
> +
> /******************************************************************/
> /** \name Internal function definitions */
> /*@{*/
> --
> 1.7.10.4
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/2] drm: Make control nodes master-less
2014-02-19 13:40 [PATCH 1/2] drm: Make control nodes master-less Thomas Hellstrom
` (2 preceding siblings ...)
2014-03-04 8:27 ` Daniel Vetter
@ 2014-03-05 14:32 ` David Herrmann
2014-03-10 9:32 ` Thomas Hellstrom
3 siblings, 1 reply; 11+ messages in thread
From: David Herrmann @ 2014-03-05 14:32 UTC (permalink / raw)
To: Thomas Hellstrom
Cc: Dave Airlie, linux-graphics-maintainer,
dri-devel@lists.freedesktop.org
Hi
On Wed, Feb 19, 2014 at 2:40 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
> Like for render-nodes, there is no point in maintaining the master concept
> for control nodes, so set the struct drm_file::master pointer to NULL.
>
> At the same time, make sure DRM_MASTER | DRM_CONTROL_ALLOW ioctls are always
> allowed when called through the control node. Previously the caller also
> needed to be master.
>
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> ---
> drivers/gpu/drm/drm_drv.c | 5 +++--
> drivers/gpu/drm/drm_fops.c | 5 +++--
> include/drm/drmP.h | 5 +++++
> 3 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 345be03..42af8bd 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -355,8 +355,9 @@ long drm_ioctl(struct file *filp,
> retcode = -EINVAL;
> } else if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) ||
> ((ioctl->flags & DRM_AUTH) && !drm_is_render_client(file_priv) && !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_MASTER) && !file_priv->is_master) &&
> + !(drm_is_control(file_priv) && (ioctl->flags & DRM_CONTROL_ALLOW))) ||
> + (!(ioctl->flags & DRM_CONTROL_ALLOW) && drm_is_control(file_priv)) ||
> (!(ioctl->flags & DRM_RENDER_ALLOW) && drm_is_render_client(file_priv))) {
> retcode = -EACCES;
> } else {
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 7f2af9a..08a3196 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -259,7 +259,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
> /* if there is no current master make this fd it, but do not create
> * any master object for render clients */
> mutex_lock(&dev->struct_mutex);
> - if (!priv->minor->master && !drm_is_render_client(priv)) {
> + if (!priv->minor->master && !drm_is_render_client(priv) &&
> + !drm_is_control(priv)) {
> /* create a new master */
> priv->minor->master = drm_master_create(priv->minor);
> if (!priv->minor->master) {
> @@ -297,7 +298,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
> goto out_close;
> }
> }
> - } else if (!drm_is_render_client(priv)) {
> + } else if (!drm_is_render_client(priv) && !drm_is_control(priv)) {
> /* get a reference to the master */
> priv->master = drm_master_get(priv->minor->master);
Maybe I missed some other patches, but how is this going to work?
drm_crtc.c expects priv->master to be non-NULL (eg.,
drm_mode_getresources()). This is fine for render-nodes as they don't
allow any of those ioctls, but the control-node does allow them.
Thanks
David
> }
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 04a7f31..ff68e26 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1246,6 +1246,11 @@ static inline bool drm_is_render_client(struct drm_file *file_priv)
> return file_priv->minor->type == DRM_MINOR_RENDER;
> }
>
> +static inline bool drm_is_control(struct drm_file *file_priv)
> +{
> + return file_priv->minor->type == DRM_MINOR_CONTROL;
> +}
> +
> /******************************************************************/
> /** \name Internal function definitions */
> /*@{*/
> --
> 1.7.10.4
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/2] drm: Make control nodes master-less
2014-03-05 14:32 ` David Herrmann
@ 2014-03-10 9:32 ` Thomas Hellstrom
2014-03-12 9:45 ` David Herrmann
0 siblings, 1 reply; 11+ messages in thread
From: Thomas Hellstrom @ 2014-03-10 9:32 UTC (permalink / raw)
To: David Herrmann
Cc: Dave Airlie, linux-graphics-maintainer,
dri-devel@lists.freedesktop.org, Daniel Vetter
On 03/05/2014 03:32 PM, David Herrmann wrote:
> Hi
>
> On Wed, Feb 19, 2014 at 2:40 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
>> Like for render-nodes, there is no point in maintaining the master concept
>> for control nodes, so set the struct drm_file::master pointer to NULL.
>>
>> At the same time, make sure DRM_MASTER | DRM_CONTROL_ALLOW ioctls are always
>> allowed when called through the control node. Previously the caller also
>> needed to be master.
>>
>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
>> ---
>> drivers/gpu/drm/drm_drv.c | 5 +++--
>> drivers/gpu/drm/drm_fops.c | 5 +++--
>> include/drm/drmP.h | 5 +++++
>> 3 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 345be03..42af8bd 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -355,8 +355,9 @@ long drm_ioctl(struct file *filp,
>> retcode = -EINVAL;
>> } else if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) ||
>> ((ioctl->flags & DRM_AUTH) && !drm_is_render_client(file_priv) && !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_MASTER) && !file_priv->is_master) &&
>> + !(drm_is_control(file_priv) && (ioctl->flags & DRM_CONTROL_ALLOW))) ||
>> + (!(ioctl->flags & DRM_CONTROL_ALLOW) && drm_is_control(file_priv)) ||
>> (!(ioctl->flags & DRM_RENDER_ALLOW) && drm_is_render_client(file_priv))) {
>> retcode = -EACCES;
>> } else {
>> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
>> index 7f2af9a..08a3196 100644
>> --- a/drivers/gpu/drm/drm_fops.c
>> +++ b/drivers/gpu/drm/drm_fops.c
>> @@ -259,7 +259,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
>> /* if there is no current master make this fd it, but do not create
>> * any master object for render clients */
>> mutex_lock(&dev->struct_mutex);
>> - if (!priv->minor->master && !drm_is_render_client(priv)) {
>> + if (!priv->minor->master && !drm_is_render_client(priv) &&
>> + !drm_is_control(priv)) {
>> /* create a new master */
>> priv->minor->master = drm_master_create(priv->minor);
>> if (!priv->minor->master) {
>> @@ -297,7 +298,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
>> goto out_close;
>> }
>> }
>> - } else if (!drm_is_render_client(priv)) {
>> + } else if (!drm_is_render_client(priv) && !drm_is_control(priv)) {
>> /* get a reference to the master */
>> priv->master = drm_master_get(priv->minor->master);
> Maybe I missed some other patches, but how is this going to work?
> drm_crtc.c expects priv->master to be non-NULL (eg.,
> drm_mode_getresources()). This is fine for render-nodes as they don't
> allow any of those ioctls, but the control-node does allow them.
>
> Thanks
> David
>
>
Hi!
You didn't miss any patches. It was I who missed the usage in drm_crtc.c.
I guess this, and Daniel's reply prompts a discussion about control
nodes and the master concept.
First I'd like to give some background about the use-case: I'd like to
use the control node for a daemon that tells the vmwgfx driver about the
host GUI layout of the screen: What connectors are enabled, the
preferred mode of each output and some driver private information. The
daemon will run as root and only a single instance per device. Trying to
do this as-is will cause the vmwgfx driver to BUG() because it currently
assumes one active master per device. Not one active master per minor
(although that could be changed if needed).
Looking at
http://dvdhrm.wordpress.com/2013/09/01/splitting-drm-and-kms-device-nodes/
it doesn't really make sense to keep the master concept on the control
node, but it perhaps makes sense to keep it on future modesetting nodes
to allow multiple modesetters to open the same device node?
In that case all master access in drm_crtc.c would be changed for now to
be conditioned on file_priv->minor->type == DRM_MINOR_LEGACY
/Thomas
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/2] drm: Make control nodes master-less
2014-03-10 9:32 ` Thomas Hellstrom
@ 2014-03-12 9:45 ` David Herrmann
2014-03-12 12:33 ` Thomas Hellstrom
0 siblings, 1 reply; 11+ messages in thread
From: David Herrmann @ 2014-03-12 9:45 UTC (permalink / raw)
To: Thomas Hellstrom
Cc: Dave Airlie, linux-graphics-maintainer,
dri-devel@lists.freedesktop.org, Daniel Vetter
Hi
> You didn't miss any patches. It was I who missed the usage in drm_crtc.c.
> I guess this, and Daniel's reply prompts a discussion about control
> nodes and the master concept.
>
> First I'd like to give some background about the use-case: I'd like to
> use the control node for a daemon that tells the vmwgfx driver about the
> host GUI layout of the screen: What connectors are enabled, the
> preferred mode of each output and some driver private information. The
> daemon will run as root and only a single instance per device. Trying to
> do this as-is will cause the vmwgfx driver to BUG() because it currently
> assumes one active master per device. Not one active master per minor
> (although that could be changed if needed).
Why do you tie this to DRM-Master? Can't you just limit these special
ioctls to the control-node and drop any master-requirements? This way
you don't care whether the FD on the control-node is master or not,
you just assume that access-rights on the control-node are highly
restricted so anyone opening it is privileged to issue these ioctls.
> Looking at
>
> http://dvdhrm.wordpress.com/2013/09/01/splitting-drm-and-kms-device-nodes/
>
> it doesn't really make sense to keep the master concept on the control
> node, but it perhaps makes sense to keep it on future modesetting nodes
> to allow multiple modesetters to open the same device node?
>
> In that case all master access in drm_crtc.c would be changed for now to
> be conditioned on file_priv->minor->type == DRM_MINOR_LEGACY
The Master-concept only really makes sense for operations affecting
global state. On non-legacy drivers this is basically KMS. Therefore,
I see no reason to extend the master-concept to non-modeset/primary
nodes. So ACK on changing drm_crtc.c to check for master only if
minor==DRM_MINOR_PRIMARY/LEGACY (there are pending patches renaming
it..).
The problem with the modeset-node concept described in my blog-post is
KMS-object hotplugging. We don't support it.. and I don't see any
proper way to extend our API in a suitable way. The only thing I still
have planned is to allow creating multiple DRM_MINOR_PRIMARY/LEGACY
nodes and splitting mode-objects among them. We would have to do that
before anyone uses the device to prevent objects from disappearing,
but this could easily be done as udev-action. Thus, setting up
multiple KMS interfaces on a single card.
Anyhow, imho we should just try to remove any master-handling from any
core DRM code. Ideally, only drm_drv.c deals with DRM-Master when
checking for permissions. Everything else should never attach any
information to these things. Especially the master-related driver
callbacks are undocumented and really weird. If we'd be able to drop
all this, I think we can start looking forward.
Thanks
David
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm: Make control nodes master-less
2014-03-12 9:45 ` David Herrmann
@ 2014-03-12 12:33 ` Thomas Hellstrom
2014-03-12 13:16 ` David Herrmann
0 siblings, 1 reply; 11+ messages in thread
From: Thomas Hellstrom @ 2014-03-12 12:33 UTC (permalink / raw)
To: David Herrmann
Cc: Dave Airlie, linux-graphics-maintainer,
dri-devel@lists.freedesktop.org, Daniel Vetter
On 03/12/2014 10:45 AM, David Herrmann wrote:
> Hi
>
>> You didn't miss any patches. It was I who missed the usage in drm_crtc.c.
>> I guess this, and Daniel's reply prompts a discussion about control
>> nodes and the master concept.
>>
>> First I'd like to give some background about the use-case: I'd like to
>> use the control node for a daemon that tells the vmwgfx driver about the
>> host GUI layout of the screen: What connectors are enabled, the
>> preferred mode of each output and some driver private information. The
>> daemon will run as root and only a single instance per device. Trying to
>> do this as-is will cause the vmwgfx driver to BUG() because it currently
>> assumes one active master per device. Not one active master per minor
>> (although that could be changed if needed).
> Why do you tie this to DRM-Master? Can't you just limit these special
> ioctls to the control-node and drop any master-requirements? This way
> you don't care whether the FD on the control-node is master or not,
> you just assume that access-rights on the control-node are highly
> restricted so anyone opening it is privileged to issue these ioctls.
That's exactly what I'm planning to do.
The BUG() in vmwgfx that happens when a device gets two masters (one
from legacy and one from control) has been there
for some time and is related to other functionality [1].
>> Looking at
>>
>> https://urldefense.proofpoint.com/v1/url?u=http://dvdhrm.wordpress.com/2013/09/01/splitting-drm-and-kms-device-nodes/&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=V8cVVz4Hn%2FjsqGcRA9UoPCYNUTXvS%2BA21CW7xsg5Mwc%3D%0A&s=3b7c73b3db46fe8d9fdc5aa7e1f83939e84be61977db211c24b40ef81d911e28
>>
>> it doesn't really make sense to keep the master concept on the control
>> node, but it perhaps makes sense to keep it on future modesetting nodes
>> to allow multiple modesetters to open the same device node?
>>
>> In that case all master access in drm_crtc.c would be changed for now to
>> be conditioned on file_priv->minor->type == DRM_MINOR_LEGACY
> The Master-concept only really makes sense for operations affecting
> global state. On non-legacy drivers this is basically KMS. Therefore,
> I see no reason to extend the master-concept to non-modeset/primary
> nodes. So ACK on changing drm_crtc.c to check for master only if
> minor==DRM_MINOR_PRIMARY/LEGACY (there are pending patches renaming
> it..).
Great.
> The problem with the modeset-node concept described in my blog-post is
> KMS-object hotplugging. We don't support it.. and I don't see any
> proper way to extend our API in a suitable way. The only thing I still
> have planned is to allow creating multiple DRM_MINOR_PRIMARY/LEGACY
> nodes and splitting mode-objects among them. We would have to do that
> before anyone uses the device to prevent objects from disappearing,
> but this could easily be done as udev-action. Thus, setting up
> multiple KMS interfaces on a single card.
OK.
>
> Anyhow, imho we should just try to remove any master-handling from any
> core DRM code. Ideally, only drm_drv.c deals with DRM-Master when
> checking for permissions. Everything else should never attach any
> information to these things. Especially the master-related driver
> callbacks are undocumented and really weird. If we'd be able to drop
> all this, I think we can start looking forward.
[1]
I'm partly guilty of that. There is an, IMHO quite nasty, security
problem with the current master concept:
Imagine you have a master (say X server) with a number of authenticated
clients.
Then the master drops master privileges and another X server becomes master.
Now the old master's authenticated clients may still access the new X
server's (legacy) shared buffers, since drm doesn't
differentiate which master a client is authenticated with.
This is, as far as I can tell, even violating X server security policy.
While render nodes will make future solutions work around this, they
don't plug this security problem, and since nobody else
really seems to care, we use the driver hooks to suspend authenticated
clients when their master drops master privileges, just like DRI1 used
to do - hence the TTM lock tied to a master.
So IMHO once there is a core drm solution in place for this, we should
probably be ready to drop the driver master_set and master_drop hooks.
/Thomas
>
> Thanks
> David
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm: Make control nodes master-less
2014-03-12 12:33 ` Thomas Hellstrom
@ 2014-03-12 13:16 ` David Herrmann
2014-03-12 13:36 ` Thomas Hellstrom
0 siblings, 1 reply; 11+ messages in thread
From: David Herrmann @ 2014-03-12 13:16 UTC (permalink / raw)
To: Thomas Hellstrom
Cc: Dave Airlie, linux-graphics-maintainer,
dri-devel@lists.freedesktop.org, Daniel Vetter
Hi
>>> You didn't miss any patches. It was I who missed the usage in drm_crtc.c.
>>> I guess this, and Daniel's reply prompts a discussion about control
>>> nodes and the master concept.
>>>
>>> First I'd like to give some background about the use-case: I'd like to
>>> use the control node for a daemon that tells the vmwgfx driver about the
>>> host GUI layout of the screen: What connectors are enabled, the
>>> preferred mode of each output and some driver private information. The
>>> daemon will run as root and only a single instance per device. Trying to
>>> do this as-is will cause the vmwgfx driver to BUG() because it currently
>>> assumes one active master per device. Not one active master per minor
>>> (although that could be changed if needed).
>> Why do you tie this to DRM-Master? Can't you just limit these special
>> ioctls to the control-node and drop any master-requirements? This way
>> you don't care whether the FD on the control-node is master or not,
>> you just assume that access-rights on the control-node are highly
>> restricted so anyone opening it is privileged to issue these ioctls.
>
>
> That's exactly what I'm planning to do.
> The BUG() in vmwgfx that happens when a device gets two masters (one
> from legacy and one from control) has been there
> for some time and is related to other functionality [1].
Great.
>> Anyhow, imho we should just try to remove any master-handling from any
>> core DRM code. Ideally, only drm_drv.c deals with DRM-Master when
>> checking for permissions. Everything else should never attach any
>> information to these things. Especially the master-related driver
>> callbacks are undocumented and really weird. If we'd be able to drop
>> all this, I think we can start looking forward.
>
> [1]
> I'm partly guilty of that. There is an, IMHO quite nasty, security
> problem with the current master concept:
> Imagine you have a master (say X server) with a number of authenticated
> clients.
> Then the master drops master privileges and another X server becomes master.
> Now the old master's authenticated clients may still access the new X
> server's (legacy) shared buffers, since drm doesn't
> differentiate which master a client is authenticated with.
>
> This is, as far as I can tell, even violating X server security policy.
>
> While render nodes will make future solutions work around this, they
> don't plug this security problem, and since nobody else
> really seems to care, we use the driver hooks to suspend authenticated
> clients when their master drops master privileges, just like DRI1 used
> to do - hence the TTM lock tied to a master.
>
> So IMHO once there is a core drm solution in place for this, we should
> probably be ready to drop the driver master_set and master_drop hooks.
What shared buffers are you talking about here? GEM objects are tied
to drm_file, so one authenticated client can never access bos of other
clients. There is one exception: FLINK. However, that one is _not_
allowed on render-nodes.
drm_framebuffer objects are somewhat broken, indeed. But we modified
drmModeGetFB() to _not_ return any handle if you're not master. So
clients cannot get access to the underlying buffer. Furthermore, we
plan to tie FBs to drm_file the same way we do that for gem handles.
That should fix this leak (we need to allow CAP_SYS_ADMIN to access
any FB, though.. backwards compat..).
That's of course only the theory. Hand-crafted exec-buffers can
obviously access other buffers of other clients if you have no
stream-validation and/or virtual memory on the GPU. Imho, that's a
totally different issue, though.
Thanks
David
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm: Make control nodes master-less
2014-03-12 13:16 ` David Herrmann
@ 2014-03-12 13:36 ` Thomas Hellstrom
0 siblings, 0 replies; 11+ messages in thread
From: Thomas Hellstrom @ 2014-03-12 13:36 UTC (permalink / raw)
To: David Herrmann
Cc: Dave Airlie, linux-graphics-maintainer,
dri-devel@lists.freedesktop.org, Daniel Vetter
Hi,
On 03/12/2014 02:16 PM, David Herrmann wrote:
> Hi
>
>>>> You didn't miss any patches. It was I who missed the usage in drm_crtc.c.
>>>> I guess this, and Daniel's reply prompts a discussion about control
>>>> nodes and the master concept.
>>>>
>>>> First I'd like to give some background about the use-case: I'd like to
>>>> use the control node for a daemon that tells the vmwgfx driver about the
>>>> host GUI layout of the screen: What connectors are enabled, the
>>>> preferred mode of each output and some driver private information. The
>>>> daemon will run as root and only a single instance per device. Trying to
>>>> do this as-is will cause the vmwgfx driver to BUG() because it currently
>>>> assumes one active master per device. Not one active master per minor
>>>> (although that could be changed if needed).
>>> Why do you tie this to DRM-Master? Can't you just limit these special
>>> ioctls to the control-node and drop any master-requirements? This way
>>> you don't care whether the FD on the control-node is master or not,
>>> you just assume that access-rights on the control-node are highly
>>> restricted so anyone opening it is privileged to issue these ioctls.
>>
>> That's exactly what I'm planning to do.
>> The BUG() in vmwgfx that happens when a device gets two masters (one
>> from legacy and one from control) has been there
>> for some time and is related to other functionality [1].
> Great.
>
>>> Anyhow, imho we should just try to remove any master-handling from any
>>> core DRM code. Ideally, only drm_drv.c deals with DRM-Master when
>>> checking for permissions. Everything else should never attach any
>>> information to these things. Especially the master-related driver
>>> callbacks are undocumented and really weird. If we'd be able to drop
>>> all this, I think we can start looking forward.
>> [1]
>> I'm partly guilty of that. There is an, IMHO quite nasty, security
>> problem with the current master concept:
>> Imagine you have a master (say X server) with a number of authenticated
>> clients.
>> Then the master drops master privileges and another X server becomes master.
>> Now the old master's authenticated clients may still access the new X
>> server's (legacy) shared buffers, since drm doesn't
>> differentiate which master a client is authenticated with.
>>
>> This is, as far as I can tell, even violating X server security policy.
>>
>> While render nodes will make future solutions work around this, they
>> don't plug this security problem, and since nobody else
>> really seems to care, we use the driver hooks to suspend authenticated
>> clients when their master drops master privileges, just like DRI1 used
>> to do - hence the TTM lock tied to a master.
>>
>> So IMHO once there is a core drm solution in place for this, we should
>> probably be ready to drop the driver master_set and master_drop hooks.
> What shared buffers are you talking about here? GEM objects are tied
> to drm_file, so one authenticated client can never access bos of other
> clients. There is one exception: FLINK. However, that one is _not_
> allowed on render-nodes.
Yes, I was referring to FLINK and its TTM counterpart.
But, as said, render-nodes provide a way to work around this issue for
future
display servers but I'm arguing you can't fix a security leak in an old
interface
by providing a new interface as long as the old interface remains.
>
> drm_framebuffer objects are somewhat broken, indeed. But we modified
> drmModeGetFB() to _not_ return any handle if you're not master. So
> clients cannot get access to the underlying buffer. Furthermore, we
> plan to tie FBs to drm_file the same way we do that for gem handles.
> That should fix this leak (we need to allow CAP_SYS_ADMIN to access
> any FB, though.. backwards compat..).
That sounds good.
>
> That's of course only the theory. Hand-crafted exec-buffers can
> obviously access other buffers of other clients if you have no
> stream-validation and/or virtual memory on the GPU. Imho, that's a
> totally different issue, though.
Agreed, although I think for "non-render-node (legacy) mode", suspending
authenticated clients on first-access-after-master-drop should fix that
as well.
>
> Thanks
> David
Thanks,
Thomas
^ permalink raw reply [flat|nested] 11+ messages in thread