Re: [PATCH] drm/amdgpu: fix deadlock while reading mqd from debugfs

["Sharma, Shashank" <shashank.sharma@xxxxxxx>]

+ Johannes

Regards
Shashank

On 13/03/2024 18:22, Sharma, Shashank wrote:
> Hello Johannes,
>
> On 07/03/2024 23:07, Johannes Weiner wrote:
> > An errant disk backup on my desktop got into debugfs and triggered the
> > following deadlock scenario in the amdgpu debugfs files. The machine
> > also hard-resets immediately after those lines are printed (although I
> > wasn't able to reproduce that part when reading by hand):
> >
> > [ 1318.016074][ T1082] 
> > ======================================================
> > [ 1318.016607][ T1082] WARNING: possible circular locking dependency 
> > detected
> > [ 1318.017107][ T1082] 6.8.0-rc7-00015-ge0c8221b72c0 #17 Not tainted
> > [ 1318.017598][ T1082] 
> > ------------------------------------------------------
> > [ 1318.018096][ T1082] tar/1082 is trying to acquire lock:
> > [ 1318.018585][ T1082] ffff98c44175d6a0 (&mm->mmap_lock){++++}-{3:3}, 
> > at: __might_fault+0x40/0x80
> > [ 1318.019084][ T1082]
> > [ 1318.019084][ T1082] but task is already holding lock:
> > [ 1318.020052][ T1082] ffff98c4c13f55f8 
> > (reservation_ww_class_mutex){+.+.}-{3:3}, at: 
> > amdgpu_debugfs_mqd_read+0x6a/0x250 [amdgpu]
> > [ 1318.020607][ T1082]
> > [ 1318.020607][ T1082] which lock already depends on the new lock.
> > [ 1318.020607][ T1082]
> > [ 1318.022081][ T1082]
> > [ 1318.022081][ T1082] the existing dependency chain (in reverse 
> > order) is:
> > [ 1318.023083][ T1082]
> > [ 1318.023083][ T1082] -> #2 (reservation_ww_class_mutex){+.+.}-{3:3}:
> > [ 1318.024114][ T1082] __ww_mutex_lock.constprop.0+0xe0/0x12f0
> > [ 1318.024639][ T1082]        ww_mutex_lock+0x32/0x90
> > [ 1318.025161][ T1082]        dma_resv_lockdep+0x18a/0x330
> > [ 1318.025683][ T1082]        do_one_initcall+0x6a/0x350
> > [ 1318.026210][ T1082]        kernel_init_freeable+0x1a3/0x310
> > [ 1318.026728][ T1082]        kernel_init+0x15/0x1a0
> > [ 1318.027242][ T1082]        ret_from_fork+0x2c/0x40
> > [ 1318.027759][ T1082]        ret_from_fork_asm+0x11/0x20
> > [ 1318.028281][ T1082]
> > [ 1318.028281][ T1082] -> #1 (reservation_ww_class_acquire){+.+.}-{0:0}:
> > [ 1318.029297][ T1082]        dma_resv_lockdep+0x16c/0x330
> > [ 1318.029790][ T1082]        do_one_initcall+0x6a/0x350
> > [ 1318.030263][ T1082]        kernel_init_freeable+0x1a3/0x310
> > [ 1318.030722][ T1082]        kernel_init+0x15/0x1a0
> > [ 1318.031168][ T1082]        ret_from_fork+0x2c/0x40
> > [ 1318.031598][ T1082]        ret_from_fork_asm+0x11/0x20
> > [ 1318.032011][ T1082]
> > [ 1318.032011][ T1082] -> #0 (&mm->mmap_lock){++++}-{3:3}:
> > [ 1318.032778][ T1082]        __lock_acquire+0x14bf/0x2680
> > [ 1318.033141][ T1082]        lock_acquire+0xcd/0x2c0
> > [ 1318.033487][ T1082]        __might_fault+0x58/0x80
> > [ 1318.033814][ T1082] amdgpu_debugfs_mqd_read+0x103/0x250 [amdgpu]
> > [ 1318.034181][ T1082]        full_proxy_read+0x55/0x80
> > [ 1318.034487][ T1082]        vfs_read+0xa7/0x360
> > [ 1318.034788][ T1082]        ksys_read+0x70/0xf0
> > [ 1318.035085][ T1082]        do_syscall_64+0x94/0x180
> > [ 1318.035375][ T1082] entry_SYSCALL_64_after_hwframe+0x46/0x4e
> > [ 1318.035664][ T1082]
> > [ 1318.035664][ T1082] other info that might help us debug this:
> > [ 1318.035664][ T1082]
> > [ 1318.036487][ T1082] Chain exists of:
> > [ 1318.036487][ T1082]   &mm->mmap_lock --> 
> > reservation_ww_class_acquire --> reservation_ww_class_mutex
> > [ 1318.036487][ T1082]
> > [ 1318.037310][ T1082]  Possible unsafe locking scenario:
> > [ 1318.037310][ T1082]
> > [ 1318.037838][ T1082]        CPU0                    CPU1
> > [ 1318.038101][ T1082]        ----                    ----
> > [ 1318.038350][ T1082]   lock(reservation_ww_class_mutex);
> > [ 1318.038590][ T1082] lock(reservation_ww_class_acquire);
> > [ 1318.038839][ T1082] lock(reservation_ww_class_mutex);
> > [ 1318.039083][ T1082]   rlock(&mm->mmap_lock);
> > [ 1318.039328][ T1082]
> > [ 1318.039328][ T1082]  *** DEADLOCK ***
> > [ 1318.039328][ T1082]
> > [ 1318.040029][ T1082] 1 lock held by tar/1082:
> > [ 1318.040259][ T1082]  #0: ffff98c4c13f55f8 
> > (reservation_ww_class_mutex){+.+.}-{3:3}, at: 
> > amdgpu_debugfs_mqd_read+0x6a/0x250 [amdgpu]
> > [ 1318.040560][ T1082]
> > [ 1318.040560][ T1082] stack backtrace:
> > [ 1318.041053][ T1082] CPU: 22 PID: 1082 Comm: tar Not tainted 
> > 6.8.0-rc7-00015-ge0c8221b72c0 #17 
> > 3316c85d50e282c5643b075d1f01a4f6365e39c2
> > [ 1318.041329][ T1082] Hardware name: Gigabyte Technology Co., Ltd. 
> > B650 AORUS PRO AX/B650 AORUS PRO AX, BIOS F20 12/14/2023
> > [ 1318.041614][ T1082] Call Trace:
> > [ 1318.041895][ T1082]  <TASK>
> > [ 1318.042175][ T1082]  dump_stack_lvl+0x4a/0x80
> > [ 1318.042460][ T1082]  check_noncircular+0x145/0x160
> > [ 1318.042743][ T1082]  __lock_acquire+0x14bf/0x2680
> > [ 1318.043022][ T1082]  lock_acquire+0xcd/0x2c0
> > [ 1318.043301][ T1082]  ? __might_fault+0x40/0x80
> > [ 1318.043580][ T1082]  ? __might_fault+0x40/0x80
> > [ 1318.043856][ T1082]  __might_fault+0x58/0x80
> > [ 1318.044131][ T1082]  ? __might_fault+0x40/0x80
> > [ 1318.044408][ T1082]  amdgpu_debugfs_mqd_read+0x103/0x250 [amdgpu 
> > 8fe2afaa910cbd7654c8cab23563a94d6caebaab]
> > [ 1318.044749][ T1082]  full_proxy_read+0x55/0x80
> > [ 1318.045042][ T1082]  vfs_read+0xa7/0x360
> > [ 1318.045333][ T1082]  ksys_read+0x70/0xf0
> > [ 1318.045623][ T1082]  do_syscall_64+0x94/0x180
> > [ 1318.045913][ T1082]  ? do_syscall_64+0xa0/0x180
> > [ 1318.046201][ T1082]  ? lockdep_hardirqs_on+0x7d/0x100
> > [ 1318.046487][ T1082]  ? do_syscall_64+0xa0/0x180
> > [ 1318.046773][ T1082]  ? do_syscall_64+0xa0/0x180
> > [ 1318.047057][ T1082]  ? do_syscall_64+0xa0/0x180
> > [ 1318.047337][ T1082]  ? do_syscall_64+0xa0/0x180
> > [ 1318.047611][ T1082]  entry_SYSCALL_64_after_hwframe+0x46/0x4e
> > [ 1318.047887][ T1082] RIP: 0033:0x7f480b70a39d
> > [ 1318.048162][ T1082] Code: 91 ba 0d 00 f7 d8 64 89 02 b8 ff ff ff 
> > ff eb b2 e8 18 a3 01 00 0f 1f 84 00 00 00 00 00 80 3d a9 3c 0e 00 00 
> > 74 17 31 c0 0f 05 <48> 3d 00 f0 ff ff 77 5b c3 66 2e 0f 1f 84 00 00 
> > 00 00 00 53 48 83
> > [ 1318.048769][ T1082] RSP: 002b:00007ffde77f5c68 EFLAGS: 00000246 
> > ORIG_RAX: 0000000000000000
> > [ 1318.049083][ T1082] RAX: ffffffffffffffda RBX: 0000000000000800 
> > RCX: 00007f480b70a39d
> > [ 1318.049392][ T1082] RDX: 0000000000000800 RSI: 000055c9f2120c00 
> > RDI: 0000000000000008
> > [ 1318.049703][ T1082] RBP: 0000000000000800 R08: 000055c9f2120a94 
> > R09: 0000000000000007
> > [ 1318.050011][ T1082] R10: 0000000000000000 R11: 0000000000000246 
> > R12: 000055c9f2120c00
> > [ 1318.050324][ T1082] R13: 0000000000000008 R14: 0000000000000008 
> > R15: 0000000000000800
> > [ 1318.050638][ T1082]  </TASK>
> >
> > amdgpu_debugfs_mqd_read() holds a reservation when it calls
> > put_user(), which may fault and acquire the mmap_sem. This violates
> > the established locking order.
> >
> > Bounce the mqd data through a kernel buffer to get put_user() out of
> > the illegal section.
> >
> > Fixes: 445d85e3c1df ("drm/amdgpu: add debugfs interface for reading 
> > MQDs")
> > Cc: stable@xxxxxxxxxxxxxxx        [v6.5+]
> > Signed-off-by: Johannes Weiner <hannes@xxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 46 +++++++++++++++---------
> >   1 file changed, 29 insertions(+), 17 deletions(-)
> >
> > This fixes the lockdep splat for me, and the hexdump of the output
> > looks sane after the patch. However, I'm not at all familiar with this
> > code to say for sure that this is the right solution. The mqd seems
> > small enough that the kmalloc won't get crazy.
> Yeah, MQDs sizes are within range (so far :))
> > I'm also assuming that
> > ring->mqd_size is safe to access before the reserve & kmap. Lastly I
> > went with an open loop instead of a memcpy() as I wasn't sure if that
> > memory is safe to address a byte at at time.
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> > index 5505d646f43a..06f0a6534a94 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> > @@ -524,46 +524,58 @@ static ssize_t amdgpu_debugfs_mqd_read(struct 
> > file *f, char __user *buf,
> >   {
> >       struct amdgpu_ring *ring = file_inode(f)->i_private;
> >       volatile u32 *mqd;
> > -    int r;
> > +    u32 *kbuf;
> > +    int r, i;
> >       uint32_t value, result;
> >         if (*pos & 3 || size & 3)
> >           return -EINVAL;
> >   -    result = 0;
> > +    kbuf = kmalloc(ring->mqd_size, GFP_KERNEL);
> > +    if (!kbuf)
> > +        return -ENOMEM;
> >         r = amdgpu_bo_reserve(ring->mqd_obj, false);
> >       if (unlikely(r != 0))
> > -        return r;
> > +        goto err_free;
> >         r = amdgpu_bo_kmap(ring->mqd_obj, (void **)&mqd);
> > -    if (r) {
> > -        amdgpu_bo_unreserve(ring->mqd_obj);
> > -        return r;
> > -    }
> > +    if (r)
> > +        goto err_unreserve;
> >   +    /*
> > +     * Copy to local buffer to avoid put_user(), which might fault
> > +     * and acquire mmap_sem, under reservation_ww_class_mutex.
> > +     */
> > +    for (i = 0; i < ring->mqd_size/sizeof(u32); i++)
> > +        kbuf[i] = mqd[i];
>
> memcpy is safe to use here.
>
> With that change, please feel free to use: Reviewed-by: Shashank 
> Sharma <shashank.sharma@xxxxxxx>
>
> - Shashank
>
> > +
> > +    amdgpu_bo_kunmap(ring->mqd_obj);
> > +    amdgpu_bo_unreserve(ring->mqd_obj);
> > +
> > +    result = 0;
> >       while (size) {
> >           if (*pos >= ring->mqd_size)
> > -            goto done;
> > +            break;
> >   -        value = mqd[*pos/4];
> > +        value = kbuf[*pos/4];
> >           r = put_user(value, (uint32_t *)buf);
> >           if (r)
> > -            goto done;
> > +            goto err_free;
> >           buf += 4;
> >           result += 4;
> >           size -= 4;
> >           *pos += 4;
> >       }
> >   -done:
> > -    amdgpu_bo_kunmap(ring->mqd_obj);
> > -    mqd = NULL;
> > -    amdgpu_bo_unreserve(ring->mqd_obj);
> > -    if (r)
> > -        return r;
> > -
> > +    kfree(kbuf);
> >       return result;
> > +
> > +err_unreserve:
> > +    amdgpu_bo_unreserve(ring->mqd_obj);
> > +err_free:
> > +    kfree(kbuf);
> > +    return r;
> >   }
> >     static const struct file_operations amdgpu_debugfs_mqd_fops = {