Thanks Mikhail ! Merged.
Vitaly
On 2026-10-01 12:13, 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 in amdgpu_lockdep.c 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. The copies under the locks
> are as old as the bank selection in both files; commit 1d0f5838b126
> ("drm/amdgpu: Add lockdep annotations for lock ordering validation")
> only made them visible to lockdep.
>
> 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.
> The buffer is sized by the count passed to read() or write(), so an
> access larger than PAGE_SIZE is now rejected with -EINVAL; umr reads and
> writes one register at a time. 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: 566281595037 ("drm/amd/amdgpu: Add bank selection for MMIO debugfs
> (v3)")
> Fixes: 37df9560cd3e ("drm/amd/amdgpu: New debugfs interface for MMIO
> registers (v5)")
> Acked-by: Christian König <[email protected]>
> Acked-by: Tom St Denis <[email protected]>
> Reviewed-by: Vitaly Prosyak <[email protected]>
> Signed-off-by: Mikhail Gavrilov <[email protected]>
> ---
>
> Notes:
> v2:
> - Fixes: point at the commits that introduced the user copies under the
> locks, not at the lockdep annotation that made them visible
> (Christian).
> - Reject an access larger than PAGE_SIZE before allocating the buffer
> (Vitaly); umr only does 4-byte accesses (Tom).
> - Allocate the buffer with kmalloc() instead of kvmalloc() (Christian).
> - amdgpu_debugfs_gprwave_read() needs no change: it already calls
> put_user() after dropping grbm_idx_mutex.
> - Christian's and Tom's Acked-by were given on v1; the size check and
> kmalloc() are the only code changes since.
> - The test notes went out as a separate reply to v1; they are part of
> the patch mail now.
>
> v1:
> https://lore.kernel.org/amd-gfx/[email protected]/
>
> Tested v2 on Navi 31 (gfx1100), 7.3.0-rc5 at 551c722f4080, with lockdep.
>
> Without this patch the first GRBM-banked access through amdgpu_regs2
> produces the report above: from the write branch when umr halts
> waves, and from the read branch for a single
> "umr -b x x x -r '*.gfx1100.regGRBM_STATUS'" on a fresh boot.
>
> With this patch, and with the chain from reservation_ww_class_mutex
> through &amdgpu_notifier_lock_key down to &amdgpu_grbm_lock_key present
> in /proc/lockdep, none of the following produced a report, and
> debug_locks stayed 1:
> - the banked read of GRBM_STATUS* above;
> - "umr -O halt_waves -wa gfx_0.0.0" (banked SQ_CMD writes);
> - writing SCRATCH_REG0 and reading it back through amdgpu_regs2 with
> GRBM broadcast, and through amdgpu_regs with the bank bits in the
> offset; both reads returned the value written.
> A 4100-byte read of amdgpu_regs and of amdgpu_regs2 fails with EINVAL.
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 77 ++++++++++++---------
> 1 file changed, 46 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> index aca1a8045a..ab7ccfaea2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> @@ -121,6 +121,20 @@ static int amdgpu_debugfs_process_reg_op(bool read,
> struct file *f,
>
> *pos &= (1UL << 22) - 1;
>
> + if (size > PAGE_SIZE)
> + 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(kfree) = kmalloc(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 +166,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 +190,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 +244,24 @@ 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;
>
> + if (size > PAGE_SIZE)
> + 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(kfree) = kmalloc(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 +300,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 +328,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;
> }
>
>
> base-commit: 551c722f40809618230001baccf219193e22fc5a
