[PATCH 00/16] vmwgfx render-node support v2

List overview All Threads
Download

newer

older

[PATCH 0/3] drm/exynos: more...

[PULL] drm-intel-next

Thomas Hellstrom

27 Mar 2014 27 Mar '14

8:23 p.m.

Resending only those patches that were commented on (4, 6) and one that was affected by a change in 4 (12).

If there are no objections, I'll include this in a vmwgfx-next pull request.

Thanks, Thomas

Show replies by date

Thomas Hellstrom

27 Mar 27 Mar

8:23 p.m.

New subject: [PATCH 04/16] drm: Improve on minor type helpers v3

Add a drm_is_legacy() helper, constify argument to drm_is_render_client(), and use / change helpers where appropriate.

v2: s/drm_is_legacy/drm_is_legacy_client/ and adapt to new code context. v3: s/legacy_client/primary_client/

Signed-off-by: Thomas Hellstrom thellstrom@vmware.com Reviewed-by: Brian Paul brianp@vmware.com --- drivers/gpu/drm/drm_crtc.c | 4 ++-- drivers/gpu/drm/drm_fops.c | 6 ++---- include/drm/drmP.h | 7 ++++++- 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 5fb02d5..960ca98 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1492,7 +1492,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data, mutex_unlock(&file_priv->fbs_lock);

drm_modeset_lock_all(dev); - if (file_priv->minor->type != DRM_MINOR_LEGACY) { + if (!drm_is_primary_client(file_priv)) {

mode_group = NULL; list_for_each(lh, &dev->mode_config.crtc_list) @@ -2848,7 +2848,7 @@ int drm_mode_getfb(struct drm_device *dev, r->pitch = fb->pitches[0]; if (fb->funcs->create_handle) { if (file_priv->is_master || capable(CAP_SYS_ADMIN) || - file_priv->minor->type == DRM_MINOR_CONTROL) { + drm_is_control_client(file_priv)) { ret = fb->funcs->create_handle(fb, file_priv, &r->handle); } else { diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 5432a1a..c7792b1 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -232,8 +232,7 @@ 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) && - !drm_is_control_client(priv)) { + if (drm_is_primary_client(priv) && !priv->minor->master) { /* create a new master */ priv->minor->master = drm_master_create(priv->minor); if (!priv->minor->master) { @@ -271,8 +270,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, goto out_close; } } - } else if (!drm_is_render_client(priv) && - !drm_is_control_client(priv)) { + } else if (drm_is_primary_client(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 f708aa19..7d3a7ce 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1232,7 +1232,7 @@ static inline bool drm_modeset_is_locked(struct drm_device *dev) return mutex_is_locked(&dev->mode_config.mutex); }

-static inline bool drm_is_render_client(struct drm_file *file_priv) +static inline bool drm_is_render_client(const struct drm_file *file_priv) { return file_priv->minor->type == DRM_MINOR_RENDER; } @@ -1242,6 +1242,11 @@ static inline bool drm_is_control_client(const struct drm_file *file_priv) return file_priv->minor->type == DRM_MINOR_CONTROL; }

+static inline bool drm_is_primary_client(const struct drm_file *file_priv) +{ + return file_priv->minor->type == DRM_MINOR_LEGACY; +} + /******************************************************************/ /** \name Internal function definitions */ /*@{*/

-- 1.7.10.4

Thomas Hellstrom

8:23 p.m.

New subject: [PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex v2

The master management was previously protected by the drm_device::struct_mutex. In order to avoid locking order violations in a reworked dropped master security check in the vmwgfx driver, break it out into a separate master_mutex.

Also remove drm_master::blocked since it's not used.

v2: Add an inline comment about what drm_device::master_mutex is protecting.

Signed-off-by: Thomas Hellstrom thellstrom@vmware.com Reviewed-by: Brian Paul brianp@vmware.com --- drivers/gpu/drm/drm_fops.c | 23 +++++++++++++--------- drivers/gpu/drm/drm_stub.c | 38 ++++++++++++++++++++++-------------- include/drm/drmP.h | 46 ++++++++++++++++++++++++-------------------- 3 files changed, 63 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index c7792b1..af55e2b 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -231,12 +231,13 @@ 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); + mutex_lock(&dev->master_mutex); if (drm_is_primary_client(priv) && !priv->minor->master) { /* create a new master */ + mutex_lock(&dev->struct_mutex); priv->minor->master = drm_master_create(priv->minor); + mutex_unlock(&dev->struct_mutex); if (!priv->minor->master) { - mutex_unlock(&dev->struct_mutex); ret = -ENOMEM; goto out_close; } @@ -245,28 +246,25 @@ static int drm_open_helper(struct inode *inode, struct file *filp, /* take another reference for the copy in the local file priv */ priv->master = drm_master_get(priv->minor->master);

+ mutex_lock(&dev->struct_mutex); priv->authenticated = 1;

mutex_unlock(&dev->struct_mutex); if (dev->driver->master_create) { ret = dev->driver->master_create(dev, priv->master); if (ret) { - mutex_lock(&dev->struct_mutex); /* drop both references if this fails */ drm_master_put(&priv->minor->master); drm_master_put(&priv->master); - mutex_unlock(&dev->struct_mutex); goto out_close; } } - mutex_lock(&dev->struct_mutex); if (dev->driver->master_set) { ret = dev->driver->master_set(dev, priv, true); if (ret) { /* drop both references if this fails */ drm_master_put(&priv->minor->master); drm_master_put(&priv->master); - mutex_unlock(&dev->struct_mutex); goto out_close; } } @@ -274,8 +272,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp, /* get a reference to the master */ priv->master = drm_master_get(priv->minor->master); } - mutex_unlock(&dev->struct_mutex);

+ mutex_unlock(&dev->master_mutex); mutex_lock(&dev->struct_mutex); list_add(&priv->lhead, &dev->filelist); mutex_unlock(&dev->struct_mutex); @@ -302,6 +300,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, return 0;

out_close: + mutex_unlock(&dev->master_mutex); if (dev->driver->postclose) dev->driver->postclose(dev, priv); out_prime_destroy: @@ -489,11 +488,13 @@ int drm_release(struct inode *inode, struct file *filp) } mutex_unlock(&dev->ctxlist_mutex);

- mutex_lock(&dev->struct_mutex); + mutex_lock(&dev->master_mutex);

if (file_priv->is_master) { 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)) @@ -512,6 +513,7 @@ int drm_release(struct inode *inode, struct file *filp) master->lock.file_priv = NULL; wake_up_interruptible_all(&master->lock.lock_queue); } + mutex_unlock(&dev->struct_mutex);

if (file_priv->minor->master == file_priv->master) { /* drop the reference held my the minor */ @@ -521,10 +523,13 @@ int drm_release(struct inode *inode, struct file *filp) } }

