Re: [PATCH] Revert "drm: add a locked version of drm_is_current_master"

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 23/6/21 4:14 pm, Daniel Vetter wrote:
On Wed, Jun 23, 2021 at 10:09 AM Desmond Cheong Zhi Xi
<desmondcheongzx@xxxxxxxxx> wrote:

On 22/6/21 3:54 pm, Daniel Vetter wrote:
This reverts commit 1815d9c86e3090477fbde066ff314a7e9721ee0f.

Unfortunately this inverts the locking hierarchy, so back to the
drawing board. Full lockdep splat below:

======================================================
WARNING: possible circular locking dependency detected
5.13.0-rc7-CI-CI_DRM_10254+ #1 Not tainted
------------------------------------------------------
kms_frontbuffer/1087 is trying to acquire lock:
ffff88810dcd01a8 (&dev->master_mutex){+.+.}-{3:3}, at: drm_is_current_master+0x1b/0x40
but task is already holding lock:
ffff88810dcd0488 (&dev->mode_config.mutex){+.+.}-{3:3}, at: drm_mode_getconnector+0x1c6/0x4a0
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #2 (&dev->mode_config.mutex){+.+.}-{3:3}:
         __mutex_lock+0xab/0x970
         drm_client_modeset_probe+0x22e/0xca0
         __drm_fb_helper_initial_config_and_unlock+0x42/0x540
         intel_fbdev_initial_config+0xf/0x20 [i915]
         async_run_entry_fn+0x28/0x130
         process_one_work+0x26d/0x5c0
         worker_thread+0x37/0x380
         kthread+0x144/0x170
         ret_from_fork+0x1f/0x30
-> #1 (&client->modeset_mutex){+.+.}-{3:3}:
         __mutex_lock+0xab/0x970
         drm_client_modeset_commit_locked+0x1c/0x180
         drm_client_modeset_commit+0x1c/0x40
         __drm_fb_helper_restore_fbdev_mode_unlocked+0x88/0xb0
         drm_fb_helper_set_par+0x34/0x40
         intel_fbdev_set_par+0x11/0x40 [i915]
         fbcon_init+0x270/0x4f0
         visual_init+0xc6/0x130
         do_bind_con_driver+0x1e5/0x2d0
         do_take_over_console+0x10e/0x180
         do_fbcon_takeover+0x53/0xb0
         register_framebuffer+0x22d/0x310
         __drm_fb_helper_initial_config_and_unlock+0x36c/0x540
         intel_fbdev_initial_config+0xf/0x20 [i915]
         async_run_entry_fn+0x28/0x130
         process_one_work+0x26d/0x5c0
         worker_thread+0x37/0x380
         kthread+0x144/0x170
         ret_from_fork+0x1f/0x30
-> #0 (&dev->master_mutex){+.+.}-{3:3}:
         __lock_acquire+0x151e/0x2590
         lock_acquire+0xd1/0x3d0
         __mutex_lock+0xab/0x970
         drm_is_current_master+0x1b/0x40
         drm_mode_getconnector+0x37e/0x4a0
         drm_ioctl_kernel+0xa8/0xf0
         drm_ioctl+0x1e8/0x390
         __x64_sys_ioctl+0x6a/0xa0
         do_syscall_64+0x39/0xb0
         entry_SYSCALL_64_after_hwframe+0x44/0xae
other info that might help us debug this:
Chain exists of: &dev->master_mutex --> &client->modeset_mutex --> &dev->mode_config.mutex
   Possible unsafe locking scenario:
         CPU0                    CPU1
         ----                    ----
    lock(&dev->mode_config.mutex);
                                 lock(&client->modeset_mutex);
                                 lock(&dev->mode_config.mutex);
    lock(&dev->master_mutex);
*** DEADLOCK ***

Hi Daniel,

Just a thought.

Since &dev->mode_config.mutex is the modeset BKL and its scope isn't
clear, keeping the dependency as is would mean that any lock that
depends on &dev->mode_config.mutex would also depend on
&client->modeset_mutex. Seems like this might be vulnerable to more
circular dependencies.

