LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] drm: avoid races with modesetting rights
@ 2021-08-15 15:37 Desmond Cheong Zhi Xi
  2021-08-15 18:47 ` kernel test robot
  2021-08-15 18:47 ` kernel test robot
  0 siblings, 2 replies; 9+ messages in thread
From: Desmond Cheong Zhi Xi @ 2021-08-15 15:37 UTC (permalink / raw)
  To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel
  Cc: Desmond Cheong Zhi Xi, dri-devel, linux-kernel, intel-gfx, 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_mutex 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 by flushing all users that might have seen
old modesetting permissions before returning to userspace.

We do this using rwsem: users that perform modesetting should hold a
read lock on the new drm_device.master_rwsem, and users that change
these permissions should flush these readers before returning to
userspace.

Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---

Hi,

I opted to leave the drm_master_unlock_and_flush helper out of this
patch, but happy to add it in if it'd be useful. Imo, when comparing it
with a mutex_unlock followed by drm_master_flush, it didn't add clarity.
And since we don't always hold the drm_device.master_mutex before
flushing (such as in drm_mode_revoke_lease_ioctl), perhaps it's better
to stick with one method to flush readers with drm_master_flush.

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

 drivers/gpu/drm/drm_auth.c  | 45 ++++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_drv.c   |  1 +
 drivers/gpu/drm/drm_ioctl.c |  9 +++++++-
 drivers/gpu/drm/drm_lease.c |  1 +
 include/drm/drm_auth.h      |  1 +
 include/drm/drm_device.h    | 18 +++++++++++++++
 6 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 60a6b21474b1..175bc4d1e4b4 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -29,6 +29,7 @@
  */
 
 #include <linux/slab.h>
+#include <linux/task_work.h>
 
 #include <drm/drm_auth.h>
 #include <drm/drm_drv.h>
@@ -282,6 +283,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);
+	drm_master_flush(dev);
 	return ret;
 }
 
@@ -321,8 +323,10 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
 	}
 
 	drm_drop_master(dev, file_priv);
+
 out_unlock:
 	mutex_unlock(&dev->master_mutex);
+	drm_master_flush(dev);
 	return ret;
 }
 
@@ -344,6 +348,8 @@ int drm_master_open(struct drm_file *file_priv)
 	}
 	mutex_unlock(&dev->master_mutex);
 
+	drm_master_flush(dev);
+
 	return ret;
 }
 
@@ -450,11 +456,15 @@ EXPORT_SYMBOL(drm_master_put);
 /* Used by drm_client and drm_fb_helper */
 bool drm_master_internal_acquire(struct drm_device *dev)
 {
+	down_read(&dev->master_rwsem);
+
 	mutex_lock(&dev->master_mutex);
 	if (dev->master) {
 		mutex_unlock(&dev->master_mutex);
+		up_read(&dev->master_rwsem);
 		return false;
 	}
+	mutex_unlock(&dev->master_mutex);
 
 	return true;
 }
@@ -463,6 +473,39 @@ 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);
+
+/* After flushing, all readers that might have seen old master/lease
+ * permissions are guaranteed to have completed.
+ */
+void master_flush(struct callback_head *cb)
+{
+	struct drm_device *dev = container_of(cb, struct drm_device,
+					      master_flush_work);
+
+	down_write(&dev->master_rwsem);
+	up_write(&dev->master_rwsem);
+}
+
+/**
+ * drm_master_flush - queues work to flush readers of master/lease permissions
+ * before returning to userspace
+ * @dev: DRM device
+ *
+ * Queues up work to flush all readers of master/lease permissions. This work
+ * is run before this task returns to user mode. Calling this function when a
+ * task changes modesetting rights ensures that other processes that perform
+ * modesetting do not race with userspace.
+ */
+void drm_master_flush(struct drm_device *dev)
+{
+	init_task_work(&dev->master_flush_work, master_flush);
+	task_work_add(current, &dev->master_flush_work, TWA_RESUME);
+	/* If task_work_add fails, then the task is exiting. In this case, it
+	 * doesn't matter if master_flush is run, so we don't need an
+	 * alternative mechanism for flushing.
+	 */
+}
+EXPORT_SYMBOL(drm_master_flush);
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 7a5097467ba5..10f7e256f999 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -612,6 +612,7 @@ static int drm_dev_init(struct drm_device *dev,
 	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 be4a52dc4d6f..b6e05adecb42 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -785,9 +785,12 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata,
 	if (drm_dev_is_unplugged(dev))
 		return -ENODEV;
 
+	if (unlikely(flags & DRM_MASTER))
+		down_read(&dev->master_rwsem);
+
 	retcode = drm_ioctl_permit(flags, file_priv);
 	if (unlikely(retcode))
-		return retcode;
+		goto release_master;
 
 	/* Enforce sane locking for modern driver ioctls. */
 	if (likely(!drm_core_check_feature(dev, DRIVER_LEGACY)) ||
@@ -798,6 +801,10 @@ long drm_ioctl_kernel(struct file *file, drm_ioctl_t *func, void *kdata,
 		retcode = func(dev, kdata, file_priv);
 		mutex_unlock(&drm_global_mutex);
 	}
+
+release_master:
+	if (unlikely(flags & DRM_MASTER))
+		up_read(&dev->master_rwsem);
 	return retcode;
 }
 EXPORT_SYMBOL(drm_ioctl_kernel);
diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index dee4f24a1808..983701198ffd 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -723,6 +723,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
 	}
 
 	_drm_lease_revoke(lessee);
