| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <wsdy24ihyizjw5ce4jnomhos3zwnbzdjx63qxywv5ud6kekcf5@ocqq2p6qobu3>
Date: Tue, 5 Aug 2025 15:16:17 -0300
From: Enzo Matsumiya <ematsumiya@...e.de>
To: Steve French <smfrench@...il.com>
Cc: Wang Zhaolong <wangzhaolong@...weicloud.com>, pshilov@...rosoft.com,
linux-cifs@...r.kernel.org, samba-technical@...ts.samba.org, linux-kernel@...r.kernel.org,
chengzhihao1@...wei.com, yi.zhang@...wei.com, yangerkun@...wei.com
Subject: Re: [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client
On 08/05, Steve French wrote:
>The first three patches (cleanup) look fine and have added to
>cifs-2.6.git for-next (also added Enzo Acked-by) but the fourth patch
>("smb: client: fix mid_q_entry memleak leak with per-mid locking")
>causes xfstest generic/001 to fail with signing enabled. See
>http://smb311-linux-testing.southcentralus.cloudapp.azure.com/#/builders/5/builds/58/steps/34/logs/stdio
>and http://smb311-linux-testing.southcentralus.cloudapp.azure.com/#/builders/5/builds/59/steps/34/logs/stdio
Was about to reply here as I was testing (an unrelated patch) with generic/100
and got the same backtrace.
@Wang btw sorry I missed your reproducer in the bugzilla link, I'll take
a look. Thanks!
>[Tue Aug 5 11:03:32 2025] run fstests generic/001 at 2025-08-05 11:03:32
>[Tue Aug 5 11:03:33 2025] =============================
>[Tue Aug 5 11:03:33 2025] [ BUG: Invalid wait context ]
>[Tue Aug 5 11:03:33 2025] 6.16.0 #1 Tainted: G E
>[Tue Aug 5 11:03:33 2025] -----------------------------
>[Tue Aug 5 11:03:33 2025] cifsd/24912 is trying to lock:
>[Tue Aug 5 11:03:33 2025] ffffffffafc14630
>(crypto_alg_sem){++++}-{4:4}, at: crypto_alg_lookup+0x40/0x120
>[Tue Aug 5 11:03:33 2025] other info that might help us debug this:
>[Tue Aug 5 11:03:33 2025] context-{5:5}
>[Tue Aug 5 11:03:33 2025] 1 lock held by cifsd/24912:
>[Tue Aug 5 11:03:33 2025] #0: ff11000134c25870
>(&temp->mid_lock){+.+.}-{3:3}, at: mid_execute_callback+0x19/0x40
>[cifs]
>[Tue Aug 5 11:03:33 2025] stack backtrace:
>[Tue Aug 5 11:03:33 2025] CPU: 1 UID: 0 PID: 24912 Comm: cifsd
>Tainted: G E 6.16.0 #1 PREEMPT(voluntary)
>[Tue Aug 5 11:03:33 2025] Tainted: [E]=UNSIGNED_MODULE
>[Tue Aug 5 11:03:33 2025] Hardware name: Red Hat KVM, BIOS
>1.16.3-4.el9 04/01/2014
>[Tue Aug 5 11:03:33 2025] Call Trace:
>[Tue Aug 5 11:03:33 2025] <TASK>
>[Tue Aug 5 11:03:33 2025] dump_stack_lvl+0x79/0xb0
>[Tue Aug 5 11:03:33 2025] __lock_acquire+0xace/0x21c0
>[Tue Aug 5 11:03:33 2025] ? check_irq_usage+0xa4/0xa80
>[Tue Aug 5 11:03:33 2025] lock_acquire+0x143/0x2d0
>[Tue Aug 5 11:03:33 2025] ? crypto_alg_lookup+0x40/0x120
>[Tue Aug 5 11:03:33 2025] ? check_noncircular+0x71/0x120
>[Tue Aug 5 11:03:33 2025] down_read+0x7c/0x2e0
>[Tue Aug 5 11:03:33 2025] ? crypto_alg_lookup+0x40/0x120
>[Tue Aug 5 11:03:33 2025] ? __pfx_down_read+0x10/0x10
>[Tue Aug 5 11:03:33 2025] ? lockdep_unlock+0x51/0xc0
>[Tue Aug 5 11:03:33 2025] ? __lock_acquire+0x11ee/0x21c0
>[Tue Aug 5 11:03:33 2025] crypto_alg_lookup+0x40/0x120
>[Tue Aug 5 11:03:33 2025] crypto_alg_mod_lookup+0x53/0x2b0
>[Tue Aug 5 11:03:33 2025] crypto_alloc_tfm_node+0x76/0x130
>[Tue Aug 5 11:03:33 2025] cifs_alloc_hash+0x44/0x130 [cifs]
>[Tue Aug 5 11:03:33 2025] smb3_calc_signature+0x4f0/0x7b0 [cifs]
>[Tue Aug 5 11:03:33 2025] ? __pfx_smb3_calc_signature+0x10/0x10 [cifs]
>[Tue Aug 5 11:03:33 2025] ? find_held_lock+0x2b/0x80
>[Tue Aug 5 11:03:33 2025] ? tcp_recvmsg+0xc9/0x2d0
>[Tue Aug 5 11:03:33 2025] ? rcu_is_watching+0x20/0x50
>[Tue Aug 5 11:03:33 2025] ? trace_irq_enable.constprop.0+0xac/0xe0
>[Tue Aug 5 11:03:33 2025] ? tcp_recvmsg+0xc9/0x2d0
>[Tue Aug 5 11:03:33 2025] ? __local_bh_enable_ip+0x90/0xf0
>[Tue Aug 5 11:03:33 2025] ? sock_has_perm+0x97/0x1a0
>[Tue Aug 5 11:03:33 2025] smb2_verify_signature+0x178/0x290 [cifs]
>[Tue Aug 5 11:03:33 2025] ? __pfx_smb2_verify_signature+0x10/0x10 [cifs]
>[Tue Aug 5 11:03:33 2025] ? look_up_lock_class+0x5d/0x140
>[Tue Aug 5 11:03:33 2025] smb2_check_receive+0x154/0x1c0 [cifs]
>[Tue Aug 5 11:03:33 2025] ? __pfx_smb2_check_receive+0x10/0x10 [cifs]
>[Tue Aug 5 11:03:33 2025] ? __lock_acquire+0x3f1/0x21c0
>[Tue Aug 5 11:03:33 2025] ? __lock_acquire+0x3f1/0x21c0
>[Tue Aug 5 11:03:33 2025] smb2_writev_callback+0x1f2/0x870 [cifs]
>[Tue Aug 5 11:03:33 2025] ? lock_acquire+0x143/0x2d0
>[Tue Aug 5 11:03:33 2025] ? mid_execute_callback+0x19/0x40 [cifs]
>[Tue Aug 5 11:03:33 2025] ? __pfx_smb2_writev_callback+0x10/0x10 [cifs]
>[Tue Aug 5 11:03:33 2025] ? do_raw_spin_lock+0x10c/0x190
>[Tue Aug 5 11:03:33 2025] ? __pfx_do_raw_spin_lock+0x10/0x10
>[Tue Aug 5 11:03:33 2025] ? _raw_spin_unlock+0x23/0x40
>[Tue Aug 5 11:03:33 2025] mid_execute_callback+0x33/0x40 [cifs]
>[Tue Aug 5 11:03:33 2025] cifs_demultiplex_thread+0xc95/0x15e0 [cifs]
>[Tue Aug 5 11:03:33 2025] ? __pfx_cifs_demultiplex_thread+0x10/0x10 [cifs]
>[Tue Aug 5 11:03:33 2025] ? find_held_lock+0x2b/0x80
>[Tue Aug 5 11:03:33 2025] ? __kthread_parkme+0x4b/0xd0
>[Tue Aug 5 11:03:33 2025] ? __pfx_cifs_demultiplex_thread+0x10/0x10 [cifs]
>[Tue Aug 5 11:03:33 2025] kthread+0x216/0x3e0
>[Tue Aug 5 11:03:33 2025] ? __pfx_kthread+0x10/0x10
>[Tue Aug 5 11:03:33 2025] ? __pfx_kthread+0x10/0x10
>[Tue Aug 5 11:03:33 2025] ? lock_release+0xc4/0x270
>[Tue Aug 5 11:03:33 2025] ? rcu_is_watching+0x20/0x50
>[Tue Aug 5 11:03:33 2025] ? __pfx_kthread+0x10/0x10
>[Tue Aug 5 11:03:33 2025] ret_from_fork+0x23a/0x2e0
>[Tue Aug 5 11:03:33 2025] ? __pfx_kthread+0x10/0x10
>[Tue Aug 5 11:03:33 2025] ret_from_fork_asm+0x1a/0x30
>[Tue Aug 5 11:03:33 2025] </TASK>
>
>(it worked without the patch see e.g.
>http://smb311-linux-testing.southcentralus.cloudapp.azure.com/#/builders/5/builds/60
>and http://smb311-linux-testing.southcentralus.cloudapp.azure.com/#/builders/5/builds/56)
>
>On Tue, Aug 5, 2025 at 1:54 AM Wang Zhaolong
><wangzhaolong@...weicloud.com> wrote:
>>
>> I've been investigating a pretty nasty memory leak in the SMB client. When
>> compound requests get interrupted by signals, we end up with mid_q_entry
>> structures and server buffers that never get freed[1].
>>
>> User foreground process cifsd
>> cifs_readdir
>> open_cached_dir
>> cifs_send_recv
>> compound_send_recv
>> smb2_setup_request
>> smb2_mid_entry_alloc
>> smb2_get_mid_entry
>> smb2_mid_entry_alloc
>> mempool_alloc // alloc mid
>> kref_init(&temp->refcount); // refcount = 1
>> mid[0]->callback = cifs_compound_callback;
>> mid[1]->callback = cifs_compound_last_callback;
>> smb_send_rqst
>> rc = wait_for_response
>> wait_event_state TASK_KILLABLE
>> cifs_demultiplex_thread
>> allocate_buffers
>> server->bigbuf = cifs_buf_get()
>> standard_receive3
>> ->find_mid()
>> smb2_find_mid
>> __smb2_find_mid
>> kref_get(&mid->refcount) // +1
>> cifs_handle_standard
>> handle_mid
>> /* bigbuf will also leak */
>> mid->resp_buf = server->bigbuf
>> server->bigbuf = NULL;
>> dequeue_mid
>> /* in for loop */
>> mids[0]->callback
>> cifs_compound_callback
>> /* Signal interrupts wait: rc = -ERESTARTSYS */
>> /* if (... || midQ[i]->mid_state == MID_RESPONSE_RECEIVED) *?
>> midQ[0]->callback = cifs_cancelled_callback;
>> cancelled_mid[i] = true;
>> /* The change comes too late */
>> mid->mid_state = MID_RESPONSE_READY
>> release_mid // -1
>> /* cancelled_mid[i] == true causes mid won't be released
>> in compound_send_recv cleanup */
>> /* cifs_cancelled_callback won't executed to release mid */
>>
>> The core issue is a race condition where cifs_cancelled_callback never
>> gets a chance to run, so cleanup never happens. I've spent quite a bit
>> of time trying to understand how to fix this safely.
>>
>> Honestly, my first instinct was to just patch the callback assignment
>> logic directly. But the more I dug into it, the more I realized that
>> the current locking scheme makes this really tricky to do safely. We
>> have one big lock protecting multiple different things, and trying to
>> fix the race condition directly felt like playing with fire.
>>
>> I kept running into scenarios where a "simple" fix could introduce
>> deadlocks or new race conditions. After looking at this from different
>> angles, I came to the conclusion that I needed to refactor the locking
>> first to create a safe foundation for the actual fix.
>>
>> Patches 1-3 are foundational refactoring. These three patches rename
>> locks for clarity, separate counter protection from queue operations,
>> and replace the confusing mid_flags bitmask with explicit boolean
>> fields. Basically, they untangle the current locking mess so I can
>> implement the real fix without breaking anything.
>>
>> The 4th patch in the series is where the real fix happens. With
>> the previous refactoring in place, I could safely add a lock to each
>> mid_q_entry and implement atomic callback execution. This eliminates
>> the race condition that was causing the leaks.
>>
>> In summary, my approach to the fix is to use smaller-grained locking to
>> avoid race conditions. However, during the implementation process,
>> this approach involves more changes than I initially hoped for. If
>> there's a simpler or more elegant way to fix this race condition that
>> I've missed, I'd love to hear about it. I've tried to be thorough in
>> my analysis, but I know there are folks with more experience in this
>> codebase who might see a better path.
>>
>> V1 -> V2:
>> - Inline the mid_execute_callback() in the smb2ops.c to eliminate
>> the sparse warning.
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=220404 [1]
>>
>> Wang Zhaolong (4):
>> smb: client: rename server mid_lock to mid_queue_lock
>> smb: client: add mid_counter_lock to protect the mid counter counter
>> smb: client: smb: client: eliminate mid_flags field
>> smb: client: fix mid_q_entry memleak leak with per-mid locking
>>
>> fs/smb/client/cifs_debug.c | 12 ++++--
>> fs/smb/client/cifsglob.h | 22 ++++++-----
>> fs/smb/client/connect.c | 57 +++++++++++++++++----------
>> fs/smb/client/smb1ops.c | 23 +++++++----
>> fs/smb/client/smb2ops.c | 72 +++++++++++++++++++----------------
>> fs/smb/client/smb2transport.c | 5 ++-
>> fs/smb/client/transport.c | 71 ++++++++++++++++++----------------
>> 7 files changed, 152 insertions(+), 110 deletions(-)
>>
>> --
>> 2.39.2
>>
>>
>
>
>--
>Thanks,
>
>Steve
>
Powered by blists - more mailing lists