[PATCH] drm: fix returning -EINVAL on setmaster if another master is active

[PATCH] drm: fix returning -EINVAL on setmaster if another master is active

[David Herrmann]

We link every DRM "file_priv" to a "drm_master" structure. Currently, the
drmSetMaster() call returns 0 when there is _any_ active master associated
with the "drm_master" structure of the calling "file_priv". This means,
that after drmSetMaster() we are not guaranteed to be DRM-Master and might
not be able to perform mode-setting.

A way to reproduce this is by starting weston with the DRM backend from
within an X-console (eg., xterm). Because the xserver's "drm_master" is
currently active, weston is assigned to the same master but is inactive
because its VT is inactive and the xserver is still active. But when
"fake-activating" weston, it calls drmSetMaster(). With current behavior
this returns "0/success" and weston thinks that it is DRM-Master, even
though it is not (as the xserver is still DRM-Master).
Expected behavior would be drmSetMaster() to return -EINVAL, because the
xserver is still DRM-Master. This patch changes exactly that.

The only way this bogus behavior would be useful is for clients to check
whether their associated "drm_master" is currently the active DRM-Master.
But this logic fails if no DRM-Master is currently active at all. Because
then the client itself would become DRM-Master (if it is root) and this
makes this whole thing useles.

Also note that the second "if-condition":
  file_priv->minor->master != file_priv->master
is always true and can be skipped.

Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
Note:
Note that this only removes the "if-clause". The code that performs the
setmaster() is actually left unchanged but makes the patch look scarier than it
really is.

 drivers/gpu/drm/drm_stub.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index c236fd2..581e61d 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -221,20 +221,20 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
 	if (!file_priv->master)
 		return -EINVAL;
 
-	if (!file_priv->minor->master &&
-	    file_priv->minor->master != file_priv->master) {
-		mutex_lock(&dev->struct_mutex);
-		file_priv->minor->master = drm_master_get(file_priv->master);
-		file_priv->is_master = 1;
-		if (dev->driver->master_set) {
-			ret = dev->driver->master_set(dev, file_priv, false);
-			if (unlikely(ret != 0)) {
-				file_priv->is_master = 0;
-				drm_master_put(&file_priv->minor->master);
-			}
+	if (file_priv->minor->master)
+		return -EINVAL;
+
+	mutex_lock(&dev->struct_mutex);
+	file_priv->minor->master = drm_master_get(file_priv->master);
+	file_priv->is_master = 1;
+	if (dev->driver->master_set) {
+		ret = dev->driver->master_set(dev, file_priv, false);
+		if (unlikely(ret != 0)) {
+			file_priv->is_master = 0;
+			drm_master_put(&file_priv->minor->master);
 		}
-		mutex_unlock(&dev->struct_mutex);
 	}
+	mutex_unlock(&dev->struct_mutex);
 
 	return 0;
 }
-- 
1.7.12.2

Re: [PATCH] drm: fix returning -EINVAL on setmaster if another master is active

[Laurent Pinchart]

Hi David,

Would you have time to document the master_set operation in 
Documentation/DocBook/drm.tmpl ? :-)

