Skip to content

Commit f54b3ca

Browse files
committed
Revert "drm: add a locked version of drm_is_current_master"
This reverts commit 1815d9c. 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 *** 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 Note that this broke the intel-gfx CI pretty much across the board because it has to reboot machines after it hits a lockdep splat. Testcase: igt/debugfs_test/read_all_entries Acked-by: Petri Latvala <[email protected]> Fixes: 1815d9c ("drm: add a locked version of drm_is_current_master") Cc: Desmond Cheong Zhi Xi <[email protected]> Cc: Emil Velikov <[email protected]> Cc: [email protected] Signed-off-by: Daniel Vetter <[email protected]> Cc: Maarten Lankhorst <[email protected]> Cc: Maxime Ripard <[email protected]> Cc: Thomas Zimmermann <[email protected]> Cc: David Airlie <[email protected]> Cc: Daniel Vetter <[email protected]> Link: https://patchwork.freedesktop.org/patch/msgid/[email protected]
1 parent 1815d9c commit f54b3ca

File tree

1 file changed

+19
-32
lines changed

1 file changed

+19
-32
lines changed

drivers/gpu/drm/drm_auth.c

Lines changed: 19 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -61,35 +61,6 @@
6161
* trusted clients.
6262
*/
6363

64-
static bool drm_is_current_master_locked(struct drm_file *fpriv)
65-
{
66-
lockdep_assert_held_once(&fpriv->master->dev->master_mutex);
67-
68-
return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master;
69-
}
70-
71-
/**
72-
* drm_is_current_master - checks whether @priv is the current master
73-
* @fpriv: DRM file private
74-
*
75-
* Checks whether @fpriv is current master on its device. This decides whether a
76-
* client is allowed to run DRM_MASTER IOCTLs.
77-
*
78-
* Most of the modern IOCTL which require DRM_MASTER are for kernel modesetting
79-
* - the current master is assumed to own the non-shareable display hardware.
80-
*/
81-
bool drm_is_current_master(struct drm_file *fpriv)
82-
{
83-
bool ret;
84-
85-
mutex_lock(&fpriv->master->dev->master_mutex);
86-
ret = drm_is_current_master_locked(fpriv);
87-
mutex_unlock(&fpriv->master->dev->master_mutex);
88-
89-
return ret;
90-
}
91-
EXPORT_SYMBOL(drm_is_current_master);
92-
9364
int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
9465
{
9566
struct drm_auth *auth = data;
@@ -252,7 +223,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
252223
if (ret)
253224
goto out_unlock;
254225

255-
if (drm_is_current_master_locked(file_priv))
226+
if (drm_is_current_master(file_priv))
256227
goto out_unlock;
257228

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

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

353-
if (!drm_is_current_master_locked(file_priv))
324+
if (!drm_is_current_master(file_priv))
354325
goto out;
355326

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

345+
/**
346+
* drm_is_current_master - checks whether @priv is the current master
347+
* @fpriv: DRM file private
348+
*
349+
* Checks whether @fpriv is current master on its device. This decides whether a
350+
* client is allowed to run DRM_MASTER IOCTLs.
351+
*
352+
* Most of the modern IOCTL which require DRM_MASTER are for kernel modesetting
353+
* - the current master is assumed to own the non-shareable display hardware.
354+
*/
355+
bool drm_is_current_master(struct drm_file *fpriv)
356+
{
357+
return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master;
358+
}
359+
EXPORT_SYMBOL(drm_is_current_master);
360+
374361
/**
375362
* drm_master_get - reference a master pointer
376363
* @master: &struct drm_master

0 commit comments

Comments
 (0)