[PATCH] Revert "drm: add a locked version of drm_is_current_master"
Desmond Cheong Zhi Xi
desmondcheongzx at gmail.com
Wed Jun 23 09:51:23 UTC 2021
On 23/6/21 4:14 pm, Daniel Vetter wrote:
> On Wed, Jun 23, 2021 at 10:09 AM Desmond Cheong Zhi Xi
> <desmondcheongzx at gmail.com> 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 at 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 at gmail.com>
>>> Cc: Emil Velikov <emil.l.velikov at gmail.com>
>>> Cc: stable at vger.kernel.org
>>> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
>>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>> Cc: Maxime Ripard <mripard at kernel.org>
>>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
>>> Cc: David Airlie <airlied at linux.ie>
>>> Cc: Daniel Vetter <daniel at ffwll.ch>
>>> ---
>>> 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
>>>
>>
>
>
More information about the dri-devel
mailing list