+	drm_master_flush(dev);
 
 fail:
 	mutex_unlock(&dev->mode_config.idr_mutex);
diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
index ba248ca8866f..eda3672df6c3 100644
--- a/include/drm/drm_auth.h
+++ b/include/drm/drm_auth.h
@@ -155,6 +155,7 @@ struct drm_master *drm_master_get(struct drm_master *master);
 struct drm_master *drm_file_get_master(struct drm_file *file_priv);
 void drm_master_put(struct drm_master **master);
 bool drm_is_current_master(struct drm_file *fpriv);
+void drm_master_flush(struct drm_device *dev);
 
 struct drm_master *drm_master_create(struct drm_device *dev);
 
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index 604b1d1b2d72..eeb58e164788 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -111,6 +111,24 @@ struct drm_device {
 	 */
 	struct drm_master *master;
 
+	/**
+	 * @master_rwsem:
+	 *
+	 * 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 flush
+	 * readers before returning to userspace using drm_master_flush().
+	 */
+	struct rw_semaphore master_rwsem;
+
+	/**
+	 * @master_flush_work:
+	 *
+	 * Callback structure used internally to queue work to flush readers of
+	 * master/lease permissions.
+	 */
+	struct callback_head master_flush_work;
+
 	/**
 	 * @driver_features: per-device driver features
 	 *
-- 
2.25.1


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

* Re: [PATCH v2] drm: avoid races with modesetting rights
  2021-08-15 15:37 [PATCH v2] drm: avoid races with modesetting rights Desmond Cheong Zhi Xi
@ 2021-08-15 18:47 ` kernel test robot
  2021-08-16  8:53   ` Desmond Cheong Zhi Xi
  2021-08-15 18:47 ` kernel test robot
  1 sibling, 1 reply; 9+ messages in thread
From: kernel test robot @ 2021-08-15 18:47 UTC (permalink / raw)
  To: Desmond Cheong Zhi Xi, maarten.lankhorst, mripard, tzimmermann,
	airlied, daniel
  Cc: kbuild-all, Desmond Cheong Zhi Xi, dri-devel, linux-kernel,
	intel-gfx, skhan

[-- Attachment #1: Type: text/plain, Size: 1486 bytes --]

Hi Desmond,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on next-20210813]
[also build test ERROR on v5.14-rc5]
[cannot apply to linus/master v5.14-rc5 v5.14-rc4 v5.14-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Desmond-Cheong-Zhi-Xi/drm-avoid-races-with-modesetting-rights/20210815-234145
base:    4b358aabb93a2c654cd1dcab1a25a589f6e2b153
config: i386-randconfig-a004-20210815 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/cf6d8354b7d7953cd866fad004cbb189adfa074f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Desmond-Cheong-Zhi-Xi/drm-avoid-races-with-modesetting-rights/20210815-234145
        git checkout cf6d8354b7d7953cd866fad004cbb189adfa074f
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "task_work_add" [drivers/gpu/drm/drm.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32111 bytes --]

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

* Re: [PATCH v2] drm: avoid races with modesetting rights
  2021-08-15 15:37 [PATCH v2] drm: avoid races with modesetting rights Desmond Cheong Zhi Xi
  2021-08-15 18:47 ` kernel test robot
@ 2021-08-15 18:47 ` kernel test robot
  2021-08-16  6:09   ` Desmond Cheong Zhi Xi
  1 sibling, 1 reply; 9+ messages in thread
From: kernel test robot @ 2021-08-15 18:47 UTC (permalink / raw)
  To: Desmond Cheong Zhi Xi, maarten.lankhorst, mripard, tzimmermann,
	airlied, daniel
  Cc: kbuild-all, Desmond Cheong Zhi Xi, dri-devel, linux-kernel,
	intel-gfx, skhan

[-- Attachment #1: Type: text/plain, Size: 2259 bytes --]

Hi Desmond,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20210813]
[also build test WARNING on v5.14-rc5]
[cannot apply to linus/master v5.14-rc5 v5.14-rc4 v5.14-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Desmond-Cheong-Zhi-Xi/drm-avoid-races-with-modesetting-rights/20210815-234145
base:    4b358aabb93a2c654cd1dcab1a25a589f6e2b153
config: arc-randconfig-r031-20210815 (attached as .config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/cf6d8354b7d7953cd866fad004cbb189adfa074f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Desmond-Cheong-Zhi-Xi/drm-avoid-races-with-modesetting-rights/20210815-234145
        git checkout cf6d8354b7d7953cd866fad004cbb189adfa074f
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=arc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/drm_auth.c:483:6: warning: no previous prototype for 'master_flush' [-Wmissing-prototypes]
     483 | void master_flush(struct callback_head *cb)
         |      ^~~~~~~~~~~~


vim +/master_flush +483 drivers/gpu/drm/drm_auth.c

   479	
   480	/* After flushing, all readers that might have seen old master/lease
   481	 * permissions are guaranteed to have completed.
   482	 */
 > 483	void master_flush(struct callback_head *cb)
   484	{
   485		struct drm_device *dev = container_of(cb, struct drm_device,
   486						      master_flush_work);
   487	
   488		down_write(&dev->master_rwsem);
   489		up_write(&dev->master_rwsem);
   490	}
   491	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30520 bytes --]

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