- /* drop the reference held my the file priv */ + /* drop the master reference held by the file priv */ if (file_priv->master) drm_master_put(&file_priv->master); file_priv->is_master = 0; + mutex_unlock(&dev->master_mutex); + + mutex_lock(&dev->struct_mutex); list_del(&file_priv->lhead); mutex_unlock(&dev->struct_mutex);

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index d344513..10c8303 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -152,6 +152,7 @@ static void drm_master_destroy(struct kref *kref) struct drm_device *dev = master->minor->dev; struct drm_map_list *r_list, *list_temp;

+ mutex_lock(&dev->struct_mutex); if (dev->driver->master_destroy) dev->driver->master_destroy(dev, master);

@@ -179,6 +180,7 @@ static void drm_master_destroy(struct kref *kref)

drm_ht_remove(&master->magiclist);

+ mutex_unlock(&dev->struct_mutex); kfree(master); }

@@ -194,19 +196,17 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, { int ret = 0;

+ mutex_lock(&dev->master_mutex); if (file_priv->is_master) - return 0; + goto out_unlock;

- if (file_priv->minor->master && file_priv->minor->master != file_priv->master) - return -EINVAL; + ret = -EINVAL; + if (file_priv->minor->master) + goto out_unlock;

if (!file_priv->master) - return -EINVAL; + goto out_unlock;

- 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) { @@ -216,27 +216,33 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, drm_master_put(&file_priv->minor->master); } } - mutex_unlock(&dev->struct_mutex);

+out_unlock: + mutex_unlock(&dev->master_mutex); return ret; }

int drm_dropmaster_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { + int ret = -EINVAL; + + mutex_lock(&dev->master_mutex); if (!file_priv->is_master) - return -EINVAL; + goto out_unlock;

if (!file_priv->minor->master) - return -EINVAL; + goto out_unlock;

- mutex_lock(&dev->struct_mutex); + ret = 0; if (dev->driver->master_drop) dev->driver->master_drop(dev, file_priv, false); drm_master_put(&file_priv->minor->master); file_priv->is_master = 0; - mutex_unlock(&dev->struct_mutex); - return 0; + +out_unlock: + mutex_unlock(&dev->master_mutex); + return ret; }

/* @@ -567,6 +573,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver, spin_lock_init(&dev->event_lock); mutex_init(&dev->struct_mutex); mutex_init(&dev->ctxlist_mutex); + mutex_init(&dev->master_mutex);

dev->anon_inode = drm_fs_inode_new(); if (IS_ERR(dev->anon_inode)) { @@ -620,6 +627,7 @@ err_minors: drm_minor_free(dev, DRM_MINOR_CONTROL); drm_fs_inode_free(dev->anon_inode); err_free: + mutex_destroy(&dev->master_mutex); kfree(dev); return NULL; } @@ -641,6 +649,8 @@ static void drm_dev_release(struct kref *ref) drm_minor_free(dev, DRM_MINOR_CONTROL);

kfree(dev->devname); + + mutex_destroy(&dev->master_mutex); kfree(dev); }

diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 30670d3..9a9a657 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -435,7 +435,8 @@ struct drm_prime_file_private { struct drm_file { unsigned always_authenticated :1; unsigned authenticated :1; - unsigned is_master :1; /* this file private is a master for a minor */ + /* Whether we're master for a minor. Protected by master_mutex */ + unsigned is_master :1; /* true when the client has asked us to expose stereo 3D mode flags */ unsigned stereo_allowed :1;

@@ -714,28 +715,29 @@ struct drm_gem_object {

#include <drm/drm_crtc.h>

-/* per-master structure */ +/** + * struct drm_master - drm master structure + * + * @refcount: Refcount for this master object. + * @minor: Link back to minor char device we are master for. Immutable. + * @unique: Unique identifier: e.g. busid. Protected by drm_global_mutex. + * @unique_len: Length of unique field. Protected by drm_global_mutex. + * @unique_size: Amount allocated. Protected by drm_global_mutex. + * @magiclist: Hash of used authentication tokens. Protected by struct_mutex. + * @magicfree: List of used authentication tokens. Protected by struct_mutex. + * @lock: DRI lock information. + * @driver_priv: Pointer to driver-private information. + */ struct drm_master { - - struct kref refcount; /* refcount for this master */ - - struct drm_minor *minor; /**< link back to minor we are a master for */ - - char *unique; /**< Unique identifier: e.g., busid */ - int unique_len; /**< Length of unique field */ - int unique_size; /**< amount allocated */ - - int blocked; /**< Blocked due to VC switch? */ - - /** \name Authentication */ - /*@{ */ + struct kref refcount; + struct drm_minor *minor; + char *unique; + int unique_len; + int unique_size; struct drm_open_hash magiclist; struct list_head magicfree; - /*@} */ - - struct drm_lock_data lock; /**< Information on hardware lock */ - - void *driver_priv; /**< Private structure for driver to use */ + struct drm_lock_data lock; + void *driver_priv; };

/* Size of ringbuffer for vblank timestamps. Just double-buffer @@ -1050,7 +1052,8 @@ struct drm_minor { struct list_head debugfs_list; struct mutex debugfs_lock; /* Protects debugfs_list. */

- struct drm_master *master; /* currently active master for this node */ + /* currently active master for this node. Protected by master_mutex */ + struct drm_master *master; struct drm_mode_group mode_group; };

