| Message ID | 1406129207-1302-9-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 23, 2014 at 05:26:43PM +0200, David Herrmann wrote: > If an active DRM-Master closes its device, we deauthenticate all clients > on that master. However, if an inactive DRM-Master closes its device, we > do nothing. This is quite inconsistent and breaks several scenarios: > > 1) If this was used as security mechanism, it fails horribly if a master > closes a device while VT switched away. Furthermore, none of the few > drivers using ->master_*() callbacks seems to require it, anyway. > > 2) If you spawn weston (or any other non-UMS compositor) in background > while another compositor is active, both will get assigned to the > same "drm_master" object. If the foreground compositor now exits, all > clients of both the foreground AND background compositor will be > de-authenticated leading to unexpected behavior. > > Stop this non-sense and keep clients authenticated. We don't do this when > dropping DRM-Master (i.e., switching VTs) so don't do it on active-close > either! > > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> Yeah, we have a bit a confusion about what master does. Iirc back in the dri days the idea was that the master completely gates access for clients, but we've long since moved away from this model (if it ever really was fully implemented in the drm core). Dropping this and making master a pure priv flag for certain compositor operations (modeset, flink_open, ...) makes a lot of sense. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_fops.c | 13 ++----------- > include/drm/drmP.h | 1 - > 2 files changed, 2 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c > index f65087e..ff0a13f 100644 > --- a/drivers/gpu/drm/drm_fops.c > +++ b/drivers/gpu/drm/drm_fops.c > @@ -252,8 +252,7 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor) > priv->minor = minor; > > /* for compatibility root is always authenticated */ > - priv->always_authenticated = capable(CAP_SYS_ADMIN); > - priv->authenticated = priv->always_authenticated; > + priv->authenticated = capable(CAP_SYS_ADMIN); > priv->lock_count = 0; > > INIT_LIST_HEAD(&priv->lhead); > @@ -453,20 +452,12 @@ int drm_release(struct inode *inode, struct file *filp) > > if (drm_is_master(file_priv)) { > struct drm_master *master = file_priv->master; > - struct drm_file *temp; > - > - mutex_lock(&dev->struct_mutex); > - list_for_each_entry(temp, &dev->filelist, lhead) { > - if ((temp->master == file_priv->master) && > - (temp != file_priv)) > - temp->authenticated = temp->always_authenticated; > - } > > /** > * Since the master is disappearing, so is the > * possibility to lock. > */ > - > + mutex_lock(&dev->struct_mutex); > if (master->lock.hw_lock) { > if (dev->sigdata.lock == master->lock.hw_lock) > dev->sigdata.lock = NULL; > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index e1bb585..48d2fe7 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -385,7 +385,6 @@ struct drm_prime_file_private { > > /** File private data */ > struct drm_file { > - unsigned always_authenticated :1; > unsigned authenticated :1; > /* true when the client has asked us to expose stereo 3D mode flags */ > unsigned stereo_allowed :1; > -- > 2.0.2 >
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index f65087e..ff0a13f 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -252,8 +252,7 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor) priv->minor = minor; /* for compatibility root is always authenticated */ - priv->always_authenticated = capable(CAP_SYS_ADMIN); - priv->authenticated = priv->always_authenticated; + priv->authenticated = capable(CAP_SYS_ADMIN); priv->lock_count = 0; INIT_LIST_HEAD(&priv->lhead); @@ -453,20 +452,12 @@ int drm_release(struct inode *inode, struct file *filp) if (drm_is_master(file_priv)) { struct drm_master *master = file_priv->master; - struct drm_file *temp; - - mutex_lock(&dev->struct_mutex); - list_for_each_entry(temp, &dev->filelist, lhead) { - if ((temp->master == file_priv->master) && - (temp != file_priv)) - temp->authenticated = temp->always_authenticated; - } /** * Since the master is disappearing, so is the * possibility to lock. */ - + mutex_lock(&dev->struct_mutex); if (master->lock.hw_lock) { if (dev->sigdata.lock == master->lock.hw_lock) dev->sigdata.lock = NULL; diff --git a/include/drm/drmP.h b/include/drm/drmP.h index e1bb585..48d2fe7 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -385,7 +385,6 @@ struct drm_prime_file_private { /** File private data */ struct drm_file { - unsigned always_authenticated :1; unsigned authenticated :1; /* true when the client has asked us to expose stereo 3D mode flags */ unsigned stereo_allowed :1;
If an active DRM-Master closes its device, we deauthenticate all clients on that master. However, if an inactive DRM-Master closes its device, we do nothing. This is quite inconsistent and breaks several scenarios: 1) If this was used as security mechanism, it fails horribly if a master closes a device while VT switched away. Furthermore, none of the few drivers using ->master_*() callbacks seems to require it, anyway. 2) If you spawn weston (or any other non-UMS compositor) in background while another compositor is active, both will get assigned to the same "drm_master" object. If the foreground compositor now exits, all clients of both the foreground AND background compositor will be de-authenticated leading to unexpected behavior. Stop this non-sense and keep clients authenticated. We don't do this when dropping DRM-Master (i.e., switching VTs) so don't do it on active-close either! Signed-off-by: David Herrmann <dh.herrmann@gmail.com> --- drivers/gpu/drm/drm_fops.c | 13 ++----------- include/drm/drmP.h | 1 - 2 files changed, 2 insertions(+), 12 deletions(-)