On Sunday 07 October 2012 19:53:26 David Herrmann wrote:
> We link every DRM "file_priv" to a "drm_master" structure. Currently, the
> drmSetMaster() call returns 0 when there is _any_ active master associated
> with the "drm_master" structure of the calling "file_priv". This means,
> that after drmSetMaster() we are not guaranteed to be DRM-Master and might
> not be able to perform mode-setting.
> 
> A way to reproduce this is by starting weston with the DRM backend from
> within an X-console (eg., xterm). Because the xserver's "drm_master" is
> currently active, weston is assigned to the same master but is inactive
> because its VT is inactive and the xserver is still active. But when
> "fake-activating" weston, it calls drmSetMaster(). With current behavior
> this returns "0/success" and weston thinks that it is DRM-Master, even
> though it is not (as the xserver is still DRM-Master).
> Expected behavior would be drmSetMaster() to return -EINVAL, because the
> xserver is still DRM-Master. This patch changes exactly that.
> 
> The only way this bogus behavior would be useful is for clients to check
> whether their associated "drm_master" is currently the active DRM-Master.
> But this logic fails if no DRM-Master is currently active at all. Because
> then the client itself would become DRM-Master (if it is root) and this
> makes this whole thing useles.
> 
> Also note that the second "if-condition":
>   file_priv->minor->master != file_priv->master
> is always true and can be skipped.
> 
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
> ---
> Note:
> Note that this only removes the "if-clause". The code that performs the
> setmaster() is actually left unchanged but makes the patch look scarier than
> it really is.
> 
>  drivers/gpu/drm/drm_stub.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> index c236fd2..581e61d 100644
> --- a/drivers/gpu/drm/drm_stub.c
> +++ b/drivers/gpu/drm/drm_stub.c
> @@ -221,20 +221,20 @@ int drm_setmaster_ioctl(struct drm_device *dev, void
> *data, if (!file_priv->master)
>  		return -EINVAL;
> 
> -	if (!file_priv->minor->master &&
> -	    file_priv->minor->master != file_priv->master) {
> -		mutex_lock(&dev->struct_mutex);
> -		file_priv->minor->master = drm_master_get(file_priv->master);
> -		file_priv->is_master = 1;
> -		if (dev->driver->master_set) {
> -			ret = dev->driver->master_set(dev, file_priv, false);
> -			if (unlikely(ret != 0)) {
> -				file_priv->is_master = 0;
> -				drm_master_put(&file_priv->minor->master);
> -			}
> +	if (file_priv->minor->master)
> +		return -EINVAL;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	file_priv->minor->master = drm_master_get(file_priv->master);
> +	file_priv->is_master = 1;
> +	if (dev->driver->master_set) {
> +		ret = dev->driver->master_set(dev, file_priv, false);
> +		if (unlikely(ret != 0)) {
> +			file_priv->is_master = 0;
> +			drm_master_put(&file_priv->minor->master);
>  		}
> -		mutex_unlock(&dev->struct_mutex);
>  	}
> +	mutex_unlock(&dev->struct_mutex);
> 
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm: fix returning -EINVAL on setmaster if another master is active

[David Herrmann]

Hi Laurent

On Thu, Oct 11, 2012 at 12:35 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi David,
>
> Would you have time to document the master_set operation in
> Documentation/DocBook/drm.tmpl ? :-)

I have actually some drafts for "drmSetMaster/drmDropMaster" man-pages
for libdrm on my machine. However, I am still waiting for my other
man-pages being applied to libdrm (they're pending on the list).

The drmSetMaster() man-page does explain the DRM-Master mess in all
detail, so I'd like to wait for this being reviewed before adding the
same information to kernel-docbook (if that is required at all).

Regards
David

Re: [PATCH] drm: fix returning -EINVAL on setmaster if another master is active

[Laurent Pinchart]

Hi David,

On Thursday 11 October 2012 12:41:43 David Herrmann wrote:
> On Thu, Oct 11, 2012 at 12:35 PM, Laurent Pinchart wrote:
> > Hi David,
> > 
> > Would you have time to document the master_set operation in
> > Documentation/DocBook/drm.tmpl ? :-)
> 
> I have actually some drafts for "drmSetMaster/drmDropMaster" man-pages
> for libdrm on my machine. However, I am still waiting for my other
> man-pages being applied to libdrm (they're pending on the list).
> 
> The drmSetMaster() man-page does explain the DRM-Master mess in all
> detail, so I'd like to wait for this being reviewed before adding the
> same information to kernel-docbook

Sure, there's no rush.

> (if that is required at all).

The DocBook documentation (currently) documents the in-kernel APIs only and 
mostly serves as a document for driver developers. It's currently missing 
documentation for the master_set operation, so it would be nice if you could 
add that.

-- 
Regards,

Laurent Pinchart