@@ -1100,6 +1103,7 @@ struct drm_device { /*@{ */ spinlock_t count_lock; /**< For inuse, drm_device::open_count, drm_device::buf_use */ struct mutex struct_mutex; /**< For others */ + struct mutex master_mutex; /**< For drm_minor::master and drm_file::is_master */ /*@} */

/** \name Usage Counters */

-- 1.7.10.4

David Herrmann

28 Mar 28 Mar

12:19 a.m.

New subject: [PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex v2

On Thu, Mar 27, 2014 at 9:23 PM, Thomas Hellstrom thellstrom@vmware.com wrote:

...

The master management was previously protected by the drm_device::struct_mutex. In order to avoid locking order violations in a reworked dropped master security check in the vmwgfx driver, break it out into a separate master_mutex.

Also remove drm_master::blocked since it's not used.

v2: Add an inline comment about what drm_device::master_mutex is protecting.

Signed-off-by: Thomas Hellstrom thellstrom@vmware.com Reviewed-by: Brian Paul brianp@vmware.com

drivers/gpu/drm/drm_fops.c | 23 +++++++++++++--------- drivers/gpu/drm/drm_stub.c | 38 ++++++++++++++++++++++-------------- include/drm/drmP.h | 46 ++++++++++++++++++++++++-------------------- 3 files changed, 63 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index c7792b1..af55e2b 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -231,12 +231,13 @@ 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);
  mutex_lock(&dev->master_mutex);
  if (drm_is_primary_client(priv) && !priv->minor->master) {
          /* create a new master */
          mutex_lock(&dev->struct_mutex);
          priv->minor->master = drm_master_create(priv->minor);
          mutex_unlock(&dev->struct_mutex);

drm_master_create() doesn't need any locking (considering your removal of master_list), so this locking is exclusively to set ->master? See below.

...

            if (!priv->minor->master) {

                  mutex_unlock(&dev->struct_mutex);
                  ret = -ENOMEM;
                  goto out_close;
          }

@@ -245,28 +246,25 @@ static int drm_open_helper(struct inode *inode, struct file *filp, /* take another reference for the copy in the local file priv */ priv->master = drm_master_get(priv->minor->master);

          mutex_lock(&dev->struct_mutex);
          priv->authenticated = 1;

          mutex_unlock(&dev->struct_mutex);
          if (dev->driver->master_create) {
                  ret = dev->driver->master_create(dev, priv->master);
                  if (ret) {

                          mutex_lock(&dev->struct_mutex);
                          /* drop both references if this fails */
                          drm_master_put(&priv->minor->master);
                          drm_master_put(&priv->master);

                          mutex_unlock(&dev->struct_mutex);

drm_master_put() resets the pointers to NULL. So _if_ the locking above is needed, then this one _must_ stay, too.

I also don't quite understand why you move the struct_mutex locking into drm_master_destroy() instead of requiring callers to lock it as before? I mean, the whole function is protected by the lock..

...

                            goto out_close;
                    }
            }

          mutex_lock(&dev->struct_mutex);
          if (dev->driver->master_set) {
                  ret = dev->driver->master_set(dev, priv, true);

vmwgfx is the only driver using that, so I guess you reviewed this lock-change.

...

                    if (ret) {
                            /* drop both references if this fails */
                            drm_master_put(&priv->minor->master);
                            drm_master_put(&priv->master);

same as above

...

                          mutex_unlock(&dev->struct_mutex);
                          goto out_close;
                  }
          }
@@ -274,8 +272,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp, /* get a reference to the master */ priv->master = drm_master_get(priv->minor->master); }
  mutex_unlock(&dev->struct_mutex);
  mutex_unlock(&dev->master_mutex);

Weird white-space change..

...

    mutex_lock(&dev->struct_mutex);
    list_add(&priv->lhead, &dev->filelist);
    mutex_unlock(&dev->struct_mutex);
@@ -302,6 +300,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, return 0;

out_close:
  mutex_unlock(&dev->master_mutex);
  if (dev->driver->postclose)
          dev->driver->postclose(dev, priv);
out_prime_destroy: @@ -489,11 +488,13 @@ int drm_release(struct inode *inode, struct file *filp) } mutex_unlock(&dev->ctxlist_mutex);
  mutex_lock(&dev->struct_mutex);
  mutex_lock(&dev->master_mutex);

  if (file_priv->is_master) {
          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))
@@ -512,6 +513,7 @@ int drm_release(struct inode *inode, struct file *filp) master->lock.file_priv = NULL; wake_up_interruptible_all(&master->lock.lock_queue); }
          mutex_unlock(&dev->struct_mutex);

          if (file_priv->minor->master == file_priv->master) {
                  /* drop the reference held my the minor */
@@ -521,10 +523,13 @@ int drm_release(struct inode *inode, struct file *filp) } }
  /* drop the reference held my the file priv */
  /* drop the master reference held by the file priv */
  if (file_priv->master)
          drm_master_put(&file_priv->master);
  file_priv->is_master = 0;
  mutex_unlock(&dev->master_mutex);
  mutex_lock(&dev->struct_mutex);
  list_del(&file_priv->lhead);
  mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index d344513..10c8303 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -152,6 +152,7 @@ static void drm_master_destroy(struct kref *kref) struct drm_device *dev = master->minor->dev; struct drm_map_list *r_list, *list_temp;
  mutex_lock(&dev->struct_mutex);
  if (dev->driver->master_destroy)
          dev->driver->master_destroy(dev, master);
@@ -179,6 +180,7 @@ static void drm_master_destroy(struct kref *kref)
    drm_ht_remove(&master->magiclist);
  mutex_unlock(&dev->struct_mutex);
  kfree(master);
}

@@ -194,19 +196,17 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, {for setting int ret = 0;
  mutex_lock(&dev->master_mutex);

Yey! One more step towards DRM_UNLOCKED.

...

    if (file_priv->is_master)

```
          return 0;
```

```
          goto out_unlock;
```

  if (file_priv->minor->master && file_priv->minor->master != file_priv->master)

```
          return -EINVAL;
```

That one was redundant.. yey..

...

```
  ret = -EINVAL;
```

This breaks all drivers with set_master == NULL. We never return 0 then.. I also dislike setting error-codes before doing the comparison just to drop the curly brackets.. that used to be common kernel-coding-style, but I thought we all stopped doing that. Same for dropmaster below, but just my personal opinion.

...

```
  if (file_priv->minor->master)
```

          goto out_unlock;

  if (!file_priv->master)

```
          return -EINVAL;
```

```
          goto out_unlock;
```

```
  if (file_priv->minor->master)
```
```
          return -EINVAL;
```

  mutex_lock(&dev->struct_mutex);
  file_priv->minor->master = drm_master_get(file_priv->master);

You set minor->master unlocked here again. See my reasoning above..

...

    file_priv->is_master = 1;
    if (dev->driver->master_set) {

@@ -216,27 +216,33 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, drm_master_put(&file_priv->minor->master); } }

```
  mutex_unlock(&dev->struct_mutex);
```

+out_unlock:

  mutex_unlock(&dev->master_mutex);
  return ret;

}

int drm_dropmaster_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) {

```
  int ret = -EINVAL;
```

  mutex_lock(&dev->master_mutex);
  if (!file_priv->is_master)

```
          return -EINVAL;
```

          goto out_unlock;

  if (!file_priv->minor->master)

```
          return -EINVAL;
```

```
          goto out_unlock;
```

```
  mutex_lock(&dev->struct_mutex);
```

  ret = 0;
  if (dev->driver->master_drop)
          dev->driver->master_drop(dev, file_priv, false);
  drm_master_put(&file_priv->minor->master);

Again setting minor->master unlocked.

Thanks David

...

    file_priv->is_master = 0;
  mutex_unlock(&dev->struct_mutex);
  return 0;
+out_unlock:
  mutex_unlock(&dev->master_mutex);
  return ret;
}

/* @@ -567,6 +573,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver, spin_lock_init(&dev->event_lock); mutex_init(&dev->struct_mutex); mutex_init(&dev->ctxlist_mutex);
  mutex_init(&dev->master_mutex);

  dev->anon_inode = drm_fs_inode_new();
  if (IS_ERR(dev->anon_inode)) {
@@ -620,6 +627,7 @@ err_minors: drm_minor_free(dev, DRM_MINOR_CONTROL); drm_fs_inode_free(dev->anon_inode); err_free:
  mutex_destroy(&dev->master_mutex);
  kfree(dev);
  return NULL;
} @@ -641,6 +649,8 @@ static void drm_dev_release(struct kref *ref) drm_minor_free(dev, DRM_MINOR_CONTROL);
    kfree(dev->devname);
  mutex_destroy(&dev->master_mutex);
  kfree(dev);
}

diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 30670d3..9a9a657 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -435,7 +435,8 @@ struct drm_prime_file_private { struct drm_file { unsigned always_authenticated :1; unsigned authenticated :1;
  unsigned is_master :1; /* this file private is a master for a minor */
  /* Whether we're master for a minor. Protected by master_mutex */
  unsigned is_master :1;
  /* true when the client has asked us to expose stereo 3D mode flags */
  unsigned stereo_allowed :1;
@@ -714,28 +715,29 @@ struct drm_gem_object {

#include <drm/drm_crtc.h>

-/* per-master structure */ +/**

struct drm_master - drm master structure

@refcount: Refcount for this master object.

@minor: Link back to minor char device we are master for. Immutable.

@unique: Unique identifier: e.g. busid. Protected by drm_global_mutex.

@unique_len: Length of unique field. Protected by drm_global_mutex.

@unique_size: Amount allocated. Protected by drm_global_mutex.

@magiclist: Hash of used authentication tokens. Protected by struct_mutex.

@magicfree: List of used authentication tokens. Protected by struct_mutex.

@lock: DRI lock information.

@driver_priv: Pointer to driver-private information.

*/

struct drm_master {
  struct kref refcount; /* refcount for this master */
  struct drm_minor *minor; /**< link back to minor we are a master for */
  char *unique;                   /**< Unique identifier: e.g., busid */
  int unique_len;                 /**< Length of unique field */
  int unique_size;                /**< amount allocated */
  int blocked;                    /**< Blocked due to VC switch? */
  /** \name Authentication */
  /*@{ */
  struct kref refcount;
  struct drm_minor *minor;
  char *unique;
  int unique_len;
  int unique_size;
  struct drm_open_hash magiclist;
  struct list_head magicfree;
  /*@} */
  struct drm_lock_data lock;      /**< Information on hardware lock */
  void *driver_priv; /**< Private structure for driver to use */
  struct drm_lock_data lock;
  void *driver_priv;
};

/* Size of ringbuffer for vblank timestamps. Just double-buffer @@ -1050,7 +1052,8 @@ struct drm_minor { struct list_head debugfs_list; struct mutex debugfs_lock; /* Protects debugfs_list. */
  struct drm_master *master; /* currently active master for this node */
  /* currently active master for this node. Protected by master_mutex */
  struct drm_master *master;
  struct drm_mode_group mode_group;
};

@@ -1100,6 +1103,7 @@ struct drm_device { /*@{ */ spinlock_t count_lock; /**< For inuse, drm_device::open_count, drm_device::buf_use */ struct mutex struct_mutex; /**< For others */
  struct mutex master_mutex;      /**< For drm_minor::master and drm_file::is_master */
  /*@} */

  /** \name Usage Counters */
-- 1.7.10.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

Thomas Hellstrom

8:08 a.m.

New subject: [PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex v2

Hi, David.

Thanks for reviewing. I'll try to address all your concerns and resend.

/Thomas

On 03/28/2014 01:19 AM, David Herrmann wrote:

...

Hi

On Thu, Mar 27, 2014 at 9:23 PM, Thomas Hellstrom thellstrom@vmware.com wrote:

...
The master management was previously protected by the drm_device::struct_mutex. In order to avoid locking order violations in a reworked dropped master security check in the vmwgfx driver, break it out into a separate master_mutex.

Also remove drm_master::blocked since it's not used.

v2: Add an inline comment about what drm_device::master_mutex is protecting.

Signed-off-by: Thomas Hellstrom thellstrom@vmware.com Reviewed-by: Brian Paul brianp@vmware.com

drivers/gpu/drm/drm_fops.c | 23 +++++++++++++--------- drivers/gpu/drm/drm_stub.c | 38 ++++++++++++++++++++++-------------- include/drm/drmP.h | 46 ++++++++++++++++++++++++-------------------- 3 files changed, 63 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index c7792b1..af55e2b 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -231,12 +231,13 @@ 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);
  mutex_lock(&dev->master_mutex);
  if (drm_is_primary_client(priv) && !priv->minor->master) {
          /* create a new master */
          mutex_lock(&dev->struct_mutex);
          priv->minor->master = drm_master_create(priv->minor);
          mutex_unlock(&dev->struct_mutex);
drm_master_create() doesn't need any locking (considering your removal of master_list), so this locking is exclusively to set ->master? See below.

...
            if (!priv->minor->master) {
                  mutex_unlock(&dev->struct_mutex);
                  ret = -ENOMEM;
                  goto out_close;
          }
@@ -245,28 +246,25 @@ static int drm_open_helper(struct inode *inode, struct file *filp, /* take another reference for the copy in the local file priv */ priv->master = drm_master_get(priv->minor->master);
          mutex_lock(&dev->struct_mutex);
          priv->authenticated = 1;

          mutex_unlock(&dev->struct_mutex);
          if (dev->driver->master_create) {
                  ret = dev->driver->master_create(dev, priv->master);
                  if (ret) {
                          mutex_lock(&dev->struct_mutex);
                          /* drop both references if this fails */
                          drm_master_put(&priv->minor->master);
                          drm_master_put(&priv->master);
                          mutex_unlock(&dev->struct_mutex);
drm_master_put() resets the pointers to NULL. So _if_ the locking above is needed, then this one _must_ stay, too.

I also don't quite understand why you move the struct_mutex locking into drm_master_destroy() instead of requiring callers to lock it as before? I mean, the whole function is protected by the lock..

...
                            goto out_close;
                    }
            }
          mutex_lock(&dev->struct_mutex);
          if (dev->driver->master_set) {
                  ret = dev->driver->master_set(dev, priv, true);
vmwgfx is the only driver using that, so I guess you reviewed this lock-change.

...
                    if (ret) {
                            /* drop both references if this fails */
                            drm_master_put(&priv->minor->master);
                            drm_master_put(&priv->master);
same as above

...
                          mutex_unlock(&dev->struct_mutex);
                          goto out_close;
                  }
          }
@@ -274,8 +272,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp, /* get a reference to the master */ priv->master = drm_master_get(priv->minor->master); }
  mutex_unlock(&dev->struct_mutex);
  mutex_unlock(&dev->master_mutex);
Weird white-space change..

...
    mutex_lock(&dev->struct_mutex);
    list_add(&priv->lhead, &dev->filelist);
    mutex_unlock(&dev->struct_mutex);
@@ -302,6 +300,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, return 0;

out_close:
  mutex_unlock(&dev->master_mutex);
  if (dev->driver->postclose)
          dev->driver->postclose(dev, priv);
out_prime_destroy: @@ -489,11 +488,13 @@ int drm_release(struct inode *inode, struct file *filp) } mutex_unlock(&dev->ctxlist_mutex);
  mutex_lock(&dev->struct_mutex);
  mutex_lock(&dev->master_mutex);

  if (file_priv->is_master) {
          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))
@@ -512,6 +513,7 @@ int drm_release(struct inode *inode, struct file *filp) master->lock.file_priv = NULL; wake_up_interruptible_all(&master->lock.lock_queue); }
          mutex_unlock(&dev->struct_mutex);

          if (file_priv->minor->master == file_priv->master) {
                  /* drop the reference held my the minor */
@@ -521,10 +523,13 @@ int drm_release(struct inode *inode, struct file *filp) } }
  /* drop the reference held my the file priv */
  /* drop the master reference held by the file priv */
  if (file_priv->master)
          drm_master_put(&file_priv->master);
  file_priv->is_master = 0;
  mutex_unlock(&dev->master_mutex);
  mutex_lock(&dev->struct_mutex);
  list_del(&file_priv->lhead);
  mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index d344513..10c8303 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -152,6 +152,7 @@ static void drm_master_destroy(struct kref *kref) struct drm_device *dev = master->minor->dev; struct drm_map_list *r_list, *list_temp;
  mutex_lock(&dev->struct_mutex);
  if (dev->driver->master_destroy)
          dev->driver->master_destroy(dev, master);
@@ -179,6 +180,7 @@ static void drm_master_destroy(struct kref *kref)
    drm_ht_remove(&master->magiclist);
  mutex_unlock(&dev->struct_mutex);
  kfree(master);
}

