github-mirror/linux
1
0
Fork 0
mirror of https://github.com/torvalds/linux.git synced 2026-10-11 12:58:02 +02:00
Code Issues Actions Packages Projects Releases Wiki Activity Graph

smb: smbdirect: avoid recursive listen.lock during cleanup

Browse Source 
__smbdirect_socket_schedule_cleanup() and smbdirect_socket_cleanup_work()
take sc->listen.lock and walk the listener's pending list, recursing
into smbdirect_socket_schedule_cleanup() for each child, and every
child takes its own listen.lock inside that walk.

This cannot deadlock for real: the nesting is strictly listener into
child, a child never listens, so the outer and the inner lock are
always different instances.  lockdep only tracks lock classes, though;
it sees the same class acquired twice and reports a possible recursive
locking deadlock.  This change therefore works around a lockdep
limitation rather than fixing a real hang, but the report is still
worth avoiding: lockdep disables itself after the first splat and then
hides real locking bugs for the rest of the run.

Only a socket that was a listener owns a populated listen.ready/pending
list; a child has empty lists and nothing to do in these blocks.  Guard
both of them with sc->listen.backlog != -1, the "was a listener" marker
that smbdirect_socket_destroy() already uses: listen.backlog leaves its
initial -1 exactly once, when smbdirect_socket_listen() succeeds.  The
alternative !sc->accept.listener test reads as "not a listener" while
meaning the opposite, and it is also true for an accepted child, whose
accept.listener has been cleared on hand-over.  With the guard the walk
only runs for a listener and never nests a child's listen.lock under
it; a pending child stays on its listener's list for the free path
(smbdirect_socket_destroy) to reap.

[  741.705044] WARNING: possible recursive locking detected
[  741.705403] 7.1.0-next-20260623+ #75 Not tainted
[  741.705695] --------------------------------------------
[  741.706022] ksmbd.control/18502 is trying to acquire lock:
[  741.706379] ffff888108d612f8 (&sc->listen.lock){....}-{3:3}, at: __smbdirect_socket_schedule_cleanup+0x719/0xd70
[  741.707008]
[  741.707008] but task is already holding lock:
[  741.707396] ffff8881087642f8 (&sc->listen.lock){....}-{3:3}, at: __smbdirect_socket_schedule_cleanup+0x719/0xd70
[  741.708025]
[  741.708025] other info that might help us debug this:
[  741.708448]  Possible unsafe locking scenario:
[  741.708448]
[  741.708845]        CPU0
[  741.709016]        ----
[  741.709186]   lock(&sc->listen.lock);
[  741.709453]   lock(&sc->listen.lock);
[  741.709705]
[  741.709705]  *** DEADLOCK ***
[  741.709705]
[  741.710095]  May be due to missing lock nesting notation
[  741.710095]
[  741.710663] 6 locks held by ksmbd.control/18502:
[  741.710975]  #0: ffff888109e51420 (sb_writers#7){.+.+}-{0:0}, at: vfs_write+0x1e7/0xc70
[  741.711561]  #1: ffff888126ec3880 (&of->mutex){+.+.}-{4:4}, at: kernfs_fop_write_iter+0x1be/0x4d0
[  741.712147]  #2: ffff888102af17b0 (kn->active#45){.+.+}-{0:0}, at: kernfs_fop_write_iter+0x205/0x4d0
[  741.712803]  #3: ffffffff85ad1e00 (ctrl_lock){+.+.}-{4:4}, at: kill_server_store+0x1e0/0x2b0
[  741.713381]  #4: ffffffff85ad41a0 (init_lock){+.+.}-{4:4}, at: ksmbd_conn_transport_destroy+0x5b/0x3c0
[  741.713995]  #5: ffff8881087642f8 (&sc->listen.lock){....}-{3:3}, at: __smbdirect_socket_schedule_cleanup+0x719/0xd70
[  741.714736]
[  741.714736] stack backtrace:
[  741.715038] CPU: 4 UID: 0 PID: 18502 Comm: ksmbd.control Not tainted 7.1.0-next-20260623+ #75 PREEMPT(lazy)
[  741.715043] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.17.0-debian-1.17.0-1 04/01/2014
[  741.715046] Call Trace:
[  741.715049]  <TASK>
[  741.715052]  dump_stack_lvl+0x77/0xa0
[  741.715058]  print_deadlock_bug+0x279/0x290
[  741.715065]  __lock_acquire+0x272a/0x2e30
[  741.715070]  ? stack_trace_save+0xae/0x100
[  741.715075]  ? smb_direct_logging_vaprintf+0x1a0/0x230
[  741.715079]  ? __pfx_smb_direct_logging_vaprintf+0x10/0x10
[  741.715082]  ? __timer_delete+0x58/0x320
[  741.715087]  lock_acquire+0xd3/0x270
[  741.715091]  ? __smbdirect_socket_schedule_cleanup+0x719/0xd70
[  741.715095]  ? entry_SYSCALL_64_after_hwframe+0x77/0x7f
[  741.715099]  _raw_spin_lock_irqsave+0x42/0x60
[  741.715105]  ? __smbdirect_socket_schedule_cleanup+0x719/0xd70

Note the two addresses above: ffff888108d612f8 is the child's lock,
ffff8881087642f8 the listener's, always distinct objects.

Found with ksmbdzzer [2], a KSMBD fuzzer that drives libFuzzer with a
kcov-dataflow [1] coverage vector: it folds each instrumented
comparison/argument's runtime operand value together with its PC (the
default arm mixes them as pc⊕val) so that a new operand value at a known
site counts as new coverage.

[1] https://lwn.net/Articles/1077606/
[2] https://github.com/yskzalloc/kcov-dataflow

Fixes: dc691b91ad ("smb: smbdirect: introduce smbdirect_socket_{listen,accept}()")
Signed-off-by: Yunseong Kim <yunseong.kim@est.tech>
Reviewed-by: Stefan Metzmacher <metze@samba.org>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
This commit is contained in:
Yunseong Kim 2026-08-05 02:46:56 +02:00 committed by Namjae Jeon
parent 383a9480f5
commit 76fa42c004
1 changed files with 32 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
fs/smb/smbdirect/socket.c
View File

@ -305,12 +305,26 @@ void __smbdirect_socket_schedule_cleanup(struct smbdirect_socket *sc,
* disconnect all pending and ready sockets
*
* First we move ready sockets to pending again.
*
* Only a socket that was a listener (listen.backlog != -1) owns a
* populated listen.ready/pending list. Guarding on that also keeps
* lockdep quiet: without it, the listener holds sc->listen.lock while
* the loop recurses into each child psc, which takes psc->listen.lock.
* Those are always different instances of the same lock class -- a
* child never listens, so the nesting is strictly listener -> child
* and cannot really deadlock -- but lockdep only sees the class and
* reports "possible recursive locking". A child has empty listen
* lists and nothing to do here, so skipping it loses nothing, and a
* pending child stays on its listener's list for the free path
* (smbdirect_socket_destroy) to reap.
*/
spin_lock_irqsave(&sc->listen.lock, flags);
list_splice_init(&sc->listen.ready, &sc->listen.pending);
list_for_each_entry_safe(psc, tsc, &sc->listen.pending, accept.list)
smbdirect_socket_schedule_cleanup(psc, sc->first_error);
spin_unlock_irqrestore(&sc->listen.lock, flags);
if (sc->listen.backlog != -1) { /* was a listener */
spin_lock_irqsave(&sc->listen.lock, flags);
list_splice_init(&sc->listen.ready, &sc->listen.pending);
list_for_each_entry_safe(psc, tsc, &sc->listen.pending, accept.list)
smbdirect_socket_schedule_cleanup(psc, sc->first_error);
spin_unlock_irqrestore(&sc->listen.lock, flags);
}
switch (sc->status) {
case SMBDIRECT_SOCKET_RESOLVE_ADDR_FAILED:
@ -405,12 +419,20 @@ static void smbdirect_socket_cleanup_work(struct work_struct *work)
* disconnect all pending and ready sockets
*
* First we move ready sockets to pending again.
*
* Guarded on listen.backlog != -1 for the same reason as in
* __smbdirect_socket_schedule_cleanup(): only a listener owns a
* populated listen list, and skipping the block for a child avoids
* nesting psc->listen.lock under a listener's listen.lock (different
* instances of one class -- harmless, but lockdep cannot tell).
*/
spin_lock_irqsave(&sc->listen.lock, flags);
list_splice_init(&sc->listen.ready, &sc->listen.pending);
list_for_each_entry_safe(psc, tsc, &sc->listen.pending, accept.list)
smbdirect_socket_schedule_cleanup(psc, sc->first_error);
spin_unlock_irqrestore(&sc->listen.lock, flags);
if (sc->listen.backlog != -1) { /* was a listener */
spin_lock_irqsave(&sc->listen.lock, flags);
list_splice_init(&sc->listen.ready, &sc->listen.pending);
list_for_each_entry_safe(psc, tsc, &sc->listen.pending, accept.list)
smbdirect_socket_schedule_cleanup(psc, sc->first_error);
spin_unlock_irqrestore(&sc->listen.lock, flags);
}
switch (sc->status) {
case SMBDIRECT_SOCKET_NEGOTIATE_NEEDED: