Skip to content

Commit

Permalink
Revert "drm: add a locked version of drm_is_current_master"
Browse files Browse the repository at this point in the history
commit f54b3ca upstream.

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);
  • Loading branch information
danvet authored and Sasha Levin committed Jun 30, 2021
1 parent 54ab8b0 commit 2b2e592
Showing 1 changed file with 19 additions and 32 deletions.
51 changes: 19 additions & 32 deletions drivers/gpu/drm/drm_auth.c
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Expand All @@ -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
Expand Down

0 comments on commit 2b2e592

Please sign in to comment.