linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v8 0/7] drm: update locking for modesetting
@ 2021-08-26  2:01 Desmond Cheong Zhi Xi
  2021-08-26  2:01 ` [PATCH v8 1/7] drm: fix null ptr dereference in drm_master_release Desmond Cheong Zhi Xi
                   ` (6 more replies)
  0 siblings, 7 replies; 20+ messages in thread
From: Desmond Cheong Zhi Xi @ 2021-08-26  2:01 UTC (permalink / raw)
  To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel,
	sumit.semwal, christian.koenig, jani.nikula, joonas.lahtinen,
	rodrigo.vivi, chris, ville.syrjala, matthew.auld, dan.carpenter,
	tvrtko.ursulin, matthew.d.roper, lucas.demarchi, karthik.b.s,
	jose.souza, manasi.d.navare, airlied, aditya.swarup, andrescj,
	linux-graphics-maintainer, zackr
  Cc: Desmond Cheong Zhi Xi, dri-devel, linux-kernel, intel-gfx,
	linux-media, linaro-mm-sig, skhan, gregkh, linux-kernel-mentees

Hi,

Seems that Intel-gfx CI still doesn't like what's going on, so I updated
the series to remove more recursive locking again.

Note: patch 5 touches a number of files, including the Intel and VMware
drivers, but most changes are simply switching a function call to the
appropriate locked/unlocked version.

Overall, this series fixes races with modesetting rights, converts
drm_device.master_mutex into master_rwsem, and removes
drm_file.master_lookup_lock.

- Patch 1: Fix a potential null ptr dereference in drm_master_release

- Patch 2: Convert master_mutex into rwsem (avoids creating a new lock)

- Patch 3: Update global mutex locking in the ioctl handler (avoids
deadlock when grabbing read lock on master_rwsem in drm_ioctl_kernel)

- Patch 4: Plug races with drm modesetting rights

- Patch 5: Modify drm_mode_object_find to fix potential recursive
locking of master_rwsem and lock inversions between modeset_mutex and
master_rwsem

- Patch 6: Remove remaining potential recursive locking of master_rwsem
and lock inversions between modeset_mutex and master_rwsem from calling
drm_lease_held

- Patch 7: Replace master_lookup_lock with master_rwsem

v7 -> v8:
- Avoid calling drm_lease_held in drm_mode_setcrtc and
drm_wait_vblank_ioctl, caught by Intel-gfx CI (patch 6)

v6 -> v7:
- Export __drm_mode_object_find for loadable modules, caught by the
Intel-gfx CI (patch 5)

v5 -> v6:
- Fix recursive locking on master_rwsem, caught by the Intel-gfx CI
(patch 5 & 6)

v4 -> v5:
- Avoid calling drm_file_get_master while holding on to the modeset
mutex, caught by the Intel-gfx CI (patch 5 & 6)

v3 -> v4 (suggested by Daniel Vetter):
- Drop a patch that added an unnecessary master_lookup_lock in
drm_master_release
- Drop a patch that addressed a non-existent race in
drm_is_current_master_locked
- Remove fixes for non-existent null ptr dereferences
- Protect drm_master.magic_map,unique{_len} with master_rwsem instead of
master_lookup_lock
- Drop the patch that moved master_lookup_lock into struct drm_device
- Drop a patch to export task_work_add
- Revert the check for the global mutex in the ioctl handler to use
drm_core_check_feature instead of drm_dev_needs_global_mutex
- Push down master_rwsem locking for selected ioctls to avoid lock
hierarchy inversions, and to allow us to hold write locks on
master_rwsem instead of flushing readers
- Remove master_lookup_lock by replacing it with master_rwsem

v2 -> v3:
- Unexport drm_master_flush, as suggested by Daniel Vetter.
- Merge master_mutex and master_rwsem, as suggested by Daniel Vetter.
- Export task_work_add, reported by kernel test robot.
- Make master_flush static, reported by kernel test robot.
- Move master_lookup_lock into struct drm_device.
- Add a missing lock on master_lookup_lock in drm_master_release.
- Fix a potential race in drm_is_current_master_locked.
- Fix potential null ptr dereferences in drm_{auth, ioctl}.
- Protect magic_map,unique{_len} with  master_lookup_lock.
- Convert master_mutex into a rwsem.
- Update global mutex locking in the ioctl handler.

v1 -> v2 (suggested by Daniel Vetter):
- Address an additional race when drm_open runs.
- Switch from SRCU to rwsem to synchronise readers and writers.
- Implement drm_master_flush with task_work so that flushes can be
queued to run before returning to userspace without creating a new
DRM_MASTER_FLUSH ioctl flag.

Best wishes,
Desmond

Desmond Cheong Zhi Xi (7):
  drm: fix null ptr dereference in drm_master_release
  drm: convert drm_device.master_mutex into a rwsem
  drm: lock drm_global_mutex earlier in the ioctl handler
  drm: avoid races with modesetting rights
  drm: avoid circular locks in drm_mode_object_find
  drm: avoid circular locks in drm_lease_held
  drm: remove drm_file.master_lookup_lock

 drivers/gpu/drm/drm_atomic_uapi.c            |  7 +-
 drivers/gpu/drm/drm_auth.c                   | 57 ++++++------
 drivers/gpu/drm/drm_color_mgmt.c             |  2 +-
 drivers/gpu/drm/drm_crtc.c                   |  9 +-
 drivers/gpu/drm/drm_debugfs.c                |  4 +-
 drivers/gpu/drm/drm_drv.c                    |  3 +-
 drivers/gpu/drm/drm_encoder.c                |  7 +-
 drivers/gpu/drm/drm_file.c                   |  7 +-
 drivers/gpu/drm/drm_framebuffer.c            |  2 +-
 drivers/gpu/drm/drm_internal.h               |  1 +
 drivers/gpu/drm/drm_ioctl.c                  | 48 ++++++----
 drivers/gpu/drm/drm_lease.c                  | 94 ++++++++++----------
 drivers/gpu/drm/drm_mode_object.c            | 28 +++++-
 drivers/gpu/drm/drm_plane.c                  | 26 ++++--
 drivers/gpu/drm/drm_property.c               |  6 +-
 drivers/gpu/drm/i915/display/intel_overlay.c |  2 +-
 drivers/gpu/drm/i915/display/intel_sprite.c  |  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c          |  2 +-
 include/drm/drm_auth.h                       |  6 +-
 include/drm/drm_connector.h                  | 23 +++++
 include/drm/drm_crtc.h                       | 22 +++++
 include/drm/drm_device.h                     | 15 +++-
 include/drm/drm_file.h                       | 17 ++--
 include/drm/drm_lease.h                      |  2 +
 include/drm/drm_mode_object.h                |  3 +
 include/drm/drm_plane.h                      | 20 +++++
 26 files changed, 270 insertions(+), 145 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v8 1/7] drm: fix null ptr dereference in drm_master_release
  2021-08-26  2:01 [PATCH v8 0/7] drm: update locking for modesetting Desmond Cheong Zhi Xi
@ 2021-08-26  2:01 ` Desmond Cheong Zhi Xi
  2021-08-26  9:53   ` Daniel Vetter
  2021-08-26  2:01 ` [PATCH v8 2/7] drm: convert drm_device.master_mutex into a rwsem Desmond Cheong Zhi Xi
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: Desmond Cheong Zhi Xi @ 2021-08-26  2:01 UTC (permalink / raw)
  To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel,
	sumit.semwal, christian.koenig, jani.nikula, joonas.lahtinen,
	rodrigo.vivi, chris, ville.syrjala, matthew.auld, dan.carpenter,
	tvrtko.ursulin, matthew.d.roper, lucas.demarchi, karthik.b.s,
	jose.souza, manasi.d.navare, airlied, aditya.swarup, andrescj,
	linux-graphics-maintainer, zackr
  Cc: Desmond Cheong Zhi Xi, dri-devel, linux-kernel, intel-gfx,
	linux-media, linaro-mm-sig, skhan, gregkh, linux-kernel-mentees

drm_master_release can be called on a drm_file without a master, which
results in a null ptr dereference of file_priv->master->magic_map. The
three cases are:

1. Error path in drm_open_helper
  drm_open():
    drm_open_helper():
      drm_master_open():
        drm_new_set_master(); <--- returns -ENOMEM,
                                   drm_file.master not set
      drm_file_free():
        drm_master_release(); <--- NULL ptr dereference
                                   (file_priv->master->magic_map)

2. Error path in mock_drm_getfile
  mock_drm_getfile():
    anon_inode_getfile(); <--- returns error, drm_file.master not set
    drm_file_free():
      drm_master_release(); <--- NULL ptr dereference
                                 (file_priv->master->magic_map)

3. In drm_client_close, as drm_client_open doesn't set up a master

drm_file.master is set up in drm_open_helper through the call to
drm_master_open, so we mirror it with a call to drm_master_release in
drm_close_helper, and remove drm_master_release from drm_file_free to
avoid the null ptr dereference.

Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---
 drivers/gpu/drm/drm_file.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index ed25168619fc..90b62f360da1 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -282,9 +282,6 @@ void drm_file_free(struct drm_file *file)
 
 	drm_legacy_ctxbitmap_flush(dev, file);
 
-	if (drm_is_primary_client(file))
-		drm_master_release(file);
-
 	if (dev->driver->postclose)
 		dev->driver->postclose(dev, file);
 
@@ -305,6 +302,9 @@ static void drm_close_helper(struct file *filp)
 	list_del(&file_priv->lhead);
 	mutex_unlock(&dev->filelist_mutex);
 
+	if (drm_is_primary_client(file_priv))
+		drm_master_release(file_priv);
+
 	drm_file_free(file_priv);
 }
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v8 2/7] drm: convert drm_device.master_mutex into a rwsem
  2021-08-26  2:01 [PATCH v8 0/7] drm: update locking for modesetting Desmond Cheong Zhi Xi
  2021-08-26  2:01 ` [PATCH v8 1/7] drm: fix null ptr dereference in drm_master_release Desmond Cheong Zhi Xi
@ 2021-08-26  2:01 ` Desmond Cheong Zhi Xi
  2021-08-26  9:55   ` Daniel Vetter
  2021-08-26  2:01 ` [PATCH v8 3/7] drm: lock drm_global_mutex earlier in the ioctl handler Desmond Cheong Zhi Xi
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: Desmond Cheong Zhi Xi @ 2021-08-26  2:01 UTC (permalink / raw)
  To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel,
	sumit.semwal, christian.koenig, jani.nikula, joonas.lahtinen,
	rodrigo.vivi, chris, ville.syrjala, matthew.auld, dan.carpenter,
	tvrtko.ursulin, matthew.d.roper, lucas.demarchi, karthik.b.s,
	jose.souza, manasi.d.navare, airlied, aditya.swarup, andrescj,
	linux-graphics-maintainer, zackr
  Cc: Desmond Cheong Zhi Xi, dri-devel, linux-kernel, intel-gfx,
	linux-media, linaro-mm-sig, skhan, gregkh, linux-kernel-mentees

drm_device.master_mutex currently protects the following:
- drm_device.master
- drm_file.master
- drm_file.was_master
- drm_file.is_master
- drm_master.unique
- drm_master.unique_len
- drm_master.magic_map

There is a clear separation between functions that read or change
these attributes. Hence, convert master_mutex into a rwsem to enable
concurrent readers.

Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---
 drivers/gpu/drm/drm_auth.c    | 35 ++++++++++++++++++-----------------
 drivers/gpu/drm/drm_debugfs.c |  4 ++--
 drivers/gpu/drm/drm_drv.c     |  3 +--
 drivers/gpu/drm/drm_ioctl.c   | 10 +++++-----
 include/drm/drm_auth.h        |  6 +++---
 include/drm/drm_device.h      | 10 ++++++----
 include/drm/drm_file.h        | 12 ++++++------
 7 files changed, 41 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 60a6b21474b1..73ade0513ccb 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -64,7 +64,7 @@
 static bool drm_is_current_master_locked(struct drm_file *fpriv)
 {
 	lockdep_assert_once(lockdep_is_held(&fpriv->master_lookup_lock) ||
-			    lockdep_is_held(&fpriv->minor->dev->master_mutex));
+			    lockdep_is_held(&fpriv->minor->dev->master_rwsem));
 
 	return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master;
 }
@@ -96,7 +96,7 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
 	struct drm_auth *auth = data;
 	int ret = 0;
 
-	mutex_lock(&dev->master_mutex);
+	down_write(&dev->master_rwsem);
 	if (!file_priv->magic) {
 		ret = idr_alloc(&file_priv->master->magic_map, file_priv,
 				1, 0, GFP_KERNEL);
@@ -104,7 +104,7 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
 			file_priv->magic = ret;
 	}
 	auth->magic = file_priv->magic;
-	mutex_unlock(&dev->master_mutex);
+	up_write(&dev->master_rwsem);
 
 	DRM_DEBUG("%u\n", auth->magic);
 
@@ -119,13 +119,13 @@ int drm_authmagic(struct drm_device *dev, void *data,
 
 	DRM_DEBUG("%u\n", auth->magic);
 
-	mutex_lock(&dev->master_mutex);
+	down_write(&dev->master_rwsem);
 	file = idr_find(&file_priv->master->magic_map, auth->magic);
 	if (file) {
 		file->authenticated = 1;
 		idr_replace(&file_priv->master->magic_map, NULL, auth->magic);
 	}
-	mutex_unlock(&dev->master_mutex);
+	up_write(&dev->master_rwsem);
 
 	return file ? 0 : -EINVAL;
 }
@@ -167,7 +167,7 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
 	struct drm_master *old_master;
 	struct drm_master *new_master;
 
-	lockdep_assert_held_once(&dev->master_mutex);
+	lockdep_assert_held_once(&dev->master_rwsem);
 
 	WARN_ON(fpriv->is_master);
 	old_master = fpriv->master;
@@ -249,7 +249,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
 {
 	int ret;
 
-	mutex_lock(&dev->master_mutex);
+	down_write(&dev->master_rwsem);
 
 	ret = drm_master_check_perm(dev, file_priv);
 	if (ret)
@@ -281,7 +281,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
 
 	drm_set_master(dev, file_priv, false);
 out_unlock:
-	mutex_unlock(&dev->master_mutex);
+	up_write(&dev->master_rwsem);
 	return ret;
 }
 
@@ -298,7 +298,7 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
 {
 	int ret;
 
-	mutex_lock(&dev->master_mutex);
+	down_write(&dev->master_rwsem);
 
 	ret = drm_master_check_perm(dev, file_priv);
 	if (ret)
@@ -321,8 +321,9 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
 	}
 
 	drm_drop_master(dev, file_priv);
+
 out_unlock:
-	mutex_unlock(&dev->master_mutex);
+	up_write(&dev->master_rwsem);
 	return ret;
 }
 
@@ -334,7 +335,7 @@ int drm_master_open(struct drm_file *file_priv)
 	/* if there is no current master make this fd it, but do not create
 	 * any master object for render clients
 	 */
-	mutex_lock(&dev->master_mutex);
+	down_write(&dev->master_rwsem);
 	if (!dev->master) {
 		ret = drm_new_set_master(dev, file_priv);
 	} else {
@@ -342,7 +343,7 @@ int drm_master_open(struct drm_file *file_priv)
 		file_priv->master = drm_master_get(dev->master);
 		spin_unlock(&file_priv->master_lookup_lock);
 	}
-	mutex_unlock(&dev->master_mutex);
+	up_write(&dev->master_rwsem);
 
 	return ret;
 }
@@ -352,7 +353,7 @@ void drm_master_release(struct drm_file *file_priv)
 	struct drm_device *dev = file_priv->minor->dev;
 	struct drm_master *master;
 
-	mutex_lock(&dev->master_mutex);
+	down_write(&dev->master_rwsem);
 	master = file_priv->master;
 	if (file_priv->magic)
 		idr_remove(&file_priv->master->magic_map, file_priv->magic);
@@ -375,7 +376,7 @@ void drm_master_release(struct drm_file *file_priv)
 	/* drop the master reference held by the file priv */
 	if (file_priv->master)
 		drm_master_put(&file_priv->master);
-	mutex_unlock(&dev->master_mutex);
+	up_write(&dev->master_rwsem);
 }
 
 /**
@@ -450,9 +451,9 @@ EXPORT_SYMBOL(drm_master_put);
 /* Used by drm_client and drm_fb_helper */
 bool drm_master_internal_acquire(struct drm_device *dev)
 {
-	mutex_lock(&dev->master_mutex);
+	down_read(&dev->master_rwsem);
 	if (dev->master) {
-		mutex_unlock(&dev->master_mutex);
+		up_read(&dev->master_rwsem);
 		return false;
 	}
 
@@ -463,6 +464,6 @@ EXPORT_SYMBOL(drm_master_internal_acquire);
 /* Used by drm_client and drm_fb_helper */
 void drm_master_internal_release(struct drm_device *dev)
 {
-	mutex_unlock(&dev->master_mutex);
+	up_read(&dev->master_rwsem);
 }
 EXPORT_SYMBOL(drm_master_internal_release);
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index b0a826489488..b34c9c263188 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -55,7 +55,7 @@ static int drm_name_info(struct seq_file *m, void *data)
 	struct drm_device *dev = minor->dev;
 	struct drm_master *master;
 
-	mutex_lock(&dev->master_mutex);
+	down_read(&dev->master_rwsem);
 	master = dev->master;
 	seq_printf(m, "%s", dev->driver->name);
 	if (dev->dev)
@@ -65,7 +65,7 @@ static int drm_name_info(struct seq_file *m, void *data)
 	if (dev->unique)
 		seq_printf(m, " unique=%s", dev->unique);
 	seq_printf(m, "\n");
-	mutex_unlock(&dev->master_mutex);
+	up_read(&dev->master_rwsem);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 7a5097467ba5..4556bf42954c 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -570,7 +570,6 @@ static void drm_dev_init_release(struct drm_device *dev, void *res)
 	/* Prevent use-after-free in drm_managed_release when debugging is
 	 * enabled. Slightly awkward, but can't really be helped. */
 	dev->dev = NULL;
-	mutex_destroy(&dev->master_mutex);
 	mutex_destroy(&dev->clientlist_mutex);
 	mutex_destroy(&dev->filelist_mutex);
 	mutex_destroy(&dev->struct_mutex);
@@ -611,7 +610,7 @@ static int drm_dev_init(struct drm_device *dev,
 	mutex_init(&dev->struct_mutex);
 	mutex_init(&dev->filelist_mutex);
 	mutex_init(&dev->clientlist_mutex);
-	mutex_init(&dev->master_mutex);
+	init_rwsem(&dev->master_rwsem);
 
 	ret = drmm_add_action(dev, drm_dev_init_release, NULL);
 	if (ret)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 26f3a9ede8fe..d25713b09b80 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -119,16 +119,16 @@ int drm_getunique(struct drm_device *dev, void *data,
 	struct drm_unique *u = data;
 	struct drm_master *master;
 
-	mutex_lock(&dev->master_mutex);
+	down_read(&dev->master_rwsem);
 	master = file_priv->master;
 	if (u->unique_len >= master->unique_len) {
 		if (copy_to_user(u->unique, master->unique, master->unique_len)) {
-			mutex_unlock(&dev->master_mutex);
+			up_read(&dev->master_rwsem);
 			return -EFAULT;
 		}
 	}
 	u->unique_len = master->unique_len;
-	mutex_unlock(&dev->master_mutex);
+	up_read(&dev->master_rwsem);
 
 	return 0;
 }
@@ -385,7 +385,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
 	struct drm_set_version *sv = data;
 	int if_version, retcode = 0;
 
-	mutex_lock(&dev->master_mutex);
+	down_write(&dev->master_rwsem);
 	if (sv->drm_di_major != -1) {
 		if (sv->drm_di_major != DRM_IF_MAJOR ||
 		    sv->drm_di_minor < 0 || sv->drm_di_minor > DRM_IF_MINOR) {
@@ -420,7 +420,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
 	sv->drm_di_minor = DRM_IF_MINOR;
 	sv->drm_dd_major = dev->driver->major;
 	sv->drm_dd_minor = dev->driver->minor;
-	mutex_unlock(&dev->master_mutex);
+	up_write(&dev->master_rwsem);
 
 	return retcode;
 }
diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
index ba248ca8866f..f0a89e5fcaad 100644
--- a/include/drm/drm_auth.h
+++ b/include/drm/drm_auth.h
@@ -67,17 +67,17 @@ struct drm_master {
 	struct drm_device *dev;
 	/**
 	 * @unique: Unique identifier: e.g. busid. Protected by
-	 * &drm_device.master_mutex.
+	 * &drm_device.master_rwsem.
 	 */
 	char *unique;
 	/**
 	 * @unique_len: Length of unique field. Protected by
-	 * &drm_device.master_mutex.
+	 * &drm_device.master_rwsem.
 	 */
 	int unique_len;
 	/**
 	 * @magic_map: Map of used authentication tokens. Protected by
-	 * &drm_device.master_mutex.
+	 * &drm_device.master_rwsem.
 	 */
 	struct idr magic_map;
 	void *driver_priv;
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index 604b1d1b2d72..142fb2f6e74d 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -107,7 +107,7 @@ struct drm_device {
 	 * @master:
 	 *
 	 * Currently active master for this device.
-	 * Protected by &master_mutex
+	 * Protected by &master_rwsem
 	 */
 	struct drm_master *master;
 
@@ -146,11 +146,13 @@ struct drm_device {
 	struct mutex struct_mutex;
 
 	/**
-	 * @master_mutex:
+	 * @master_rwsem:
 	 *
-	 * Lock for &drm_minor.master and &drm_file.is_master
+	 * Lock for &drm_device.master, &drm_file.was_master,
+	 * &drm_file.is_master, &drm_file.master, &drm_master.unique,
+	 * &drm_master.unique_len, and &drm_master.magic_map.
 	 */
-	struct mutex master_mutex;
+	struct rw_semaphore master_rwsem;
 
 	/**
 	 * @open_count:
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index a3acb7ac3550..d12bb2ba7814 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -205,7 +205,7 @@ struct drm_file {
 	 * @was_master:
 	 *
 	 * This client has or had, master capability. Protected by struct
-	 * &drm_device.master_mutex.
+	 * &drm_device.master_rwsem.
 	 *
 	 * This is used to ensure that CAP_SYS_ADMIN is not enforced, if the
 	 * client is or was master in the past.
@@ -216,7 +216,7 @@ struct drm_file {
 	 * @is_master:
 	 *
 	 * This client is the creator of @master. Protected by struct
-	 * &drm_device.master_mutex.
+	 * &drm_device.master_rwsem.
 	 *
 	 * See also the :ref:`section on primary nodes and authentication
 	 * <drm_primary_node>`.
@@ -227,19 +227,19 @@ struct drm_file {
 	 * @master:
 	 *
 	 * Master this node is currently associated with. Protected by struct
-	 * &drm_device.master_mutex, and serialized by @master_lookup_lock.
+	 * &drm_device.master_rwsem, and serialized by @master_lookup_lock.
 	 *
 	 * Only relevant if drm_is_primary_client() returns true. Note that
 	 * this only matches &drm_device.master if the master is the currently
 	 * active one.
 	 *
-	 * To update @master, both &drm_device.master_mutex and
+	 * To update @master, both &drm_device.master_rwsem and
 	 * @master_lookup_lock need to be held, therefore holding either of
 	 * them is safe and enough for the read side.
 	 *
 	 * When dereferencing this pointer, either hold struct
-	 * &drm_device.master_mutex for the duration of the pointer's use, or
-	 * use drm_file_get_master() if struct &drm_device.master_mutex is not
+	 * &drm_device.master_rwsem for the duration of the pointer's use, or
+	 * use drm_file_get_master() if struct &drm_device.master_rwsem is not
 	 * currently held and there is no other need to hold it. This prevents
 	 * @master from being freed during use.
 	 *
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v8 3/7] drm: lock drm_global_mutex earlier in the ioctl handler
  2021-08-26  2:01 [PATCH v8 0/7] drm: update locking for modesetting Desmond Cheong Zhi Xi
  2021-08-26  2:01 ` [PATCH v8 1/7] drm: fix null ptr dereference in drm_master_release Desmond Cheong Zhi Xi
  2021-08-26  2:01 ` [PATCH v8 2/7] drm: convert drm_device.master_mutex into a rwsem Desmond Cheong Zhi Xi
@ 2021-08-26  2:01 ` Desmond Cheong Zhi Xi
  2021-08-26  9:58   ` Daniel Vetter
  2021-08-26  2:01 ` [PATCH v8 4/7] drm: avoid races with modesetting rights Desmond Cheong Zhi Xi
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: Desmond Cheong Zhi Xi @ 2021-08-26  2:01 UTC (permalink / raw)
  To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel,
	sumit.semwal, christian.koenig, jani.nikula, joonas.lahtinen,
	rodrigo.vivi, chris, ville.syrjala, matthew.auld, dan.carpenter,
	tvrtko.ursulin, matthew.d.roper, lucas.demarchi, karthik.b.s,
	jose.souza, manasi.d.navare, airlied, aditya.swarup, andrescj,
	linux-graphics-maintainer, zackr
  Cc: Desmond Cheong Zhi Xi, dri-devel, linux-kernel, intel-gfx,
	linux-media, linaro-mm-sig, skhan, gregkh, linux-kernel-mentees