@@ -194,19 +196,17 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, {for setting int ret = 0;
  mutex_lock(&dev->master_mutex);
Yey! One more step towards DRM_UNLOCKED.

...
    if (file_priv->is_master)
          return 0;
          goto out_unlock;
  if (file_priv->minor->master && file_priv->minor->master != file_priv->master)
          return -EINVAL;
That one was redundant.. yey..

...
  ret = -EINVAL;
This breaks all drivers with set_master == NULL. We never return 0 then.. I also dislike setting error-codes before doing the comparison just to drop the curly brackets.. that used to be common kernel-coding-style, but I thought we all stopped doing that. Same for dropmaster below, but just my personal opinion.

...
  if (file_priv->minor->master)
          goto out_unlock;

  if (!file_priv->master)
          return -EINVAL;
          goto out_unlock;
  if (file_priv->minor->master)
          return -EINVAL;
  mutex_lock(&dev->struct_mutex);
  file_priv->minor->master = drm_master_get(file_priv->master);
You set minor->master unlocked here again. See my reasoning above..

...
    file_priv->is_master = 1;
    if (dev->driver->master_set) {
@@ -216,27 +216,33 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, drm_master_put(&file_priv->minor->master); } }
  mutex_unlock(&dev->struct_mutex);
+out_unlock:
  mutex_unlock(&dev->master_mutex);
  return ret;
}

int drm_dropmaster_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) {
  int ret = -EINVAL;
  mutex_lock(&dev->master_mutex);
  if (!file_priv->is_master)
          return -EINVAL;
          goto out_unlock;

  if (!file_priv->minor->master)
          return -EINVAL;
          goto out_unlock;
  mutex_lock(&dev->struct_mutex);
  ret = 0;
  if (dev->driver->master_drop)
          dev->driver->master_drop(dev, file_priv, false);
  drm_master_put(&file_priv->minor->master);
Again setting minor->master unlocked.

Thanks David

...
    file_priv->is_master = 0;
  mutex_unlock(&dev->struct_mutex);
  return 0;
+out_unlock:
  mutex_unlock(&dev->master_mutex);
  return ret;
}

/* @@ -567,6 +573,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver, spin_lock_init(&dev->event_lock); mutex_init(&dev->struct_mutex); mutex_init(&dev->ctxlist_mutex);
  mutex_init(&dev->master_mutex);

  dev->anon_inode = drm_fs_inode_new();
  if (IS_ERR(dev->anon_inode)) {
@@ -620,6 +627,7 @@ err_minors: drm_minor_free(dev, DRM_MINOR_CONTROL); drm_fs_inode_free(dev->anon_inode); err_free:
  mutex_destroy(&dev->master_mutex);
  kfree(dev);
  return NULL;
} @@ -641,6 +649,8 @@ static void drm_dev_release(struct kref *ref) drm_minor_free(dev, DRM_MINOR_CONTROL);
    kfree(dev->devname);
  mutex_destroy(&dev->master_mutex);
  kfree(dev);
}

diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 30670d3..9a9a657 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -435,7 +435,8 @@ struct drm_prime_file_private { struct drm_file { unsigned always_authenticated :1; unsigned authenticated :1;
  unsigned is_master :1; /* this file private is a master for a minor */
  /* Whether we're master for a minor. Protected by master_mutex */
  unsigned is_master :1;
  /* true when the client has asked us to expose stereo 3D mode flags */
  unsigned stereo_allowed :1;
@@ -714,28 +715,29 @@ struct drm_gem_object {

#include <drm/drm_crtc.h>

-/* per-master structure */ +/**

struct drm_master - drm master structure

@refcount: Refcount for this master object.

@minor: Link back to minor char device we are master for. Immutable.

@unique: Unique identifier: e.g. busid. Protected by drm_global_mutex.

@unique_len: Length of unique field. Protected by drm_global_mutex.

@unique_size: Amount allocated. Protected by drm_global_mutex.

@magiclist: Hash of used authentication tokens. Protected by struct_mutex.

@magicfree: List of used authentication tokens. Protected by struct_mutex.

@lock: DRI lock information.

@driver_priv: Pointer to driver-private information.

*/

struct drm_master {
  struct kref refcount; /* refcount for this master */
  struct drm_minor *minor; /**< link back to minor we are a master for */
  char *unique;                   /**< Unique identifier: e.g., busid */
  int unique_len;                 /**< Length of unique field */
  int unique_size;                /**< amount allocated */
  int blocked;                    /**< Blocked due to VC switch? */
  /** \name Authentication */
  /*@{ */
  struct kref refcount;
  struct drm_minor *minor;
  char *unique;
  int unique_len;
  int unique_size;
  struct drm_open_hash magiclist;
  struct list_head magicfree;
  /*@} */
  struct drm_lock_data lock;      /**< Information on hardware lock */
  void *driver_priv; /**< Private structure for driver to use */
  struct drm_lock_data lock;
  void *driver_priv;
};

/* Size of ringbuffer for vblank timestamps. Just double-buffer @@ -1050,7 +1052,8 @@ struct drm_minor { struct list_head debugfs_list; struct mutex debugfs_lock; /* Protects debugfs_list. */
  struct drm_master *master; /* currently active master for this node */
  /* currently active master for this node. Protected by master_mutex */
  struct drm_master *master;
  struct drm_mode_group mode_group;
};