Would it make sense to invert the locking dependency for
&client->modeset_mutex and &dev->mode_config.mutex to become
&dev->mode_config.mutex --> &client->modeset_mutex? Something like this:

This isn't the problem I think, the problem is the inversion against
master_mutex. fbdev emulation code holds master mutex while doing
modesets, whereas getconnector checks for master status while holding
a modeset mutex. I think just pulling the drm_is_current_master check
out of getconnector should be fine to break the loop here?

Wrt changing the layering here: drm_client sits on top of the
lower-level modeset interfaces. So drm_client locks should defintiely
be outside of modeset locks, not the other way round. Also I'm not
sure why you're focusing on the client lock here, that's just a lock
that sits in the middle of the chain but otherwise doesn't matter. If
you flip these these two intermediate chain links are flipped, but the
overall chain is still closed. Heck you could completely remove
client->modeset_mutex, and the bug is still the same.

Cheers, Daniel


Right, scrap that thought then.

I was initially confused by the lockdep splat report because I was on the wrong drm-misc branch. Pulling drm_is_current_master out from the &dev->mode_config.mutex lock makes sense to me now. Thanks.

Best wishes,
Desmond



diff --git a/drivers/gpu/drm/drm_client_modeset.c
b/drivers/gpu/drm/drm_client_modeset.c
index ced09c7c06f9..859f99d97cde 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -811,9 +811,9 @@ int drm_client_modeset_probe(struct drm_client_dev
*client, unsigned int width,
                 goto out;
         }

+       mutex_lock(&dev->mode_config.mutex);
         mutex_lock(&client->modeset_mutex);

-       mutex_lock(&dev->mode_config.mutex);
         for (i = 0; i < connector_count; i++)
                 total_modes_count += connectors[i]->funcs->fill_modes(connectors[i],
width, height);
         if (!total_modes_count)
@@ -838,7 +838,6 @@ int drm_client_modeset_probe(struct drm_client_dev
*client, unsigned int width,
                 drm_client_pick_crtcs(client, connectors, connector_count,
                                       crtcs, modes, 0, width, height);
         }
-       mutex_unlock(&dev->mode_config.mutex);

         drm_client_modeset_release(client);

@@ -869,6 +868,7 @@ int drm_client_modeset_probe(struct drm_client_dev
*client, unsigned int width,
         }

         mutex_unlock(&client->modeset_mutex);
+       mutex_unlock(&dev->mode_config.mutex);
   out:
         kfree(crtcs);
         kfree(modes);
diff --git a/drivers/gpu/drm/drm_fb_helper.c
b/drivers/gpu/drm/drm_fb_helper.c
index f6baa2046124..74302d110609 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1443,8 +1443,8 @@ static int pan_display_legacy(struct
fb_var_screeninfo *var,
         struct drm_mode_set *modeset;
         int ret = 0;

-       mutex_lock(&client->modeset_mutex);
         drm_modeset_lock_all(fb_helper->dev);
+       mutex_lock(&client->modeset_mutex);
         drm_client_for_each_modeset(modeset, client) {
                 modeset->x = var->xoffset;
                 modeset->y = var->yoffset;
@@ -1457,8 +1457,8 @@ static int pan_display_legacy(struct
fb_var_screeninfo *var,
                         }
                 }
         }
-       drm_modeset_unlock_all(fb_helper->dev);
         mutex_unlock(&client->modeset_mutex);
+       drm_modeset_unlock_all(fb_helper->dev);

         return ret;
   }

1 lock held by kms_frontbuffer/1087:
   #0: ffff88810dcd0488 (&dev->mode_config.mutex){+.+.}-{3:3}, at: drm_mode_getconnector+0x1c6/0x4a0