In a future patch, a read lock on drm_device.master_rwsem is
held in the ioctl handler before the check for ioctl
permissions. However, this inverts the lock hierarchy of
drm_global_mutex --> master_rwsem.

To avoid this, we do some prep work to grab the drm_global_mutex
before checking for ioctl permissions.

Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---
 drivers/gpu/drm/drm_ioctl.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index d25713b09b80..158629d88319 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -772,19 +772,19 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata,
 	if (drm_dev_is_unplugged(dev))
 		return -ENODEV;
 
+	/* Enforce sane locking for modern driver ioctls. */
+	if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)) && !(flags & DRM_UNLOCKED))
+		mutex_lock(&drm_global_mutex);
+
 	retcode = drm_ioctl_permit(flags, file_priv);
 	if (unlikely(retcode))
-		return retcode;
+		goto out;
 
-	/* Enforce sane locking for modern driver ioctls. */
-	if (likely(!drm_core_check_feature(dev, DRIVER_LEGACY)) ||
-	    (flags & DRM_UNLOCKED))
-		retcode = func(dev, kdata, file_priv);
-	else {
-		mutex_lock(&drm_global_mutex);
-		retcode = func(dev, kdata, file_priv);
+	retcode = func(dev, kdata, file_priv);
+
+out:
+	if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)) && !(flags & DRM_UNLOCKED))
 		mutex_unlock(&drm_global_mutex);
-	}
 	return retcode;
 }
 EXPORT_SYMBOL(drm_ioctl_kernel);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v8 4/7] drm: avoid races with modesetting rights
  2021-08-26  2:01 [PATCH v8 0/7] drm: update locking for modesetting Desmond Cheong Zhi Xi
                   ` (2 preceding siblings ...)
  2021-08-26  2:01 ` [PATCH v8 3/7] drm: lock drm_global_mutex earlier in the ioctl handler Desmond Cheong Zhi Xi