@@ -1100,6 +1103,7 @@ struct drm_device { /*@{ */ spinlock_t count_lock; /**< For inuse, drm_device::open_count, drm_device::buf_use */ struct mutex struct_mutex; /**< For others */
  struct mutex master_mutex;      /**< For drm_minor::master and drm_file::is_master */
  /*@} */

  /** \name Usage Counters */
-- 1.7.10.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://urldefense.proofpoint.com/v1/url?u=http://lists.freedesktop.org/mail...

Thomas Hellstrom

8:57 a.m.

New subject: [PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex v2

Hi, again!

I've looked through the code again and have some answers to your questions. Will post an updated patch soon.

On 03/28/2014 01:19 AM, David Herrmann wrote:

...

Hi

On Thu, Mar 27, 2014 at 9:23 PM, Thomas Hellstrom thellstrom@vmware.com wrote:

...
The master management was previously protected by the drm_device::struct_mutex. In order to avoid locking order violations in a reworked dropped master security check in the vmwgfx driver, break it out into a separate master_mutex.

Also remove drm_master::blocked since it's not used.

v2: Add an inline comment about what drm_device::master_mutex is protecting.

Signed-off-by: Thomas Hellstrom thellstrom@vmware.com Reviewed-by: Brian Paul brianp@vmware.com

drivers/gpu/drm/drm_fops.c | 23 +++++++++++++--------- drivers/gpu/drm/drm_stub.c | 38 ++++++++++++++++++++++-------------- include/drm/drmP.h | 46 ++++++++++++++++++++++++-------------------- 3 files changed, 63 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index c7792b1..af55e2b 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -231,12 +231,13 @@ 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);
  mutex_lock(&dev->master_mutex);
  if (drm_is_primary_client(priv) && !priv->minor->master) {
          /* create a new master */
          mutex_lock(&dev->struct_mutex);
          priv->minor->master = drm_master_create(priv->minor);
          mutex_unlock(&dev->struct_mutex);
drm_master_create() doesn't need any locking (considering your removal of master_list), so this locking is exclusively to set ->master? See below.

No. The locking here is, as you say, unneeded. drm_minor::master is protected by master_mutex. I'll remove, and while doing that, I'll also remove the unneeded locking around priv->authenticated = 1.

...

            if (!priv->minor->master) {

                  mutex_unlock(&dev->struct_mutex);
                  ret = -ENOMEM;
                  goto out_close;
          }

          mutex_lock(&dev->struct_mutex);
          priv->authenticated = 1;

          mutex_unlock(&dev->struct_mutex);
          if (dev->driver->master_create) {
                  ret = dev->driver->master_create(dev, priv->master);
                  if (ret) {

                          mutex_lock(&dev->struct_mutex);
                          /* drop both references if this fails */
                          drm_master_put(&priv->minor->master);
                          drm_master_put(&priv->master);

                          mutex_unlock(&dev->struct_mutex);

drm_master_put() resets the pointers to NULL. So _if_ the locking above is needed, then this one _must_ stay, too.

It's unneeded, so this should be OK. As for drm_file::master, it should be considered immutable, except for drm_file open() and release() where nobody is racing us.

...

I also don't quite understand why you move the struct_mutex locking into drm_master_destroy() instead of requiring callers to lock it as before? I mean, the whole function is protected by the lock..

...
                            goto out_close;
                    }
            }
          mutex_lock(&dev->struct_mutex);
          if (dev->driver->master_set) {
                  ret = dev->driver->master_set(dev, priv, true);
vmwgfx is the only driver using that, so I guess you reviewed this lock-change.

Yes.

...

                    if (ret) {
                            /* drop both references if this fails */
                            drm_master_put(&priv->minor->master);
                            drm_master_put(&priv->master);

same as above

No change needed.

...

...
                          mutex_unlock(&dev->struct_mutex);
                          goto out_close;
                  }
          }
@@ -274,8 +272,8 @@ static int drm_open_helper(struct inode *inode, struct file *filp, /* get a reference to the master */ priv->master = drm_master_get(priv->minor->master); }
  mutex_unlock(&dev->struct_mutex);
  mutex_unlock(&dev->master_mutex);
Weird white-space change..

Will fix.

...

...
    mutex_lock(&dev->struct_mutex);
    list_add(&priv->lhead, &dev->filelist);
    mutex_unlock(&dev->struct_mutex);
@@ -302,6 +300,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, return 0;

out_close:
  mutex_unlock(&dev->master_mutex);
  if (dev->driver->postclose)
          dev->driver->postclose(dev, priv);
out_prime_destroy: @@ -489,11 +488,13 @@ int drm_release(struct inode *inode, struct file *filp) } mutex_unlock(&dev->ctxlist_mutex);
  mutex_lock(&dev->struct_mutex);
  mutex_lock(&dev->master_mutex);

  if (file_priv->is_master) {
          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))
@@ -512,6 +513,7 @@ int drm_release(struct inode *inode, struct file *filp) master->lock.file_priv = NULL; wake_up_interruptible_all(&master->lock.lock_queue); }
          mutex_unlock(&dev->struct_mutex);

          if (file_priv->minor->master == file_priv->master) {
                  /* drop the reference held my the minor */
@@ -521,10 +523,13 @@ int drm_release(struct inode *inode, struct file *filp) } }
  /* drop the reference held my the file priv */
  /* drop the master reference held by the file priv */
  if (file_priv->master)
          drm_master_put(&file_priv->master);
  file_priv->is_master = 0;
  mutex_unlock(&dev->master_mutex);
  mutex_lock(&dev->struct_mutex);
  list_del(&file_priv->lhead);
  mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index d344513..10c8303 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -152,6 +152,7 @@ static void drm_master_destroy(struct kref *kref) struct drm_device *dev = master->minor->dev; struct drm_map_list *r_list, *list_temp;
  mutex_lock(&dev->struct_mutex);
  if (dev->driver->master_destroy)
          dev->driver->master_destroy(dev, master);
@@ -179,6 +180,7 @@ static void drm_master_destroy(struct kref *kref)
    drm_ht_remove(&master->magiclist);
  mutex_unlock(&dev->struct_mutex);
  kfree(master);
}