stack backtrace:
CPU: 7 PID: 1087 Comm: kms_frontbuffer Not tainted 5.13.0-rc7-CI-CI_DRM_10254+ #1
Hardware name: Intel Corporation Ice Lake Client Platform/IceLake U DDR4 SODIMM PD RVP TLC, BIOS ICLSFWR1.R00.3234.A01.1906141750 06/14/2019
Call Trace:
   dump_stack+0x7f/0xad
   check_noncircular+0x12e/0x150
   __lock_acquire+0x151e/0x2590
   lock_acquire+0xd1/0x3d0
   __mutex_lock+0xab/0x970
   drm_is_current_master+0x1b/0x40
   drm_mode_getconnector+0x37e/0x4a0
   drm_ioctl_kernel+0xa8/0xf0
   drm_ioctl+0x1e8/0x390
   __x64_sys_ioctl+0x6a/0xa0
   do_syscall_64+0x39/0xb0
   entry_SYSCALL_64_after_hwframe+0x44/0xae

daniel@phenom:~/linux/drm-misc-fixes$ dim fixes 1815d9c86e3090477fbde066ff314a7e9721ee0f
Fixes: 1815d9c86e30 ("drm: add a locked version of drm_is_current_master")
Cc: Desmond Cheong Zhi Xi <desmondcheongzx@xxxxxxxxx>
Cc: Emil Velikov <emil.l.velikov@xxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
Cc: Maxime Ripard <mripard@xxxxxxxxxx>
Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
Cc: David Airlie <airlied@xxxxxxxx>
Cc: Daniel Vetter <daniel@xxxxxxxx>
---
   drivers/gpu/drm/drm_auth.c | 51 ++++++++++++++------------------------
   1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 86d4b72e95cb..232abbba3686 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -61,35 +61,6 @@
    * trusted clients.
    */

-static bool drm_is_current_master_locked(struct drm_file *fpriv)
-{
-     lockdep_assert_held_once(&fpriv->master->dev->master_mutex);
-
-     return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master;
-}
-
-/**
- * drm_is_current_master - checks whether @priv is the current master
- * @fpriv: DRM file private
- *
- * Checks whether @fpriv is current master on its device. This decides whether a
- * client is allowed to run DRM_MASTER IOCTLs.
- *
- * Most of the modern IOCTL which require DRM_MASTER are for kernel modesetting
- * - the current master is assumed to own the non-shareable display hardware.
- */
-bool drm_is_current_master(struct drm_file *fpriv)
-{
-     bool ret;
-
-     mutex_lock(&fpriv->master->dev->master_mutex);
-     ret = drm_is_current_master_locked(fpriv);
-     mutex_unlock(&fpriv->master->dev->master_mutex);
-
-     return ret;
-}
-EXPORT_SYMBOL(drm_is_current_master);
-
   int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
   {
       struct drm_auth *auth = data;
@@ -252,7 +223,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
       if (ret)
               goto out_unlock;

-     if (drm_is_current_master_locked(file_priv))
+     if (drm_is_current_master(file_priv))
               goto out_unlock;

       if (dev->master) {
@@ -301,7 +272,7 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
       if (ret)
               goto out_unlock;

-     if (!drm_is_current_master_locked(file_priv)) {
+     if (!drm_is_current_master(file_priv)) {
               ret = -EINVAL;
               goto out_unlock;
       }
@@ -350,7 +321,7 @@ void drm_master_release(struct drm_file *file_priv)
       if (file_priv->magic)
               idr_remove(&file_priv->master->magic_map, file_priv->magic);

-     if (!drm_is_current_master_locked(file_priv))
+     if (!drm_is_current_master(file_priv))
               goto out;

       drm_legacy_lock_master_cleanup(dev, master);
@@ -371,6 +342,22 @@ void drm_master_release(struct drm_file *file_priv)
       mutex_unlock(&dev->master_mutex);
   }

+/**
+ * drm_is_current_master - checks whether @priv is the current master
+ * @fpriv: DRM file private
+ *
+ * Checks whether @fpriv is current master on its device. This decides whether a
+ * client is allowed to run DRM_MASTER IOCTLs.
+ *
+ * Most of the modern IOCTL which require DRM_MASTER are for kernel modesetting
+ * - the current master is assumed to own the non-shareable display hardware.
+ */
+bool drm_is_current_master(struct drm_file *fpriv)
+{
+     return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master;
+}
+EXPORT_SYMBOL(drm_is_current_master);
+
   /**
    * drm_master_get - reference a master pointer
    * @master: &struct drm_master








[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux