Limiting it to a PAGE_SIZE sounds reasonable to me as well. BTW: kvmalloc() is only needed for much larger allocations (up to 2GiB), for a page kmalloc() should perfectly do it.
And I agree as well that this is probably needed for gprwave handling as well. Christian. On 10/1/26 16:25, StDenis, Tom wrote: > 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; >>> } >>>