@ 2021-08-26  2:01 ` Desmond Cheong Zhi Xi
  2021-08-26 12:59   ` Daniel Vetter
  2021-08-26  2:01 ` [PATCH v8 5/7] drm: avoid circular locks in drm_mode_object_find Desmond Cheong Zhi Xi
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: Desmond Cheong Zhi Xi @ 2021-08-26  2:01 UTC (permalink / raw)
  To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel,
	sumit.semwal, christian.koenig, jani.nikula, joonas.lahtinen,
	rodrigo.vivi, chris, ville.syrjala, matthew.auld, dan.carpenter,
	tvrtko.ursulin, matthew.d.roper, lucas.demarchi, karthik.b.s,
	jose.souza, manasi.d.navare, airlied, aditya.swarup, andrescj,
	linux-graphics-maintainer, zackr
  Cc: Desmond Cheong Zhi Xi, dri-devel, linux-kernel, intel-gfx,
	linux-media, linaro-mm-sig, skhan, gregkh, linux-kernel-mentees,
	Daniel Vetter

In drm_client_modeset.c and drm_fb_helper.c,
drm_master_internal_{acquire,release} are used to avoid races with DRM
userspace. These functions hold onto drm_device.master_rwsem while
committing, and bail if there's already a master.

However, there are other places where modesetting rights can race. A
time-of-check-to-time-of-use error can occur if an ioctl that changes
the modeset has its rights revoked after it validates its permissions,
but before it completes.

There are four places where modesetting permissions can change:

- DROP_MASTER ioctl removes rights for a master and its leases

- REVOKE_LEASE ioctl revokes rights for a specific lease

- SET_MASTER ioctl sets the device master if the master role hasn't
been acquired yet

- drm_open which can create a new master for a device if one does not
currently exist

These races can be avoided using drm_device.master_rwsem: users that
perform modesetting should hold a read lock on the new
drm_device.master_rwsem, and users that change these permissions
should hold a write lock.

To avoid deadlocks with master_rwsem, for ioctls that need to check
for modesetting permissions, but also need to hold a write lock on
master_rwsem to protect some other attribute (or recurses to some
function that holds a write lock, like drm_mode_create_lease_ioctl
which eventually calls drm_master_open), we remove the DRM_MASTER flag
and push the master_rwsem lock and permissions check into the ioctl.

Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---
 drivers/gpu/drm/drm_auth.c  |  4 ++++
 drivers/gpu/drm/drm_ioctl.c | 20 +++++++++++++++-----
 drivers/gpu/drm/drm_lease.c | 35 ++++++++++++++++++++++++-----------
 include/drm/drm_device.h    |  5 +++++
 4 files changed, 48 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 73ade0513ccb..65065f7e1499 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -120,6 +120,10 @@ int drm_authmagic(struct drm_device *dev, void *data,
 	DRM_DEBUG("%u\n", auth->magic);
 
 	down_write(&dev->master_rwsem);
+	if (unlikely(!drm_is_current_master(file_priv))) {
+		up_write(&dev->master_rwsem);
+		return -EACCES;
+	}
 	file = idr_find(&file_priv->master->magic_map, auth->magic);
 	if (file) {
 		file->authenticated = 1;
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 158629d88319..8bea39ffc5c0 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -386,6 +386,10 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
 	int if_version, retcode = 0;
 
 	down_write(&dev->master_rwsem);
+	if (unlikely(!drm_is_current_master(file_priv))) {
+		retcode = -EACCES;
+		goto unlock;
+	}
 	if (sv->drm_di_major != -1) {
 		if (sv->drm_di_major != DRM_IF_MAJOR ||
 		    sv->drm_di_minor < 0 || sv->drm_di_minor > DRM_IF_MINOR) {
@@ -420,8 +424,9 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
 	sv->drm_di_minor = DRM_IF_MINOR;
 	sv->drm_dd_major = dev->driver->major;
 	sv->drm_dd_minor = dev->driver->minor;
-	up_write(&dev->master_rwsem);
 
+unlock:
+	up_write(&dev->master_rwsem);
 	return retcode;
 }
 
@@ -574,12 +579,12 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_GET_STATS, drm_getstats, 0),
 	DRM_IOCTL_DEF(DRM_IOCTL_GET_CAP, drm_getcap, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF(DRM_IOCTL_SET_CLIENT_CAP, drm_setclientcap, 0),
-	DRM_IOCTL_DEF(DRM_IOCTL_SET_VERSION, drm_setversion, DRM_MASTER),
+	DRM_IOCTL_DEF(DRM_IOCTL_SET_VERSION, drm_setversion, 0),
 
 	DRM_IOCTL_DEF(DRM_IOCTL_SET_UNIQUE, drm_invalid_op, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF(DRM_IOCTL_BLOCK, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF(DRM_IOCTL_UNBLOCK, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
-	DRM_IOCTL_DEF(DRM_IOCTL_AUTH_MAGIC, drm_authmagic, DRM_MASTER),
+	DRM_IOCTL_DEF(DRM_IOCTL_AUTH_MAGIC, drm_authmagic, 0),
 
 	DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_ADD_MAP, drm_legacy_addmap_ioctl, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_RM_MAP, drm_legacy_rmmap_ioctl, DRM_AUTH),
@@ -706,10 +711,10 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 		      DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, 0),
 	DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, 0),
-	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_MASTER),
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, 0),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, DRM_MASTER),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, DRM_MASTER),
-	DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, DRM_MASTER),
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, 0),
 };
 
 #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
@@ -776,6 +781,9 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata,
 	if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)) && !(flags & DRM_UNLOCKED))
 		mutex_lock(&drm_global_mutex);
 
+	if (unlikely(flags & DRM_MASTER))
+		down_read(&dev->master_rwsem);
+
 	retcode = drm_ioctl_permit(flags, file_priv);
 	if (unlikely(retcode))
 		goto out;
@@ -783,6 +791,8 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata,
 	retcode = func(dev, kdata, file_priv);
 
 out:
+	if (unlikely(flags & DRM_MASTER))
+		up_read(&dev->master_rwsem);
 	if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)) && !(flags & DRM_UNLOCKED))
 		mutex_unlock(&drm_global_mutex);
 	return retcode;
diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index dee4f24a1808..bed6f7636cbe 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -500,6 +500,18 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
 		return -EINVAL;
 	}
 
+	/* Clone the lessor file to create a new file for us */
+	DRM_DEBUG_LEASE("Allocating lease file\n");
+	lessee_file = file_clone_open(lessor_file);
+	if (IS_ERR(lessee_file))
+		return PTR_ERR(lessee_file);
+
+	down_read(&dev->master_rwsem);
+	if (unlikely(!drm_is_current_master(lessor_priv))) {
+		ret = -EACCES;
+		goto out_file;
+	}
+
 	lessor = drm_file_get_master(lessor_priv);
 	/* Do not allow sub-leases */
 	if (lessor->lessor) {
@@ -547,14 +559,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
 		goto out_leases;
 	}
 
-	/* Clone the lessor file to create a new file for us */
-	DRM_DEBUG_LEASE("Allocating lease file\n");
-	lessee_file = file_clone_open(lessor_file);
-	if (IS_ERR(lessee_file)) {
-		ret = PTR_ERR(lessee_file);
-		goto out_lessee;
-	}
-
 	lessee_priv = lessee_file->private_data;
 	/* Change the file to a master one */
 	drm_master_put(&lessee_priv->master);
@@ -571,17 +575,19 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
 	fd_install(fd, lessee_file);
 
 	drm_master_put(&lessor);
+	up_read(&dev->master_rwsem);
 	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n");
 	return 0;
 
-out_lessee:
-	drm_master_put(&lessee);
-
 out_leases:
 	put_unused_fd(fd);
 
 out_lessor:
 	drm_master_put(&lessor);
+
+out_file:
+	up_read(&dev->master_rwsem);
+	fput(lessee_file);
 	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl failed: %d\n", ret);
 	return ret;
 }
@@ -705,6 +711,11 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EOPNOTSUPP;
 
+	down_write(&dev->master_rwsem);
+	if (unlikely(!drm_is_current_master(lessor_priv))) {
+		ret = -EACCES;
+		goto unlock;
+	}
 	lessor = drm_file_get_master(lessor_priv);
 	mutex_lock(&dev->mode_config.idr_mutex);
 
@@ -728,5 +739,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
 	mutex_unlock(&dev->mode_config.idr_mutex);
 	drm_master_put(&lessor);
 
+unlock:
+	up_write(&dev->master_rwsem);
 	return ret;
 }
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index 142fb2f6e74d..7d32bb69e6db 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -151,6 +151,11 @@ struct drm_device {
 	 * Lock for &drm_device.master, &drm_file.was_master,
 	 * &drm_file.is_master, &drm_file.master, &drm_master.unique,
 	 * &drm_master.unique_len, and &drm_master.magic_map.
+	 *
+	 * Additionally, synchronizes modesetting rights between multiple users.
+	 * Users that can change the modeset or display state must hold a read
+	 * lock on @master_rwsem, and users that change modesetting rights
+	 * should hold a write lock.
 	 */
 	struct rw_semaphore master_rwsem;
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v8 5/7] drm: avoid circular locks in drm_mode_object_find
  2021-08-26  2:01 [PATCH v8 0/7] drm: update locking for modesetting Desmond Cheong Zhi Xi
                   ` (3 preceding siblings ...)
  2021-08-26  2:01 ` [PATCH v8 4/7] drm: avoid races with modesetting rights Desmond Cheong Zhi Xi
@ 2021-08-26  2:01 ` Desmond Cheong Zhi Xi
  2021-08-26 13:13   ` Daniel Vetter
  2021-08-26  2:01 ` [PATCH v8 6/7] drm: avoid circular locks in drm_lease_held Desmond Cheong Zhi Xi
  2021-08-26  2:01 ` [PATCH v8 7/7] drm: remove drm_file.master_lookup_lock Desmond Cheong Zhi Xi
  6 siblings, 1 reply; 20+ messages in thread
From: Desmond Cheong Zhi Xi @ 2021-08-26  2:01 UTC (permalink / raw)
  To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel,
	sumit.semwal, christian.koenig, jani.nikula, joonas.lahtinen,
	rodrigo.vivi, chris, ville.syrjala, matthew.auld, dan.carpenter,
	tvrtko.ursulin, matthew.d.roper, lucas.demarchi, karthik.b.s,
	jose.souza, manasi.d.navare, airlied, aditya.swarup, andrescj,
	linux-graphics-maintainer, zackr
  Cc: Desmond Cheong Zhi Xi, dri-devel, linux-kernel, intel-gfx,
	linux-media, linaro-mm-sig, skhan, gregkh, linux-kernel-mentees

__drm_mode_object_find checks if the given drm file holds the required
lease on a object by calling _drm_lease_held. _drm_lease_held in turn
uses drm_file_get_master to access drm_file.master.

However, in a future patch, the drm_file.master_lookup_lock in
drm_file_get_master will be replaced by drm_device.master_rwsem. This
is an issue for two reasons:

1. master_rwsem is sometimes already held when __drm_mode_object_find
is called, which leads to recursive locks on master_rwsem

2. drm_mode_object_find is sometimes called with the modeset_mutex
held, which leads to an inversion of the master_rwsem -->
modeset_mutex lock hierarchy

To fix this, we make __drm_mode_object_find the locked version of
drm_mode_object_find, and wrap calls to __drm_mode_object_find with
locks on master_rwsem. This allows us to safely access drm_file.master
in _drm_lease_held (__drm_mode_object_find is its only caller) without
the use of drm_file_get_master.

Functions that already lock master_rwsem are modified to call
__drm_mode_object_find, whereas functions that haven't locked
master_rwsem should call drm_mode_object_find. These two options
allow us to grab master_rwsem before modeset_mutex (such as in
drm_mode_get_obj_get_properties_ioctl).

This new rule requires more extensive changes to three functions:
drn_connector_lookup, drm_crtc_find, and drm_plane_find. These
functions are only sometimes called with master_rwsem held. Hence, we
have to further split them into locked and unlocked versions that call
__drm_mode_object_find and drm_mode_object_find respectively.

Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c            |  7 ++---
 drivers/gpu/drm/drm_color_mgmt.c             |  2 +-
 drivers/gpu/drm/drm_crtc.c                   |  5 ++--
 drivers/gpu/drm/drm_framebuffer.c            |  2 +-
 drivers/gpu/drm/drm_lease.c                  | 21 +++++----------
 drivers/gpu/drm/drm_mode_object.c            | 28 +++++++++++++++++---
 drivers/gpu/drm/drm_plane.c                  |  8 +++---
 drivers/gpu/drm/drm_property.c               |  6 ++---
 drivers/gpu/drm/i915/display/intel_overlay.c |  2 +-
 drivers/gpu/drm/i915/display/intel_sprite.c  |  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c          |  2 +-
 include/drm/drm_connector.h                  | 23 ++++++++++++++++
 include/drm/drm_crtc.h                       | 22 +++++++++++++++
 include/drm/drm_mode_object.h                |  3 +++
 include/drm/drm_plane.h                      | 20 ++++++++++++++
 15 files changed, 118 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 909f31833181..cda9a501cf74 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -557,7 +557,7 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
 			return -EINVAL;
 
 	} else if (property == config->prop_crtc_id) {
-		struct drm_crtc *crtc = drm_crtc_find(dev, file_priv, val);
+		struct drm_crtc *crtc = __drm_crtc_find(dev, file_priv, val);
 
 		if (val && !crtc)
 			return -EACCES;
@@ -709,7 +709,7 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
 	int ret;
 
 	if (property == config->prop_crtc_id) {
-		struct drm_crtc *crtc = drm_crtc_find(dev, file_priv, val);
+		struct drm_crtc *crtc = __drm_crtc_find(dev, file_priv, val);
 
 		if (val && !crtc)
 			return -EACCES;
@@ -1385,7 +1385,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 			goto out;
 		}
 
-		obj = drm_mode_object_find(dev, file_priv, obj_id, DRM_MODE_OBJECT_ANY);
+		obj = __drm_mode_object_find(dev, file_priv, obj_id,
+					     DRM_MODE_OBJECT_ANY);
 		if (!obj) {
 			ret = -ENOENT;
 			goto out;
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index bb14f488c8f6..9dcb2ccca3ab 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -365,7 +365,7 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EOPNOTSUPP;
 
-	crtc = drm_crtc_find(dev, file_priv, crtc_lut->crtc_id);
+	crtc = __drm_crtc_find(dev, file_priv, crtc_lut->crtc_id);
 	if (!crtc)
 		return -ENOENT;
 
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 26a77a735905..b1279bb3fa61 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -656,7 +656,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 	if (crtc_req->x & 0xffff0000 || crtc_req->y & 0xffff0000)
 		return -ERANGE;
 
-	crtc = drm_crtc_find(dev, file_priv, crtc_req->crtc_id);
+	crtc = __drm_crtc_find(dev, file_priv, crtc_req->crtc_id);
 	if (!crtc) {
 		DRM_DEBUG_KMS("Unknown CRTC ID %d\n", crtc_req->crtc_id);
 		return -ENOENT;
@@ -787,7 +787,8 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 				goto out;
 			}
 
-			connector = drm_connector_lookup(dev, file_priv, out_id);
+			connector = __drm_connector_lookup(dev, file_priv,
+							   out_id);
 			if (!connector) {
 				DRM_DEBUG_KMS("Connector id %d unknown\n",
 						out_id);
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 07f5abc875e9..9c1db91b150d 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -887,7 +887,7 @@ struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev,
 	struct drm_mode_object *obj;
 	struct drm_framebuffer *fb = NULL;
 
-	obj = __drm_mode_object_find(dev, file_priv, id, DRM_MODE_OBJECT_FB);
+	obj = drm_mode_object_find(dev, file_priv, id, DRM_MODE_OBJECT_FB);
 	if (obj)
 		fb = obj_to_fb(obj);
 	return fb;
diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index bed6f7636cbe..1b156c85d1c8 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -105,22 +105,13 @@ static bool _drm_has_leased(struct drm_master *master, int id)
 	return false;
 }
 
-/* Called with idr_mutex held */
+/* Called with idr_mutex and master_rwsem held */
 bool _drm_lease_held(struct drm_file *file_priv, int id)
 {
-	bool ret;
-	struct drm_master *master;
-
-	if (!file_priv)
+	if (!file_priv || !file_priv->master)
 		return true;
 
-	master = drm_file_get_master(file_priv);
-	if (!master)
-		return true;
-	ret = _drm_lease_held_master(master, id);
-	drm_master_put(&master);
-
-	return ret;
+	return _drm_lease_held_master(file_priv->master, id);
 }
 
 bool drm_lease_held(struct drm_file *file_priv, int id)
@@ -391,9 +382,9 @@ static int fill_object_idr(struct drm_device *dev,
 	/* step one - get references to all the mode objects
 	   and check for validity. */
 	for (o = 0; o < object_count; o++) {
-		objects[o] = drm_mode_object_find(dev, lessor_priv,
-						  object_ids[o],
-						  DRM_MODE_OBJECT_ANY);
+		objects[o] = __drm_mode_object_find(dev, lessor_priv,
+						    object_ids[o],
+						    DRM_MODE_OBJECT_ANY);
 		if (!objects[o]) {
 			ret = -ENOENT;
 			goto out_free_objects;
diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
index 86d9e907c0b2..90c23997aa53 100644
--- a/drivers/gpu/drm/drm_mode_object.c
+++ b/drivers/gpu/drm/drm_mode_object.c
@@ -133,12 +133,27 @@ bool drm_mode_object_lease_required(uint32_t type)
 	}
 }
 
+/**
+ * __drm_mode_object_find - look up a drm object with static lifetime
+ * @dev: drm device
+ * @file_priv: drm file
+ * @id: id of the mode object
+ * @type: type of the mode object
+ *
+ * This function is used to look up a modeset object. It will acquire a
+ * reference for reference counted objects. This reference must be dropped
+ * again by calling drm_mode_object_put().
+ *
+ * Similar to drm_mode_object_find(), but called with &drm_device.master_rwsem
+ * held.
+ */
 struct drm_mode_object *__drm_mode_object_find(struct drm_device *dev,
 					       struct drm_file *file_priv,
-					       uint32_t id, uint32_t type)
+					       u32 id, u32 type)
 {
 	struct drm_mode_object *obj = NULL;
 
+	lockdep_assert_held_once(&dev->master_rwsem);
 	mutex_lock(&dev->mode_config.idr_mutex);
 	obj = idr_find(&dev->mode_config.object_idr, id);
 	if (obj && type != DRM_MODE_OBJECT_ANY && obj->type != type)
@@ -158,6 +173,7 @@ struct drm_mode_object *__drm_mode_object_find(struct drm_device *dev,
 
 	return obj;
 }
+EXPORT_SYMBOL(__drm_mode_object_find);
 
 /**
  * drm_mode_object_find - look up a drm object with static lifetime
@@ -176,7 +192,9 @@ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
 {
 	struct drm_mode_object *obj = NULL;
 
+	down_read(&dev->master_rwsem);
 	obj = __drm_mode_object_find(dev, file_priv, id, type);
+	up_read(&dev->master_rwsem);
 	return obj;
 }
 EXPORT_SYMBOL(drm_mode_object_find);
@@ -408,9 +426,12 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EOPNOTSUPP;
 
+	down_read(&dev->master_rwsem);
 	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
 
-	obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
+	obj = __drm_mode_object_find(dev, file_priv, arg->obj_id,
+				     arg->obj_type);
+	up_read(&dev->master_rwsem);
 	if (!obj) {
 		ret = -ENOENT;
 		goto out;
@@ -534,7 +555,8 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EOPNOTSUPP;
 
-	arg_obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
+	arg_obj = __drm_mode_object_find(dev, file_priv, arg->obj_id,
+					 arg->obj_type);
 	if (!arg_obj)
 		return -ENOENT;
 
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 82afb854141b..b5566167a798 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -971,7 +971,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 	 * First, find the plane, crtc, and fb objects.  If not available,
 	 * we don't bother to call the driver.
 	 */
-	plane = drm_plane_find(dev, file_priv, plane_req->plane_id);
+	plane = __drm_plane_find(dev, file_priv, plane_req->plane_id);
 	if (!plane) {
 		DRM_DEBUG_KMS("Unknown plane ID %d\n",
 			      plane_req->plane_id);
@@ -986,7 +986,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 			return -ENOENT;
 		}
 
-		crtc = drm_crtc_find(dev, file_priv, plane_req->crtc_id);
+		crtc = __drm_crtc_find(dev, file_priv, plane_req->crtc_id);
 		if (!crtc) {
 			drm_framebuffer_put(fb);
 			DRM_DEBUG_KMS("Unknown crtc ID %d\n",
@@ -1108,7 +1108,7 @@ static int drm_mode_cursor_common(struct drm_device *dev,
 	if (!req->flags || (~DRM_MODE_CURSOR_FLAGS & req->flags))
 		return -EINVAL;
 
-	crtc = drm_crtc_find(dev, file_priv, req->crtc_id);
+	crtc = __drm_crtc_find(dev, file_priv, req->crtc_id);
 	if (!crtc) {
 		DRM_DEBUG_KMS("Unknown CRTC ID %d\n", req->crtc_id);
 		return -ENOENT;
@@ -1229,7 +1229,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	if ((page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC) && !dev->mode_config.async_page_flip)
 		return -EINVAL;
 
-	crtc = drm_crtc_find(dev, file_priv, page_flip->crtc_id);
+	crtc = __drm_crtc_find(dev, file_priv, page_flip->crtc_id);
 	if (!crtc)
 		return -ENOENT;
 
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
index 6c353c9dc772..9f04dcb81d07 100644
--- a/drivers/gpu/drm/drm_property.c
+++ b/drivers/gpu/drm/drm_property.c
@@ -656,7 +656,7 @@ struct drm_property_blob *drm_property_lookup_blob(struct drm_device *dev,
 	struct drm_mode_object *obj;
 	struct drm_property_blob *blob = NULL;
 
-	obj = __drm_mode_object_find(dev, NULL, id, DRM_MODE_OBJECT_BLOB);
+	obj = drm_mode_object_find(dev, NULL, id, DRM_MODE_OBJECT_BLOB);
 	if (obj)
 		blob = obj_to_blob(obj);
 	return blob;
@@ -919,8 +919,8 @@ bool drm_property_change_valid_get(struct drm_property *property,
 		if (value == 0)
 			return true;
 
-		*ref = __drm_mode_object_find(property->dev, NULL, value,
-					      property->values[0]);
+		*ref = drm_mode_object_find(property->dev, NULL, value,
+					    property->values[0]);
 		return *ref != NULL;
 	}
 
diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c
index 7e3f5c6ca484..1d4af29e31c9 100644
--- a/drivers/gpu/drm/i915/display/intel_overlay.c
+++ b/drivers/gpu/drm/i915/display/intel_overlay.c
@@ -1120,7 +1120,7 @@ int intel_overlay_put_image_ioctl(struct drm_device *dev, void *data,
 		return ret;
 	}
 
-	drmmode_crtc = drm_crtc_find(dev, file_priv, params->crtc_id);
+	drmmode_crtc = __drm_crtc_find(dev, file_priv, params->crtc_id);
 	if (!drmmode_crtc)
 		return -ENOENT;
 	crtc = to_intel_crtc(drmmode_crtc);
diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
index 4ae9a7455b23..e19ef2d90bac 100644
--- a/drivers/gpu/drm/i915/display/intel_sprite.c
+++ b/drivers/gpu/drm/i915/display/intel_sprite.c
@@ -1505,7 +1505,7 @@ int intel_sprite_set_colorkey_ioctl(struct drm_device *dev, void *data,
 	    set->flags & I915_SET_COLORKEY_DESTINATION)
 		return -EINVAL;
 
-	plane = drm_plane_find(dev, file_priv, set->plane_id);
+	plane = __drm_plane_find(dev, file_priv, set->plane_id);
 	if (!plane || plane->type != DRM_PLANE_TYPE_OVERLAY)
 		return -ENOENT;
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 74fa41909213..d368b2bcb1fa 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -1862,7 +1862,7 @@ int vmw_kms_cursor_bypass_ioctl(struct drm_device *dev, void *data,
 		return 0;
 	}
 
-	crtc = drm_crtc_find(dev, file_priv, arg->crtc_id);
+	crtc = __drm_crtc_find(dev, file_priv, arg->crtc_id);
 	if (!crtc) {
 		ret = -ENOENT;
 		goto out;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 1647960c9e50..322c823583c7 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1591,6 +1591,29 @@ static inline u32 drm_connector_mask(const struct drm_connector *connector)
 	return 1 << connector->index;
 }
 
+/**
+ * __drm_connector_lookup - lookup connector object
+ * @dev: DRM device
+ * @file_priv: drm file to check for lease against.
+ * @id: connector object id
+ *
+ * This function looks up the connector object specified by id
+ * add takes a reference to it.
+ *
+ * Similar to drm_connector_lookup(), but called with &drm_device.master_rwsem
+ * held.
+ */
+static inline struct drm_connector *__drm_connector_lookup(struct drm_device *dev,
+							   struct drm_file *file_priv,
+							   uint32_t id)
+{
+	struct drm_mode_object *mo;
+
+	mo = __drm_mode_object_find(dev, file_priv, id,
+				    DRM_MODE_OBJECT_CONNECTOR);
+	return mo ? obj_to_connector(mo) : NULL;
+}
+
 /**
  * drm_connector_lookup - lookup connector object
  * @dev: DRM device
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 13eeba2a750a..69df854dd322 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1283,6 +1283,28 @@ static inline uint32_t drm_crtc_mask(const struct drm_crtc *crtc)
 int drm_mode_set_config_internal(struct drm_mode_set *set);
 struct drm_crtc *drm_crtc_from_index(struct drm_device *dev, int idx);
 
+/**
+ * __drm_crtc_find - look up a CRTC object from its ID
+ * @dev: DRM device
+ * @file_priv: drm file to check for lease against.
+ * @id: &drm_mode_object ID
+ *
+ * This can be used to look up a CRTC from its userspace ID. Only used by
+ * drivers for legacy IOCTLs and interface, nowadays extensions to the KMS
+ * userspace interface should be done using &drm_property.
+ *
+ * Similar to drm_crtc_find(), but called with &drm_device.master_rwsem held.
+ */
+static inline struct drm_crtc *__drm_crtc_find(struct drm_device *dev,
+					       struct drm_file *file_priv,
+					       uint32_t id)
+{
+	struct drm_mode_object *mo;
+
+	mo = __drm_mode_object_find(dev, file_priv, id, DRM_MODE_OBJECT_CRTC);
+	return mo ? obj_to_crtc(mo) : NULL;
+}
+
 /**
  * drm_crtc_find - look up a CRTC object from its ID
  * @dev: DRM device
diff --git a/include/drm/drm_mode_object.h b/include/drm/drm_mode_object.h
index c34a3e8030e1..1906af9cae9b 100644
--- a/include/drm/drm_mode_object.h
+++ b/include/drm/drm_mode_object.h
@@ -114,6 +114,9 @@ struct drm_object_properties {
 		return "(unknown)";				\
 	}
 
+struct drm_mode_object *__drm_mode_object_find(struct drm_device *dev,
+					       struct drm_file *file_priv,
+					       u32 id, u32 type);
 struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
 					     struct drm_file *file_priv,
 					     uint32_t id, uint32_t type);
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index fed97e35626f..49e35d3724c9 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -842,6 +842,26 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
 				       struct drm_property *property,
 				       uint64_t value);
 
+/**
+ * __drm_plane_find - find a &drm_plane
+ * @dev: DRM device
+ * @file_priv: drm file to check for lease against.
+ * @id: plane id
+ *
+ * Returns the plane with @id, NULL if it doesn't exist.
+ *
+ * Similar to drm_plane_find(), but called with &drm_device.master_rwsem held.
+ */
+static inline struct drm_plane *__drm_plane_find(struct drm_device *dev,
+						 struct drm_file *file_priv,
+						 uint32_t id)
+{
+	struct drm_mode_object *mo;
+
+	mo = __drm_mode_object_find(dev, file_priv, id, DRM_MODE_OBJECT_PLANE);
+	return mo ? obj_to_plane(mo) : NULL;
+}
+
 /**
  * drm_plane_find - find a &drm_plane
  * @dev: DRM device
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v8 6/7] drm: avoid circular locks in drm_lease_held
  2021-08-26  2:01 [PATCH v8 0/7] drm: update locking for modesetting Desmond Cheong Zhi Xi
                   ` (4 preceding siblings ...)
  2021-08-26  2:01 ` [PATCH v8 5/7] drm: avoid circular locks in drm_mode_object_find Desmond Cheong Zhi Xi
@ 2021-08-26  2:01 ` Desmond Cheong Zhi Xi
  2021-08-26  2:01 ` [PATCH v8 7/7] drm: remove drm_file.master_lookup_lock Desmond Cheong Zhi Xi
  6 siblings, 0 replies; 20+ messages in thread
From: Desmond Cheong Zhi Xi @ 2021-08-26  2:01 UTC (permalink / raw)
  To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel,
	sumit.semwal, christian.koenig, jani.nikula, joonas.lahtinen,
	rodrigo.vivi, chris, ville.syrjala, matthew.auld, dan.carpenter,
	tvrtko.ursulin, matthew.d.roper, lucas.demarchi, karthik.b.s,
	jose.souza, manasi.d.navare, airlied, aditya.swarup, andrescj,
	linux-graphics-maintainer, zackr
  Cc: Desmond Cheong Zhi Xi, dri-devel, linux-kernel, intel-gfx,
	linux-media, linaro-mm-sig, skhan, gregkh, linux-kernel-mentees

drm_lease_held calls drm_file_get_master. However, this function is
sometimes called while holding on to drm_device.master_rwsem or
modeset_mutex. Since master_rwsem will replace
drm_file.master_lookup_lock in drm_file_get_master in a future patch,
this results in both recursive locking, and an inversion of the
master_rwsem --> modeset_mutex lock hierarchy.

To fix this, we create a new drm_lease_held_master helper function
that enables us to avoid calling drm_file_get_master after locking
master_rwsem or modeset_mutex.

Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---
 drivers/gpu/drm/drm_auth.c    |  3 +++
 drivers/gpu/drm/drm_crtc.c    |  4 +++-
 drivers/gpu/drm/drm_encoder.c |  7 ++++++-
 drivers/gpu/drm/drm_lease.c   | 30 +++++++++++++++---------------
 drivers/gpu/drm/drm_plane.c   | 18 ++++++++++++++----
 include/drm/drm_lease.h       |  2 ++
 6 files changed, 43 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 65065f7e1499..f2b2f197052a 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -410,6 +410,9 @@ struct drm_master *drm_file_get_master(struct drm_file *file_priv)
 {
 	struct drm_master *master = NULL;
 
+	if (!file_priv)
+		return NULL;
+
 	spin_lock(&file_priv->master_lookup_lock);
 	if (!file_priv->master)
 		goto unlock;
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index b1279bb3fa61..0b1e76d2f9ff 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -665,8 +665,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 
 	plane = crtc->primary;
 
+	lockdep_assert_held_once(&dev->master_rwsem);
 	/* allow disabling with the primary plane leased */
-	if (crtc_req->mode_valid && !drm_lease_held(file_priv, plane->base.id))
+	if (crtc_req->mode_valid &&
+	    !drm_lease_held_master(file_priv->master, plane->base.id))
 		return -EACCES;
 
 	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx,
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index 72e982323a5e..bacb2f6a325c 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -22,6 +22,7 @@
 
 #include <linux/export.h>
 
+#include <drm/drm_auth.h>
 #include <drm/drm_bridge.h>
 #include <drm/drm_device.h>
 #include <drm/drm_drv.h>
@@ -281,6 +282,7 @@ int drm_mode_getencoder(struct drm_device *dev, void *data,
 	struct drm_mode_get_encoder *enc_resp = data;
 	struct drm_encoder *encoder;
 	struct drm_crtc *crtc;
+	struct drm_master *master;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EOPNOTSUPP;
@@ -289,13 +291,16 @@ int drm_mode_getencoder(struct drm_device *dev, void *data,
 	if (!encoder)
 		return -ENOENT;
 
+	master = drm_file_get_master(file_priv);
 	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 	crtc = drm_encoder_get_crtc(encoder);
-	if (crtc && drm_lease_held(file_priv, crtc->base.id))
+	if (crtc && drm_lease_held_master(master, crtc->base.id))
 		enc_resp->crtc_id = crtc->base.id;
 	else
 		enc_resp->crtc_id = 0;
 	drm_modeset_unlock(&dev->mode_config.connection_mutex);
+	if (master)
+		drm_master_put(&master);
 
 	enc_resp->encoder_type = encoder->encoder_type;
 	enc_resp->encoder_id = encoder->base.id;
diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index 1b156c85d1c8..15bf3a3c76d1 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -114,27 +114,30 @@ bool _drm_lease_held(struct drm_file *file_priv, int id)
 	return _drm_lease_held_master(file_priv->master, id);
 }
 
-bool drm_lease_held(struct drm_file *file_priv, int id)
+bool drm_lease_held_master(struct drm_master *master, int id)
 {
-	struct drm_master *master;
 	bool ret;
 
-	if (!file_priv)
+	if (!master || !master->lessor)
 		return true;
 
-	master = drm_file_get_master(file_priv);
-	if (!master)
-		return true;
-	if (!master->lessor) {
-		ret = true;
-		goto out;
-	}
 	mutex_lock(&master->dev->mode_config.idr_mutex);
 	ret = _drm_lease_held_master(master, id);
 	mutex_unlock(&master->dev->mode_config.idr_mutex);
 
-out:
-	drm_master_put(&master);
+	return ret;
+}
+
+bool drm_lease_held(struct drm_file *file_priv, int id)
+{
+	struct drm_master *master;
+	bool ret;
+
+	master = drm_file_get_master(file_priv);
+	ret = drm_lease_held_master(master, id);
+	if (master)
+		drm_master_put(&master);
+
 	return ret;
 }
 
@@ -150,9 +153,6 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
 	int count_in, count_out;
 	uint32_t crtcs_out = 0;
 
-	if (!file_priv)
-		return crtcs_in;
-
 	master = drm_file_get_master(file_priv);
 	if (!master)
 		return crtcs_in;
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index b5566167a798..907b026fd916 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -23,6 +23,7 @@
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 
+#include <drm/drm_auth.h>
 #include <drm/drm_plane.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_print.h>
@@ -687,6 +688,7 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
 	struct drm_mode_get_plane *plane_resp = data;
 	struct drm_plane *plane;
 	uint32_t __user *format_ptr;
+	struct drm_master *master;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EOPNOTSUPP;
@@ -695,10 +697,13 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
 	if (!plane)
 		return -ENOENT;
 
+	master = drm_file_get_master(file_priv);
 	drm_modeset_lock(&plane->mutex, NULL);
-	if (plane->state && plane->state->crtc && drm_lease_held(file_priv, plane->state->crtc->base.id))
+	if (plane->state && plane->state->crtc &&
+	    drm_lease_held_master(master, plane->state->crtc->base.id))
 		plane_resp->crtc_id = plane->state->crtc->base.id;
-	else if (!plane->state && plane->crtc && drm_lease_held(file_priv, plane->crtc->base.id))
+	else if (!plane->state && plane->crtc &&
+		 drm_lease_held_master(master, plane->crtc->base.id))
 		plane_resp->crtc_id = plane->crtc->base.id;
 	else
 		plane_resp->crtc_id = 0;
@@ -710,6 +715,8 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
 	else
 		plane_resp->fb_id = 0;
 	drm_modeset_unlock(&plane->mutex);
+	if (master)
+		drm_master_put(&master);
 
 	plane_resp->plane_id = plane->base.id;
 	plane_resp->possible_crtcs = drm_lease_filter_crtcs(file_priv,
@@ -1114,6 +1121,7 @@ static int drm_mode_cursor_common(struct drm_device *dev,
 		return -ENOENT;
 	}
 
+	lockdep_assert_held_once(&dev->master_rwsem);
 	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
 retry:
 	ret = drm_modeset_lock(&crtc->mutex, &ctx);
@@ -1128,7 +1136,8 @@ static int drm_mode_cursor_common(struct drm_device *dev,
 		if (ret)
 			goto out;
 
-		if (!drm_lease_held(file_priv, crtc->cursor->base.id)) {
+		if (!drm_lease_held_master(file_priv->master,
+					   crtc->cursor->base.id)) {
 			ret = -EACCES;
 			goto out;
 		}
@@ -1235,7 +1244,8 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 
 	plane = crtc->primary;
 
-	if (!drm_lease_held(file_priv, plane->base.id))
+	lockdep_assert_held_once(&dev->master_rwsem);
+	if (!drm_lease_held_master(file_priv->master, plane->base.id))
 		return -EACCES;
 
 	if (crtc->funcs->page_flip_target) {
diff --git a/include/drm/drm_lease.h b/include/drm/drm_lease.h
index 5c9ef6a2aeae..426ea86d3c6a 100644
--- a/include/drm/drm_lease.h
+++ b/include/drm/drm_lease.h
@@ -18,6 +18,8 @@ bool drm_lease_held(struct drm_file *file_priv, int id);
 
 bool _drm_lease_held(struct drm_file *file_priv, int id);
 
+bool drm_lease_held_master(struct drm_master *master, int id);
+
 void drm_lease_revoke(struct drm_master *master);
 
 uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v8 7/7] drm: remove drm_file.master_lookup_lock
  2021-08-26  2:01 [PATCH v8 0/7] drm: update locking for modesetting Desmond Cheong Zhi Xi
                   ` (5 preceding siblings ...)
  2021-08-26  2:01 ` [PATCH v8 6/7] drm: avoid circular locks in drm_lease_held Desmond Cheong Zhi Xi
@ 2021-08-26  2:01 ` Desmond Cheong Zhi Xi
  2021-08-26 13:21   ` Daniel Vetter
  6 siblings, 1 reply; 20+ messages in thread
From: Desmond Cheong Zhi Xi @ 2021-08-26  2:01 UTC (permalink / raw)
  To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel,
	sumit.semwal, christian.koenig, jani.nikula, joonas.lahtinen,
	rodrigo.vivi, chris, ville.syrjala, matthew.auld, dan.carpenter,
	tvrtko.ursulin, matthew.d.roper, lucas.demarchi, karthik.b.s,
	jose.souza, manasi.d.navare, airlied, aditya.swarup, andrescj,
	linux-graphics-maintainer, zackr
  Cc: Desmond Cheong Zhi Xi, dri-devel, linux-kernel, intel-gfx,
	linux-media, linaro-mm-sig, skhan, gregkh, linux-kernel-mentees

Previously, master_lookup_lock was introduced in
commit 0b0860a3cf5e ("drm: serialize drm_file.master with a new
spinlock") to serialize accesses to drm_file.master. This then allowed
us to write drm_file_get_master in commit 56f0729a510f ("drm: protect
drm_master pointers in drm_lease.c").

The rationale behind introducing a new spinlock at the time was that
the other lock that could have been used (drm_device.master_mutex) was
the outermost lock, so embedding calls to drm_file_get_master and
drm_is_current_master in various functions easily caused us to invert
the lock hierarchy.

Following the conversion of master_mutex into a rwsem, and its use to
plug races with modesetting rights, we've untangled some lock
hierarchies and removed the need for using drm_file_get_master and the
unlocked version of drm_is_current_master in multiple places.

Hence, we can take this opportunity to clean up the locking design by
replacing master_lookup_lock with drm_device.master_rwsem.

Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---
 drivers/gpu/drm/drm_auth.c     | 19 +++++++------------
 drivers/gpu/drm/drm_file.c     |  1 -
 drivers/gpu/drm/drm_internal.h |  1 +
 drivers/gpu/drm/drm_ioctl.c    |  4 ++--
 drivers/gpu/drm/drm_lease.c    | 18 ++++++++----------
 include/drm/drm_file.h         |  9 +--------
 6 files changed, 19 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index f2b2f197052a..232416119407 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -61,10 +61,9 @@
  * trusted clients.
  */
 
-static bool drm_is_current_master_locked(struct drm_file *fpriv)
+bool drm_is_current_master_locked(struct drm_file *fpriv)
 {
-	lockdep_assert_once(lockdep_is_held(&fpriv->master_lookup_lock) ||
-			    lockdep_is_held(&fpriv->minor->dev->master_rwsem));
+	lockdep_assert_held_once(&fpriv->minor->dev->master_rwsem);
 
 	return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master;
 }
@@ -83,9 +82,9 @@ bool drm_is_current_master(struct drm_file *fpriv)
 {
 	bool ret;
 
-	spin_lock(&fpriv->master_lookup_lock);
+	down_read(&fpriv->minor->dev->master_rwsem);
 	ret = drm_is_current_master_locked(fpriv);
-	spin_unlock(&fpriv->master_lookup_lock);
+	up_read(&fpriv->minor->dev->master_rwsem);
 
 	return ret;
 }
@@ -120,7 +119,7 @@ int drm_authmagic(struct drm_device *dev, void *data,
 	DRM_DEBUG("%u\n", auth->magic);
 
 	down_write(&dev->master_rwsem);
-	if (unlikely(!drm_is_current_master(file_priv))) {
+	if (unlikely(!drm_is_current_master_locked(file_priv))) {
 		up_write(&dev->master_rwsem);
 		return -EACCES;
 	}
@@ -178,9 +177,7 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
 	new_master = drm_master_create(dev);
 	if (!new_master)
 		return -ENOMEM;
-	spin_lock(&fpriv->master_lookup_lock);
 	fpriv->master = new_master;
-	spin_unlock(&fpriv->master_lookup_lock);
 
 	fpriv->is_master = 1;
 	fpriv->authenticated = 1;
@@ -343,9 +340,7 @@ int drm_master_open(struct drm_file *file_priv)
 	if (!dev->master) {
 		ret = drm_new_set_master(dev, file_priv);
 	} else {
-		spin_lock(&file_priv->master_lookup_lock);
 		file_priv->master = drm_master_get(dev->master);
-		spin_unlock(&file_priv->master_lookup_lock);
 	}
 	up_write(&dev->master_rwsem);
 
@@ -413,13 +408,13 @@ struct drm_master *drm_file_get_master(struct drm_file *file_priv)
 	if (!file_priv)
 		return NULL;
 
-	spin_lock(&file_priv->master_lookup_lock);
+	down_read(&file_priv->minor->dev->master_rwsem);
 	if (!file_priv->master)
 		goto unlock;
 	master = drm_master_get(file_priv->master);
 
 unlock:
-	spin_unlock(&file_priv->master_lookup_lock);
+	up_read(&file_priv->minor->dev->master_rwsem);
 	return master;
 }
 EXPORT_SYMBOL(drm_file_get_master);
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 90b62f360da1..8c846e0179d7 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -176,7 +176,6 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
 	init_waitqueue_head(&file->event_wait);
 	file->event_space = 4096; /* set aside 4k for event buffer */
 
-	spin_lock_init(&file->master_lookup_lock);
 	mutex_init(&file->event_read_lock);
 
 	if (drm_core_check_feature(dev, DRIVER_GEM))
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 17f3548c8ed2..5d421f749a17 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -132,6 +132,7 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
 				  struct drm_file *filp);
 
 /* drm_auth.c */
+bool drm_is_current_master_locked(struct drm_file *fpriv);
 int drm_getmagic(struct drm_device *dev, void *data,
 		 struct drm_file *file_priv);
 int drm_authmagic(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 8bea39ffc5c0..c728437466c3 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -386,7 +386,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
 	int if_version, retcode = 0;
 
 	down_write(&dev->master_rwsem);
-	if (unlikely(!drm_is_current_master(file_priv))) {
+	if (unlikely(!drm_is_current_master_locked(file_priv))) {
 		retcode = -EACCES;
 		goto unlock;
 	}
@@ -540,7 +540,7 @@ static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
 
 	/* MASTER is only for master or control clients */
 	if (unlikely((flags & DRM_MASTER) &&
-		     !drm_is_current_master(file_priv)))
+		     !drm_is_current_master_locked(file_priv)))
 		return -EACCES;
 
 	/* Render clients must be explicitly allowed */
diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index 15bf3a3c76d1..0eecf320b1ab 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -498,12 +498,12 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
 		return PTR_ERR(lessee_file);
 
 	down_read(&dev->master_rwsem);
-	if (unlikely(!drm_is_current_master(lessor_priv))) {
+	if (unlikely(!drm_is_current_master_locked(lessor_priv))) {
 		ret = -EACCES;
 		goto out_file;
 	}
 
-	lessor = drm_file_get_master(lessor_priv);
+	lessor = lessor_priv->master;
 	/* Do not allow sub-leases */
 	if (lessor->lessor) {
 		DRM_DEBUG_LEASE("recursive leasing not allowed\n");
@@ -565,7 +565,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
 	/* Hook up the fd */
 	fd_install(fd, lessee_file);
 
-	drm_master_put(&lessor);
 	up_read(&dev->master_rwsem);
 	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n");
 	return 0;
@@ -600,7 +599,8 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EOPNOTSUPP;
 
-	lessor = drm_file_get_master(lessor_priv);
+	lockdep_assert_held_once(&dev->master_rwsem);
+	lessor = lessor_priv->master;
 	DRM_DEBUG_LEASE("List lessees for %d\n", lessor->lessee_id);
 
 	mutex_lock(&dev->mode_config.idr_mutex);
@@ -624,7 +624,6 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
 		arg->count_lessees = count;
 
 	mutex_unlock(&dev->mode_config.idr_mutex);
-	drm_master_put(&lessor);
 
 	return ret;
 }
@@ -650,7 +649,8 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EOPNOTSUPP;
 
-	lessee = drm_file_get_master(lessee_priv);
+	lockdep_assert_held_once(&dev->master_rwsem);
+	lessee = lessee_priv->master;
 	DRM_DEBUG_LEASE("get lease for %d\n", lessee->lessee_id);
 
 	mutex_lock(&dev->mode_config.idr_mutex);
@@ -678,7 +678,6 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
 		arg->count_objects = count;
 
 	mutex_unlock(&dev->mode_config.idr_mutex);
-	drm_master_put(&lessee);
 
 	return ret;
 }
@@ -703,11 +702,11 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
 		return -EOPNOTSUPP;
 
 	down_write(&dev->master_rwsem);
-	if (unlikely(!drm_is_current_master(lessor_priv))) {
+	if (unlikely(!drm_is_current_master_locked(lessor_priv))) {
 		ret = -EACCES;
 		goto unlock;
 	}
-	lessor = drm_file_get_master(lessor_priv);
+	lessor = lessor_priv->master;
 	mutex_lock(&dev->mode_config.idr_mutex);
 
 	lessee = _drm_find_lessee(lessor, arg->lessee_id);
@@ -728,7 +727,6 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
 
 fail:
 	mutex_unlock(&dev->mode_config.idr_mutex);
-	drm_master_put(&lessor);
 
 unlock:
 	up_write(&dev->master_rwsem);
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index d12bb2ba7814..e2d49fe3e32d 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -227,16 +227,12 @@ struct drm_file {
 	 * @master:
 	 *
 	 * Master this node is currently associated with. Protected by struct
-	 * &drm_device.master_rwsem, and serialized by @master_lookup_lock.
+	 * &drm_device.master_rwsem.
 	 *
 	 * Only relevant if drm_is_primary_client() returns true. Note that
 	 * this only matches &drm_device.master if the master is the currently
 	 * active one.
 	 *
-	 * To update @master, both &drm_device.master_rwsem and
-	 * @master_lookup_lock need to be held, therefore holding either of
-	 * them is safe and enough for the read side.
-	 *
 	 * When dereferencing this pointer, either hold struct
 	 * &drm_device.master_rwsem for the duration of the pointer's use, or
 	 * use drm_file_get_master() if struct &drm_device.master_rwsem is not
@@ -248,9 +244,6 @@ struct drm_file {
 	 */
 	struct drm_master *master;
 
-	/** @master_lock: Serializes @master. */
-	spinlock_t master_lookup_lock;
-
 	/** @pid: Process that opened this file. */
 	struct pid *pid;
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v8 1/7] drm: fix null ptr dereference in drm_master_release
  2021-08-26  2:01 ` [PATCH v8 1/7] drm: fix null ptr dereference in drm_master_release Desmond Cheong Zhi Xi