* Re: [PATCH v2] drm: avoid races with modesetting rights
  2021-08-15 18:47 ` kernel test robot
@ 2021-08-16  6:09   ` Desmond Cheong Zhi Xi
  0 siblings, 0 replies; 9+ messages in thread
From: Desmond Cheong Zhi Xi @ 2021-08-16  6:09 UTC (permalink / raw)
  To: kernel test robot, maarten.lankhorst, mripard, tzimmermann,
	airlied, daniel
  Cc: kbuild-all, dri-devel, linux-kernel, intel-gfx, skhan

On 16/8/21 2:47 am, kernel test robot wrote:
> Hi Desmond,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on next-20210813]
> [also build test WARNING on v5.14-rc5]
> [cannot apply to linus/master v5.14-rc5 v5.14-rc4 v5.14-rc3]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Desmond-Cheong-Zhi-Xi/drm-avoid-races-with-modesetting-rights/20210815-234145
> base:    4b358aabb93a2c654cd1dcab1a25a589f6e2b153
> config: arc-randconfig-r031-20210815 (attached as .config)
> compiler: arceb-elf-gcc (GCC) 11.2.0
> reproduce (this is a W=1 build):
>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>          chmod +x ~/bin/make.cross
>          # https://github.com/0day-ci/linux/commit/cf6d8354b7d7953cd866fad004cbb189adfa074f
>          git remote add linux-review https://github.com/0day-ci/linux
>          git fetch --no-tags linux-review Desmond-Cheong-Zhi-Xi/drm-avoid-races-with-modesetting-rights/20210815-234145
>          git checkout cf6d8354b7d7953cd866fad004cbb189adfa074f
>          # save the attached .config to linux build tree
>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=arc
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>):
> 
>>> drivers/gpu/drm/drm_auth.c:483:6: warning: no previous prototype for 'master_flush' [-Wmissing-prototypes]
>       483 | void master_flush(struct callback_head *cb)
>           |      ^~~~~~~~~~~~
> 

My bad, this should have been declared with static. I'll add it in, thanks.

> 
> vim +/master_flush +483 drivers/gpu/drm/drm_auth.c
> 
>     479	
>     480	/* After flushing, all readers that might have seen old master/lease
>     481	 * permissions are guaranteed to have completed.
>     482	 */
>   > 483	void master_flush(struct callback_head *cb)
>     484	{
>     485		struct drm_device *dev = container_of(cb, struct drm_device,
>     486						      master_flush_work);
>     487	
>     488		down_write(&dev->master_rwsem);
>     489		up_write(&dev->master_rwsem);
>     490	}
>     491	
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> 


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