@@ -194,19 +196,17 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, {for setting int ret = 0;
  mutex_lock(&dev->master_mutex);
Yey! One more step towards DRM_UNLOCKED.

...
    if (file_priv->is_master)
          return 0;
          goto out_unlock;
  if (file_priv->minor->master && file_priv->minor->master != file_priv->master)
          return -EINVAL;
That one was redundant.. yey..

...
  ret = -EINVAL;
This breaks all drivers with set_master == NULL. We never return 0 then.. I also dislike setting error-codes before doing the comparison just to drop the curly brackets.. that used to be common kernel-coding-style, but I thought we all stopped doing that. Same for dropmaster below, but just my personal opinion.

Will fix.

...

```
  if (file_priv->minor->master)
```

          goto out_unlock;

  if (!file_priv->master)

```
          return -EINVAL;
```

```
          goto out_unlock;
```

```
  if (file_priv->minor->master)
```
```
          return -EINVAL;
```

  mutex_lock(&dev->struct_mutex);
  file_priv->minor->master = drm_master_get(file_priv->master);

You set minor->master unlocked here again. See my reasoning above..

Protected by master_mutex.

...

...
    file_priv->is_master = 1;
    if (dev->driver->master_set) {
@@ -216,27 +216,33 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, drm_master_put(&file_priv->minor->master); } }
  mutex_unlock(&dev->struct_mutex);
+out_unlock:
  mutex_unlock(&dev->master_mutex);
  return ret;
}

int drm_dropmaster_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) {
  int ret = -EINVAL;
  mutex_lock(&dev->master_mutex);
  if (!file_priv->is_master)
          return -EINVAL;
          goto out_unlock;

  if (!file_priv->minor->master)
          return -EINVAL;
          goto out_unlock;
  mutex_lock(&dev->struct_mutex);
  ret = 0;
  if (dev->driver->master_drop)
          dev->driver->master_drop(dev, file_priv, false);
  drm_master_put(&file_priv->minor->master);
Again setting minor->master unlocked.

Protected by master_mutex.

...

Thanks David

Thanks, Thomas

Thomas Hellstrom

9:45 a.m.

New subject: [PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex v2

On 03/28/2014 01:19 AM, David Herrmann wrote:

...

Hi

I also don't quite understand why you move the struct_mutex locking into drm_master_destroy() instead of requiring callers to lock it as before? I mean, the whole function is protected by the lock..

Before, struct_mutex was required outside of drm_master_put(), because the argument to drm_master_put was also protected by struct_mutex. Now that's no longer the case and I chose to move the locking into the destructor, avoiding a number of unnecessary struct_mutex locks (the master destructor is called more seldom than master_put()). Also in general I believe one should avoid locking around unreferencing if possible, because it leads to ugly constructs (and even static code analyzers complaining) if the lock has to be temporarily released in the destructor to avoid locking order violations. But in the end I guess that's really a matter of taste.

/Thomas

Thomas Hellstrom

27 Mar 27 Mar

8:23 p.m.

New subject: [PATCH 12/16] drm/vmwgfx: Tighten security around surface sharing v2

If using legacy (non-prime) surface sharing, only allow surfaces to be shared between clients with the same master. This will block malicious clients from peeking at contents at surfaces from other (possibly vt-switched) masters.

v2: s/legacy_client/primary_client/

Signed-off-by: Thomas Hellstrom thellstrom@vmware.com Reviewed-by: Brian Paul brianp@vmware.com --- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c index d50cd76..4ecdbf3 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c @@ -36,11 +36,13 @@ * @base: The TTM base object handling user-space visibility. * @srf: The surface metadata. * @size: TTM accounting size for the surface. + * @master: master of the creating client. Used for security check. */ struct vmw_user_surface { struct ttm_prime_object prime; struct vmw_surface srf; uint32_t size; + struct drm_master *master; };

/** @@ -624,6 +626,8 @@ static void vmw_user_surface_free(struct vmw_resource *res) struct vmw_private *dev_priv = srf->res.dev_priv; uint32_t size = user_srf->size;

+ if (user_srf->master) + drm_master_put(&user_srf->master); kfree(srf->offsets); kfree(srf->sizes); kfree(srf->snooper.image); @@ -819,6 +823,8 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data,

user_srf->prime.base.shareable = false; user_srf->prime.base.tfile = NULL; + if (drm_is_primary_client(file_priv)) + user_srf->master = drm_master_get(file_priv->master);

/** * From this point, the generic resource management functions @@ -885,6 +891,7 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, struct ttm_base_object **base_p) { struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile; + struct vmw_user_surface *user_srf; uint32_t handle; struct ttm_base_object *base; int ret; @@ -915,6 +922,21 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, }

if (handle_type != DRM_VMW_HANDLE_PRIME) { + user_srf = container_of(base, struct vmw_user_surface, + prime.base); + + /* + * Make sure the surface creator has the same + * authenticating master. + */ + if (drm_is_primary_client(file_priv) && + user_srf->master != file_priv->master) { + DRM_ERROR("Trying to reference surface outside of" + " master domain.\n"); + ret = -EACCES; + goto out_bad_resource; + } + ret = ttm_ref_object_add(tfile, base, TTM_REF_USAGE, NULL); if (unlikely(ret != 0)) { DRM_ERROR("Could not add a reference to a surface.\n"); @@ -1273,6 +1295,8 @@ int vmw_gb_surface_define_ioctl(struct drm_device *dev, void *data,

user_srf->prime.base.shareable = false; user_srf->prime.base.tfile = NULL; + if (drm_is_primary_client(file_priv)) + user_srf->master = drm_master_get(file_priv->master);

/** * From this point, the generic resource management functions

-- 1.7.10.4

4054

Age (days ago)

4055

Last active (days ago)

dri-devel@lists.freedesktop.org

7 comments

2 participants

tags (0)

participants (2)

David Herrmann
Thomas Hellstrom