On 9/30/26 17:49, Mikhail Gavrilov wrote:
> Reading or writing a register through the amdgpu_regs2 debugfs file
> with GRBM bank selection, which umr does for every banked access (a
> single "umr -b x x x -r <reg>" is enough, as is the wave dump with
> "-O halt_waves"), makes lockdep report a circular locking dependency,
> after which lockdep stays off until reboot:
>
> WARNING: possible circular locking dependency detected
> umr/63306 is trying to acquire lock:
> ffff8882cf4b8350 (&mm->mmap_lock){++++}-{4:4}, at: __might_fault+0x9b/0x140
> but task is already holding lock:
> ffff8881aec010d0 (&amdgpu_grbm_lock_key){+.+.}-{4:4}, at:
> amdgpu_debugfs_regs2_op.isra.0+0x20d/0xd00 [amdgpu]
> ...
> Chain exists of:
> &mm->mmap_lock --> &amdgpu_srbm_lock_key --> &amdgpu_grbm_lock_key
> ...
> __might_fault+0xc5/0x140
> amdgpu_debugfs_regs2_op.isra.0+0x441/0xd00 [amdgpu]
> full_proxy_write+0x10e/0x1d0
> vfs_write+0x1d0/0xf50
> ksys_write+0x116/0x250
>
> amdgpu_debugfs_regs2_op() and amdgpu_debugfs_process_reg_op() call
> get_user()/put_user() for every dword while holding grbm_idx_mutex or
> srbm_mutex, and pm.mutex if requested, so a fault there takes mmap_lock
> inside those locks. The lock hierarchy declared in amdgpu_lockdep.c since
> commit 1d0f5838b126
> ("drm/amdgpu: Add lockdep annotations for lock ordering validation") puts
> srbm_mutex and grbm_idx_mutex below notifier_lock, which
> amdgpu_cs_submit() takes with the BO reservations held, and
> dma_resv_lockdep() orders the reservations inside mmap_lock. So
> mmap_lock is outside grbm_idx_mutex and srbm_mutex, and these two
> debugfs paths nest it the other way round.
>
> Copy the user buffer in before any of the locks is taken and the result
> out after all of them are dropped, as amdgpu_debugfs_wave_read(),
> amdgpu_debugfs_gpr_read() and amdgpu_debugfs_gprwave_read() already do.
> A fault on the user buffer of a write now fails with -EFAULT before any
> register is written, instead of after a partial write.
>
> Fixes: 1d0f5838b126 ("drm/amdgpu: Add lockdep annotations for lock ordering
> validation")
Well that's not really correct since that patch only makes the problem visible
and not introduce it :)
But apart from that rather good catch, Vitalies work to add lockdep annotations
pays of.
> Signed-off-by: Mikhail Gavrilov <[email protected]>
I don't have time to review this in detail. Vitaly and Tom please take a look
at this.
But feel free to add Acked-by: Christian König <[email protected]>
Thanks in advance,
Christian.
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 71 ++++++++++++---------
> 1 file changed, 40 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> index aca1a8045afa..5618bd3e2610 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> @@ -121,6 +121,17 @@ static int amdgpu_debugfs_process_reg_op(bool read,
> struct file *f,
>
> *pos &= (1UL << 22) - 1;
>
> + /*
> + * Access the user buffer only while none of the locks below are held:
> + * a fault takes mmap_lock, and grbm_idx_mutex and srbm_mutex nest
> + * inside it.
> + */
> + u32 *data __free(kvfree) = kvmalloc(size, GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> + if (!read && copy_from_user(data, buf, size))
> + return -EFAULT;
> +
> r = pm_runtime_get_sync(adev_to_drm(adev)->dev);
> if (r < 0) {
> pm_runtime_put_autosuspend(adev_to_drm(adev)->dev);
> @@ -152,28 +163,16 @@ static int amdgpu_debugfs_process_reg_op(bool read,
> struct file *f,
> mutex_lock(&adev->pm.mutex);
>
> while (size) {
> - uint32_t value;
> -
> - if (read) {
> - value = RREG32(*pos >> 2);
> - r = put_user(value, (uint32_t *)buf);
> - } else {
> - r = get_user(value, (uint32_t *)buf);
> - if (!r)
> - amdgpu_mm_wreg_mmio_rlc(adev, *pos >> 2, value,
> 0);
> - }
> - if (r) {
> - result = r;
> - goto end;
> - }
> + if (read)
> + data[result >> 2] = RREG32(*pos >> 2);
> + else
> + amdgpu_mm_wreg_mmio_rlc(adev, *pos >> 2, data[result >>
> 2], 0);
>
> result += 4;
> - buf += 4;
> *pos += 4;
> size -= 4;
> }
>
> -end:
> if (use_bank) {
> amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff,
> 0xffffffff, 0);
> mutex_unlock(&adev->grbm_idx_mutex);
> @@ -188,6 +187,10 @@ static int amdgpu_debugfs_process_reg_op(bool read,
> struct file *f,
> pm_runtime_put_autosuspend(adev_to_drm(adev)->dev);
>
> amdgpu_virt_disable_access_debugfs(adev);
> +
> + if (read && copy_to_user(buf, data, result))
> + return -EFAULT;
> +
> return result;
> }
>
> @@ -238,11 +241,21 @@ static ssize_t amdgpu_debugfs_regs2_op(struct file *f,
> char __user *buf, u32 off
> struct amdgpu_device *adev = rd->adev;
> ssize_t result = 0;
> int r;
> - uint32_t value;
>
> if (size & 0x3 || offset & 0x3)
> return -EINVAL;
>
> + /*
> + * Access the user buffer only while none of the locks below are held:
> + * a fault takes mmap_lock, and grbm_idx_mutex and srbm_mutex nest
> + * inside it.
> + */
> + u32 *data __free(kvfree) = kvmalloc(size, GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> + if (write_en && copy_from_user(data, buf, size))
> + return -EFAULT;
> +
> r = pm_runtime_get_sync(adev_to_drm(adev)->dev);
> if (r < 0) {
> pm_runtime_put_autosuspend(adev_to_drm(adev)->dev);
> @@ -281,24 +294,16 @@ static ssize_t amdgpu_debugfs_regs2_op(struct file *f,
> char __user *buf, u32 off
> mutex_lock(&adev->pm.mutex);
>
> while (size) {
> - if (!write_en) {
> - value = RREG32(offset >> 2);
> - r = put_user(value, (uint32_t *)buf);
> - } else {
> - r = get_user(value, (uint32_t *)buf);
> - if (!r)
> - amdgpu_mm_wreg_mmio_rlc(adev, offset >> 2,
> value, rd->id.xcc_id);
> - }
> - if (r) {
> - result = r;
> - goto end;
> - }
> + if (!write_en)
> + data[result >> 2] = RREG32(offset >> 2);
> + else
> + amdgpu_mm_wreg_mmio_rlc(adev, offset >> 2, data[result
> >> 2],
> + rd->id.xcc_id);
> offset += 4;
> size -= 4;
> result += 4;
> - buf += 4;
> }
> -end:
> +
> if (rd->id.use_grbm) {
> amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff,
> 0xffffffff, rd->id.xcc_id);
> mutex_unlock(&adev->grbm_idx_mutex);
> @@ -317,6 +322,10 @@ static ssize_t amdgpu_debugfs_regs2_op(struct file *f,
> char __user *buf, u32 off
> pm_runtime_put_autosuspend(adev_to_drm(adev)->dev);
>
> amdgpu_virt_disable_access_debugfs(adev);
> +
> + if (!write_en && copy_to_user(buf, data, result))
> + return -EFAULT;
> +
> return result;
> }
>
