There is minimal checking on top (e.g. size & 3 and offset & 3) which is required by both the hardware and umr. I agree the PAGE_SIZE check is a good idea.
umr really should only be doing 4 byte accesses at a time which is how the inner library API is designed it's generally assumed registers aren't contiguous. I wonder if a similar fix is needed for gprwave reads as well? Tom ________________________________________ From: Prosyak, Vitaly <[email protected]> Sent: Thursday, October 1, 2026 10:00 To: Koenig, Christian; Mikhail Gavrilov; [email protected] Cc: Deucher, Alexander; Prosyak, Vitaly; StDenis, Tom Subject: Re: [PATCH] drm/amdgpu: copy debugfs register data outside the GRBM/SRBM locks Mikhail's proposed kvmalloc(size, GFP_KERNEL) correctly stages user data outside the GRBM/SRBM locks, avoiding a potential page fault while holding those locks. However, since size is derived directly from a userspace read or write request, we must validate it before allocating memory in both amdgpu_debugfs_process_reg_op() and amdgpu_debugfs_regs2_op(). Tom, do you agree with this? Will adding a restriction here break any UMR requests? Please add a bounds check in each handler after the existing alignment/offset validation and immediately before kvmalloc(): if (size > PAGE_SIZE) return -EINVAL; data = kvmalloc(size, GFP_KERNEL); if (!data) return -ENOMEM; With that input validation addressed: Reviewed-by: Vitaly Prosyak <[email protected]> On 2026-10-01 02:12, Christian König wrote: > 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; >> } >>