* Re: [PATCH v2] drm: avoid races with modesetting rights
  2021-08-15 18:47 ` kernel test robot
@ 2021-08-16  8:53   ` Desmond Cheong Zhi Xi
  2021-08-16  9:04     ` Daniel Vetter
  0 siblings, 1 reply; 9+ messages in thread
From: Desmond Cheong Zhi Xi @ 2021-08-16  8:53 UTC (permalink / raw)
  To: kernel test robot, maarten.lankhorst, mripard, tzimmermann,
	airlied, daniel
  Cc: kbuild-all, dri-devel, linux-kernel, intel-gfx, skhan

On 16/8/21 2:47 am, kernel test robot wrote:
> Hi Desmond,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on next-20210813]
> [also build test ERROR on v5.14-rc5]
> [cannot apply to linus/master v5.14-rc5 v5.14-rc4 v5.14-rc3]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Desmond-Cheong-Zhi-Xi/drm-avoid-races-with-modesetting-rights/20210815-234145
> base:    4b358aabb93a2c654cd1dcab1a25a589f6e2b153
> config: i386-randconfig-a004-20210815 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> reproduce (this is a W=1 build):
>          # https://github.com/0day-ci/linux/commit/cf6d8354b7d7953cd866fad004cbb189adfa074f
>          git remote add linux-review https://github.com/0day-ci/linux
>          git fetch --no-tags linux-review Desmond-Cheong-Zhi-Xi/drm-avoid-races-with-modesetting-rights/20210815-234145
>          git checkout cf6d8354b7d7953cd866fad004cbb189adfa074f
>          # save the attached .config to linux build tree
>          make W=1 ARCH=i386
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>, old ones prefixed by <<):
> 
>>> ERROR: modpost: "task_work_add" [drivers/gpu/drm/drm.ko] undefined!
> 

I'm a bit uncertain about this. Looking into the .config used, this 
error seems to happen because task_work_add isn't an exported symbol, 
but DRM is being compiled as a loadable kernel module (CONFIG_DRM=m).

One way to deal with this is to export the symbol, but there was a 
proposed patch to do this a few months back that wasn't picked up [1], 
so I'm not sure what to make of this.

I'll export the symbol as part of a v3 series, and check in with the 
task-work maintainers.

Link: 
https://lore.kernel.org/lkml/20210127150029.13766-3-joshi.k@samsung.com/ [1]

> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> 


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

* Re: [PATCH v2] drm: avoid races with modesetting rights
  2021-08-16  8:53   ` Desmond Cheong Zhi Xi
@ 2021-08-16  9:04     ` Daniel Vetter
  2021-08-16 10:31       ` Desmond Cheong Zhi Xi
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2021-08-16  9:04 UTC (permalink / raw)
  To: Desmond Cheong Zhi Xi
  Cc: kernel test robot, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Dave Airlie, kbuild-all, dri-devel,
	Linux Kernel Mailing List, intel-gfx, Shuah Khan

On Mon, Aug 16, 2021 at 10:53 AM Desmond Cheong Zhi Xi
<desmondcheongzx@gmail.com> wrote:
> On 16/8/21 2:47 am, kernel test robot wrote:
> > Hi Desmond,
> >
> > Thank you for the patch! Yet something to improve:
> >
> > [auto build test ERROR on next-20210813]
> > [also build test ERROR on v5.14-rc5]
> > [cannot apply to linus/master v5.14-rc5 v5.14-rc4 v5.14-rc3]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch]
> >
> > url:    https://github.com/0day-ci/linux/commits/Desmond-Cheong-Zhi-Xi/drm-avoid-races-with-modesetting-rights/20210815-234145
> > base:    4b358aabb93a2c654cd1dcab1a25a589f6e2b153
> > config: i386-randconfig-a004-20210815 (attached as .config)
> > compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> > reproduce (this is a W=1 build):
> >          # https://github.com/0day-ci/linux/commit/cf6d8354b7d7953cd866fad004cbb189adfa074f
> >          git remote add linux-review https://github.com/0day-ci/linux
> >          git fetch --no-tags linux-review Desmond-Cheong-Zhi-Xi/drm-avoid-races-with-modesetting-rights/20210815-234145
> >          git checkout cf6d8354b7d7953cd866fad004cbb189adfa074f
> >          # save the attached .config to linux build tree
> >          make W=1 ARCH=i386
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@intel.com>
> >
> > All errors (new ones prefixed by >>, old ones prefixed by <<):
> >
> >>> ERROR: modpost: "task_work_add" [drivers/gpu/drm/drm.ko] undefined!
> >
>
> I'm a bit uncertain about this. Looking into the .config used, this
> error seems to happen because task_work_add isn't an exported symbol,
> but DRM is being compiled as a loadable kernel module (CONFIG_DRM=m).
>
> One way to deal with this is to export the symbol, but there was a
> proposed patch to do this a few months back that wasn't picked up [1],
> so I'm not sure what to make of this.
>
> I'll export the symbol as part of a v3 series, and check in with the
> task-work maintainers.
>
> Link:
> https://lore.kernel.org/lkml/20210127150029.13766-3-joshi.k@samsung.com/ [1]

Yeah that sounds best. I have two more thoughts on the patch:
- drm_master_flush isn't used by any modules outside of drm.ko, so we
can unexport it and drop the kerneldoc (the comment is still good).
These kind of internal functions have their declaration in
drm-internal.h - there's already a few there from drm_auth.c

- We know have 3 locks for master state, that feels a bit like
overkill. The spinlock I think we need to keep due to lock inversions,
but the master_mutex and master_rwsem look like we should be able to
merge them? I.e. anywhere we currently grab the master_mutex we could
instead grab the rwsem in either write mode (when we change stuff) or
read mode (when we just check, like in master_internal_acquire).

Thoughts?
-Daniel

>
> > ---
> > 0-DAY CI Kernel Test Service, Intel Corporation
> > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> >
>


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

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

* Re: [PATCH v2] drm: avoid races with modesetting rights
  2021-08-16  9:04     ` Daniel Vetter
@ 2021-08-16 10:31       ` Desmond Cheong Zhi Xi
  2021-08-16 13:59         ` Daniel Vetter
  0 siblings, 1 reply; 9+ messages in thread
From: Desmond Cheong Zhi Xi @ 2021-08-16 10:31 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: kernel test robot, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Dave Airlie, kbuild-all, dri-devel,
	Linux Kernel Mailing List, intel-gfx, Shuah Khan

On 16/8/21 5:04 pm, Daniel Vetter wrote:
> On Mon, Aug 16, 2021 at 10:53 AM Desmond Cheong Zhi Xi
> <desmondcheongzx@gmail.com> wrote:
>> On 16/8/21 2:47 am, kernel test robot wrote:
>>> Hi Desmond,
>>>
>>> Thank you for the patch! Yet something to improve:
>>>
>>> [auto build test ERROR on next-20210813]
>>> [also build test ERROR on v5.14-rc5]
>>> [cannot apply to linus/master v5.14-rc5 v5.14-rc4 v5.14-rc3]
>>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>>> And when submitting patch, we suggest to use '--base' as documented in
>>> https://git-scm.com/docs/git-format-patch]
>>>
>>> url:    https://github.com/0day-ci/linux/commits/Desmond-Cheong-Zhi-Xi/drm-avoid-races-with-modesetting-rights/20210815-234145
>>> base:    4b358aabb93a2c654cd1dcab1a25a589f6e2b153
>>> config: i386-randconfig-a004-20210815 (attached as .config)
>>> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
>>> reproduce (this is a W=1 build):
>>>           # https://github.com/0day-ci/linux/commit/cf6d8354b7d7953cd866fad004cbb189adfa074f
>>>           git remote add linux-review https://github.com/0day-ci/linux
>>>           git fetch --no-tags linux-review Desmond-Cheong-Zhi-Xi/drm-avoid-races-with-modesetting-rights/20210815-234145
>>>           git checkout cf6d8354b7d7953cd866fad004cbb189adfa074f
>>>           # save the attached .config to linux build tree
>>>           make W=1 ARCH=i386
>>>
>>> If you fix the issue, kindly add following tag as appropriate
>>> Reported-by: kernel test robot <lkp@intel.com>
>>>
>>> All errors (new ones prefixed by >>, old ones prefixed by <<):
>>>
>>>>> ERROR: modpost: "task_work_add" [drivers/gpu/drm/drm.ko] undefined!
>>>
>>
>> I'm a bit uncertain about this. Looking into the .config used, this
>> error seems to happen because task_work_add isn't an exported symbol,
>> but DRM is being compiled as a loadable kernel module (CONFIG_DRM=m).
>>
>> One way to deal with this is to export the symbol, but there was a
>> proposed patch to do this a few months back that wasn't picked up [1],
>> so I'm not sure what to make of this.
>>
>> I'll export the symbol as part of a v3 series, and check in with the
>> task-work maintainers.
>>
>> Link:
>> https://lore.kernel.org/lkml/20210127150029.13766-3-joshi.k@samsung.com/ [1]
> 
> Yeah that sounds best. I have two more thoughts on the patch:
> - drm_master_flush isn't used by any modules outside of drm.ko, so we
> can unexport it and drop the kerneldoc (the comment is still good).
> These kind of internal functions have their declaration in
> drm-internal.h - there's already a few there from drm_auth.c
> 

Sounds good, I'll do that and move the declaration from drm_auth.h to 
drm_internal.h.

> - We know have 3 locks for master state, that feels a bit like
> overkill. The spinlock I think we need to keep due to lock inversions,
> but the master_mutex and master_rwsem look like we should be able to
> merge them? I.e. anywhere we currently grab the master_mutex we could
> instead grab the rwsem in either write mode (when we change stuff) or
> read mode (when we just check, like in master_internal_acquire).
> 
> Thoughts?
> -Daniel
> 

Using rwsem in the places where we currently hold the mutex seems pretty 
doable.

There are some tricky bits once we add rwsem read locks to the ioctl 
handler. Some ioctl functions like drm_authmagic need a write lock.

In this particular case, it might make sense to break master_mutex down 
into finer-grained locks, since the function doesn't change master 
permissions. It just needs to prevent concurrent writes to the 
drm_master.magic_map idr.

For other ioctls, I'll take a closer look on a case-by-case basis.

>>
>>> ---
>>> 0-DAY CI Kernel Test Service, Intel Corporation
>>> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>>>
>>
> 
> 


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

* Re: [PATCH v2] drm: avoid races with modesetting rights
  2021-08-16 10:31       ` Desmond Cheong Zhi Xi