@ 2021-08-26  9:53   ` Daniel Vetter
  2021-08-26 11:53     ` Desmond Cheong Zhi Xi
  0 siblings, 1 reply; 20+ messages in thread
From: Daniel Vetter @ 2021-08-26  9:53 UTC (permalink / raw)
  To: Desmond Cheong Zhi Xi
  Cc: maarten.lankhorst, mripard, tzimmermann, airlied, daniel,
	sumit.semwal, christian.koenig, jani.nikula, joonas.lahtinen,
	rodrigo.vivi, chris, ville.syrjala, matthew.auld, dan.carpenter,
	tvrtko.ursulin, matthew.d.roper, lucas.demarchi, karthik.b.s,
	jose.souza, manasi.d.navare, airlied, aditya.swarup, andrescj,
	linux-graphics-maintainer, zackr, dri-devel, linux-kernel,
	intel-gfx, linux-media, linaro-mm-sig, skhan, gregkh,
	linux-kernel-mentees

On Thu, Aug 26, 2021 at 10:01:16AM +0800, Desmond Cheong Zhi Xi wrote:
> drm_master_release can be called on a drm_file without a master, which
> results in a null ptr dereference of file_priv->master->magic_map. The
> three cases are:
> 
> 1. Error path in drm_open_helper
>   drm_open():
>     drm_open_helper():
>       drm_master_open():
>         drm_new_set_master(); <--- returns -ENOMEM,
>                                    drm_file.master not set
>       drm_file_free():
>         drm_master_release(); <--- NULL ptr dereference
>                                    (file_priv->master->magic_map)
> 
> 2. Error path in mock_drm_getfile
>   mock_drm_getfile():
>     anon_inode_getfile(); <--- returns error, drm_file.master not set
>     drm_file_free():
>       drm_master_release(); <--- NULL ptr dereference
>                                  (file_priv->master->magic_map)
> 
> 3. In drm_client_close, as drm_client_open doesn't set up a master
> 
> drm_file.master is set up in drm_open_helper through the call to
> drm_master_open, so we mirror it with a call to drm_master_release in
> drm_close_helper, and remove drm_master_release from drm_file_free to
> avoid the null ptr dereference.
> 
> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I guess we should also have a cc: stable on this one? I think this bug
existed since pretty much forever, but maybe more prominent with the
drm_client stuff added a while ago.
-Daniel

> ---
>  drivers/gpu/drm/drm_file.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index ed25168619fc..90b62f360da1 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -282,9 +282,6 @@ void drm_file_free(struct drm_file *file)
>  
>  	drm_legacy_ctxbitmap_flush(dev, file);
>  
> -	if (drm_is_primary_client(file))
> -		drm_master_release(file);
> -
>  	if (dev->driver->postclose)
>  		dev->driver->postclose(dev, file);
>  
> @@ -305,6 +302,9 @@ static void drm_close_helper(struct file *filp)
>  	list_del(&file_priv->lhead);
>  	mutex_unlock(&dev->filelist_mutex);
>  
> +	if (drm_is_primary_client(file_priv))
> +		drm_master_release(file_priv);
> +
>  	drm_file_free(file_priv);
>  }
>  
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v8 2/7] drm: convert drm_device.master_mutex into a rwsem
  2021-08-26  2:01 ` [PATCH v8 2/7] drm: convert drm_device.master_mutex into a rwsem Desmond Cheong Zhi Xi
@ 2021-08-26  9:55   ` Daniel Vetter
  0 siblings, 0 replies; 20+ messages in thread
From: Daniel Vetter @ 2021-08-26  9:55 UTC (permalink / raw)
  To: Desmond Cheong Zhi Xi
  Cc: maarten.lankhorst, mripard, tzimmermann, airlied, daniel,
	sumit.semwal, christian.koenig, jani.nikula, joonas.lahtinen,
	rodrigo.vivi, chris, ville.syrjala, matthew.auld, dan.carpenter,
	tvrtko.ursulin, matthew.d.roper, lucas.demarchi, karthik.b.s,
	jose.souza, manasi.d.navare, airlied, aditya.swarup, andrescj,
	linux-graphics-maintainer, zackr, dri-devel, linux-kernel,
	intel-gfx, linux-media, linaro-mm-sig, skhan, gregkh,
	linux-kernel-mentees

On Thu, Aug 26, 2021 at 10:01:17AM +0800, Desmond Cheong Zhi Xi wrote:
> drm_device.master_mutex currently protects the following:
> - drm_device.master
> - drm_file.master
> - drm_file.was_master
> - drm_file.is_master
> - drm_master.unique
> - drm_master.unique_len
> - drm_master.magic_map
> 
> There is a clear separation between functions that read or change
> these attributes. Hence, convert master_mutex into a rwsem to enable
> concurrent readers.
> 
> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_auth.c    | 35 ++++++++++++++++++-----------------
>  drivers/gpu/drm/drm_debugfs.c |  4 ++--
>  drivers/gpu/drm/drm_drv.c     |  3 +--
>  drivers/gpu/drm/drm_ioctl.c   | 10 +++++-----
>  include/drm/drm_auth.h        |  6 +++---
>  include/drm/drm_device.h      | 10 ++++++----
>  include/drm/drm_file.h        | 12 ++++++------
>  7 files changed, 41 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index 60a6b21474b1..73ade0513ccb 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -64,7 +64,7 @@
>  static bool drm_is_current_master_locked(struct drm_file *fpriv)
>  {
>  	lockdep_assert_once(lockdep_is_held(&fpriv->master_lookup_lock) ||
> -			    lockdep_is_held(&fpriv->minor->dev->master_mutex));
> +			    lockdep_is_held(&fpriv->minor->dev->master_rwsem));
>  
>  	return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master;
>  }
> @@ -96,7 +96,7 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
>  	struct drm_auth *auth = data;
>  	int ret = 0;
>  
> -	mutex_lock(&dev->master_mutex);
> +	down_write(&dev->master_rwsem);
>  	if (!file_priv->magic) {
>  		ret = idr_alloc(&file_priv->master->magic_map, file_priv,
>  				1, 0, GFP_KERNEL);
> @@ -104,7 +104,7 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
>  			file_priv->magic = ret;
>  	}
>  	auth->magic = file_priv->magic;
> -	mutex_unlock(&dev->master_mutex);
> +	up_write(&dev->master_rwsem);
>  
>  	DRM_DEBUG("%u\n", auth->magic);
>  
> @@ -119,13 +119,13 @@ int drm_authmagic(struct drm_device *dev, void *data,
>  
>  	DRM_DEBUG("%u\n", auth->magic);
>  
> -	mutex_lock(&dev->master_mutex);
> +	down_write(&dev->master_rwsem);
>  	file = idr_find(&file_priv->master->magic_map, auth->magic);
>  	if (file) {
>  		file->authenticated = 1;
>  		idr_replace(&file_priv->master->magic_map, NULL, auth->magic);
>  	}
> -	mutex_unlock(&dev->master_mutex);
> +	up_write(&dev->master_rwsem);
>  
>  	return file ? 0 : -EINVAL;
>  }
> @@ -167,7 +167,7 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
>  	struct drm_master *old_master;
>  	struct drm_master *new_master;
>  
> -	lockdep_assert_held_once(&dev->master_mutex);
> +	lockdep_assert_held_once(&dev->master_rwsem);
>  
>  	WARN_ON(fpriv->is_master);
>  	old_master = fpriv->master;
> @@ -249,7 +249,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
>  {
>  	int ret;
>  
> -	mutex_lock(&dev->master_mutex);
> +	down_write(&dev->master_rwsem);
>  
>  	ret = drm_master_check_perm(dev, file_priv);
>  	if (ret)
> @@ -281,7 +281,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
>  
>  	drm_set_master(dev, file_priv, false);
>  out_unlock:
> -	mutex_unlock(&dev->master_mutex);
> +	up_write(&dev->master_rwsem);
>  	return ret;
>  }
>  
> @@ -298,7 +298,7 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
>  {
>  	int ret;
>  
> -	mutex_lock(&dev->master_mutex);
> +	down_write(&dev->master_rwsem);
>  
>  	ret = drm_master_check_perm(dev, file_priv);
>  	if (ret)
> @@ -321,8 +321,9 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
>  	}
>  
>  	drm_drop_master(dev, file_priv);
> +
>  out_unlock:
> -	mutex_unlock(&dev->master_mutex);
> +	up_write(&dev->master_rwsem);
>  	return ret;
>  }
>  
> @@ -334,7 +335,7 @@ int drm_master_open(struct drm_file *file_priv)
>  	/* if there is no current master make this fd it, but do not create
>  	 * any master object for render clients
>  	 */
> -	mutex_lock(&dev->master_mutex);
> +	down_write(&dev->master_rwsem);
>  	if (!dev->master) {
>  		ret = drm_new_set_master(dev, file_priv);
>  	} else {
> @@ -342,7 +343,7 @@ int drm_master_open(struct drm_file *file_priv)
>  		file_priv->master = drm_master_get(dev->master);
>  		spin_unlock(&file_priv->master_lookup_lock);
>  	}
> -	mutex_unlock(&dev->master_mutex);
> +	up_write(&dev->master_rwsem);
>  
>  	return ret;
>  }
> @@ -352,7 +353,7 @@ void drm_master_release(struct drm_file *file_priv)
>  	struct drm_device *dev = file_priv->minor->dev;
>  	struct drm_master *master;
>  
> -	mutex_lock(&dev->master_mutex);
> +	down_write(&dev->master_rwsem);
>  	master = file_priv->master;
>  	if (file_priv->magic)
>  		idr_remove(&file_priv->master->magic_map, file_priv->magic);
> @@ -375,7 +376,7 @@ void drm_master_release(struct drm_file *file_priv)
>  	/* drop the master reference held by the file priv */
>  	if (file_priv->master)
>  		drm_master_put(&file_priv->master);
> -	mutex_unlock(&dev->master_mutex);
> +	up_write(&dev->master_rwsem);
>  }
>  
>  /**
> @@ -450,9 +451,9 @@ EXPORT_SYMBOL(drm_master_put);
>  /* Used by drm_client and drm_fb_helper */
>  bool drm_master_internal_acquire(struct drm_device *dev)
>  {
> -	mutex_lock(&dev->master_mutex);
> +	down_read(&dev->master_rwsem);
>  	if (dev->master) {
> -		mutex_unlock(&dev->master_mutex);
> +		up_read(&dev->master_rwsem);
>  		return false;
>  	}
>  
> @@ -463,6 +464,6 @@ EXPORT_SYMBOL(drm_master_internal_acquire);
>  /* Used by drm_client and drm_fb_helper */
>  void drm_master_internal_release(struct drm_device *dev)
>  {
> -	mutex_unlock(&dev->master_mutex);
> +	up_read(&dev->master_rwsem);
>  }
>  EXPORT_SYMBOL(drm_master_internal_release);
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index b0a826489488..b34c9c263188 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -55,7 +55,7 @@ static int drm_name_info(struct seq_file *m, void *data)
>  	struct drm_device *dev = minor->dev;
>  	struct drm_master *master;
>  
> -	mutex_lock(&dev->master_mutex);
> +	down_read(&dev->master_rwsem);
>  	master = dev->master;
>  	seq_printf(m, "%s", dev->driver->name);
>  	if (dev->dev)
> @@ -65,7 +65,7 @@ static int drm_name_info(struct seq_file *m, void *data)
>  	if (dev->unique)
>  		seq_printf(m, " unique=%s", dev->unique);
>  	seq_printf(m, "\n");
> -	mutex_unlock(&dev->master_mutex);
> +	up_read(&dev->master_rwsem);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 7a5097467ba5..4556bf42954c 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -570,7 +570,6 @@ static void drm_dev_init_release(struct drm_device *dev, void *res)
>  	/* Prevent use-after-free in drm_managed_release when debugging is
>  	 * enabled. Slightly awkward, but can't really be helped. */
>  	dev->dev = NULL;
> -	mutex_destroy(&dev->master_mutex);
>  	mutex_destroy(&dev->clientlist_mutex);
>  	mutex_destroy(&dev->filelist_mutex);
>  	mutex_destroy(&dev->struct_mutex);
> @@ -611,7 +610,7 @@ static int drm_dev_init(struct drm_device *dev,
>  	mutex_init(&dev->struct_mutex);
>  	mutex_init(&dev->filelist_mutex);
>  	mutex_init(&dev->clientlist_mutex);
> -	mutex_init(&dev->master_mutex);
> +	init_rwsem(&dev->master_rwsem);
>  
>  	ret = drmm_add_action(dev, drm_dev_init_release, NULL);
>  	if (ret)
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 26f3a9ede8fe..d25713b09b80 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -119,16 +119,16 @@ int drm_getunique(struct drm_device *dev, void *data,
>  	struct drm_unique *u = data;
>  	struct drm_master *master;
>  
> -	mutex_lock(&dev->master_mutex);
> +	down_read(&dev->master_rwsem);
>  	master = file_priv->master;
>  	if (u->unique_len >= master->unique_len) {
>  		if (copy_to_user(u->unique, master->unique, master->unique_len)) {
> -			mutex_unlock(&dev->master_mutex);
> +			up_read(&dev->master_rwsem);
>  			return -EFAULT;
>  		}
>  	}
>  	u->unique_len = master->unique_len;
> -	mutex_unlock(&dev->master_mutex);
> +	up_read(&dev->master_rwsem);
>  
>  	return 0;
>  }
> @@ -385,7 +385,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
>  	struct drm_set_version *sv = data;
>  	int if_version, retcode = 0;
>  
> -	mutex_lock(&dev->master_mutex);
> +	down_write(&dev->master_rwsem);
>  	if (sv->drm_di_major != -1) {
>  		if (sv->drm_di_major != DRM_IF_MAJOR ||
>  		    sv->drm_di_minor < 0 || sv->drm_di_minor > DRM_IF_MINOR) {
> @@ -420,7 +420,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
>  	sv->drm_di_minor = DRM_IF_MINOR;
>  	sv->drm_dd_major = dev->driver->major;
>  	sv->drm_dd_minor = dev->driver->minor;
> -	mutex_unlock(&dev->master_mutex);
> +	up_write(&dev->master_rwsem);
>  
>  	return retcode;
>  }
> diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
> index ba248ca8866f..f0a89e5fcaad 100644
> --- a/include/drm/drm_auth.h
> +++ b/include/drm/drm_auth.h
> @@ -67,17 +67,17 @@ struct drm_master {
>  	struct drm_device *dev;
>  	/**
>  	 * @unique: Unique identifier: e.g. busid. Protected by
> -	 * &drm_device.master_mutex.
> +	 * &drm_device.master_rwsem.
>  	 */
>  	char *unique;
>  	/**
>  	 * @unique_len: Length of unique field. Protected by
> -	 * &drm_device.master_mutex.
> +	 * &drm_device.master_rwsem.
>  	 */
>  	int unique_len;
>  	/**
>  	 * @magic_map: Map of used authentication tokens. Protected by
> -	 * &drm_device.master_mutex.
> +	 * &drm_device.master_rwsem.
>  	 */
>  	struct idr magic_map;
>  	void *driver_priv;
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index 604b1d1b2d72..142fb2f6e74d 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -107,7 +107,7 @@ struct drm_device {
>  	 * @master:
>  	 *
>  	 * Currently active master for this device.
> -	 * Protected by &master_mutex
> +	 * Protected by &master_rwsem
>  	 */
>  	struct drm_master *master;
>  
> @@ -146,11 +146,13 @@ struct drm_device {
>  	struct mutex struct_mutex;
>  
>  	/**
> -	 * @master_mutex:
> +	 * @master_rwsem:
>  	 *
> -	 * Lock for &drm_minor.master and &drm_file.is_master
> +	 * Lock for &drm_device.master, &drm_file.was_master,
> +	 * &drm_file.is_master, &drm_file.master, &drm_master.unique,
> +	 * &drm_master.unique_len, and &drm_master.magic_map.
>  	 */
> -	struct mutex master_mutex;
> +	struct rw_semaphore master_rwsem;
>  
>  	/**
>  	 * @open_count:
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index a3acb7ac3550..d12bb2ba7814 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -205,7 +205,7 @@ struct drm_file {
>  	 * @was_master:
>  	 *
>  	 * This client has or had, master capability. Protected by struct
> -	 * &drm_device.master_mutex.
> +	 * &drm_device.master_rwsem.
>  	 *
>  	 * This is used to ensure that CAP_SYS_ADMIN is not enforced, if the
>  	 * client is or was master in the past.
> @@ -216,7 +216,7 @@ struct drm_file {
>  	 * @is_master:
>  	 *
>  	 * This client is the creator of @master. Protected by struct
> -	 * &drm_device.master_mutex.
> +	 * &drm_device.master_rwsem.
>  	 *
>  	 * See also the :ref:`section on primary nodes and authentication
>  	 * <drm_primary_node>`.
> @@ -227,19 +227,19 @@ struct drm_file {
>  	 * @master:
>  	 *
>  	 * Master this node is currently associated with. Protected by struct
> -	 * &drm_device.master_mutex, and serialized by @master_lookup_lock.
> +	 * &drm_device.master_rwsem, and serialized by @master_lookup_lock.
>  	 *
>  	 * Only relevant if drm_is_primary_client() returns true. Note that
>  	 * this only matches &drm_device.master if the master is the currently
>  	 * active one.
>  	 *
> -	 * To update @master, both &drm_device.master_mutex and
> +	 * To update @master, both &drm_device.master_rwsem and
>  	 * @master_lookup_lock need to be held, therefore holding either of
>  	 * them is safe and enough for the read side.
>  	 *
>  	 * When dereferencing this pointer, either hold struct
> -	 * &drm_device.master_mutex for the duration of the pointer's use, or
> -	 * use drm_file_get_master() if struct &drm_device.master_mutex is not
> +	 * &drm_device.master_rwsem for the duration of the pointer's use, or
> +	 * use drm_file_get_master() if struct &drm_device.master_rwsem is not
>  	 * currently held and there is no other need to hold it. This prevents
>  	 * @master from being freed during use.
>  	 *
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v8 3/7] drm: lock drm_global_mutex earlier in the ioctl handler
  2021-08-26  2:01 ` [PATCH v8 3/7] drm: lock drm_global_mutex earlier in the ioctl handler Desmond Cheong Zhi Xi
@ 2021-08-26  9:58   ` Daniel Vetter
  2021-08-30 21:04     ` Desmond Cheong Zhi Xi
  0 siblings, 1 reply; 20+ messages in thread
From: Daniel Vetter @ 2021-08-26  9:58 UTC (permalink / raw)
  To: Desmond Cheong Zhi Xi
  Cc: maarten.lankhorst, mripard, tzimmermann, airlied, daniel,
	sumit.semwal, christian.koenig, jani.nikula, joonas.lahtinen,
	rodrigo.vivi, chris, ville.syrjala, matthew.auld, dan.carpenter,
	tvrtko.ursulin, matthew.d.roper, lucas.demarchi, karthik.b.s,
	jose.souza, manasi.d.navare, airlied, aditya.swarup, andrescj,
	linux-graphics-maintainer, zackr, dri-devel, linux-kernel,
	intel-gfx, linux-media, linaro-mm-sig, skhan, gregkh,
	linux-kernel-mentees

On Thu, Aug 26, 2021 at 10:01:18AM +0800, Desmond Cheong Zhi Xi wrote:
> In a future patch, a read lock on drm_device.master_rwsem is
> held in the ioctl handler before the check for ioctl
> permissions. However, this inverts the lock hierarchy of
> drm_global_mutex --> master_rwsem.
> 
> To avoid this, we do some prep work to grab the drm_global_mutex
> before checking for ioctl permissions.
> 
> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
> ---
>  drivers/gpu/drm/drm_ioctl.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index d25713b09b80..158629d88319 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -772,19 +772,19 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata,
>  	if (drm_dev_is_unplugged(dev))
>  		return -ENODEV;
>  
> +	/* Enforce sane locking for modern driver ioctls. */
> +	if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)) && !(flags & DRM_UNLOCKED))

Maybe have a local bool locked_ioctl for this so it's extremely clear it's
the same condition in both?

Either way: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +		mutex_lock(&drm_global_mutex);
> +
>  	retcode = drm_ioctl_permit(flags, file_priv);
>  	if (unlikely(retcode))
> -		return retcode;
> +		goto out;
>  
> -	/* Enforce sane locking for modern driver ioctls. */
> -	if (likely(!drm_core_check_feature(dev, DRIVER_LEGACY)) ||
> -	    (flags & DRM_UNLOCKED))
> -		retcode = func(dev, kdata, file_priv);
> -	else {
> -		mutex_lock(&drm_global_mutex);
> -		retcode = func(dev, kdata, file_priv);
> +	retcode = func(dev, kdata, file_priv);
> +
> +out:
> +	if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)) && !(flags & DRM_UNLOCKED))
>  		mutex_unlock(&drm_global_mutex);
> -	}
>  	return retcode;
>  }
>  EXPORT_SYMBOL(drm_ioctl_kernel);
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v8 1/7] drm: fix null ptr dereference in drm_master_release
  2021-08-26  9:53   ` Daniel Vetter
@ 2021-08-26 11:53     ` Desmond Cheong Zhi Xi
  2021-08-26 12:51       ` [Intel-gfx] " Daniel Vetter
  0 siblings, 1 reply; 20+ messages in thread
From: Desmond Cheong Zhi Xi @ 2021-08-26 11:53 UTC (permalink / raw)
  To: maarten.lankhorst, mripard, tzimmermann, airlied, sumit.semwal,
	christian.koenig, jani.nikula, joonas.lahtinen, rodrigo.vivi,
	chris, ville.syrjala, matthew.auld, dan.carpenter,
	tvrtko.ursulin, matthew.d.roper, lucas.demarchi, karthik.b.s,
	jose.souza, manasi.d.navare, airlied, aditya.swarup, andrescj,
	linux-graphics-maintainer, zackr, dri-devel, linux-kernel,
	intel-gfx, linux-media, linaro-mm-sig, skhan, gregkh,
	linux-kernel-mentees

On 26/8/21 5:53 pm, Daniel Vetter wrote:
> On Thu, Aug 26, 2021 at 10:01:16AM +0800, Desmond Cheong Zhi Xi wrote:
>> drm_master_release can be called on a drm_file without a master, which
>> results in a null ptr dereference of file_priv->master->magic_map. The
>> three cases are:
>>
>> 1. Error path in drm_open_helper
>>    drm_open():
>>      drm_open_helper():
>>        drm_master_open():
>>          drm_new_set_master(); <--- returns -ENOMEM,
>>                                     drm_file.master not set
>>        drm_file_free():
>>          drm_master_release(); <--- NULL ptr dereference
>>                                     (file_priv->master->magic_map)
>>
>> 2. Error path in mock_drm_getfile
>>    mock_drm_getfile():
>>      anon_inode_getfile(); <--- returns error, drm_file.master not set
>>      drm_file_free():
>>        drm_master_release(); <--- NULL ptr dereference
>>                                   (file_priv->master->magic_map)
>>
>> 3. In drm_client_close, as drm_client_open doesn't set up a master
>>
>> drm_file.master is set up in drm_open_helper through the call to
>> drm_master_open, so we mirror it with a call to drm_master_release in
>> drm_close_helper, and remove drm_master_release from drm_file_free to
>> avoid the null ptr dereference.
>>
>> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> I guess we should also have a cc: stable on this one? I think this bug
> existed since pretty much forever, but maybe more prominent with the
> drm_client stuff added a while ago.
> -Daniel
> 

Thanks for the reviews, Daniel.

Took a closer look. I think if we cc: stable, this fix should accompany 
commit 7eeaeb90a6a5 ("drm/file: Don't set master on in-kernel clients") 
which moves the drm_master_open out from drm_file_alloc into 
drm_open_helper.

>> ---
>>   drivers/gpu/drm/drm_file.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>> index ed25168619fc..90b62f360da1 100644
>> --- a/drivers/gpu/drm/drm_file.c
>> +++ b/drivers/gpu/drm/drm_file.c
>> @@ -282,9 +282,6 @@ void drm_file_free(struct drm_file *file)
>>   
>>   	drm_legacy_ctxbitmap_flush(dev, file);
>>   
>> -	if (drm_is_primary_client(file))
>> -		drm_master_release(file);
>> -
>>   	if (dev->driver->postclose)
>>   		dev->driver->postclose(dev, file);
>>   
>> @@ -305,6 +302,9 @@ static void drm_close_helper(struct file *filp)
>>   	list_del(&file_priv->lhead);
>>   	mutex_unlock(&dev->filelist_mutex);
>>   
>> +	if (drm_is_primary_client(file_priv))
>> +		drm_master_release(file_priv);
>> +
>>   	drm_file_free(file_priv);
>>   }
>>   
>> -- 
>> 2.25.1
>>
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Intel-gfx] [PATCH v8 1/7] drm: fix null ptr dereference in drm_master_release
  2021-08-26 11:53     ` Desmond Cheong Zhi Xi
@ 2021-08-26 12:51       ` Daniel Vetter
  0 siblings, 0 replies; 20+ messages in thread
From: Daniel Vetter @ 2021-08-26 12:51 UTC (permalink / raw)
  To: Desmond Cheong Zhi Xi
  Cc: maarten.lankhorst, mripard, tzimmermann, airlied, sumit.semwal,
	christian.koenig, jani.nikula, joonas.lahtinen, rodrigo.vivi,
	chris, ville.syrjala, matthew.auld, dan.carpenter,
	tvrtko.ursulin, matthew.d.roper, lucas.demarchi, karthik.b.s,
	jose.souza, manasi.d.navare, airlied, aditya.swarup, andrescj,
	linux-graphics-maintainer, zackr, dri-devel, linux-kernel,
	intel-gfx, linux-media, linaro-mm-sig, skhan, gregkh,
	linux-kernel-mentees

On Thu, Aug 26, 2021 at 07:53:58PM +0800, Desmond Cheong Zhi Xi wrote:
> On 26/8/21 5:53 pm, Daniel Vetter wrote:
> > On Thu, Aug 26, 2021 at 10:01:16AM +0800, Desmond Cheong Zhi Xi wrote:
> > > drm_master_release can be called on a drm_file without a master, which
> > > results in a null ptr dereference of file_priv->master->magic_map. The
> > > three cases are:
> > > 
> > > 1. Error path in drm_open_helper
> > >    drm_open():
> > >      drm_open_helper():
> > >        drm_master_open():
> > >          drm_new_set_master(); <--- returns -ENOMEM,
> > >                                     drm_file.master not set
> > >        drm_file_free():
> > >          drm_master_release(); <--- NULL ptr dereference
> > >                                     (file_priv->master->magic_map)
> > > 
> > > 2. Error path in mock_drm_getfile
> > >    mock_drm_getfile():
> > >      anon_inode_getfile(); <--- returns error, drm_file.master not set
> > >      drm_file_free():
> > >        drm_master_release(); <--- NULL ptr dereference
> > >                                   (file_priv->master->magic_map)
> > > 
> > > 3. In drm_client_close, as drm_client_open doesn't set up a master
> > > 
> > > drm_file.master is set up in drm_open_helper through the call to
> > > drm_master_open, so we mirror it with a call to drm_master_release in
> > > drm_close_helper, and remove drm_master_release from drm_file_free to
> > > avoid the null ptr dereference.
> > > 
> > > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > I guess we should also have a cc: stable on this one? I think this bug
> > existed since pretty much forever, but maybe more prominent with the
> > drm_client stuff added a while ago.
> > -Daniel
> > 
> 
> Thanks for the reviews, Daniel.
> 
> Took a closer look. I think if we cc: stable, this fix should accompany
> commit 7eeaeb90a6a5 ("drm/file: Don't set master on in-kernel clients")
> which moves the drm_master_open out from drm_file_alloc into
> drm_open_helper.

Ah right, please reference that commit with a Fixes: line.
-Daniel

> 
> > > ---
> > >   drivers/gpu/drm/drm_file.c | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > > index ed25168619fc..90b62f360da1 100644
> > > --- a/drivers/gpu/drm/drm_file.c
> > > +++ b/drivers/gpu/drm/drm_file.c
> > > @@ -282,9 +282,6 @@ void drm_file_free(struct drm_file *file)
> > >   	drm_legacy_ctxbitmap_flush(dev, file);
> > > -	if (drm_is_primary_client(file))
> > > -		drm_master_release(file);
> > > -
> > >   	if (dev->driver->postclose)
> > >   		dev->driver->postclose(dev, file);
> > > @@ -305,6 +302,9 @@ static void drm_close_helper(struct file *filp)
> > >   	list_del(&file_priv->lhead);
> > >   	mutex_unlock(&dev->filelist_mutex);
> > > +	if (drm_is_primary_client(file_priv))
> > > +		drm_master_release(file_priv);
> > > +
> > >   	drm_file_free(file_priv);
> > >   }
> > > -- 
> > > 2.25.1
> > > 
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v8 4/7] drm: avoid races with modesetting rights
  2021-08-26  2:01 ` [PATCH v8 4/7] drm: avoid races with modesetting rights Desmond Cheong Zhi Xi
@ 2021-08-26 12:59   ` Daniel Vetter
  2021-08-30 21:36     ` Desmond Cheong Zhi Xi
  0 siblings, 1 reply; 20+ messages in thread
From: Daniel Vetter @ 2021-08-26 12:59 UTC (permalink / raw)
  To: Desmond Cheong Zhi Xi
  Cc: maarten.lankhorst, mripard, tzimmermann, airlied, daniel,
	sumit.semwal, christian.koenig, jani.nikula, joonas.lahtinen,
	rodrigo.vivi, chris, ville.syrjala, matthew.auld, dan.carpenter,
	tvrtko.ursulin, matthew.d.roper, lucas.demarchi, karthik.b.s,
	jose.souza, manasi.d.navare, airlied, aditya.swarup, andrescj,
	linux-graphics-maintainer, zackr, dri-devel, linux-kernel,
	intel-gfx, linux-media, linaro-mm-sig, skhan, gregkh,
	linux-kernel-mentees, Daniel Vetter

On Thu, Aug 26, 2021 at 10:01:19AM +0800, Desmond Cheong Zhi Xi wrote:
> In drm_client_modeset.c and drm_fb_helper.c,
> drm_master_internal_{acquire,release} are used to avoid races with DRM
> userspace. These functions hold onto drm_device.master_rwsem while
> committing, and bail if there's already a master.
> 
> However, there are other places where modesetting rights can race. A
> time-of-check-to-time-of-use error can occur if an ioctl that changes
> the modeset has its rights revoked after it validates its permissions,
> but before it completes.
> 
> There are four places where modesetting permissions can change:
> 
> - DROP_MASTER ioctl removes rights for a master and its leases
> 
> - REVOKE_LEASE ioctl revokes rights for a specific lease
> 
> - SET_MASTER ioctl sets the device master if the master role hasn't
> been acquired yet
> 
> - drm_open which can create a new master for a device if one does not
> currently exist
> 
> These races can be avoided using drm_device.master_rwsem: users that
> perform modesetting should hold a read lock on the new
> drm_device.master_rwsem, and users that change these permissions
> should hold a write lock.
> 
> To avoid deadlocks with master_rwsem, for ioctls that need to check
> for modesetting permissions, but also need to hold a write lock on
> master_rwsem to protect some other attribute (or recurses to some
> function that holds a write lock, like drm_mode_create_lease_ioctl
> which eventually calls drm_master_open), we remove the DRM_MASTER flag
> and push the master_rwsem lock and permissions check into the ioctl.
> 
> Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
> ---
>  drivers/gpu/drm/drm_auth.c  |  4 ++++
>  drivers/gpu/drm/drm_ioctl.c | 20 +++++++++++++++-----
>  drivers/gpu/drm/drm_lease.c | 35 ++++++++++++++++++++++++-----------
>  include/drm/drm_device.h    |  5 +++++
>  4 files changed, 48 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index 73ade0513ccb..65065f7e1499 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -120,6 +120,10 @@ int drm_authmagic(struct drm_device *dev, void *data,
>  	DRM_DEBUG("%u\n", auth->magic);
>  
>  	down_write(&dev->master_rwsem);
> +	if (unlikely(!drm_is_current_master(file_priv))) {
> +		up_write(&dev->master_rwsem);
> +		return -EACCES;
> +	}
>  	file = idr_find(&file_priv->master->magic_map, auth->magic);
>  	if (file) {
>  		file->authenticated = 1;
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 158629d88319..8bea39ffc5c0 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -386,6 +386,10 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
>  	int if_version, retcode = 0;
>  
>  	down_write(&dev->master_rwsem);
> +	if (unlikely(!drm_is_current_master(file_priv))) {
> +		retcode = -EACCES;
> +		goto unlock;
> +	}
>  	if (sv->drm_di_major != -1) {
>  		if (sv->drm_di_major != DRM_IF_MAJOR ||
>  		    sv->drm_di_minor < 0 || sv->drm_di_minor > DRM_IF_MINOR) {
> @@ -420,8 +424,9 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
>  	sv->drm_di_minor = DRM_IF_MINOR;
>  	sv->drm_dd_major = dev->driver->major;
>  	sv->drm_dd_minor = dev->driver->minor;
> -	up_write(&dev->master_rwsem);
>  
> +unlock:
> +	up_write(&dev->master_rwsem);
>  	return retcode;
>  }
>  
> @@ -574,12 +579,12 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>  	DRM_IOCTL_DEF(DRM_IOCTL_GET_STATS, drm_getstats, 0),
>  	DRM_IOCTL_DEF(DRM_IOCTL_GET_CAP, drm_getcap, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF(DRM_IOCTL_SET_CLIENT_CAP, drm_setclientcap, 0),
> -	DRM_IOCTL_DEF(DRM_IOCTL_SET_VERSION, drm_setversion, DRM_MASTER),
> +	DRM_IOCTL_DEF(DRM_IOCTL_SET_VERSION, drm_setversion, 0),

Random bikeshed, if you're bored: In newer code we've given ioctl
callbacks an _ioctl suffix, so they'r easier to spot. Could do that in a
follow-up if you want.

>  
>  	DRM_IOCTL_DEF(DRM_IOCTL_SET_UNIQUE, drm_invalid_op, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
>  	DRM_IOCTL_DEF(DRM_IOCTL_BLOCK, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
>  	DRM_IOCTL_DEF(DRM_IOCTL_UNBLOCK, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
> -	DRM_IOCTL_DEF(DRM_IOCTL_AUTH_MAGIC, drm_authmagic, DRM_MASTER),
> +	DRM_IOCTL_DEF(DRM_IOCTL_AUTH_MAGIC, drm_authmagic, 0),
>  
>  	DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_ADD_MAP, drm_legacy_addmap_ioctl, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
>  	DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_RM_MAP, drm_legacy_rmmap_ioctl, DRM_AUTH),
> @@ -706,10 +711,10 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>  		      DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, 0),
>  	DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, 0),
> -	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_MASTER),
> +	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, 0),
>  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, DRM_MASTER),
>  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, DRM_MASTER),
> -	DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, DRM_MASTER),
> +	DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, 0),
>  };
>  
>  #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
> @@ -776,6 +781,9 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata,
>  	if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)) && !(flags & DRM_UNLOCKED))
>  		mutex_lock(&drm_global_mutex);
>  
> +	if (unlikely(flags & DRM_MASTER))
> +		down_read(&dev->master_rwsem);
> +
>  	retcode = drm_ioctl_permit(flags, file_priv);
>  	if (unlikely(retcode))
>  		goto out;
> @@ -783,6 +791,8 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata,
>  	retcode = func(dev, kdata, file_priv);
>  
>  out:
> +	if (unlikely(flags & DRM_MASTER))
> +		up_read(&dev->master_rwsem);
>  	if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)) && !(flags & DRM_UNLOCKED))
>  		mutex_unlock(&drm_global_mutex);
>  	return retcode;
> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> index dee4f24a1808..bed6f7636cbe 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -500,6 +500,18 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>  		return -EINVAL;
>  	}
>  
> +	/* Clone the lessor file to create a new file for us */
> +	DRM_DEBUG_LEASE("Allocating lease file\n");
> +	lessee_file = file_clone_open(lessor_file);
> +	if (IS_ERR(lessee_file))
> +		return PTR_ERR(lessee_file);
> +
> +	down_read(&dev->master_rwsem);
> +	if (unlikely(!drm_is_current_master(lessor_priv))) {
> +		ret = -EACCES;
> +		goto out_file;
> +	}
> +
>  	lessor = drm_file_get_master(lessor_priv);
>  	/* Do not allow sub-leases */
>  	if (lessor->lessor) {
> @@ -547,14 +559,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>  		goto out_leases;
>  	}
>  
> -	/* Clone the lessor file to create a new file for us */
> -	DRM_DEBUG_LEASE("Allocating lease file\n");
> -	lessee_file = file_clone_open(lessor_file);
> -	if (IS_ERR(lessee_file)) {
> -		ret = PTR_ERR(lessee_file);
> -		goto out_lessee;
> -	}
> -
>  	lessee_priv = lessee_file->private_data;
>  	/* Change the file to a master one */
>  	drm_master_put(&lessee_priv->master);
> @@ -571,17 +575,19 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>  	fd_install(fd, lessee_file);
>  
>  	drm_master_put(&lessor);

Hm if we're unlucky this might be the last reference (against nasty
userspace only), and that could then perhaps collide with us still holding
the lock. It looks like we're fine, the only thing that drm_master_put
might be locking is

	mutex_lock(&dev->mode_config.idr_mutex);

through drm_lease_destroy(). We don't care about legacy drivers and hence
the dev->struct_mutex (plus that lock is a complete mess anyway).

Maybe another patch to add a might_lock to drm_master_put, just to be
safe?

> +	up_read(&dev->master_rwsem);
>  	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n");
>  	return 0;
>  
> -out_lessee:
> -	drm_master_put(&lessee);
> -
>  out_leases:
>  	put_unused_fd(fd);
>  
>  out_lessor:
>  	drm_master_put(&lessor);
> +
> +out_file:
> +	up_read(&dev->master_rwsem);
> +	fput(lessee_file);
>  	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl failed: %d\n", ret);
>  	return ret;
>  }
> @@ -705,6 +711,11 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EOPNOTSUPP;
>  
> +	down_write(&dev->master_rwsem);
> +	if (unlikely(!drm_is_current_master(lessor_priv))) {
> +		ret = -EACCES;
> +		goto unlock;
> +	}
>  	lessor = drm_file_get_master(lessor_priv);
>  	mutex_lock(&dev->mode_config.idr_mutex);
>  
> @@ -728,5 +739,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
>  	mutex_unlock(&dev->mode_config.idr_mutex);
>  	drm_master_put(&lessor);
>  
> +unlock:
> +	up_write(&dev->master_rwsem);
>  	return ret;
>  }
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index 142fb2f6e74d..7d32bb69e6db 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -151,6 +151,11 @@ struct drm_device {
>  	 * Lock for &drm_device.master, &drm_file.was_master,
>  	 * &drm_file.is_master, &drm_file.master, &drm_master.unique,
>  	 * &drm_master.unique_len, and &drm_master.magic_map.
> +	 *
> +	 * Additionally, synchronizes modesetting rights between multiple users.

Current modern drives only use this for modesetting rights, but this is
about anything which is exclusively owned by the drm_master. So maybe
reword to "synchronizes access rights to exclusive resources like
modesetting access".

With that clarified:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

The other suggestions are for follow-up patches, if you feel like.
-Daniel

> +	 * Users that can change the modeset or display state must hold a read
> +	 * lock on @master_rwsem, and users that change modesetting rights
> +	 * should hold a write lock.
>  	 */
>  	struct rw_semaphore master_rwsem;
>  
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v8 5/7] drm: avoid circular locks in drm_mode_object_find
  2021-08-26  2:01 ` [PATCH v8 5/7] drm: avoid circular locks in drm_mode_object_find Desmond Cheong Zhi Xi
@ 2021-08-26 13:13   ` Daniel Vetter
  0 siblings, 0 replies; 20+ messages in thread
From: Daniel Vetter @ 2021-08-26 13:13 UTC (permalink / raw)
  To: Desmond Cheong Zhi Xi
  Cc: maarten.lankhorst, mripard, tzimmermann, airlied, daniel,
	sumit.semwal, christian.koenig, jani.nikula, joonas.lahtinen,
	rodrigo.vivi, chris, ville.syrjala, matthew.auld, dan.carpenter,
	tvrtko.ursulin, matthew.d.roper, lucas.demarchi, karthik.b.s,
	jose.souza, manasi.d.navare, airlied, aditya.swarup, andrescj,
	linux-graphics-maintainer, zackr, dri-devel, linux-kernel,
	intel-gfx, linux-media, linaro-mm-sig, skhan, gregkh,
	linux-kernel-mentees

On Thu, Aug 26, 2021 at 10:01:20AM +0800, Desmond Cheong Zhi Xi wrote:
> __drm_mode_object_find checks if the given drm file holds the required
> lease on a object by calling _drm_lease_held. _drm_lease_held in turn
> uses drm_file_get_master to access drm_file.master.
> 
> However, in a future patch, the drm_file.master_lookup_lock in
> drm_file_get_master will be replaced by drm_device.master_rwsem. This
> is an issue for two reasons:
> 
> 1. master_rwsem is sometimes already held when __drm_mode_object_find
> is called, which leads to recursive locks on master_rwsem
> 
> 2. drm_mode_object_find is sometimes called with the modeset_mutex
> held, which leads to an inversion of the master_rwsem -->
> modeset_mutex lock hierarchy
> 
> To fix this, we make __drm_mode_object_find the locked version of
> drm_mode_object_find, and wrap calls to __drm_mode_object_find with
> locks on master_rwsem. This allows us to safely access drm_file.master
> in _drm_lease_held (__drm_mode_object_find is its only caller) without
> the use of drm_file_get_master.
> 
> Functions that already lock master_rwsem are modified to call
> __drm_mode_object_find, whereas functions that haven't locked
> master_rwsem should call drm_mode_object_find. These two options
> allow us to grab master_rwsem before modeset_mutex (such as in
> drm_mode_get_obj_get_properties_ioctl).
> 
> This new rule requires more extensive changes to three functions:
> drn_connector_lookup, drm_crtc_find, and drm_plane_find. These
> functions are only sometimes called with master_rwsem held. Hence, we
> have to further split them into locked and unlocked versions that call
> __drm_mode_object_find and drm_mode_object_find respectively.

I think approach looks good, but the naming isn't so great. Usually __
prefix means "do not call directly, this is only exported for static
inline and other helpers". For these the usual rule is to add a _locked or
_unlocked suffix. I'd leave the normal _find functions as-is (since those
take the lock) themselves, and annotate the _locked ones.

Also same for the other lookup helpers.

> 
> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c            |  7 ++---
>  drivers/gpu/drm/drm_color_mgmt.c             |  2 +-
>  drivers/gpu/drm/drm_crtc.c                   |  5 ++--
>  drivers/gpu/drm/drm_framebuffer.c            |  2 +-
>  drivers/gpu/drm/drm_lease.c                  | 21 +++++----------
>  drivers/gpu/drm/drm_mode_object.c            | 28 +++++++++++++++++---
>  drivers/gpu/drm/drm_plane.c                  |  8 +++---
>  drivers/gpu/drm/drm_property.c               |  6 ++---
>  drivers/gpu/drm/i915/display/intel_overlay.c |  2 +-
>  drivers/gpu/drm/i915/display/intel_sprite.c  |  2 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c          |  2 +-
>  include/drm/drm_connector.h                  | 23 ++++++++++++++++
>  include/drm/drm_crtc.h                       | 22 +++++++++++++++
>  include/drm/drm_mode_object.h                |  3 +++
>  include/drm/drm_plane.h                      | 20 ++++++++++++++
>  15 files changed, 118 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 909f31833181..cda9a501cf74 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -557,7 +557,7 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>  			return -EINVAL;
>  
>  	} else if (property == config->prop_crtc_id) {
> -		struct drm_crtc *crtc = drm_crtc_find(dev, file_priv, val);
> +		struct drm_crtc *crtc = __drm_crtc_find(dev, file_priv, val);
>  
>  		if (val && !crtc)
>  			return -EACCES;
> @@ -709,7 +709,7 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>  	int ret;
>  
>  	if (property == config->prop_crtc_id) {
> -		struct drm_crtc *crtc = drm_crtc_find(dev, file_priv, val);
> +		struct drm_crtc *crtc = __drm_crtc_find(dev, file_priv, val);
>  
>  		if (val && !crtc)
>  			return -EACCES;
> @@ -1385,7 +1385,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  			goto out;
>  		}
>  
> -		obj = drm_mode_object_find(dev, file_priv, obj_id, DRM_MODE_OBJECT_ANY);
> +		obj = __drm_mode_object_find(dev, file_priv, obj_id,
> +					     DRM_MODE_OBJECT_ANY);
>  		if (!obj) {
>  			ret = -ENOENT;
>  			goto out;
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index bb14f488c8f6..9dcb2ccca3ab 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -365,7 +365,7 @@ int drm_mode_gamma_set_ioctl(struct drm_device *dev,
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EOPNOTSUPP;
>  
> -	crtc = drm_crtc_find(dev, file_priv, crtc_lut->crtc_id);
> +	crtc = __drm_crtc_find(dev, file_priv, crtc_lut->crtc_id);
>  	if (!crtc)
>  		return -ENOENT;
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 26a77a735905..b1279bb3fa61 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -656,7 +656,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  	if (crtc_req->x & 0xffff0000 || crtc_req->y & 0xffff0000)
>  		return -ERANGE;
>  
> -	crtc = drm_crtc_find(dev, file_priv, crtc_req->crtc_id);
> +	crtc = __drm_crtc_find(dev, file_priv, crtc_req->crtc_id);
>  	if (!crtc) {
>  		DRM_DEBUG_KMS("Unknown CRTC ID %d\n", crtc_req->crtc_id);
>  		return -ENOENT;
> @@ -787,7 +787,8 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  				goto out;
>  			}
>  
> -			connector = drm_connector_lookup(dev, file_priv, out_id);
> +			connector = __drm_connector_lookup(dev, file_priv,
> +							   out_id);
>  			if (!connector) {
>  				DRM_DEBUG_KMS("Connector id %d unknown\n",
>  						out_id);
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 07f5abc875e9..9c1db91b150d 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -887,7 +887,7 @@ struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev,
>  	struct drm_mode_object *obj;
>  	struct drm_framebuffer *fb = NULL;
>  
> -	obj = __drm_mode_object_find(dev, file_priv, id, DRM_MODE_OBJECT_FB);
> +	obj = drm_mode_object_find(dev, file_priv, id, DRM_MODE_OBJECT_FB);

I expect this to go boom, also for property and blob objects. These lookup
functions can be called from the atomic_ioctl machinery, where we're
already holding all kinds of locks. So grabbing the master_rwsem is not a
good idea.

We should always use the the drm_mode_object_find_unlocked for anything
object which is not in the list of drm_mode_object_lease_required().

>  	if (obj)
>  		fb = obj_to_fb(obj);
>  	return fb;
> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> index bed6f7636cbe..1b156c85d1c8 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -105,22 +105,13 @@ static bool _drm_has_leased(struct drm_master *master, int id)
>  	return false;
>  }
>  
> -/* Called with idr_mutex held */
> +/* Called with idr_mutex and master_rwsem held */

Please verify this with lockdep_assert_held.

The reason is that our locking gets rather funny here, because only for
some types of objects do we need to check master status.
drm_mode_object_lease_required() decides that.

>  bool _drm_lease_held(struct drm_file *file_priv, int id)
>  {
> -	bool ret;
> -	struct drm_master *master;
> -
> -	if (!file_priv)
> +	if (!file_priv || !file_priv->master)
>  		return true;
>  
> -	master = drm_file_get_master(file_priv);
> -	if (!master)
> -		return true;
> -	ret = _drm_lease_held_master(master, id);
> -	drm_master_put(&master);
> -
> -	return ret;
> +	return _drm_lease_held_master(file_priv->master, id);
>  }
>  
>  bool drm_lease_held(struct drm_file *file_priv, int id)
> @@ -391,9 +382,9 @@ static int fill_object_idr(struct drm_device *dev,
>  	/* step one - get references to all the mode objects
>  	   and check for validity. */
>  	for (o = 0; o < object_count; o++) {
> -		objects[o] = drm_mode_object_find(dev, lessor_priv,
> -						  object_ids[o],
> -						  DRM_MODE_OBJECT_ANY);
> +		objects[o] = __drm_mode_object_find(dev, lessor_priv,
> +						    object_ids[o],
> +						    DRM_MODE_OBJECT_ANY);
>  		if (!objects[o]) {
>  			ret = -ENOENT;
>  			goto out_free_objects;
> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> index 86d9e907c0b2..90c23997aa53 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -133,12 +133,27 @@ bool drm_mode_object_lease_required(uint32_t type)
>  	}
>  }
>  
> +/**
> + * __drm_mode_object_find - look up a drm object with static lifetime
> + * @dev: drm device
> + * @file_priv: drm file
> + * @id: id of the mode object
> + * @type: type of the mode object
> + *
> + * This function is used to look up a modeset object. It will acquire a
> + * reference for reference counted objects. This reference must be dropped
> + * again by calling drm_mode_object_put().
> + *
> + * Similar to drm_mode_object_find(), but called with &drm_device.master_rwsem
> + * held.
> + */
>  struct drm_mode_object *__drm_mode_object_find(struct drm_device *dev,
>  					       struct drm_file *file_priv,
> -					       uint32_t id, uint32_t type)
> +					       u32 id, u32 type)
>  {
>  	struct drm_mode_object *obj = NULL;
>  
> +	lockdep_assert_held_once(&dev->master_rwsem);
>  	mutex_lock(&dev->mode_config.idr_mutex);
>  	obj = idr_find(&dev->mode_config.object_idr, id);
>  	if (obj && type != DRM_MODE_OBJECT_ANY && obj->type != type)
> @@ -158,6 +173,7 @@ struct drm_mode_object *__drm_mode_object_find(struct drm_device *dev,
>  
>  	return obj;
>  }
> +EXPORT_SYMBOL(__drm_mode_object_find);
>  
>  /**
>   * drm_mode_object_find - look up a drm object with static lifetime
> @@ -176,7 +192,9 @@ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
>  {
>  	struct drm_mode_object *obj = NULL;
>  
> +	down_read(&dev->master_rwsem);
>  	obj = __drm_mode_object_find(dev, file_priv, id, type);
> +	up_read(&dev->master_rwsem);
>  	return obj;
>  }
>  EXPORT_SYMBOL(drm_mode_object_find);
> @@ -408,9 +426,12 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EOPNOTSUPP;
>  
> +	down_read(&dev->master_rwsem);
>  	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>  
> -	obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
> +	obj = __drm_mode_object_find(dev, file_priv, arg->obj_id,
> +				     arg->obj_type);
> +	up_read(&dev->master_rwsem);
>  	if (!obj) {
>  		ret = -ENOENT;
>  		goto out;
> @@ -534,7 +555,8 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EOPNOTSUPP;
>  
> -	arg_obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
> +	arg_obj = __drm_mode_object_find(dev, file_priv, arg->obj_id,
> +					 arg->obj_type);
>  	if (!arg_obj)
>  		return -ENOENT;
>  
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 82afb854141b..b5566167a798 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -971,7 +971,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  	 * First, find the plane, crtc, and fb objects.  If not available,
>  	 * we don't bother to call the driver.
>  	 */
> -	plane = drm_plane_find(dev, file_priv, plane_req->plane_id);
> +	plane = __drm_plane_find(dev, file_priv, plane_req->plane_id);
>  	if (!plane) {
>  		DRM_DEBUG_KMS("Unknown plane ID %d\n",
>  			      plane_req->plane_id);
> @@ -986,7 +986,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  			return -ENOENT;
>  		}
>  
> -		crtc = drm_crtc_find(dev, file_priv, plane_req->crtc_id);
> +		crtc = __drm_crtc_find(dev, file_priv, plane_req->crtc_id);
>  		if (!crtc) {
>  			drm_framebuffer_put(fb);
>  			DRM_DEBUG_KMS("Unknown crtc ID %d\n",
> @@ -1108,7 +1108,7 @@ static int drm_mode_cursor_common(struct drm_device *dev,
>  	if (!req->flags || (~DRM_MODE_CURSOR_FLAGS & req->flags))
>  		return -EINVAL;
>  
> -	crtc = drm_crtc_find(dev, file_priv, req->crtc_id);
> +	crtc = __drm_crtc_find(dev, file_priv, req->crtc_id);
>  	if (!crtc) {
>  		DRM_DEBUG_KMS("Unknown CRTC ID %d\n", req->crtc_id);
>  		return -ENOENT;
> @@ -1229,7 +1229,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	if ((page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC) && !dev->mode_config.async_page_flip)
>  		return -EINVAL;
>  
> -	crtc = drm_crtc_find(dev, file_priv, page_flip->crtc_id);
> +	crtc = __drm_crtc_find(dev, file_priv, page_flip->crtc_id);
>  	if (!crtc)
>  		return -ENOENT;
>  
> diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
> index 6c353c9dc772..9f04dcb81d07 100644
> --- a/drivers/gpu/drm/drm_property.c
> +++ b/drivers/gpu/drm/drm_property.c
> @@ -656,7 +656,7 @@ struct drm_property_blob *drm_property_lookup_blob(struct drm_device *dev,
>  	struct drm_mode_object *obj;
>  	struct drm_property_blob *blob = NULL;
>  
> -	obj = __drm_mode_object_find(dev, NULL, id, DRM_MODE_OBJECT_BLOB);
> +	obj = drm_mode_object_find(dev, NULL, id, DRM_MODE_OBJECT_BLOB);
>  	if (obj)
>  		blob = obj_to_blob(obj);
>  	return blob;
> @@ -919,8 +919,8 @@ bool drm_property_change_valid_get(struct drm_property *property,
>  		if (value == 0)
>  			return true;
>  
> -		*ref = __drm_mode_object_find(property->dev, NULL, value,
> -					      property->values[0]);
> +		*ref = drm_mode_object_find(property->dev, NULL, value,
> +					    property->values[0]);
>  		return *ref != NULL;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c
> index 7e3f5c6ca484..1d4af29e31c9 100644
> --- a/drivers/gpu/drm/i915/display/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/display/intel_overlay.c
> @@ -1120,7 +1120,7 @@ int intel_overlay_put_image_ioctl(struct drm_device *dev, void *data,
>  		return ret;
>  	}
>  
> -	drmmode_crtc = drm_crtc_find(dev, file_priv, params->crtc_id);
> +	drmmode_crtc = __drm_crtc_find(dev, file_priv, params->crtc_id);
>  	if (!drmmode_crtc)
>  		return -ENOENT;
>  	crtc = to_intel_crtc(drmmode_crtc);
> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> index 4ae9a7455b23..e19ef2d90bac 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -1505,7 +1505,7 @@ int intel_sprite_set_colorkey_ioctl(struct drm_device *dev, void *data,
>  	    set->flags & I915_SET_COLORKEY_DESTINATION)
>  		return -EINVAL;
>  
> -	plane = drm_plane_find(dev, file_priv, set->plane_id);
> +	plane = __drm_plane_find(dev, file_priv, set->plane_id);
>  	if (!plane || plane->type != DRM_PLANE_TYPE_OVERLAY)
>  		return -ENOENT;
>  
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 74fa41909213..d368b2bcb1fa 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -1862,7 +1862,7 @@ int vmw_kms_cursor_bypass_ioctl(struct drm_device *dev, void *data,
>  		return 0;
>  	}
>  
> -	crtc = drm_crtc_find(dev, file_priv, arg->crtc_id);
> +	crtc = __drm_crtc_find(dev, file_priv, arg->crtc_id);
>  	if (!crtc) {
>  		ret = -ENOENT;
>  		goto out;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 1647960c9e50..322c823583c7 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1591,6 +1591,29 @@ static inline u32 drm_connector_mask(const struct drm_connector *connector)
>  	return 1 << connector->index;
>  }
>  
> +/**
> + * __drm_connector_lookup - lookup connector object
> + * @dev: DRM device
> + * @file_priv: drm file to check for lease against.
> + * @id: connector object id
> + *
> + * This function looks up the connector object specified by id
> + * add takes a reference to it.
> + *
> + * Similar to drm_connector_lookup(), but called with &drm_device.master_rwsem
> + * held.
> + */
> +static inline struct drm_connector *__drm_connector_lookup(struct drm_device *dev,
> +							   struct drm_file *file_priv,
> +							   uint32_t id)
> +{
> +	struct drm_mode_object *mo;
> +
> +	mo = __drm_mode_object_find(dev, file_priv, id,
> +				    DRM_MODE_OBJECT_CONNECTOR);
> +	return mo ? obj_to_connector(mo) : NULL;
> +}
> +
>  /**
>   * drm_connector_lookup - lookup connector object
>   * @dev: DRM device
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 13eeba2a750a..69df854dd322 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1283,6 +1283,28 @@ static inline uint32_t drm_crtc_mask(const struct drm_crtc *crtc)
>  int drm_mode_set_config_internal(struct drm_mode_set *set);
>  struct drm_crtc *drm_crtc_from_index(struct drm_device *dev, int idx);
>  
> +/**
> + * __drm_crtc_find - look up a CRTC object from its ID
> + * @dev: DRM device
> + * @file_priv: drm file to check for lease against.
> + * @id: &drm_mode_object ID
> + *
> + * This can be used to look up a CRTC from its userspace ID. Only used by
> + * drivers for legacy IOCTLs and interface, nowadays extensions to the KMS
> + * userspace interface should be done using &drm_property.
> + *
> + * Similar to drm_crtc_find(), but called with &drm_device.master_rwsem held.
> + */
> +static inline struct drm_crtc *__drm_crtc_find(struct drm_device *dev,
> +					       struct drm_file *file_priv,
> +					       uint32_t id)
> +{
> +	struct drm_mode_object *mo;
> +
> +	mo = __drm_mode_object_find(dev, file_priv, id, DRM_MODE_OBJECT_CRTC);
> +	return mo ? obj_to_crtc(mo) : NULL;
> +}
> +
>  /**
>   * drm_crtc_find - look up a CRTC object from its ID
>   * @dev: DRM device
> diff --git a/include/drm/drm_mode_object.h b/include/drm/drm_mode_object.h
> index c34a3e8030e1..1906af9cae9b 100644
> --- a/include/drm/drm_mode_object.h
> +++ b/include/drm/drm_mode_object.h
> @@ -114,6 +114,9 @@ struct drm_object_properties {
>  		return "(unknown)";				\
>  	}
>  
> +struct drm_mode_object *__drm_mode_object_find(struct drm_device *dev,
> +					       struct drm_file *file_priv,
> +					       u32 id, u32 type);
>  struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
>  					     struct drm_file *file_priv,
>  					     uint32_t id, uint32_t type);
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index fed97e35626f..49e35d3724c9 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -842,6 +842,26 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
>  				       struct drm_property *property,
>  				       uint64_t value);
>  
> +/**
> + * __drm_plane_find - find a &drm_plane
> + * @dev: DRM device
> + * @file_priv: drm file to check for lease against.
> + * @id: plane id
> + *
> + * Returns the plane with @id, NULL if it doesn't exist.
> + *
> + * Similar to drm_plane_find(), but called with &drm_device.master_rwsem held.
> + */
> +static inline struct drm_plane *__drm_plane_find(struct drm_device *dev,
> +						 struct drm_file *file_priv,
> +						 uint32_t id)
> +{
> +	struct drm_mode_object *mo;
> +
> +	mo = __drm_mode_object_find(dev, file_priv, id, DRM_MODE_OBJECT_PLANE);
> +	return mo ? obj_to_plane(mo) : NULL;
> +}
> +
>  /**
>   * drm_plane_find - find a &drm_plane
>   * @dev: DRM device

Aside from the one area for mode objects that cannoted be leased I think
this looks correct.

Sinc the spinlock+rwsem unification is a bit tricky, maybe you want to
split out the patch series so that we can land the initial patches
already?
-Daniel

> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v8 7/7] drm: remove drm_file.master_lookup_lock
  2021-08-26  2:01 ` [PATCH v8 7/7] drm: remove drm_file.master_lookup_lock Desmond Cheong Zhi Xi
@ 2021-08-26 13:21   ` Daniel Vetter
  2021-08-31  6:02     ` Desmond Cheong Zhi Xi
  0 siblings, 1 reply; 20+ messages in thread
From: Daniel Vetter @ 2021-08-26 13:21 UTC (permalink / raw)
  To: Desmond Cheong Zhi Xi
  Cc: maarten.lankhorst, mripard, tzimmermann, airlied, daniel,
	sumit.semwal, christian.koenig, jani.nikula, joonas.lahtinen,
	rodrigo.vivi, chris, ville.syrjala, matthew.auld, dan.carpenter,
	tvrtko.ursulin, matthew.d.roper, lucas.demarchi, karthik.b.s,
	jose.souza, manasi.d.navare, airlied, aditya.swarup, andrescj,
	linux-graphics-maintainer, zackr, dri-devel, linux-kernel,
	intel-gfx, linux-media, linaro-mm-sig, skhan, gregkh,
	linux-kernel-mentees

On Thu, Aug 26, 2021 at 10:01:22AM +0800, Desmond Cheong Zhi Xi wrote:
> Previously, master_lookup_lock was introduced in
> commit 0b0860a3cf5e ("drm: serialize drm_file.master with a new
> spinlock") to serialize accesses to drm_file.master. This then allowed
> us to write drm_file_get_master in commit 56f0729a510f ("drm: protect
> drm_master pointers in drm_lease.c").
> 
> The rationale behind introducing a new spinlock at the time was that
> the other lock that could have been used (drm_device.master_mutex) was
> the outermost lock, so embedding calls to drm_file_get_master and
> drm_is_current_master in various functions easily caused us to invert
> the lock hierarchy.
> 
> Following the conversion of master_mutex into a rwsem, and its use to
> plug races with modesetting rights, we've untangled some lock
> hierarchies and removed the need for using drm_file_get_master and the
> unlocked version of drm_is_current_master in multiple places.
> 
> Hence, we can take this opportunity to clean up the locking design by
> replacing master_lookup_lock with drm_device.master_rwsem.
> 
> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
> ---
>  drivers/gpu/drm/drm_auth.c     | 19 +++++++------------
>  drivers/gpu/drm/drm_file.c     |  1 -
>  drivers/gpu/drm/drm_internal.h |  1 +
>  drivers/gpu/drm/drm_ioctl.c    |  4 ++--
>  drivers/gpu/drm/drm_lease.c    | 18 ++++++++----------
>  include/drm/drm_file.h         |  9 +--------
>  6 files changed, 19 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index f2b2f197052a..232416119407 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -61,10 +61,9 @@
>   * trusted clients.
>   */
>  
> -static bool drm_is_current_master_locked(struct drm_file *fpriv)
> +bool drm_is_current_master_locked(struct drm_file *fpriv)
>  {
> -	lockdep_assert_once(lockdep_is_held(&fpriv->master_lookup_lock) ||
> -			    lockdep_is_held(&fpriv->minor->dev->master_rwsem));
> +	lockdep_assert_held_once(&fpriv->minor->dev->master_rwsem);
>  
>  	return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master;
>  }
> @@ -83,9 +82,9 @@ bool drm_is_current_master(struct drm_file *fpriv)
>  {
>  	bool ret;
>  
> -	spin_lock(&fpriv->master_lookup_lock);
> +	down_read(&fpriv->minor->dev->master_rwsem);

Looking at the 3 patches and the need to have a locked version of pretty
much everything I'm wondering: Can't we just drop the spinlock completely,
and everywhere we've taking it thus far replace it with a
lockdep_assert_held_once?

The thing is, if there's any path left that doesn't hold the rwsem in at
least read mode we have a bug. And the right way to fix such a bug is to
grab the rwsem sufficiently high up in the callchain. That way I think we
should be able to avoid all these tedious changes to everything, including
touching i915 and vmwgfx drivers.

Or am I missing something big time?
-Daniel

>  	ret = drm_is_current_master_locked(fpriv);
> -	spin_unlock(&fpriv->master_lookup_lock);
> +	up_read(&fpriv->minor->dev->master_rwsem);
>  
>  	return ret;
>  }
> @@ -120,7 +119,7 @@ int drm_authmagic(struct drm_device *dev, void *data,
>  	DRM_DEBUG("%u\n", auth->magic);
>  
>  	down_write(&dev->master_rwsem);
> -	if (unlikely(!drm_is_current_master(file_priv))) {
> +	if (unlikely(!drm_is_current_master_locked(file_priv))) {
>  		up_write(&dev->master_rwsem);
>  		return -EACCES;
>  	}
> @@ -178,9 +177,7 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
>  	new_master = drm_master_create(dev);
>  	if (!new_master)
>  		return -ENOMEM;
> -	spin_lock(&fpriv->master_lookup_lock);
>  	fpriv->master = new_master;
> -	spin_unlock(&fpriv->master_lookup_lock);
>  
>  	fpriv->is_master = 1;
>  	fpriv->authenticated = 1;
> @@ -343,9 +340,7 @@ int drm_master_open(struct drm_file *file_priv)
>  	if (!dev->master) {
>  		ret = drm_new_set_master(dev, file_priv);
>  	} else {
> -		spin_lock(&file_priv->master_lookup_lock);
>  		file_priv->master = drm_master_get(dev->master);
> -		spin_unlock(&file_priv->master_lookup_lock);
>  	}
>  	up_write(&dev->master_rwsem);
>  
> @@ -413,13 +408,13 @@ struct drm_master *drm_file_get_master(struct drm_file *file_priv)
>  	if (!file_priv)
>  		return NULL;
>  
> -	spin_lock(&file_priv->master_lookup_lock);
> +	down_read(&file_priv->minor->dev->master_rwsem);
>  	if (!file_priv->master)
>  		goto unlock;
>  	master = drm_master_get(file_priv->master);
>  
>  unlock:
> -	spin_unlock(&file_priv->master_lookup_lock);
> +	up_read(&file_priv->minor->dev->master_rwsem);
>  	return master;
>  }
>  EXPORT_SYMBOL(drm_file_get_master);
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 90b62f360da1..8c846e0179d7 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -176,7 +176,6 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>  	init_waitqueue_head(&file->event_wait);
>  	file->event_space = 4096; /* set aside 4k for event buffer */
>  
> -	spin_lock_init(&file->master_lookup_lock);
>  	mutex_init(&file->event_read_lock);
>  
>  	if (drm_core_check_feature(dev, DRIVER_GEM))
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 17f3548c8ed2..5d421f749a17 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -132,6 +132,7 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
>  				  struct drm_file *filp);
>  
>  /* drm_auth.c */
> +bool drm_is_current_master_locked(struct drm_file *fpriv);
>  int drm_getmagic(struct drm_device *dev, void *data,
>  		 struct drm_file *file_priv);
>  int drm_authmagic(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 8bea39ffc5c0..c728437466c3 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -386,7 +386,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
>  	int if_version, retcode = 0;
>  
>  	down_write(&dev->master_rwsem);
> -	if (unlikely(!drm_is_current_master(file_priv))) {
> +	if (unlikely(!drm_is_current_master_locked(file_priv))) {
>  		retcode = -EACCES;
>  		goto unlock;
>  	}
> @@ -540,7 +540,7 @@ static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
>  
>  	/* MASTER is only for master or control clients */
>  	if (unlikely((flags & DRM_MASTER) &&
> -		     !drm_is_current_master(file_priv)))
> +		     !drm_is_current_master_locked(file_priv)))
>  		return -EACCES;
>  
>  	/* Render clients must be explicitly allowed */
> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> index 15bf3a3c76d1..0eecf320b1ab 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -498,12 +498,12 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>  		return PTR_ERR(lessee_file);
>  
>  	down_read(&dev->master_rwsem);
> -	if (unlikely(!drm_is_current_master(lessor_priv))) {
> +	if (unlikely(!drm_is_current_master_locked(lessor_priv))) {
>  		ret = -EACCES;
>  		goto out_file;
>  	}
>  
> -	lessor = drm_file_get_master(lessor_priv);
> +	lessor = lessor_priv->master;
>  	/* Do not allow sub-leases */
>  	if (lessor->lessor) {
>  		DRM_DEBUG_LEASE("recursive leasing not allowed\n");
> @@ -565,7 +565,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>  	/* Hook up the fd */
>  	fd_install(fd, lessee_file);
>  
> -	drm_master_put(&lessor);
>  	up_read(&dev->master_rwsem);
>  	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n");
>  	return 0;
> @@ -600,7 +599,8 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EOPNOTSUPP;
>  
> -	lessor = drm_file_get_master(lessor_priv);
> +	lockdep_assert_held_once(&dev->master_rwsem);
> +	lessor = lessor_priv->master;
>  	DRM_DEBUG_LEASE("List lessees for %d\n", lessor->lessee_id);
>  
>  	mutex_lock(&dev->mode_config.idr_mutex);
> @@ -624,7 +624,6 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
>  		arg->count_lessees = count;
>  
>  	mutex_unlock(&dev->mode_config.idr_mutex);
> -	drm_master_put(&lessor);
>  
>  	return ret;
>  }
> @@ -650,7 +649,8 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EOPNOTSUPP;
>  
> -	lessee = drm_file_get_master(lessee_priv);
> +	lockdep_assert_held_once(&dev->master_rwsem);
> +	lessee = lessee_priv->master;
>  	DRM_DEBUG_LEASE("get lease for %d\n", lessee->lessee_id);
>  
>  	mutex_lock(&dev->mode_config.idr_mutex);
> @@ -678,7 +678,6 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
>  		arg->count_objects = count;
>  
>  	mutex_unlock(&dev->mode_config.idr_mutex);
> -	drm_master_put(&lessee);
>  
>  	return ret;
>  }
> @@ -703,11 +702,11 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
>  		return -EOPNOTSUPP;
>  
>  	down_write(&dev->master_rwsem);
> -	if (unlikely(!drm_is_current_master(lessor_priv))) {
> +	if (unlikely(!drm_is_current_master_locked(lessor_priv))) {
>  		ret = -EACCES;
>  		goto unlock;
>  	}
> -	lessor = drm_file_get_master(lessor_priv);
> +	lessor = lessor_priv->master;
>  	mutex_lock(&dev->mode_config.idr_mutex);
>  
>  	lessee = _drm_find_lessee(lessor, arg->lessee_id);
> @@ -728,7 +727,6 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
>  
>  fail:
>  	mutex_unlock(&dev->mode_config.idr_mutex);
> -	drm_master_put(&lessor);
>  
>  unlock:
>  	up_write(&dev->master_rwsem);
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index d12bb2ba7814..e2d49fe3e32d 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -227,16 +227,12 @@ struct drm_file {
>  	 * @master:
>  	 *
>  	 * Master this node is currently associated with. Protected by struct
> -	 * &drm_device.master_rwsem, and serialized by @master_lookup_lock.
> +	 * &drm_device.master_rwsem.
>  	 *
>  	 * Only relevant if drm_is_primary_client() returns true. Note that
>  	 * this only matches &drm_device.master if the master is the currently
>  	 * active one.
>  	 *
> -	 * To update @master, both &drm_device.master_rwsem and
> -	 * @master_lookup_lock need to be held, therefore holding either of
> -	 * them is safe and enough for the read side.
> -	 *
>  	 * When dereferencing this pointer, either hold struct
>  	 * &drm_device.master_rwsem for the duration of the pointer's use, or
>  	 * use drm_file_get_master() if struct &drm_device.master_rwsem is not
> @@ -248,9 +244,6 @@ struct drm_file {
>  	 */
>  	struct drm_master *master;
>  
> -	/** @master_lock: Serializes @master. */
> -	spinlock_t master_lookup_lock;
> -
>  	/** @pid: Process that opened this file. */
>  	struct pid *pid;
>  
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v8 3/7] drm: lock drm_global_mutex earlier in the ioctl handler
  2021-08-26  9:58   ` Daniel Vetter
@ 2021-08-30 21:04     ` Desmond Cheong Zhi Xi
  0 siblings, 0 replies; 20+ messages in thread
From: Desmond Cheong Zhi Xi @ 2021-08-30 21:04 UTC (permalink / raw)
  To: maarten.lankhorst, mripard, tzimmermann, airlied, sumit.semwal,
	christian.koenig, jani.nikula, joonas.lahtinen, rodrigo.vivi,
	chris, ville.syrjala, matthew.auld, dan.carpenter,
	tvrtko.ursulin, matthew.d.roper, lucas.demarchi, karthik.b.s,
	jose.souza, manasi.d.navare, airlied, aditya.swarup, andrescj,
	linux-graphics-maintainer, zackr, dri-devel, linux-kernel,
	intel-gfx, linux-media, linaro-mm-sig, skhan, gregkh,
	linux-kernel-mentees, Daniel Vetter

On 26/8/21 5:58 pm, Daniel Vetter wrote:
> On Thu, Aug 26, 2021 at 10:01:18AM +0800, Desmond Cheong Zhi Xi wrote:
>> In a future patch, a read lock on drm_device.master_rwsem is
>> held in the ioctl handler before the check for ioctl
>> permissions. However, this inverts the lock hierarchy of
>> drm_global_mutex --> master_rwsem.
>>
>> To avoid this, we do some prep work to grab the drm_global_mutex
>> before checking for ioctl permissions.
>>
>> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
>> ---
>>   drivers/gpu/drm/drm_ioctl.c | 18 +++++++++---------
>>   1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>> index d25713b09b80..158629d88319 100644
>> --- a/drivers/gpu/drm/drm_ioctl.c
>> +++ b/drivers/gpu/drm/drm_ioctl.c
>> @@ -772,19 +772,19 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata,
>>   	if (drm_dev_is_unplugged(dev))
>>   		return -ENODEV;
>>   
>> +	/* Enforce sane locking for modern driver ioctls. */
>> +	if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)) && !(flags & DRM_UNLOCKED))
> 
> Maybe have a local bool locked_ioctl for this so it's extremely clear it's
> the same condition in both?
> 
> Either way: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 

Thanks for the suggestion and review. Sounds good, I'll update and send 
out a new version.

(Sorry for delays, been busy with moving)

>> +		mutex_lock(&drm_global_mutex);
>> +
>>   	retcode = drm_ioctl_permit(flags, file_priv);
>>   	if (unlikely(retcode))
>> -		return retcode;
>> +		goto out;
>>   
>> -	/* Enforce sane locking for modern driver ioctls. */
>> -	if (likely(!drm_core_check_feature(dev, DRIVER_LEGACY)) ||
>> -	    (flags & DRM_UNLOCKED))
>> -		retcode = func(dev, kdata, file_priv);
>> -	else {
>> -		mutex_lock(&drm_global_mutex);
>> -		retcode = func(dev, kdata, file_priv);
>> +	retcode = func(dev, kdata, file_priv);
>> +
>> +out:
>> +	if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)) && !(flags & DRM_UNLOCKED))
>>   		mutex_unlock(&drm_global_mutex);
>> -	}
>>   	return retcode;
>>   }
>>   EXPORT_SYMBOL(drm_ioctl_kernel);
>> -- 
>> 2.25.1
>>
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v8 4/7] drm: avoid races with modesetting rights
  2021-08-26 12:59   ` Daniel Vetter
@ 2021-08-30 21:36     ` Desmond Cheong Zhi Xi
  0 siblings, 0 replies; 20+ messages in thread
From: Desmond Cheong Zhi Xi @ 2021-08-30 21:36 UTC (permalink / raw)
  To: maarten.lankhorst, mripard, tzimmermann, airlied, sumit.semwal,
	christian.koenig, jani.nikula, joonas.lahtinen, rodrigo.vivi,
	chris, ville.syrjala, matthew.auld, dan.carpenter,
	tvrtko.ursulin, matthew.d.roper, lucas.demarchi, karthik.b.s,
	jose.souza, manasi.d.navare, airlied, aditya.swarup, andrescj,
	linux-graphics-maintainer, zackr, dri-devel, linux-kernel,
	intel-gfx, linux-media, linaro-mm-sig, skhan, gregkh,
	linux-kernel-mentees

On 26/8/21 8:59 pm, Daniel Vetter wrote:
> On Thu, Aug 26, 2021 at 10:01:19AM +0800, Desmond Cheong Zhi Xi wrote:
>> In drm_client_modeset.c and drm_fb_helper.c,
>> drm_master_internal_{acquire,release} are used to avoid races with DRM
>> userspace. These functions hold onto drm_device.master_rwsem while
>> committing, and bail if there's already a master.
>>
>> However, there are other places where modesetting rights can race. A
>> time-of-check-to-time-of-use error can occur if an ioctl that changes
>> the modeset has its rights revoked after it validates its permissions,
>> but before it completes.
>>
>> There are four places where modesetting permissions can change:
>>
>> - DROP_MASTER ioctl removes rights for a master and its leases
>>
>> - REVOKE_LEASE ioctl revokes rights for a specific lease
>>
>> - SET_MASTER ioctl sets the device master if the master role hasn't
>> been acquired yet
>>
>> - drm_open which can create a new master for a device if one does not
>> currently exist
>>
>> These races can be avoided using drm_device.master_rwsem: users that
>> perform modesetting should hold a read lock on the new
>> drm_device.master_rwsem, and users that change these permissions
>> should hold a write lock.
>>
>> To avoid deadlocks with master_rwsem, for ioctls that need to check
>> for modesetting permissions, but also need to hold a write lock on
>> master_rwsem to protect some other attribute (or recurses to some
>> function that holds a write lock, like drm_mode_create_lease_ioctl
>> which eventually calls drm_master_open), we remove the DRM_MASTER flag
>> and push the master_rwsem lock and permissions check into the ioctl.
>>
>> Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
>> ---
>>   drivers/gpu/drm/drm_auth.c  |  4 ++++
>>   drivers/gpu/drm/drm_ioctl.c | 20 +++++++++++++++-----
>>   drivers/gpu/drm/drm_lease.c | 35 ++++++++++++++++++++++++-----------
>>   include/drm/drm_device.h    |  5 +++++
>>   4 files changed, 48 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
>> index 73ade0513ccb..65065f7e1499 100644
>> --- a/drivers/gpu/drm/drm_auth.c
>> +++ b/drivers/gpu/drm/drm_auth.c
>> @@ -120,6 +120,10 @@ int drm_authmagic(struct drm_device *dev, void *data,
>>   	DRM_DEBUG("%u\n", auth->magic);
>>   
>>   	down_write(&dev->master_rwsem);
>> +	if (unlikely(!drm_is_current_master(file_priv))) {
>> +		up_write(&dev->master_rwsem);
>> +		return -EACCES;
>> +	}
>>   	file = idr_find(&file_priv->master->magic_map, auth->magic);
>>   	if (file) {
>>   		file->authenticated = 1;
>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>> index 158629d88319..8bea39ffc5c0 100644
>> --- a/drivers/gpu/drm/drm_ioctl.c
>> +++ b/drivers/gpu/drm/drm_ioctl.c
>> @@ -386,6 +386,10 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
>>   	int if_version, retcode = 0;
>>   
>>   	down_write(&dev->master_rwsem);
>> +	if (unlikely(!drm_is_current_master(file_priv))) {
>> +		retcode = -EACCES;
>> +		goto unlock;
>> +	}
>>   	if (sv->drm_di_major != -1) {
>>   		if (sv->drm_di_major != DRM_IF_MAJOR ||
>>   		    sv->drm_di_minor < 0 || sv->drm_di_minor > DRM_IF_MINOR) {
>> @@ -420,8 +424,9 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
>>   	sv->drm_di_minor = DRM_IF_MINOR;
>>   	sv->drm_dd_major = dev->driver->major;
>>   	sv->drm_dd_minor = dev->driver->minor;
>> -	up_write(&dev->master_rwsem);
>>   
>> +unlock:
>> +	up_write(&dev->master_rwsem);
>>   	return retcode;
>>   }
>>   
>> @@ -574,12 +579,12 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>>   	DRM_IOCTL_DEF(DRM_IOCTL_GET_STATS, drm_getstats, 0),
>>   	DRM_IOCTL_DEF(DRM_IOCTL_GET_CAP, drm_getcap, DRM_RENDER_ALLOW),
>>   	DRM_IOCTL_DEF(DRM_IOCTL_SET_CLIENT_CAP, drm_setclientcap, 0),
>> -	DRM_IOCTL_DEF(DRM_IOCTL_SET_VERSION, drm_setversion, DRM_MASTER),
>> +	DRM_IOCTL_DEF(DRM_IOCTL_SET_VERSION, drm_setversion, 0),
> 
> Random bikeshed, if you're bored: In newer code we've given ioctl
> callbacks an _ioctl suffix, so they'r easier to spot. Could do that in a
> follow-up if you want.
> 

Makes sense, this was a small pain point when auditing the ioctl 
functions. I'll do this on a rainier day.

>>   
>>   	DRM_IOCTL_DEF(DRM_IOCTL_SET_UNIQUE, drm_invalid_op, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
>>   	DRM_IOCTL_DEF(DRM_IOCTL_BLOCK, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
>>   	DRM_IOCTL_DEF(DRM_IOCTL_UNBLOCK, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
>> -	DRM_IOCTL_DEF(DRM_IOCTL_AUTH_MAGIC, drm_authmagic, DRM_MASTER),
>> +	DRM_IOCTL_DEF(DRM_IOCTL_AUTH_MAGIC, drm_authmagic, 0),
>>   
>>   	DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_ADD_MAP, drm_legacy_addmap_ioctl, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
>>   	DRM_LEGACY_IOCTL_DEF(DRM_IOCTL_RM_MAP, drm_legacy_rmmap_ioctl, DRM_AUTH),
>> @@ -706,10 +711,10 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>>   		      DRM_RENDER_ALLOW),
>>   	DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, 0),
>>   	DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, 0),
>> -	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_MASTER),
>> +	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, 0),
>>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, DRM_MASTER),
>>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, DRM_MASTER),
>> -	DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, DRM_MASTER),
>> +	DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, 0),
>>   };
>>   
>>   #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
>> @@ -776,6 +781,9 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata,
>>   	if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)) && !(flags & DRM_UNLOCKED))
>>   		mutex_lock(&drm_global_mutex);
>>   
>> +	if (unlikely(flags & DRM_MASTER))
>> +		down_read(&dev->master_rwsem);
>> +
>>   	retcode = drm_ioctl_permit(flags, file_priv);
>>   	if (unlikely(retcode))
>>   		goto out;
>> @@ -783,6 +791,8 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata,
>>   	retcode = func(dev, kdata, file_priv);
>>   
>>   out:
>> +	if (unlikely(flags & DRM_MASTER))
>> +		up_read(&dev->master_rwsem);
>>   	if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)) && !(flags & DRM_UNLOCKED))
>>   		mutex_unlock(&drm_global_mutex);
>>   	return retcode;
>> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
>> index dee4f24a1808..bed6f7636cbe 100644
>> --- a/drivers/gpu/drm/drm_lease.c
>> +++ b/drivers/gpu/drm/drm_lease.c
>> @@ -500,6 +500,18 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>>   		return -EINVAL;
>>   	}
>>   
>> +	/* Clone the lessor file to create a new file for us */
>> +	DRM_DEBUG_LEASE("Allocating lease file\n");
>> +	lessee_file = file_clone_open(lessor_file);
>> +	if (IS_ERR(lessee_file))
>> +		return PTR_ERR(lessee_file);
>> +
>> +	down_read(&dev->master_rwsem);
>> +	if (unlikely(!drm_is_current_master(lessor_priv))) {
>> +		ret = -EACCES;
>> +		goto out_file;
>> +	}
>> +
>>   	lessor = drm_file_get_master(lessor_priv);
>>   	/* Do not allow sub-leases */
>>   	if (lessor->lessor) {
>> @@ -547,14 +559,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>>   		goto out_leases;
>>   	}
>>   
>> -	/* Clone the lessor file to create a new file for us */
>> -	DRM_DEBUG_LEASE("Allocating lease file\n");
>> -	lessee_file = file_clone_open(lessor_file);
>> -	if (IS_ERR(lessee_file)) {
>> -		ret = PTR_ERR(lessee_file);
>> -		goto out_lessee;
>> -	}
>> -
>>   	lessee_priv = lessee_file->private_data;
>>   	/* Change the file to a master one */
>>   	drm_master_put(&lessee_priv->master);
>> @@ -571,17 +575,19 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>>   	fd_install(fd, lessee_file);
>>   
>>   	drm_master_put(&lessor);
> 
> Hm if we're unlucky this might be the last reference (against nasty
> userspace only), and that could then perhaps collide with us still holding
> the lock. It looks like we're fine, the only thing that drm_master_put
> might be locking is
> 
> 	mutex_lock(&dev->mode_config.idr_mutex);
> 
> through drm_lease_destroy(). We don't care about legacy drivers and hence
> the dev->struct_mutex (plus that lock is a complete mess anyway).
> 
> Maybe another patch to add a might_lock to drm_master_put, just to be
> safe?
> 

Sounds good.

I agree that we should be safe for now, since we're taking efforts to 
make master_rwsem an outer lock, especially after we take your later 
suggestion to remove the lock from drm_file_get_master and 
drm_is_current_master.

>> +	up_read(&dev->master_rwsem);
>>   	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n");
>>   	return 0;
>>   
>> -out_lessee:
>> -	drm_master_put(&lessee);
>> -
>>   out_leases:
>>   	put_unused_fd(fd);
>>   
>>   out_lessor:
>>   	drm_master_put(&lessor);
>> +
>> +out_file:
>> +	up_read(&dev->master_rwsem);
>> +	fput(lessee_file);
>>   	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl failed: %d\n", ret);
>>   	return ret;
>>   }
>> @@ -705,6 +711,11 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
>>   	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>   		return -EOPNOTSUPP;
>>   
>> +	down_write(&dev->master_rwsem);
>> +	if (unlikely(!drm_is_current_master(lessor_priv))) {
>> +		ret = -EACCES;
>> +		goto unlock;
>> +	}
>>   	lessor = drm_file_get_master(lessor_priv);
>>   	mutex_lock(&dev->mode_config.idr_mutex);
>>   
>> @@ -728,5 +739,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
>>   	mutex_unlock(&dev->mode_config.idr_mutex);
>>   	drm_master_put(&lessor);
>>   
>> +unlock:
>> +	up_write(&dev->master_rwsem);
>>   	return ret;
>>   }
>> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
>> index 142fb2f6e74d..7d32bb69e6db 100644
>> --- a/include/drm/drm_device.h
>> +++ b/include/drm/drm_device.h
>> @@ -151,6 +151,11 @@ struct drm_device {
>>   	 * Lock for &drm_device.master, &drm_file.was_master,
>>   	 * &drm_file.is_master, &drm_file.master, &drm_master.unique,
>>   	 * &drm_master.unique_len, and &drm_master.magic_map.
>> +	 *
>> +	 * Additionally, synchronizes modesetting rights between multiple users.
> 
> Current modern drives only use this for modesetting rights, but this is
> about anything which is exclusively owned by the drm_master. So maybe
> reword to "synchronizes access rights to exclusive resources like
> modesetting access".
> 

Right, will do.

> With that clarified:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> The other suggestions are for follow-up patches, if you feel like.
> -Daniel
> 
>> +	 * Users that can change the modeset or display state must hold a read
>> +	 * lock on @master_rwsem, and users that change modesetting rights
>> +	 * should hold a write lock.
>>   	 */
>>   	struct rw_semaphore master_rwsem;
>>   
>> -- 
>> 2.25.1
>>
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v8 7/7] drm: remove drm_file.master_lookup_lock
  2021-08-26 13:21   ` Daniel Vetter
@ 2021-08-31  6:02     ` Desmond Cheong Zhi Xi
  2021-08-31 12:28       ` Daniel Vetter
  0 siblings, 1 reply; 20+ messages in thread
From: Desmond Cheong Zhi Xi @ 2021-08-31  6:02 UTC (permalink / raw)
  To: maarten.lankhorst, mripard, tzimmermann, airlied, sumit.semwal,
	christian.koenig, jani.nikula, joonas.lahtinen, rodrigo.vivi,
	chris, ville.syrjala, matthew.auld, dan.carpenter,
	tvrtko.ursulin, matthew.d.roper, lucas.demarchi, karthik.b.s,
	jose.souza, manasi.d.navare, airlied, aditya.swarup, andrescj,
	linux-graphics-maintainer, zackr, dri-devel, linux-kernel,
	intel-gfx, linux-media, linaro-mm-sig, skhan, gregkh,
	linux-kernel-mentees, Daniel Vetter

On 26/8/21 9:21 pm, Daniel Vetter wrote:
> On Thu, Aug 26, 2021 at 10:01:22AM +0800, Desmond Cheong Zhi Xi wrote:
>> Previously, master_lookup_lock was introduced in
>> commit 0b0860a3cf5e ("drm: serialize drm_file.master with a new
>> spinlock") to serialize accesses to drm_file.master. This then allowed
>> us to write drm_file_get_master in commit 56f0729a510f ("drm: protect
>> drm_master pointers in drm_lease.c").
>>
>> The rationale behind introducing a new spinlock at the time was that
>> the other lock that could have been used (drm_device.master_mutex) was
>> the outermost lock, so embedding calls to drm_file_get_master and
>> drm_is_current_master in various functions easily caused us to invert
>> the lock hierarchy.
>>
>> Following the conversion of master_mutex into a rwsem, and its use to
>> plug races with modesetting rights, we've untangled some lock
>> hierarchies and removed the need for using drm_file_get_master and the
>> unlocked version of drm_is_current_master in multiple places.
>>
>> Hence, we can take this opportunity to clean up the locking design by
>> replacing master_lookup_lock with drm_device.master_rwsem.
>>
>> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
>> ---
>>   drivers/gpu/drm/drm_auth.c     | 19 +++++++------------
>>   drivers/gpu/drm/drm_file.c     |  1 -
>>   drivers/gpu/drm/drm_internal.h |  1 +
>>   drivers/gpu/drm/drm_ioctl.c    |  4 ++--
>>   drivers/gpu/drm/drm_lease.c    | 18 ++++++++----------
>>   include/drm/drm_file.h         |  9 +--------
>>   6 files changed, 19 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
>> index f2b2f197052a..232416119407 100644
>> --- a/drivers/gpu/drm/drm_auth.c
>> +++ b/drivers/gpu/drm/drm_auth.c
>> @@ -61,10 +61,9 @@
>>    * trusted clients.
>>    */
>>   
>> -static bool drm_is_current_master_locked(struct drm_file *fpriv)
>> +bool drm_is_current_master_locked(struct drm_file *fpriv)
>>   {
>> -	lockdep_assert_once(lockdep_is_held(&fpriv->master_lookup_lock) ||
>> -			    lockdep_is_held(&fpriv->minor->dev->master_rwsem));
>> +	lockdep_assert_held_once(&fpriv->minor->dev->master_rwsem);
>>   
>>   	return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master;
>>   }
>> @@ -83,9 +82,9 @@ bool drm_is_current_master(struct drm_file *fpriv)
>>   {
>>   	bool ret;
>>   
>> -	spin_lock(&fpriv->master_lookup_lock);
>> +	down_read(&fpriv->minor->dev->master_rwsem);
> 
> Looking at the 3 patches and the need to have a locked version of pretty
> much everything I'm wondering: Can't we just drop the spinlock completely,
> and everywhere we've taking it thus far replace it with a
> lockdep_assert_held_once?
> 
> The thing is, if there's any path left that doesn't hold the rwsem in at
> least read mode we have a bug. And the right way to fix such a bug is to
> grab the rwsem sufficiently high up in the callchain. That way I think we
> should be able to avoid all these tedious changes to everything, including
> touching i915 and vmwgfx drivers.
> 
> Or am I missing something big time?
> -Daniel
> 

Thanks for taking a look at all the patches and for the suggestions, Daniel.

Just my two cents. I think it makes sense to replace the lock with the 
lockdep assertion. This avoids the weirdness with the lock being taken 
both as an outer lock and sometimes as a deeply embedded inner lock.

But we'll probably have to fix some stuff because I don't think we 
always hold the rwsem in the places where the spinlock is grabbed (i.e. 
when drm_is_current_master or drm_file_get_master is called).

I'll split the series as suggested so we can test things up to PATCH 4 
("drm: avoid races with modesetting rights"). For the rest of the series 
to remove the spinlock, I'll take a closer look and probably send out a 
patch later this week.

Best wishes,
Desmond

>>   	ret = drm_is_current_master_locked(fpriv);
>> -	spin_unlock(&fpriv->master_lookup_lock);
>> +	up_read(&fpriv->minor->dev->master_rwsem);
>>   
>>   	return ret;
>>   }
>> @@ -120,7 +119,7 @@ int drm_authmagic(struct drm_device *dev, void *data,
>>   	DRM_DEBUG("%u\n", auth->magic);
>>   
>>   	down_write(&dev->master_rwsem);
>> -	if (unlikely(!drm_is_current_master(file_priv))) {
>> +	if (unlikely(!drm_is_current_master_locked(file_priv))) {
>>   		up_write(&dev->master_rwsem);
>>   		return -EACCES;
>>   	}
>> @@ -178,9 +177,7 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
>>   	new_master = drm_master_create(dev);
>>   	if (!new_master)
>>   		return -ENOMEM;
>> -	spin_lock(&fpriv->master_lookup_lock);
>>   	fpriv->master = new_master;
>> -	spin_unlock(&fpriv->master_lookup_lock);
>>   
>>   	fpriv->is_master = 1;
>>   	fpriv->authenticated = 1;
>> @@ -343,9 +340,7 @@ int drm_master_open(struct drm_file *file_priv)
>>   	if (!dev->master) {
>>   		ret = drm_new_set_master(dev, file_priv);
>>   	} else {
>> -		spin_lock(&file_priv->master_lookup_lock);
>>   		file_priv->master = drm_master_get(dev->master);
>> -		spin_unlock(&file_priv->master_lookup_lock);
>>   	}
>>   	up_write(&dev->master_rwsem);
>>   
>> @@ -413,13 +408,13 @@ struct drm_master *drm_file_get_master(struct drm_file *file_priv)
>>   	if (!file_priv)
>>   		return NULL;
>>   
>> -	spin_lock(&file_priv->master_lookup_lock);
>> +	down_read(&file_priv->minor->dev->master_rwsem);
>>   	if (!file_priv->master)
>>   		goto unlock;
>>   	master = drm_master_get(file_priv->master);
>>   
>>   unlock:
>> -	spin_unlock(&file_priv->master_lookup_lock);
>> +	up_read(&file_priv->minor->dev->master_rwsem);
>>   	return master;
>>   }
>>   EXPORT_SYMBOL(drm_file_get_master);
>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>> index 90b62f360da1..8c846e0179d7 100644
>> --- a/drivers/gpu/drm/drm_file.c
>> +++ b/drivers/gpu/drm/drm_file.c
>> @@ -176,7 +176,6 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>>   	init_waitqueue_head(&file->event_wait);
>>   	file->event_space = 4096; /* set aside 4k for event buffer */
>>   
>> -	spin_lock_init(&file->master_lookup_lock);
>>   	mutex_init(&file->event_read_lock);
>>   
>>   	if (drm_core_check_feature(dev, DRIVER_GEM))
>> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
>> index 17f3548c8ed2..5d421f749a17 100644
>> --- a/drivers/gpu/drm/drm_internal.h
>> +++ b/drivers/gpu/drm/drm_internal.h
>> @@ -132,6 +132,7 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
>>   				  struct drm_file *filp);
>>   
>>   /* drm_auth.c */
>> +bool drm_is_current_master_locked(struct drm_file *fpriv);
>>   int drm_getmagic(struct drm_device *dev, void *data,
>>   		 struct drm_file *file_priv);
>>   int drm_authmagic(struct drm_device *dev, void *data,
>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>> index 8bea39ffc5c0..c728437466c3 100644
>> --- a/drivers/gpu/drm/drm_ioctl.c
>> +++ b/drivers/gpu/drm/drm_ioctl.c
>> @@ -386,7 +386,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
>>   	int if_version, retcode = 0;
>>   
>>   	down_write(&dev->master_rwsem);
>> -	if (unlikely(!drm_is_current_master(file_priv))) {
>> +	if (unlikely(!drm_is_current_master_locked(file_priv))) {
>>   		retcode = -EACCES;
>>   		goto unlock;
>>   	}
>> @@ -540,7 +540,7 @@ static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
>>   
>>   	/* MASTER is only for master or control clients */
>>   	if (unlikely((flags & DRM_MASTER) &&
>> -		     !drm_is_current_master(file_priv)))
>> +		     !drm_is_current_master_locked(file_priv)))
>>   		return -EACCES;
>>   
>>   	/* Render clients must be explicitly allowed */
>> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
>> index 15bf3a3c76d1..0eecf320b1ab 100644
>> --- a/drivers/gpu/drm/drm_lease.c
>> +++ b/drivers/gpu/drm/drm_lease.c
>> @@ -498,12 +498,12 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>>   		return PTR_ERR(lessee_file);
>>   
>>   	down_read(&dev->master_rwsem);
>> -	if (unlikely(!drm_is_current_master(lessor_priv))) {
>> +	if (unlikely(!drm_is_current_master_locked(lessor_priv))) {
>>   		ret = -EACCES;
>>   		goto out_file;
>>   	}
>>   
>> -	lessor = drm_file_get_master(lessor_priv);
>> +	lessor = lessor_priv->master;
>>   	/* Do not allow sub-leases */
>>   	if (lessor->lessor) {
>>   		DRM_DEBUG_LEASE("recursive leasing not allowed\n");
>> @@ -565,7 +565,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>>   	/* Hook up the fd */
>>   	fd_install(fd, lessee_file);
>>   
>> -	drm_master_put(&lessor);
>>   	up_read(&dev->master_rwsem);
>>   	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n");
>>   	return 0;
>> @@ -600,7 +599,8 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
>>   	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>   		return -EOPNOTSUPP;
>>   
>> -	lessor = drm_file_get_master(lessor_priv);
>> +	lockdep_assert_held_once(&dev->master_rwsem);
>> +	lessor = lessor_priv->master;
>>   	DRM_DEBUG_LEASE("List lessees for %d\n", lessor->lessee_id);
>>   
>>   	mutex_lock(&dev->mode_config.idr_mutex);
>> @@ -624,7 +624,6 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
>>   		arg->count_lessees = count;
>>   
>>   	mutex_unlock(&dev->mode_config.idr_mutex);
>> -	drm_master_put(&lessor);
>>   
>>   	return ret;
>>   }
>> @@ -650,7 +649,8 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
>>   	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>   		return -EOPNOTSUPP;
>>   
>> -	lessee = drm_file_get_master(lessee_priv);
>> +	lockdep_assert_held_once(&dev->master_rwsem);
>> +	lessee = lessee_priv->master;
>>   	DRM_DEBUG_LEASE("get lease for %d\n", lessee->lessee_id);
>>   
>>   	mutex_lock(&dev->mode_config.idr_mutex);
>> @@ -678,7 +678,6 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
>>   		arg->count_objects = count;
>>   
>>   	mutex_unlock(&dev->mode_config.idr_mutex);
>> -	drm_master_put(&lessee);
>>   
>>   	return ret;
>>   }
>> @@ -703,11 +702,11 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
>>   		return -EOPNOTSUPP;
>>   
>>   	down_write(&dev->master_rwsem);
>> -	if (unlikely(!drm_is_current_master(lessor_priv))) {
>> +	if (unlikely(!drm_is_current_master_locked(lessor_priv))) {
>>   		ret = -EACCES;
>>   		goto unlock;
>>   	}
>> -	lessor = drm_file_get_master(lessor_priv);
>> +	lessor = lessor_priv->master;
>>   	mutex_lock(&dev->mode_config.idr_mutex);
>>   
>>   	lessee = _drm_find_lessee(lessor, arg->lessee_id);
>> @@ -728,7 +727,6 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
>>   
>>   fail:
>>   	mutex_unlock(&dev->mode_config.idr_mutex);
>> -	drm_master_put(&lessor);
>>   
>>   unlock:
>>   	up_write(&dev->master_rwsem);
>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>> index d12bb2ba7814..e2d49fe3e32d 100644
>> --- a/include/drm/drm_file.h
>> +++ b/include/drm/drm_file.h
>> @@ -227,16 +227,12 @@ struct drm_file {
>>   	 * @master:
>>   	 *
>>   	 * Master this node is currently associated with. Protected by struct
>> -	 * &drm_device.master_rwsem, and serialized by @master_lookup_lock.
>> +	 * &drm_device.master_rwsem.
>>   	 *
>>   	 * Only relevant if drm_is_primary_client() returns true. Note that
>>   	 * this only matches &drm_device.master if the master is the currently
>>   	 * active one.
>>   	 *
>> -	 * To update @master, both &drm_device.master_rwsem and
>> -	 * @master_lookup_lock need to be held, therefore holding either of
>> -	 * them is safe and enough for the read side.
>> -	 *
>>   	 * When dereferencing this pointer, either hold struct
>>   	 * &drm_device.master_rwsem for the duration of the pointer's use, or
>>   	 * use drm_file_get_master() if struct &drm_device.master_rwsem is not
>> @@ -248,9 +244,6 @@ struct drm_file {
>>   	 */
>>   	struct drm_master *master;
>>   
>> -	/** @master_lock: Serializes @master. */
>> -	spinlock_t master_lookup_lock;
>> -
>>   	/** @pid: Process that opened this file. */
>>   	struct pid *pid;
>>   
>> -- 
>> 2.25.1
>>
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v8 7/7] drm: remove drm_file.master_lookup_lock
  2021-08-31  6:02     ` Desmond Cheong Zhi Xi
@ 2021-08-31 12:28       ` Daniel Vetter
  0 siblings, 0 replies; 20+ messages in thread
From: Daniel Vetter @ 2021-08-31 12:28 UTC (permalink / raw)
  To: Desmond Cheong Zhi Xi
  Cc: maarten.lankhorst, mripard, tzimmermann, airlied, sumit.semwal,
	christian.koenig, jani.nikula, joonas.lahtinen, rodrigo.vivi,
	chris, ville.syrjala, matthew.auld, dan.carpenter,
	tvrtko.ursulin, matthew.d.roper, lucas.demarchi, karthik.b.s,
	jose.souza, manasi.d.navare, airlied, aditya.swarup, andrescj,
	linux-graphics-maintainer, zackr, dri-devel, linux-kernel,
	intel-gfx, linux-media, linaro-mm-sig, skhan, gregkh,
	linux-kernel-mentees, Daniel Vetter

On Tue, Aug 31, 2021 at 02:02:39PM +0800, Desmond Cheong Zhi Xi wrote:
> On 26/8/21 9:21 pm, Daniel Vetter wrote:
> > On Thu, Aug 26, 2021 at 10:01:22AM +0800, Desmond Cheong Zhi Xi wrote:
> > > Previously, master_lookup_lock was introduced in
> > > commit 0b0860a3cf5e ("drm: serialize drm_file.master with a new
> > > spinlock") to serialize accesses to drm_file.master. This then allowed
> > > us to write drm_file_get_master in commit 56f0729a510f ("drm: protect
> > > drm_master pointers in drm_lease.c").
> > > 
> > > The rationale behind introducing a new spinlock at the time was that
> > > the other lock that could have been used (drm_device.master_mutex) was
> > > the outermost lock, so embedding calls to drm_file_get_master and
> > > drm_is_current_master in various functions easily caused us to invert
> > > the lock hierarchy.
> > > 
> > > Following the conversion of master_mutex into a rwsem, and its use to
> > > plug races with modesetting rights, we've untangled some lock
> > > hierarchies and removed the need for using drm_file_get_master and the
> > > unlocked version of drm_is_current_master in multiple places.
> > > 
> > > Hence, we can take this opportunity to clean up the locking design by
> > > replacing master_lookup_lock with drm_device.master_rwsem.
> > > 
> > > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
> > > ---
> > >   drivers/gpu/drm/drm_auth.c     | 19 +++++++------------
> > >   drivers/gpu/drm/drm_file.c     |  1 -
> > >   drivers/gpu/drm/drm_internal.h |  1 +
> > >   drivers/gpu/drm/drm_ioctl.c    |  4 ++--
> > >   drivers/gpu/drm/drm_lease.c    | 18 ++++++++----------
> > >   include/drm/drm_file.h         |  9 +--------
> > >   6 files changed, 19 insertions(+), 33 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> > > index f2b2f197052a..232416119407 100644
> > > --- a/drivers/gpu/drm/drm_auth.c
> > > +++ b/drivers/gpu/drm/drm_auth.c
> > > @@ -61,10 +61,9 @@
> > >    * trusted clients.
> > >    */
> > > -static bool drm_is_current_master_locked(struct drm_file *fpriv)
> > > +bool drm_is_current_master_locked(struct drm_file *fpriv)
> > >   {
> > > -	lockdep_assert_once(lockdep_is_held(&fpriv->master_lookup_lock) ||
> > > -			    lockdep_is_held(&fpriv->minor->dev->master_rwsem));
> > > +	lockdep_assert_held_once(&fpriv->minor->dev->master_rwsem);
> > >   	return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master;
> > >   }
> > > @@ -83,9 +82,9 @@ bool drm_is_current_master(struct drm_file *fpriv)
> > >   {
> > >   	bool ret;
> > > -	spin_lock(&fpriv->master_lookup_lock);
> > > +	down_read(&fpriv->minor->dev->master_rwsem);
> > 
> > Looking at the 3 patches and the need to have a locked version of pretty
> > much everything I'm wondering: Can't we just drop the spinlock completely,
> > and everywhere we've taking it thus far replace it with a
> > lockdep_assert_held_once?
> > 
> > The thing is, if there's any path left that doesn't hold the rwsem in at
> > least read mode we have a bug. And the right way to fix such a bug is to
> > grab the rwsem sufficiently high up in the callchain. That way I think we
> > should be able to avoid all these tedious changes to everything, including
> > touching i915 and vmwgfx drivers.
> > 
> > Or am I missing something big time?
> > -Daniel
> > 
> 
> Thanks for taking a look at all the patches and for the suggestions, Daniel.
> 
> Just my two cents. I think it makes sense to replace the lock with the
> lockdep assertion. This avoids the weirdness with the lock being taken both
> as an outer lock and sometimes as a deeply embedded inner lock.
> 
> But we'll probably have to fix some stuff because I don't think we always
> hold the rwsem in the places where the spinlock is grabbed (i.e. when
> drm_is_current_master or drm_file_get_master is called).

Yeah right I forgot about that again when coming up with this idea. All
the ioctl that read kms state need a read lock too. Maybe those ioctl
could just grab the master rwsem themselves? This should work now that
we've untangled the drm_global_mutex situation I think.

> I'll split the series as suggested so we can test things up to PATCH 4
> ("drm: avoid races with modesetting rights"). For the rest of the series to
> remove the spinlock, I'll take a closer look and probably send out a patch
> later this week.

Sounds great!
-Daniel


> 
> Best wishes,
> Desmond
> 
> > >   	ret = drm_is_current_master_locked(fpriv);
> > > -	spin_unlock(&fpriv->master_lookup_lock);
> > > +	up_read(&fpriv->minor->dev->master_rwsem);
> > >   	return ret;
> > >   }
> > > @@ -120,7 +119,7 @@ int drm_authmagic(struct drm_device *dev, void *data,
> > >   	DRM_DEBUG("%u\n", auth->magic);
> > >   	down_write(&dev->master_rwsem);
> > > -	if (unlikely(!drm_is_current_master(file_priv))) {
> > > +	if (unlikely(!drm_is_current_master_locked(file_priv))) {
> > >   		up_write(&dev->master_rwsem);
> > >   		return -EACCES;
> > >   	}
> > > @@ -178,9 +177,7 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
> > >   	new_master = drm_master_create(dev);
> > >   	if (!new_master)
> > >   		return -ENOMEM;
> > > -	spin_lock(&fpriv->master_lookup_lock);
> > >   	fpriv->master = new_master;
> > > -	spin_unlock(&fpriv->master_lookup_lock);
> > >   	fpriv->is_master = 1;
> > >   	fpriv->authenticated = 1;
> > > @@ -343,9 +340,7 @@ int drm_master_open(struct drm_file *file_priv)
> > >   	if (!dev->master) {
> > >   		ret = drm_new_set_master(dev, file_priv);
> > >   	} else {
> > > -		spin_lock(&file_priv->master_lookup_lock);
> > >   		file_priv->master = drm_master_get(dev->master);
> > > -		spin_unlock(&file_priv->master_lookup_lock);
> > >   	}
> > >   	up_write(&dev->master_rwsem);
> > > @@ -413,13 +408,13 @@ struct drm_master *drm_file_get_master(struct drm_file *file_priv)
> > >   	if (!file_priv)
> > >   		return NULL;
> > > -	spin_lock(&file_priv->master_lookup_lock);
> > > +	down_read(&file_priv->minor->dev->master_rwsem);
> > >   	if (!file_priv->master)
> > >   		goto unlock;
> > >   	master = drm_master_get(file_priv->master);
> > >   unlock:
> > > -	spin_unlock(&file_priv->master_lookup_lock);
> > > +	up_read(&file_priv->minor->dev->master_rwsem);
> > >   	return master;
> > >   }
> > >   EXPORT_SYMBOL(drm_file_get_master);
> > > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > > index 90b62f360da1..8c846e0179d7 100644
> > > --- a/drivers/gpu/drm/drm_file.c
> > > +++ b/drivers/gpu/drm/drm_file.c
> > > @@ -176,7 +176,6 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
> > >   	init_waitqueue_head(&file->event_wait);
> > >   	file->event_space = 4096; /* set aside 4k for event buffer */
> > > -	spin_lock_init(&file->master_lookup_lock);
> > >   	mutex_init(&file->event_read_lock);
> > >   	if (drm_core_check_feature(dev, DRIVER_GEM))
> > > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> > > index 17f3548c8ed2..5d421f749a17 100644
> > > --- a/drivers/gpu/drm/drm_internal.h
> > > +++ b/drivers/gpu/drm/drm_internal.h
> > > @@ -132,6 +132,7 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
> > >   				  struct drm_file *filp);
> > >   /* drm_auth.c */
> > > +bool drm_is_current_master_locked(struct drm_file *fpriv);
> > >   int drm_getmagic(struct drm_device *dev, void *data,
> > >   		 struct drm_file *file_priv);
> > >   int drm_authmagic(struct drm_device *dev, void *data,
> > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > > index 8bea39ffc5c0..c728437466c3 100644
> > > --- a/drivers/gpu/drm/drm_ioctl.c
> > > +++ b/drivers/gpu/drm/drm_ioctl.c
> > > @@ -386,7 +386,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
> > >   	int if_version, retcode = 0;
> > >   	down_write(&dev->master_rwsem);
> > > -	if (unlikely(!drm_is_current_master(file_priv))) {
> > > +	if (unlikely(!drm_is_current_master_locked(file_priv))) {
> > >   		retcode = -EACCES;
> > >   		goto unlock;
> > >   	}
> > > @@ -540,7 +540,7 @@ static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
> > >   	/* MASTER is only for master or control clients */
> > >   	if (unlikely((flags & DRM_MASTER) &&
> > > -		     !drm_is_current_master(file_priv)))
> > > +		     !drm_is_current_master_locked(file_priv)))
> > >   		return -EACCES;
> > >   	/* Render clients must be explicitly allowed */
> > > diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> > > index 15bf3a3c76d1..0eecf320b1ab 100644
> > > --- a/drivers/gpu/drm/drm_lease.c
> > > +++ b/drivers/gpu/drm/drm_lease.c
> > > @@ -498,12 +498,12 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
> > >   		return PTR_ERR(lessee_file);
> > >   	down_read(&dev->master_rwsem);
> > > -	if (unlikely(!drm_is_current_master(lessor_priv))) {
> > > +	if (unlikely(!drm_is_current_master_locked(lessor_priv))) {
> > >   		ret = -EACCES;
> > >   		goto out_file;
> > >   	}
> > > -	lessor = drm_file_get_master(lessor_priv);
> > > +	lessor = lessor_priv->master;
> > >   	/* Do not allow sub-leases */
> > >   	if (lessor->lessor) {
> > >   		DRM_DEBUG_LEASE("recursive leasing not allowed\n");
> > > @@ -565,7 +565,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
> > >   	/* Hook up the fd */
> > >   	fd_install(fd, lessee_file);
> > > -	drm_master_put(&lessor);
> > >   	up_read(&dev->master_rwsem);
> > >   	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n");
> > >   	return 0;
> > > @@ -600,7 +599,8 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
> > >   	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > >   		return -EOPNOTSUPP;
> > > -	lessor = drm_file_get_master(lessor_priv);
> > > +	lockdep_assert_held_once(&dev->master_rwsem);
> > > +	lessor = lessor_priv->master;
> > >   	DRM_DEBUG_LEASE("List lessees for %d\n", lessor->lessee_id);
> > >   	mutex_lock(&dev->mode_config.idr_mutex);
> > > @@ -624,7 +624,6 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
> > >   		arg->count_lessees = count;
> > >   	mutex_unlock(&dev->mode_config.idr_mutex);
> > > -	drm_master_put(&lessor);
> > >   	return ret;
> > >   }
> > > @@ -650,7 +649,8 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
> > >   	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > >   		return -EOPNOTSUPP;
> > > -	lessee = drm_file_get_master(lessee_priv);
> > > +	lockdep_assert_held_once(&dev->master_rwsem);
> > > +	lessee = lessee_priv->master;
> > >   	DRM_DEBUG_LEASE("get lease for %d\n", lessee->lessee_id);
> > >   	mutex_lock(&dev->mode_config.idr_mutex);
> > > @@ -678,7 +678,6 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
> > >   		arg->count_objects = count;
> > >   	mutex_unlock(&dev->mode_config.idr_mutex);
> > > -	drm_master_put(&lessee);
> > >   	return ret;
> > >   }
> > > @@ -703,11 +702,11 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
> > >   		return -EOPNOTSUPP;
> > >   	down_write(&dev->master_rwsem);
> > > -	if (unlikely(!drm_is_current_master(lessor_priv))) {
> > > +	if (unlikely(!drm_is_current_master_locked(lessor_priv))) {
> > >   		ret = -EACCES;
> > >   		goto unlock;
> > >   	}
> > > -	lessor = drm_file_get_master(lessor_priv);
> > > +	lessor = lessor_priv->master;
> > >   	mutex_lock(&dev->mode_config.idr_mutex);
> > >   	lessee = _drm_find_lessee(lessor, arg->lessee_id);
> > > @@ -728,7 +727,6 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
> > >   fail:
> > >   	mutex_unlock(&dev->mode_config.idr_mutex);
> > > -	drm_master_put(&lessor);
> > >   unlock:
> > >   	up_write(&dev->master_rwsem);
> > > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> > > index d12bb2ba7814..e2d49fe3e32d 100644
> > > --- a/include/drm/drm_file.h
> > > +++ b/include/drm/drm_file.h
> > > @@ -227,16 +227,12 @@ struct drm_file {
> > >   	 * @master:
> > >   	 *
> > >   	 * Master this node is currently associated with. Protected by struct
> > > -	 * &drm_device.master_rwsem, and serialized by @master_lookup_lock.
> > > +	 * &drm_device.master_rwsem.
> > >   	 *
> > >   	 * Only relevant if drm_is_primary_client() returns true. Note that
> > >   	 * this only matches &drm_device.master if the master is the currently
> > >   	 * active one.
> > >   	 *
> > > -	 * To update @master, both &drm_device.master_rwsem and
> > > -	 * @master_lookup_lock need to be held, therefore holding either of
> > > -	 * them is safe and enough for the read side.
> > > -	 *
> > >   	 * When dereferencing this pointer, either hold struct
> > >   	 * &drm_device.master_rwsem for the duration of the pointer's use, or
> > >   	 * use drm_file_get_master() if struct &drm_device.master_rwsem is not
> > > @@ -248,9 +244,6 @@ struct drm_file {
> > >   	 */
> > >   	struct drm_master *master;
> > > -	/** @master_lock: Serializes @master. */
> > > -	spinlock_t master_lookup_lock;
> > > -
> > >   	/** @pid: Process that opened this file. */
> > >   	struct pid *pid;
> > > -- 
> > > 2.25.1
> > > 
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2021-08-31 12:29 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-26  2:01 [PATCH v8 0/7] drm: update locking for modesetting Desmond Cheong Zhi Xi
2021-08-26  2:01 ` [PATCH v8 1/7] drm: fix null ptr dereference in drm_master_release Desmond Cheong Zhi Xi
2021-08-26  9:53   ` Daniel Vetter
2021-08-26 11:53     ` Desmond Cheong Zhi Xi
2021-08-26 12:51       ` [Intel-gfx] " Daniel Vetter
2021-08-26  2:01 ` [PATCH v8 2/7] drm: convert drm_device.master_mutex into a rwsem Desmond Cheong Zhi Xi
2021-08-26  9:55   ` Daniel Vetter
2021-08-26  2:01 ` [PATCH v8 3/7] drm: lock drm_global_mutex earlier in the ioctl handler Desmond Cheong Zhi Xi
2021-08-26  9:58   ` Daniel Vetter
2021-08-30 21:04     ` Desmond Cheong Zhi Xi
2021-08-26  2:01 ` [PATCH v8 4/7] drm: avoid races with modesetting rights Desmond Cheong Zhi Xi
2021-08-26 12:59   ` Daniel Vetter
2021-08-30 21:36     ` Desmond Cheong Zhi Xi
2021-08-26  2:01 ` [PATCH v8 5/7] drm: avoid circular locks in drm_mode_object_find Desmond Cheong Zhi Xi
2021-08-26 13:13   ` Daniel Vetter
2021-08-26  2:01 ` [PATCH v8 6/7] drm: avoid circular locks in drm_lease_held Desmond Cheong Zhi Xi
2021-08-26  2:01 ` [PATCH v8 7/7] drm: remove drm_file.master_lookup_lock Desmond Cheong Zhi Xi
2021-08-26 13:21   ` Daniel Vetter
2021-08-31  6:02     ` Desmond Cheong Zhi Xi
2021-08-31 12:28       ` Daniel Vetter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).