@ 2021-08-16 13:59         ` Daniel Vetter
  2021-08-17 15:06           ` Desmond Cheong Zhi Xi
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2021-08-16 13:59 UTC (permalink / raw)
  To: Desmond Cheong Zhi Xi
  Cc: kernel test robot, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Dave Airlie, kbuild-all, dri-devel,
	Linux Kernel Mailing List, intel-gfx, Shuah Khan

On Mon, Aug 16, 2021 at 12:31 PM Desmond Cheong Zhi Xi
<desmondcheongzx@gmail.com> wrote:
>
> On 16/8/21 5:04 pm, Daniel Vetter wrote:
> > On Mon, Aug 16, 2021 at 10:53 AM Desmond Cheong Zhi Xi
> > <desmondcheongzx@gmail.com> wrote:
> >> On 16/8/21 2:47 am, kernel test robot wrote:
> >>> Hi Desmond,
> >>>
> >>> Thank you for the patch! Yet something to improve:
> >>>
> >>> [auto build test ERROR on next-20210813]
> >>> [also build test ERROR on v5.14-rc5]
> >>> [cannot apply to linus/master v5.14-rc5 v5.14-rc4 v5.14-rc3]
> >>> [If your patch is applied to the wrong git tree, kindly drop us a note.
> >>> And when submitting patch, we suggest to use '--base' as documented in
> >>> https://git-scm.com/docs/git-format-patch]
> >>>
> >>> url:    https://github.com/0day-ci/linux/commits/Desmond-Cheong-Zhi-Xi/drm-avoid-races-with-modesetting-rights/20210815-234145
> >>> base:    4b358aabb93a2c654cd1dcab1a25a589f6e2b153
> >>> config: i386-randconfig-a004-20210815 (attached as .config)
> >>> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> >>> reproduce (this is a W=1 build):
> >>>           # https://github.com/0day-ci/linux/commit/cf6d8354b7d7953cd866fad004cbb189adfa074f
> >>>           git remote add linux-review https://github.com/0day-ci/linux
> >>>           git fetch --no-tags linux-review Desmond-Cheong-Zhi-Xi/drm-avoid-races-with-modesetting-rights/20210815-234145
> >>>           git checkout cf6d8354b7d7953cd866fad004cbb189adfa074f
> >>>           # save the attached .config to linux build tree
> >>>           make W=1 ARCH=i386
> >>>
> >>> If you fix the issue, kindly add following tag as appropriate
> >>> Reported-by: kernel test robot <lkp@intel.com>
> >>>
> >>> All errors (new ones prefixed by >>, old ones prefixed by <<):
> >>>
> >>>>> ERROR: modpost: "task_work_add" [drivers/gpu/drm/drm.ko] undefined!
> >>>
> >>
> >> I'm a bit uncertain about this. Looking into the .config used, this
> >> error seems to happen because task_work_add isn't an exported symbol,
> >> but DRM is being compiled as a loadable kernel module (CONFIG_DRM=m).
> >>
> >> One way to deal with this is to export the symbol, but there was a
> >> proposed patch to do this a few months back that wasn't picked up [1],
> >> so I'm not sure what to make of this.
> >>
> >> I'll export the symbol as part of a v3 series, and check in with the
> >> task-work maintainers.
> >>
> >> Link:
> >> https://lore.kernel.org/lkml/20210127150029.13766-3-joshi.k@samsung.com/ [1]
> >
> > Yeah that sounds best. I have two more thoughts on the patch:
> > - drm_master_flush isn't used by any modules outside of drm.ko, so we
> > can unexport it and drop the kerneldoc (the comment is still good).
> > These kind of internal functions have their declaration in
> > drm-internal.h - there's already a few there from drm_auth.c
> >
>
> Sounds good, I'll do that and move the declaration from drm_auth.h to
> drm_internal.h.
>
> > - We know have 3 locks for master state, that feels a bit like
> > overkill. The spinlock I think we need to keep due to lock inversions,
> > but the master_mutex and master_rwsem look like we should be able to
> > merge them? I.e. anywhere we currently grab the master_mutex we could
> > instead grab the rwsem in either write mode (when we change stuff) or
> > read mode (when we just check, like in master_internal_acquire).
> >
> > Thoughts?
> > -Daniel
> >
>
> Using rwsem in the places where we currently hold the mutex seems pretty
> doable.
>
> There are some tricky bits once we add rwsem read locks to the ioctl
> handler. Some ioctl functions like drm_authmagic need a write lock.

Ah yes, I only looked at the dropmaster/setmaster ioctl, and those
don't have the DRM_MASTER bit set.

> In this particular case, it might make sense to break master_mutex down
> into finer-grained locks, since the function doesn't change master
> permissions. It just needs to prevent concurrent writes to the
> drm_master.magic_map idr.

Yeah for authmagic we could perhaps just reuse the spinlock to protect
->magic_map?

> For other ioctls, I'll take a closer look on a case-by-case basis.

If it's too much shuffling then I think totally fine to leave things
as-is. Just feels a bit silly to have 3 locks, on of which is an
rwlock itself, for this fairly small amount of state.
-Daniel

>
> >>
> >>> ---
> >>> 0-DAY CI Kernel Test Service, Intel Corporation
> >>> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> >>>
> >>
> >
> >
>


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

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

* Re: [PATCH v2] drm: avoid races with modesetting rights
  2021-08-16 13:59         ` Daniel Vetter
@ 2021-08-17 15:06           ` Desmond Cheong Zhi Xi
  0 siblings, 0 replies; 9+ messages in thread
From: Desmond Cheong Zhi Xi @ 2021-08-17 15:06 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: kernel test robot, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Dave Airlie, kbuild-all, dri-devel,
	Linux Kernel Mailing List, intel-gfx, Shuah Khan

On 16/8/21 9:59 pm, Daniel Vetter wrote:
> On Mon, Aug 16, 2021 at 12:31 PM Desmond Cheong Zhi Xi
> <desmondcheongzx@gmail.com> wrote:
>>
>> On 16/8/21 5:04 pm, Daniel Vetter wrote:
>>> On Mon, Aug 16, 2021 at 10:53 AM Desmond Cheong Zhi Xi
>>> <desmondcheongzx@gmail.com> wrote:
>>>> On 16/8/21 2:47 am, kernel test robot wrote:
>>>>> Hi Desmond,
>>>>>
>>>>> Thank you for the patch! Yet something to improve:
>>>>>
>>>>> [auto build test ERROR on next-20210813]
>>>>> [also build test ERROR on v5.14-rc5]
>>>>> [cannot apply to linus/master v5.14-rc5 v5.14-rc4 v5.14-rc3]
>>>>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>>>>> And when submitting patch, we suggest to use '--base' as documented in
>>>>> https://git-scm.com/docs/git-format-patch]
>>>>>
>>>>> url:    https://github.com/0day-ci/linux/commits/Desmond-Cheong-Zhi-Xi/drm-avoid-races-with-modesetting-rights/20210815-234145
>>>>> base:    4b358aabb93a2c654cd1dcab1a25a589f6e2b153
>>>>> config: i386-randconfig-a004-20210815 (attached as .config)
>>>>> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
>>>>> reproduce (this is a W=1 build):
>>>>>            # https://github.com/0day-ci/linux/commit/cf6d8354b7d7953cd866fad004cbb189adfa074f
>>>>>            git remote add linux-review https://github.com/0day-ci/linux
>>>>>            git fetch --no-tags linux-review Desmond-Cheong-Zhi-Xi/drm-avoid-races-with-modesetting-rights/20210815-234145
>>>>>            git checkout cf6d8354b7d7953cd866fad004cbb189adfa074f
>>>>>            # save the attached .config to linux build tree
>>>>>            make W=1 ARCH=i386
>>>>>
>>>>> If you fix the issue, kindly add following tag as appropriate
>>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>>>
>>>>> All errors (new ones prefixed by >>, old ones prefixed by <<):
>>>>>
>>>>>>> ERROR: modpost: "task_work_add" [drivers/gpu/drm/drm.ko] undefined!
>>>>>
>>>>
>>>> I'm a bit uncertain about this. Looking into the .config used, this
>>>> error seems to happen because task_work_add isn't an exported symbol,
>>>> but DRM is being compiled as a loadable kernel module (CONFIG_DRM=m).
>>>>
>>>> One way to deal with this is to export the symbol, but there was a
>>>> proposed patch to do this a few months back that wasn't picked up [1],
>>>> so I'm not sure what to make of this.
>>>>
>>>> I'll export the symbol as part of a v3 series, and check in with the
>>>> task-work maintainers.
>>>>
>>>> Link:
>>>> https://lore.kernel.org/lkml/20210127150029.13766-3-joshi.k@samsung.com/ [1]
>>>
>>> Yeah that sounds best. I have two more thoughts on the patch:
>>> - drm_master_flush isn't used by any modules outside of drm.ko, so we
>>> can unexport it and drop the kerneldoc (the comment is still good).
>>> These kind of internal functions have their declaration in
>>> drm-internal.h - there's already a few there from drm_auth.c
>>>
>>
>> Sounds good, I'll do that and move the declaration from drm_auth.h to
>> drm_internal.h.
>>
>>> - We know have 3 locks for master state, that feels a bit like
>>> overkill. The spinlock I think we need to keep due to lock inversions,
>>> but the master_mutex and master_rwsem look like we should be able to
>>> merge them? I.e. anywhere we currently grab the master_mutex we could
>>> instead grab the rwsem in either write mode (when we change stuff) or
>>> read mode (when we just check, like in master_internal_acquire).
>>>
>>> Thoughts?
>>> -Daniel
>>>
>>
>> Using rwsem in the places where we currently hold the mutex seems pretty
>> doable.
>>
>> There are some tricky bits once we add rwsem read locks to the ioctl
>> handler. Some ioctl functions like drm_authmagic need a write lock.
> 
> Ah yes, I only looked at the dropmaster/setmaster ioctl, and those
> don't have the DRM_MASTER bit set.
> 
>> In this particular case, it might make sense to break master_mutex down
>> into finer-grained locks, since the function doesn't change master
>> permissions. It just needs to prevent concurrent writes to the
>> drm_master.magic_map idr.
> 
> Yeah for authmagic we could perhaps just reuse the spinlock to protect
> ->magic_map?
> 

Yup, I had to move the spinlock from struct drm_file to struct 
drm_device, but I think that should work.

>> For other ioctls, I'll take a closer look on a case-by-case basis.
> 
> If it's too much shuffling then I think totally fine to leave things
> as-is. Just feels a bit silly to have 3 locks, on of which is an
> rwlock itself, for this fairly small amount of state.
> -Daniel
> 

Agreed, there's a lot of overlap between the master_mutex and rwsem so 
this a good opportunity to refactor things.

I'm cleaning up a v3 series now. There's some movement, but most of it 
are fixes to potential bugs that I saw while refactoring. We can see if 
the new version is a better design.

>>
>>>>
>>>>> ---
>>>>> 0-DAY CI Kernel Test Service, Intel Corporation
>>>>> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>>>>>
>>>>
>>>
>>>
>>
> 
> 


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

end of thread, other threads:[~2021-08-17 15:06 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-15 15:37 [PATCH v2] drm: avoid races with modesetting rights Desmond Cheong Zhi Xi
2021-08-15 18:47 ` kernel test robot
2021-08-16  8:53   ` Desmond Cheong Zhi Xi
2021-08-16  9:04     ` Daniel Vetter
2021-08-16 10:31       ` Desmond Cheong Zhi Xi
2021-08-16 13:59         ` Daniel Vetter
2021-08-17 15:06           ` Desmond Cheong Zhi Xi
2021-08-15 18:47 ` kernel test robot
2021-08-16  6:09   ` Desmond Cheong Zhi Xi

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).