* [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client
@ 2025-08-05 6:47 Wang Zhaolong
2025-08-05 6:47 ` [PATCH V2 1/4] smb: client: rename server mid_lock to mid_queue_lock Wang Zhaolong
` (5 more replies)
0 siblings, 6 replies; 14+ messages in thread
From: Wang Zhaolong @ 2025-08-05 6:47 UTC (permalink / raw)
To: sfrench, pshilov
Cc: linux-cifs, samba-technical, linux-kernel, chengzhihao1, yi.zhang,
yangerkun
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V2 1/4] smb: client: rename server mid_lock to mid_queue_lock
2025-08-05 6:47 [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client Wang Zhaolong
@ 2025-08-05 6:47 ` Wang Zhaolong
2025-08-05 6:47 ` [PATCH V2 2/4] smb: client: add mid_counter_lock to protect the mid counter counter Wang Zhaolong
` (4 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Wang Zhaolong @ 2025-08-05 6:47 UTC (permalink / raw)
To: sfrench, pshilov
Cc: linux-cifs, samba-technical, linux-kernel, chengzhihao1, yi.zhang,
yangerkun
This is step 1/4 of a patch series to fix mid_q_entry memory leaks
caused by race conditions in callback execution.
The current mid_lock name is somewhat ambiguous about what it protects.
To prepare for splitting this lock into separate, more granular locks,
this patch renames mid_lock to mid_queue_lock to clearly indicate its
specific responsibility for protecting the pending_mid_q list and
related queue operations.
No functional changes are made in this patch - it only prepares the
codebase for the lock splitting that follows.
- mid_queue_lock for queue operations
- mid_counter_lock for mid counter operations
- per-mid locks for individual mid state management
Signed-off-by: Wang Zhaolong <wangzhaolong@huaweicloud.com>
---
fs/smb/client/cifs_debug.c | 8 ++++----
fs/smb/client/cifsglob.h | 4 ++--
fs/smb/client/connect.c | 20 +++++++++----------
fs/smb/client/smb1ops.c | 10 +++++-----
fs/smb/client/smb2ops.c | 26 ++++++++++++-------------
fs/smb/client/smb2transport.c | 4 ++--
fs/smb/client/transport.c | 36 +++++++++++++++++------------------
7 files changed, 54 insertions(+), 54 deletions(-)
diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
index f1cea365b6f1..80d6a51b8c11 100644
--- a/fs/smb/client/cifs_debug.c
+++ b/fs/smb/client/cifs_debug.c
@@ -58,11 +58,11 @@ void cifs_dump_mids(struct TCP_Server_Info *server)
if (server == NULL)
return;
cifs_dbg(VFS, "Dump pending requests:\n");
- spin_lock(&server->mid_lock);
+ spin_lock(&server->mid_queue_lock);
list_for_each_entry(mid_entry, &server->pending_mid_q, qhead) {
cifs_dbg(VFS, "State: %d Cmd: %d Pid: %d Cbdata: %p Mid %llu\n",
mid_entry->mid_state,
le16_to_cpu(mid_entry->command),
mid_entry->pid,
@@ -81,11 +81,11 @@ void cifs_dump_mids(struct TCP_Server_Info *server)
cifs_dump_detail(mid_entry->resp_buf, server);
cifs_dump_mem("existing buf: ",
mid_entry->resp_buf, 62);
}
}
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
#endif /* CONFIG_CIFS_DEBUG2 */
}
#ifdef CONFIG_PROC_FS
static void cifs_debug_tcon(struct seq_file *m, struct cifs_tcon *tcon)
@@ -670,20 +670,20 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
if (list_empty(&chan_server->pending_mid_q))
continue;
seq_printf(m, "\n\tServer ConnectionId: 0x%llx",
chan_server->conn_id);
- spin_lock(&chan_server->mid_lock);
+ spin_lock(&chan_server->mid_queue_lock);
list_for_each_entry(mid_entry, &chan_server->pending_mid_q, qhead) {
seq_printf(m, "\n\t\tState: %d com: %d pid: %d cbdata: %p mid %llu",
mid_entry->mid_state,
le16_to_cpu(mid_entry->command),
mid_entry->pid,
mid_entry->callback_data,
mid_entry->mid);
}
- spin_unlock(&chan_server->mid_lock);
+ spin_unlock(&chan_server->mid_queue_lock);
}
spin_unlock(&ses->chan_lock);
seq_puts(m, "\n--\n");
}
if (i == 0)
diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 19dd901fe8ab..ecd568793ce7 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -730,11 +730,11 @@ struct TCP_Server_Info {
#ifdef CONFIG_NET_NS
struct net *net;
#endif
wait_queue_head_t response_q;
wait_queue_head_t request_q; /* if more than maxmpx to srvr must block*/
- spinlock_t mid_lock; /* protect mid queue and it's entries */
+ spinlock_t mid_queue_lock; /* protect mid queue */
struct list_head pending_mid_q;
bool noblocksnd; /* use blocking sendmsg */
bool noautotune; /* do not autotune send buf sizes */
bool nosharesock;
bool tcp_nodelay;
@@ -2005,11 +2005,11 @@ require use of the stronger protocol */
* cifs_tcp_ses_lock cifs_tcp_ses_list sesInfoAlloc
* GlobalMid_Lock GlobalMaxActiveXid init_cifs
* GlobalCurrentXid
* GlobalTotalActiveXid
* TCP_Server_Info->srv_lock (anything in struct not protected by another lock and can change)
- * TCP_Server_Info->mid_lock TCP_Server_Info->pending_mid_q cifs_get_tcp_session
+ * TCP_Server_Info->mid_queue_lock TCP_Server_Info->pending_mid_q cifs_get_tcp_session
* ->CurrentMid
* (any changes in mid_q_entry fields)
* TCP_Server_Info->req_lock TCP_Server_Info->in_flight cifs_get_tcp_session
* ->credits
* ->echo_credits
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 5eec8957f2a9..e4b577ca48d5 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -319,19 +319,19 @@ cifs_abort_connection(struct TCP_Server_Info *server)
server->lstrp = jiffies;
/* mark submitted MIDs for retry and issue callback */
INIT_LIST_HEAD(&retry_list);
cifs_dbg(FYI, "%s: moving mids to private list\n", __func__);
- spin_lock(&server->mid_lock);
+ spin_lock(&server->mid_queue_lock);
list_for_each_entry_safe(mid, nmid, &server->pending_mid_q, qhead) {
kref_get(&mid->refcount);
if (mid->mid_state == MID_REQUEST_SUBMITTED)
mid->mid_state = MID_RETRY_NEEDED;
list_move(&mid->qhead, &retry_list);
mid->mid_flags |= MID_DELETED;
}
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
cifs_server_unlock(server);
cifs_dbg(FYI, "%s: issuing mid callbacks\n", __func__);
list_for_each_entry_safe(mid, nmid, &retry_list, qhead) {
list_del_init(&mid->qhead);
@@ -882,17 +882,17 @@ is_smb_response(struct TCP_Server_Info *server, unsigned char type)
* server, so reconnect would not signal connection
* aborted error to mid's callbacks. Note that for this
* server there should be exactly one pending mid
* corresponding to SMB1/SMB2 Negotiate packet.
*/
- spin_lock(&server->mid_lock);
+ spin_lock(&server->mid_queue_lock);
list_for_each_entry_safe(mid, nmid, &server->pending_mid_q, qhead) {
kref_get(&mid->refcount);
list_move(&mid->qhead, &dispose_list);
mid->mid_flags |= MID_DELETED;
}
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
/* Now try to reconnect once with NetBIOS session. */
server->with_rfc1001 = true;
rc = cifs_reconnect_once(server);
@@ -955,26 +955,26 @@ void
dequeue_mid(struct mid_q_entry *mid, bool malformed)
{
#ifdef CONFIG_CIFS_STATS2
mid->when_received = jiffies;
#endif
- spin_lock(&mid->server->mid_lock);
+ spin_lock(&mid->server->mid_queue_lock);
if (!malformed)
mid->mid_state = MID_RESPONSE_RECEIVED;
else
mid->mid_state = MID_RESPONSE_MALFORMED;
/*
* Trying to handle/dequeue a mid after the send_recv()
* function has finished processing it is a bug.
*/
if (mid->mid_flags & MID_DELETED) {
- spin_unlock(&mid->server->mid_lock);
+ spin_unlock(&mid->server->mid_queue_lock);
pr_warn_once("trying to dequeue a deleted mid\n");
} else {
list_del_init(&mid->qhead);
mid->mid_flags |= MID_DELETED;
- spin_unlock(&mid->server->mid_lock);
+ spin_unlock(&mid->server->mid_queue_lock);
}
}
static unsigned int
smb2_get_credits_from_hdr(char *buffer, struct TCP_Server_Info *server)
@@ -1099,20 +1099,20 @@ clean_demultiplex_info(struct TCP_Server_Info *server)
if (!list_empty(&server->pending_mid_q)) {
struct mid_q_entry *mid_entry;
struct list_head *tmp, *tmp2;
LIST_HEAD(dispose_list);
- spin_lock(&server->mid_lock);
+ spin_lock(&server->mid_queue_lock);
list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
cifs_dbg(FYI, "Clearing mid %llu\n", mid_entry->mid);
kref_get(&mid_entry->refcount);
mid_entry->mid_state = MID_SHUTDOWN;
list_move(&mid_entry->qhead, &dispose_list);
mid_entry->mid_flags |= MID_DELETED;
}
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
/* now walk dispose list and issue callbacks */
list_for_each_safe(tmp, tmp2, &dispose_list) {
mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
cifs_dbg(FYI, "Callback mid %llu\n", mid_entry->mid);
@@ -1820,11 +1820,11 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx,
tcp_ses->reconnect_instance = 1;
tcp_ses->lstrp = jiffies;
tcp_ses->compression.requested = ctx->compress;
spin_lock_init(&tcp_ses->req_lock);
spin_lock_init(&tcp_ses->srv_lock);
- spin_lock_init(&tcp_ses->mid_lock);
+ spin_lock_init(&tcp_ses->mid_queue_lock);
INIT_LIST_HEAD(&tcp_ses->tcp_ses_list);
INIT_LIST_HEAD(&tcp_ses->smb_ses_list);
INIT_DELAYED_WORK(&tcp_ses->echo, cifs_echo_request);
INIT_DELAYED_WORK(&tcp_ses->reconnect, smb2_reconnect_server);
mutex_init(&tcp_ses->reconnect_mutex);
diff --git a/fs/smb/client/smb1ops.c b/fs/smb/client/smb1ops.c
index e364b6515af3..a1442f697706 100644
--- a/fs/smb/client/smb1ops.c
+++ b/fs/smb/client/smb1ops.c
@@ -93,21 +93,21 @@ static struct mid_q_entry *
cifs_find_mid(struct TCP_Server_Info *server, char *buffer)
{
struct smb_hdr *buf = (struct smb_hdr *)buffer;
struct mid_q_entry *mid;
- spin_lock(&server->mid_lock);
+ spin_lock(&server->mid_queue_lock);
list_for_each_entry(mid, &server->pending_mid_q, qhead) {
if (compare_mid(mid->mid, buf) &&
mid->mid_state == MID_REQUEST_SUBMITTED &&
le16_to_cpu(mid->command) == buf->Command) {
kref_get(&mid->refcount);
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
return mid;
}
}
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
return NULL;
}
static void
cifs_add_credits(struct TCP_Server_Info *server,
@@ -167,11 +167,11 @@ cifs_get_next_mid(struct TCP_Server_Info *server)
{
__u64 mid = 0;
__u16 last_mid, cur_mid;
bool collision, reconnect = false;
- spin_lock(&server->mid_lock);
+ spin_lock(&server->mid_queue_lock);
/* mid is 16 bit only for CIFS/SMB */
cur_mid = (__u16)((server->CurrentMid) & 0xffff);
/* we do not want to loop forever */
last_mid = cur_mid;
@@ -226,11 +226,11 @@ cifs_get_next_mid(struct TCP_Server_Info *server)
server->CurrentMid = mid;
break;
}
cur_mid++;
}
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
if (reconnect) {
cifs_signal_cifsd_for_reconnect(server, false);
}
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 1b4a31894f43..c714707249c7 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -372,23 +372,23 @@ smb2_adjust_credits(struct TCP_Server_Info *server,
static __u64
smb2_get_next_mid(struct TCP_Server_Info *server)
{
__u64 mid;
/* for SMB2 we need the current value */
- spin_lock(&server->mid_lock);
+ spin_lock(&server->mid_queue_lock);
mid = server->CurrentMid++;
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
return mid;
}
static void
smb2_revert_current_mid(struct TCP_Server_Info *server, const unsigned int val)
{
- spin_lock(&server->mid_lock);
+ spin_lock(&server->mid_queue_lock);
if (server->CurrentMid >= val)
server->CurrentMid -= val;
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
}
static struct mid_q_entry *
__smb2_find_mid(struct TCP_Server_Info *server, char *buf, bool dequeue)
{
@@ -399,25 +399,25 @@ __smb2_find_mid(struct TCP_Server_Info *server, char *buf, bool dequeue)
if (shdr->ProtocolId == SMB2_TRANSFORM_PROTO_NUM) {
cifs_server_dbg(VFS, "Encrypted frame parsing not supported yet\n");
return NULL;
}
- spin_lock(&server->mid_lock);
+ spin_lock(&server->mid_queue_lock);
list_for_each_entry(mid, &server->pending_mid_q, qhead) {
if ((mid->mid == wire_mid) &&
(mid->mid_state == MID_REQUEST_SUBMITTED) &&
(mid->command == shdr->Command)) {
kref_get(&mid->refcount);
if (dequeue) {
list_del_init(&mid->qhead);
mid->mid_flags |= MID_DELETED;
}
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
return mid;
}
}
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
return NULL;
}
static struct mid_q_entry *
smb2_find_mid(struct TCP_Server_Info *server, char *buf)
@@ -458,13 +458,13 @@ smb2_negotiate(const unsigned int xid,
struct cifs_ses *ses,
struct TCP_Server_Info *server)
{
int rc;
- spin_lock(&server->mid_lock);
+ spin_lock(&server->mid_queue_lock);
server->CurrentMid = 0;
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
rc = SMB2_negotiate(xid, ses, server);
return rc;
}
static inline unsigned int
@@ -4807,22 +4807,22 @@ static void smb2_decrypt_offload(struct work_struct *work)
mid->callback(mid);
} else {
spin_lock(&dw->server->srv_lock);
if (dw->server->tcpStatus == CifsNeedReconnect) {
- spin_lock(&dw->server->mid_lock);
+ spin_lock(&dw->server->mid_queue_lock);
mid->mid_state = MID_RETRY_NEEDED;
- spin_unlock(&dw->server->mid_lock);
+ spin_unlock(&dw->server->mid_queue_lock);
spin_unlock(&dw->server->srv_lock);
mid->callback(mid);
} else {
- spin_lock(&dw->server->mid_lock);
+ spin_lock(&dw->server->mid_queue_lock);
mid->mid_state = MID_REQUEST_SUBMITTED;
mid->mid_flags &= ~(MID_DELETED);
list_add_tail(&mid->qhead,
&dw->server->pending_mid_q);
- spin_unlock(&dw->server->mid_lock);
+ spin_unlock(&dw->server->mid_queue_lock);
spin_unlock(&dw->server->srv_lock);
}
}
release_mid(mid);
}
diff --git a/fs/smb/client/smb2transport.c b/fs/smb/client/smb2transport.c
index 475b36c27f65..ff9ef7fcd010 100644
--- a/fs/smb/client/smb2transport.c
+++ b/fs/smb/client/smb2transport.c
@@ -838,13 +838,13 @@ smb2_get_mid_entry(struct cifs_ses *ses, struct TCP_Server_Info *server,
spin_unlock(&ses->ses_lock);
*mid = smb2_mid_entry_alloc(shdr, server);
if (*mid == NULL)
return -ENOMEM;
- spin_lock(&server->mid_lock);
+ spin_lock(&server->mid_queue_lock);
list_add_tail(&(*mid)->qhead, &server->pending_mid_q);
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
return 0;
}
int
diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
index 191783f553ce..12dc927aa4a2 100644
--- a/fs/smb/client/transport.c
+++ b/fs/smb/client/transport.c
@@ -158,16 +158,16 @@ void __release_mid(struct kref *refcount)
}
void
delete_mid(struct mid_q_entry *mid)
{
- spin_lock(&mid->server->mid_lock);
+ spin_lock(&mid->server->mid_queue_lock);
if (!(mid->mid_flags & MID_DELETED)) {
list_del_init(&mid->qhead);
mid->mid_flags |= MID_DELETED;
}
- spin_unlock(&mid->server->mid_lock);
+ spin_unlock(&mid->server->mid_queue_lock);
release_mid(mid);
}
/*
@@ -714,13 +714,13 @@ static int allocate_mid(struct cifs_ses *ses, struct smb_hdr *in_buf,
spin_unlock(&ses->ses_lock);
*ppmidQ = alloc_mid(in_buf, ses->server);
if (*ppmidQ == NULL)
return -ENOMEM;
- spin_lock(&ses->server->mid_lock);
+ spin_lock(&ses->server->mid_queue_lock);
list_add_tail(&(*ppmidQ)->qhead, &ses->server->pending_mid_q);
- spin_unlock(&ses->server->mid_lock);
+ spin_unlock(&ses->server->mid_queue_lock);
return 0;
}
static int
wait_for_response(struct TCP_Server_Info *server, struct mid_q_entry *midQ)
@@ -817,13 +817,13 @@ cifs_call_async(struct TCP_Server_Info *server, struct smb_rqst *rqst,
mid->callback_data = cbdata;
mid->handle = handle;
mid->mid_state = MID_REQUEST_SUBMITTED;
/* put it on the pending_mid_q */
- spin_lock(&server->mid_lock);
+ spin_lock(&server->mid_queue_lock);
list_add_tail(&mid->qhead, &server->pending_mid_q);
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
/*
* Need to store the time in mid before calling I/O. For call_async,
* I/O response may come back and free the mid entry on another thread.
*/
@@ -878,14 +878,14 @@ cifs_sync_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
int rc = 0;
cifs_dbg(FYI, "%s: cmd=%d mid=%llu state=%d\n",
__func__, le16_to_cpu(mid->command), mid->mid, mid->mid_state);
- spin_lock(&server->mid_lock);
+ spin_lock(&server->mid_queue_lock);
switch (mid->mid_state) {
case MID_RESPONSE_READY:
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
return rc;
case MID_RETRY_NEEDED:
rc = -EAGAIN;
break;
case MID_RESPONSE_MALFORMED:
@@ -900,17 +900,17 @@ cifs_sync_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
default:
if (!(mid->mid_flags & MID_DELETED)) {
list_del_init(&mid->qhead);
mid->mid_flags |= MID_DELETED;
}
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
cifs_server_dbg(VFS, "%s: invalid mid state mid=%llu state=%d\n",
__func__, mid->mid, mid->mid_state);
rc = -EIO;
goto sync_mid_done;
}
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
sync_mid_done:
release_mid(mid);
return rc;
}
@@ -1211,19 +1211,19 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
if (rc != 0) {
for (; i < num_rqst; i++) {
cifs_server_dbg(FYI, "Cancelling wait for mid %llu cmd: %d\n",
midQ[i]->mid, le16_to_cpu(midQ[i]->command));
send_cancel(server, &rqst[i], midQ[i]);
- spin_lock(&server->mid_lock);
+ spin_lock(&server->mid_queue_lock);
midQ[i]->mid_flags |= MID_WAIT_CANCELLED;
if (midQ[i]->mid_state == MID_REQUEST_SUBMITTED ||
midQ[i]->mid_state == MID_RESPONSE_RECEIVED) {
midQ[i]->callback = cifs_cancelled_callback;
cancelled_mid[i] = true;
credits[i].value = 0;
}
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
}
}
for (i = 0; i < num_rqst; i++) {
if (rc < 0)
@@ -1421,20 +1421,20 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses,
goto out;
rc = wait_for_response(server, midQ);
if (rc != 0) {
send_cancel(server, &rqst, midQ);
- spin_lock(&server->mid_lock);
+ spin_lock(&server->mid_queue_lock);
if (midQ->mid_state == MID_REQUEST_SUBMITTED ||
midQ->mid_state == MID_RESPONSE_RECEIVED) {
/* no longer considered to be "in-flight" */
midQ->callback = release_mid;
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
add_credits(server, &credits, 0);
return rc;
}
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
}
rc = cifs_sync_mid_result(midQ, server);
if (rc != 0) {
add_credits(server, &credits, 0);
@@ -1603,19 +1603,19 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifs_tcon *tcon,
}
rc = wait_for_response(server, midQ);
if (rc) {
send_cancel(server, &rqst, midQ);
- spin_lock(&server->mid_lock);
+ spin_lock(&server->mid_queue_lock);
if (midQ->mid_state == MID_REQUEST_SUBMITTED ||
midQ->mid_state == MID_RESPONSE_RECEIVED) {
/* no longer considered to be "in-flight" */
midQ->callback = release_mid;
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
return rc;
}
- spin_unlock(&server->mid_lock);
+ spin_unlock(&server->mid_queue_lock);
}
/* We got the response - restart system call. */
rstart = 1;
spin_lock(&server->srv_lock);
--
2.39.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 2/4] smb: client: add mid_counter_lock to protect the mid counter counter
2025-08-05 6:47 [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client Wang Zhaolong
2025-08-05 6:47 ` [PATCH V2 1/4] smb: client: rename server mid_lock to mid_queue_lock Wang Zhaolong
@ 2025-08-05 6:47 ` Wang Zhaolong
2025-08-05 6:47 ` [PATCH V2 3/4] smb: client: smb: client: eliminate mid_flags field Wang Zhaolong
` (3 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Wang Zhaolong @ 2025-08-05 6:47 UTC (permalink / raw)
To: sfrench, pshilov
Cc: linux-cifs, samba-technical, linux-kernel, chengzhihao1, yi.zhang,
yangerkun
This is step 2/4 of a patch series to fix mid_q_entry memory leaks
caused by race conditions in callback execution.
Add a dedicated mid_counter_lock to protect current_mid counter,
separating it from mid_queue_lock which protects pending_mid_q
operations. This reduces lock contention and prepares for finer-
grained locking in subsequent patches.
Changes:
- Add TCP_Server_Info->mid_counter_lock spinlock
- Rename CurrentMid to current_mid for consistency
- Use mid_counter_lock to protect current_mid access
- Update locking documentation in cifsglob.h
This separation allows mid allocation to proceed without blocking
queue operations, improving performance under heavy load.
Signed-off-by: Wang Zhaolong <wangzhaolong@huaweicloud.com>
---
fs/smb/client/cifsglob.h | 5 +++--
fs/smb/client/connect.c | 5 +++--
fs/smb/client/smb1ops.c | 11 ++++++-----
fs/smb/client/smb2ops.c | 40 +++++++++++++++++++--------------------
fs/smb/client/transport.c | 12 ++++++------
5 files changed, 38 insertions(+), 35 deletions(-)
diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index ecd568793ce7..1844afdf1e41 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -731,10 +731,11 @@ struct TCP_Server_Info {
struct net *net;
#endif
wait_queue_head_t response_q;
wait_queue_head_t request_q; /* if more than maxmpx to srvr must block*/
spinlock_t mid_queue_lock; /* protect mid queue */
+ spinlock_t mid_counter_lock;
struct list_head pending_mid_q;
bool noblocksnd; /* use blocking sendmsg */
bool noautotune; /* do not autotune send buf sizes */
bool nosharesock;
bool tcp_nodelay;
@@ -768,11 +769,11 @@ struct TCP_Server_Info {
unsigned int max_rw; /* maxRw specifies the maximum */
/* message size the server can send or receive for */
/* SMB_COM_WRITE_RAW or SMB_COM_READ_RAW. */
unsigned int capabilities; /* selective disabling of caps by smb sess */
int timeAdj; /* Adjust for difference in server time zone in sec */
- __u64 CurrentMid; /* multiplex id - rotating counter, protected by GlobalMid_Lock */
+ __u64 current_mid; /* multiplex id - rotating counter, protected by mid_counter_lock */
char cryptkey[CIFS_CRYPTO_KEY_SIZE]; /* used by ntlm, ntlmv2 etc */
/* 16th byte of RFC1001 workstation name is always null */
char workstation_RFC1001_name[RFC1001_NAME_LEN_WITH_NULL];
__u32 sequence_number; /* for signing, protected by srv_mutex */
__u32 reconnect_instance; /* incremented on each reconnect */
@@ -2006,12 +2007,12 @@ require use of the stronger protocol */
* GlobalMid_Lock GlobalMaxActiveXid init_cifs
* GlobalCurrentXid
* GlobalTotalActiveXid
* TCP_Server_Info->srv_lock (anything in struct not protected by another lock and can change)
* TCP_Server_Info->mid_queue_lock TCP_Server_Info->pending_mid_q cifs_get_tcp_session
- * ->CurrentMid
* (any changes in mid_q_entry fields)
+ * TCP_Server_Info->mid_counter_lock TCP_Server_Info->current_mid cifs_get_tcp_session
* TCP_Server_Info->req_lock TCP_Server_Info->in_flight cifs_get_tcp_session
* ->credits
* ->echo_credits
* ->oplock_credits
* ->reconnect_instance
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index e4b577ca48d5..74ad5881ee45 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -356,11 +356,11 @@ static bool cifs_tcp_ses_needs_reconnect(struct TCP_Server_Info *server, int num
wake_up(&server->response_q);
return false;
}
cifs_dbg(FYI, "Mark tcp session as need reconnect\n");
- trace_smb3_reconnect(server->CurrentMid, server->conn_id,
+ trace_smb3_reconnect(server->current_mid, server->conn_id,
server->hostname);
server->tcpStatus = CifsNeedReconnect;
spin_unlock(&server->srv_lock);
return true;
@@ -1240,11 +1240,11 @@ smb2_add_credits_from_hdr(char *buffer, struct TCP_Server_Info *server)
scredits = server->credits;
in_flight = server->in_flight;
spin_unlock(&server->req_lock);
wake_up(&server->request_q);
- trace_smb3_hdr_credits(server->CurrentMid,
+ trace_smb3_hdr_credits(server->current_mid,
server->conn_id, server->hostname, scredits,
le16_to_cpu(shdr->CreditRequest), in_flight);
cifs_server_dbg(FYI, "%s: added %u credits total=%d\n",
__func__, le16_to_cpu(shdr->CreditRequest),
scredits);
@@ -1821,10 +1821,11 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx,
tcp_ses->lstrp = jiffies;
tcp_ses->compression.requested = ctx->compress;
spin_lock_init(&tcp_ses->req_lock);
spin_lock_init(&tcp_ses->srv_lock);
spin_lock_init(&tcp_ses->mid_queue_lock);
+ spin_lock_init(&tcp_ses->mid_counter_lock);
INIT_LIST_HEAD(&tcp_ses->tcp_ses_list);
INIT_LIST_HEAD(&tcp_ses->smb_ses_list);
INIT_DELAYED_WORK(&tcp_ses->echo, cifs_echo_request);
INIT_DELAYED_WORK(&tcp_ses->reconnect, smb2_reconnect_server);
mutex_init(&tcp_ses->reconnect_mutex);
diff --git a/fs/smb/client/smb1ops.c b/fs/smb/client/smb1ops.c
index a1442f697706..13f600a3d0c4 100644
--- a/fs/smb/client/smb1ops.c
+++ b/fs/smb/client/smb1ops.c
@@ -167,14 +167,13 @@ cifs_get_next_mid(struct TCP_Server_Info *server)
{
__u64 mid = 0;
__u16 last_mid, cur_mid;
bool collision, reconnect = false;
- spin_lock(&server->mid_queue_lock);
-
+ spin_lock(&server->mid_counter_lock);
/* mid is 16 bit only for CIFS/SMB */
- cur_mid = (__u16)((server->CurrentMid) & 0xffff);
+ cur_mid = (__u16)((server->current_mid) & 0xffff);
/* we do not want to loop forever */
last_mid = cur_mid;
cur_mid++;
/* avoid 0xFFFF MID */
if (cur_mid == 0xffff)
@@ -196,19 +195,21 @@ cifs_get_next_mid(struct TCP_Server_Info *server)
collision = false;
if (cur_mid == 0)
cur_mid++;
num_mids = 0;
+ spin_lock(&server->mid_queue_lock);
list_for_each_entry(mid_entry, &server->pending_mid_q, qhead) {
++num_mids;
if (mid_entry->mid == cur_mid &&
mid_entry->mid_state == MID_REQUEST_SUBMITTED) {
/* This mid is in use, try a different one */
collision = true;
break;
}
}
+ spin_unlock(&server->mid_queue_lock);
/*
* if we have more than 32k mids in the list, then something
* is very wrong. Possibly a local user is trying to DoS the
* box by issuing long-running calls and SIGKILL'ing them. If
@@ -221,16 +222,16 @@ cifs_get_next_mid(struct TCP_Server_Info *server)
if (num_mids > 32768)
reconnect = true;
if (!collision) {
mid = (__u64)cur_mid;
- server->CurrentMid = mid;
+ server->current_mid = mid;
break;
}
cur_mid++;
}
- spin_unlock(&server->mid_queue_lock);
+ spin_unlock(&server->mid_counter_lock);
if (reconnect) {
cifs_signal_cifsd_for_reconnect(server, false);
}
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index c714707249c7..da2cb9585404 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -89,11 +89,11 @@ smb2_add_credits(struct TCP_Server_Info *server,
reconnect_detected = true;
if (*val > 65000) {
*val = 65000; /* Don't get near 64K credits, avoid srv bugs */
pr_warn_once("server overflowed SMB3 credits\n");
- trace_smb3_overflow_credits(server->CurrentMid,
+ trace_smb3_overflow_credits(server->current_mid,
server->conn_id, server->hostname, *val,
add, server->in_flight);
}
if (credits->in_flight_check > 1) {
pr_warn_once("rreq R=%08x[%x] Credits not in flight\n",
@@ -134,19 +134,19 @@ smb2_add_credits(struct TCP_Server_Info *server,
in_flight = server->in_flight;
spin_unlock(&server->req_lock);
wake_up(&server->request_q);
if (reconnect_detected) {
- trace_smb3_reconnect_detected(server->CurrentMid,
+ trace_smb3_reconnect_detected(server->current_mid,
server->conn_id, server->hostname, scredits, add, in_flight);
cifs_dbg(FYI, "trying to put %d credits from the old server instance %d\n",
add, instance);
}
if (reconnect_with_invalid_credits) {
- trace_smb3_reconnect_with_invalid_credits(server->CurrentMid,
+ trace_smb3_reconnect_with_invalid_credits(server->current_mid,
server->conn_id, server->hostname, scredits, add, in_flight);
cifs_dbg(FYI, "Negotiate operation when server credits is non-zero. Optype: %d, server credits: %d, credits added: %d\n",
optype, scredits, add);
}
@@ -174,11 +174,11 @@ smb2_add_credits(struct TCP_Server_Info *server,
default:
/* change_conf rebalanced credits for different types */
break;
}
- trace_smb3_add_credits(server->CurrentMid,
+ trace_smb3_add_credits(server->current_mid,
server->conn_id, server->hostname, scredits, add, in_flight);
cifs_dbg(FYI, "%s: added %u credits total=%d\n", __func__, add, scredits);
}
static void
@@ -201,11 +201,11 @@ smb2_set_credits(struct TCP_Server_Info *server, const int val)
}
scredits = server->credits;
in_flight = server->in_flight;
spin_unlock(&server->req_lock);
- trace_smb3_set_credits(server->CurrentMid,
+ trace_smb3_set_credits(server->current_mid,
server->conn_id, server->hostname, scredits, val, in_flight);
cifs_dbg(FYI, "%s: set %u credits\n", __func__, val);
/* don't log while holding the lock */
if (val == 1)
@@ -286,11 +286,11 @@ smb2_wait_mtu_credits(struct TCP_Server_Info *server, size_t size,
}
scredits = server->credits;
in_flight = server->in_flight;
spin_unlock(&server->req_lock);
- trace_smb3_wait_credits(server->CurrentMid,
+ trace_smb3_wait_credits(server->current_mid,
server->conn_id, server->hostname, scredits, -(credits->value), in_flight);
cifs_dbg(FYI, "%s: removed %u credits total=%d\n",
__func__, credits->value, scredits);
return rc;
@@ -314,11 +314,11 @@ smb2_adjust_credits(struct TCP_Server_Info *server,
subreq->subreq.debug_index,
credits->value,
server->credits, server->in_flight,
new_val - credits->value,
cifs_trace_rw_credits_no_adjust_up);
- trace_smb3_too_many_credits(server->CurrentMid,
+ trace_smb3_too_many_credits(server->current_mid,
server->conn_id, server->hostname, 0, credits->value - new_val, 0);
cifs_server_dbg(VFS, "R=%x[%x] request has less credits (%d) than required (%d)",
subreq->rreq->debug_id, subreq->subreq.debug_index,
credits->value, new_val);
@@ -336,11 +336,11 @@ smb2_adjust_credits(struct TCP_Server_Info *server,
subreq->subreq.debug_index,
credits->value,
server->credits, server->in_flight,
new_val - credits->value,
cifs_trace_rw_credits_old_session);
- trace_smb3_reconnect_detected(server->CurrentMid,
+ trace_smb3_reconnect_detected(server->current_mid,
server->conn_id, server->hostname, scredits,
credits->value - new_val, in_flight);
cifs_server_dbg(VFS, "R=%x[%x] trying to return %d credits to old session\n",
subreq->rreq->debug_id, subreq->subreq.debug_index,
credits->value - new_val);
@@ -356,11 +356,11 @@ smb2_adjust_credits(struct TCP_Server_Info *server,
scredits = server->credits;
in_flight = server->in_flight;
spin_unlock(&server->req_lock);
wake_up(&server->request_q);
- trace_smb3_adj_credits(server->CurrentMid,
+ trace_smb3_adj_credits(server->current_mid,
server->conn_id, server->hostname, scredits,
credits->value - new_val, in_flight);
cifs_dbg(FYI, "%s: adjust added %u credits total=%d\n",
__func__, credits->value - new_val, scredits);
@@ -372,23 +372,23 @@ smb2_adjust_credits(struct TCP_Server_Info *server,
static __u64
smb2_get_next_mid(struct TCP_Server_Info *server)
{
__u64 mid;
/* for SMB2 we need the current value */
- spin_lock(&server->mid_queue_lock);
- mid = server->CurrentMid++;
- spin_unlock(&server->mid_queue_lock);
+ spin_lock(&server->mid_counter_lock);
+ mid = server->current_mid++;
+ spin_unlock(&server->mid_counter_lock);
return mid;
}
static void
smb2_revert_current_mid(struct TCP_Server_Info *server, const unsigned int val)
{
- spin_lock(&server->mid_queue_lock);
- if (server->CurrentMid >= val)
- server->CurrentMid -= val;
- spin_unlock(&server->mid_queue_lock);
+ spin_lock(&server->mid_counter_lock);
+ if (server->current_mid >= val)
+ server->current_mid -= val;
+ spin_unlock(&server->mid_counter_lock);
}
static struct mid_q_entry *
__smb2_find_mid(struct TCP_Server_Info *server, char *buf, bool dequeue)
{
@@ -458,13 +458,13 @@ smb2_negotiate(const unsigned int xid,
struct cifs_ses *ses,
struct TCP_Server_Info *server)
{
int rc;
- spin_lock(&server->mid_queue_lock);
- server->CurrentMid = 0;
- spin_unlock(&server->mid_queue_lock);
+ spin_lock(&server->mid_counter_lock);
+ server->current_mid = 0;
+ spin_unlock(&server->mid_counter_lock);
rc = SMB2_negotiate(xid, ses, server);
return rc;
}
static inline unsigned int
@@ -2496,11 +2496,11 @@ smb2_is_status_pending(char *buf, struct TCP_Server_Info *server)
scredits = server->credits;
in_flight = server->in_flight;
spin_unlock(&server->req_lock);
wake_up(&server->request_q);
- trace_smb3_pend_credits(server->CurrentMid,
+ trace_smb3_pend_credits(server->current_mid,
server->conn_id, server->hostname, scredits,
le16_to_cpu(shdr->CreditRequest), in_flight);
cifs_dbg(FYI, "%s: status pending add %u credits total=%d\n",
__func__, le16_to_cpu(shdr->CreditRequest), scredits);
}
diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
index 12dc927aa4a2..8037accc3987 100644
--- a/fs/smb/client/transport.c
+++ b/fs/smb/client/transport.c
@@ -395,11 +395,11 @@ __smb_send_rqst(struct TCP_Server_Info *server, int num_rqst,
* If we have only sent part of an SMB then the next SMB could
* be taken as the remainder of this one. We need to kill the
* socket so the server throws away the partial SMB
*/
cifs_signal_cifsd_for_reconnect(server, false);
- trace_smb3_partial_send_reconnect(server->CurrentMid,
+ trace_smb3_partial_send_reconnect(server->current_mid,
server->conn_id, server->hostname);
}
smbd_done:
/*
* there's hardly any use for the layers above to know the
@@ -507,11 +507,11 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
*instance = server->reconnect_instance;
scredits = *credits;
in_flight = server->in_flight;
spin_unlock(&server->req_lock);
- trace_smb3_nblk_credits(server->CurrentMid,
+ trace_smb3_nblk_credits(server->current_mid,
server->conn_id, server->hostname, scredits, -1, in_flight);
cifs_dbg(FYI, "%s: remove %u credits total=%d\n",
__func__, 1, scredits);
return 0;
@@ -540,11 +540,11 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
spin_lock(&server->req_lock);
scredits = *credits;
in_flight = server->in_flight;
spin_unlock(&server->req_lock);
- trace_smb3_credit_timeout(server->CurrentMid,
+ trace_smb3_credit_timeout(server->current_mid,
server->conn_id, server->hostname, scredits,
num_credits, in_flight);
cifs_server_dbg(VFS, "wait timed out after %d ms\n",
timeout);
return -EBUSY;
@@ -583,11 +583,11 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
scredits = *credits;
in_flight = server->in_flight;
spin_unlock(&server->req_lock);
trace_smb3_credit_timeout(
- server->CurrentMid,
+ server->current_mid,
server->conn_id, server->hostname,
scredits, num_credits, in_flight);
cifs_server_dbg(VFS, "wait timed out after %d ms\n",
timeout);
return -EBUSY;
@@ -613,11 +613,11 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
}
scredits = *credits;
in_flight = server->in_flight;
spin_unlock(&server->req_lock);
- trace_smb3_waitff_credits(server->CurrentMid,
+ trace_smb3_waitff_credits(server->current_mid,
server->conn_id, server->hostname, scredits,
-(num_credits), in_flight);
cifs_dbg(FYI, "%s: remove %u credits total=%d\n",
__func__, num_credits, scredits);
break;
@@ -664,11 +664,11 @@ wait_for_compound_request(struct TCP_Server_Info *server, int num,
* Return immediately if no requests in flight since we will be
* stuck on waiting for credits.
*/
if (server->in_flight == 0) {
spin_unlock(&server->req_lock);
- trace_smb3_insufficient_credits(server->CurrentMid,
+ trace_smb3_insufficient_credits(server->current_mid,
server->conn_id, server->hostname, scredits,
num, in_flight);
cifs_dbg(FYI, "%s: %d requests in flight, needed %d total=%d\n",
__func__, in_flight, num, scredits);
return -EDEADLK;
--
2.39.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 3/4] smb: client: smb: client: eliminate mid_flags field
2025-08-05 6:47 [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client Wang Zhaolong
2025-08-05 6:47 ` [PATCH V2 1/4] smb: client: rename server mid_lock to mid_queue_lock Wang Zhaolong
2025-08-05 6:47 ` [PATCH V2 2/4] smb: client: add mid_counter_lock to protect the mid counter counter Wang Zhaolong
@ 2025-08-05 6:47 ` Wang Zhaolong
2025-08-05 6:47 ` [PATCH V2 4/4] smb: client: fix mid_q_entry memleak leak with per-mid locking Wang Zhaolong
` (2 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Wang Zhaolong @ 2025-08-05 6:47 UTC (permalink / raw)
To: sfrench, pshilov
Cc: linux-cifs, samba-technical, linux-kernel, chengzhihao1, yi.zhang,
yangerkun
This is step 3/4 of a patch series to fix mid_q_entry memory leaks
caused by race conditions in callback execution.
Replace the mid_flags bitmask with dedicated boolean fields to
simplify locking logic and improve code readability:
- Replace MID_DELETED with bool deleted_from_q
- Replace MID_WAIT_CANCELLED with bool wait_cancelled
- Remove mid_flags field entirely
The new boolean fields have clearer semantics:
- deleted_from_q: whether mid has been removed from pending_mid_q
- wait_cancelled: whether request was cancelled during wait
This change reduces memory usage (from 4-byte bitmask to 2 boolean
flags) and eliminates confusion about which lock protects which
flag bits, preparing for per-mid locking in the next patch.
Signed-off-by: Wang Zhaolong <wangzhaolong@huaweicloud.com>
---
fs/smb/client/cifsglob.h | 9 +++------
fs/smb/client/connect.c | 10 +++++-----
fs/smb/client/smb2ops.c | 4 ++--
fs/smb/client/transport.c | 12 ++++++------
4 files changed, 16 insertions(+), 19 deletions(-)
diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 1844afdf1e41..536dff5b4a9c 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -1728,13 +1728,14 @@ struct mid_q_entry {
struct task_struct *creator;
void *resp_buf; /* pointer to received SMB header */
unsigned int resp_buf_size;
int mid_state; /* wish this were enum but can not pass to wait_event */
int mid_rc; /* rc for MID_RC */
- unsigned int mid_flags;
__le16 command; /* smb command code */
unsigned int optype; /* operation type */
+ bool wait_cancelled:1; /* Cancelled while waiting for response */
+ bool deleted_from_q:1; /* Whether Mid has been dequeued frem pending_mid_q */
bool large_buf:1; /* if valid response, is pointer to large buf */
bool multiRsp:1; /* multiple trans2 responses for one request */
bool multiEnd:1; /* both received */
bool decrypted:1; /* decrypted entry */
};
@@ -1892,14 +1893,10 @@ static inline bool is_replayable_error(int error)
#define MID_RESPONSE_MALFORMED 0x10
#define MID_SHUTDOWN 0x20
#define MID_RESPONSE_READY 0x40 /* ready for other process handle the rsp */
#define MID_RC 0x80 /* mid_rc contains custom rc */
-/* Flags */
-#define MID_WAIT_CANCELLED 1 /* Cancelled while waiting for response */
-#define MID_DELETED 2 /* Mid has been dequeued/deleted */
-
/* Types of response buffer returned from SendReceive2 */
#define CIFS_NO_BUFFER 0 /* Response buffer not returned */
#define CIFS_SMALL_BUFFER 1
#define CIFS_LARGE_BUFFER 2
#define CIFS_IOVEC 4 /* array of response buffers */
@@ -2007,11 +2004,11 @@ require use of the stronger protocol */
* GlobalMid_Lock GlobalMaxActiveXid init_cifs
* GlobalCurrentXid
* GlobalTotalActiveXid
* TCP_Server_Info->srv_lock (anything in struct not protected by another lock and can change)
* TCP_Server_Info->mid_queue_lock TCP_Server_Info->pending_mid_q cifs_get_tcp_session
- * (any changes in mid_q_entry fields)
+ * mid_q_entry->deleted_from_q
* TCP_Server_Info->mid_counter_lock TCP_Server_Info->current_mid cifs_get_tcp_session
* TCP_Server_Info->req_lock TCP_Server_Info->in_flight cifs_get_tcp_session
* ->credits
* ->echo_credits
* ->oplock_credits
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 74ad5881ee45..587845a2452d 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -325,11 +325,11 @@ cifs_abort_connection(struct TCP_Server_Info *server)
list_for_each_entry_safe(mid, nmid, &server->pending_mid_q, qhead) {
kref_get(&mid->refcount);
if (mid->mid_state == MID_REQUEST_SUBMITTED)
mid->mid_state = MID_RETRY_NEEDED;
list_move(&mid->qhead, &retry_list);
- mid->mid_flags |= MID_DELETED;
+ mid->deleted_from_q = true;
}
spin_unlock(&server->mid_queue_lock);
cifs_server_unlock(server);
cifs_dbg(FYI, "%s: issuing mid callbacks\n", __func__);
@@ -886,11 +886,11 @@ is_smb_response(struct TCP_Server_Info *server, unsigned char type)
*/
spin_lock(&server->mid_queue_lock);
list_for_each_entry_safe(mid, nmid, &server->pending_mid_q, qhead) {
kref_get(&mid->refcount);
list_move(&mid->qhead, &dispose_list);
- mid->mid_flags |= MID_DELETED;
+ mid->deleted_from_q = true;
}
spin_unlock(&server->mid_queue_lock);
/* Now try to reconnect once with NetBIOS session. */
server->with_rfc1001 = true;
@@ -964,16 +964,16 @@ dequeue_mid(struct mid_q_entry *mid, bool malformed)
mid->mid_state = MID_RESPONSE_MALFORMED;
/*
* Trying to handle/dequeue a mid after the send_recv()
* function has finished processing it is a bug.
*/
- if (mid->mid_flags & MID_DELETED) {
+ if (mid->deleted_from_q == true) {
spin_unlock(&mid->server->mid_queue_lock);
pr_warn_once("trying to dequeue a deleted mid\n");
} else {
list_del_init(&mid->qhead);
- mid->mid_flags |= MID_DELETED;
+ mid->deleted_from_q = true;
spin_unlock(&mid->server->mid_queue_lock);
}
}
static unsigned int
@@ -1106,11 +1106,11 @@ clean_demultiplex_info(struct TCP_Server_Info *server)
mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
cifs_dbg(FYI, "Clearing mid %llu\n", mid_entry->mid);
kref_get(&mid_entry->refcount);
mid_entry->mid_state = MID_SHUTDOWN;
list_move(&mid_entry->qhead, &dispose_list);
- mid_entry->mid_flags |= MID_DELETED;
+ mid_entry->deleted_from_q = true;
}
spin_unlock(&server->mid_queue_lock);
/* now walk dispose list and issue callbacks */
list_for_each_safe(tmp, tmp2, &dispose_list) {
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index da2cb9585404..2643d86a5b5f 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -407,11 +407,11 @@ __smb2_find_mid(struct TCP_Server_Info *server, char *buf, bool dequeue)
(mid->mid_state == MID_REQUEST_SUBMITTED) &&
(mid->command == shdr->Command)) {
kref_get(&mid->refcount);
if (dequeue) {
list_del_init(&mid->qhead);
- mid->mid_flags |= MID_DELETED;
+ mid->deleted_from_q = true;
}
spin_unlock(&server->mid_queue_lock);
return mid;
}
}
@@ -4815,11 +4815,11 @@ static void smb2_decrypt_offload(struct work_struct *work)
spin_unlock(&dw->server->srv_lock);
mid->callback(mid);
} else {
spin_lock(&dw->server->mid_queue_lock);
mid->mid_state = MID_REQUEST_SUBMITTED;
- mid->mid_flags &= ~(MID_DELETED);
+ mid->deleted_from_q = false;
list_add_tail(&mid->qhead,
&dw->server->pending_mid_q);
spin_unlock(&dw->server->mid_queue_lock);
spin_unlock(&dw->server->srv_lock);
}
diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
index 8037accc3987..ca9358c24ceb 100644
--- a/fs/smb/client/transport.c
+++ b/fs/smb/client/transport.c
@@ -87,11 +87,11 @@ void __release_mid(struct kref *refcount)
unsigned long now;
unsigned long roundtrip_time;
#endif
struct TCP_Server_Info *server = midEntry->server;
- if (midEntry->resp_buf && (midEntry->mid_flags & MID_WAIT_CANCELLED) &&
+ if (midEntry->resp_buf && (midEntry->wait_cancelled) &&
(midEntry->mid_state == MID_RESPONSE_RECEIVED ||
midEntry->mid_state == MID_RESPONSE_READY) &&
server->ops->handle_cancelled_mid)
server->ops->handle_cancelled_mid(midEntry, server);
@@ -159,13 +159,13 @@ void __release_mid(struct kref *refcount)
void
delete_mid(struct mid_q_entry *mid)
{
spin_lock(&mid->server->mid_queue_lock);
- if (!(mid->mid_flags & MID_DELETED)) {
+ if (mid->deleted_from_q == false) {
list_del_init(&mid->qhead);
- mid->mid_flags |= MID_DELETED;
+ mid->deleted_from_q = true;
}
spin_unlock(&mid->server->mid_queue_lock);
release_mid(mid);
}
@@ -896,13 +896,13 @@ cifs_sync_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
break;
case MID_RC:
rc = mid->mid_rc;
break;
default:
- if (!(mid->mid_flags & MID_DELETED)) {
+ if (mid->deleted_from_q == false) {
list_del_init(&mid->qhead);
- mid->mid_flags |= MID_DELETED;
+ mid->deleted_from_q = true;
}
spin_unlock(&server->mid_queue_lock);
cifs_server_dbg(VFS, "%s: invalid mid state mid=%llu state=%d\n",
__func__, mid->mid, mid->mid_state);
rc = -EIO;
@@ -1212,11 +1212,11 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
for (; i < num_rqst; i++) {
cifs_server_dbg(FYI, "Cancelling wait for mid %llu cmd: %d\n",
midQ[i]->mid, le16_to_cpu(midQ[i]->command));
send_cancel(server, &rqst[i], midQ[i]);
spin_lock(&server->mid_queue_lock);
- midQ[i]->mid_flags |= MID_WAIT_CANCELLED;
+ midQ[i]->wait_cancelled = true;
if (midQ[i]->mid_state == MID_REQUEST_SUBMITTED ||
midQ[i]->mid_state == MID_RESPONSE_RECEIVED) {
midQ[i]->callback = cifs_cancelled_callback;
cancelled_mid[i] = true;
credits[i].value = 0;
--
2.39.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 4/4] smb: client: fix mid_q_entry memleak leak with per-mid locking
2025-08-05 6:47 [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client Wang Zhaolong
` (2 preceding siblings ...)
2025-08-05 6:47 ` [PATCH V2 3/4] smb: client: smb: client: eliminate mid_flags field Wang Zhaolong
@ 2025-08-05 6:47 ` Wang Zhaolong
2025-08-05 12:47 ` [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client Enzo Matsumiya
2025-08-05 16:39 ` Steve French
5 siblings, 0 replies; 14+ messages in thread
From: Wang Zhaolong @ 2025-08-05 6:47 UTC (permalink / raw)
To: sfrench, pshilov
Cc: linux-cifs, samba-technical, linux-kernel, chengzhihao1, yi.zhang,
yangerkun
This is step 4/4 of a patch series to fix mid_q_entry memory leaks
caused by race conditions in callback execution.
In compound_send_recv(), when wait_for_response() is interrupted by
signals, the code attempts to cancel pending requests by changing
their callbacks to cifs_cancelled_callback. However, there's a race
condition between signal interruption and network response processing
that causes both mid_q_entry and server buffer leaks:
```
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 callback assignment (mid->callback = cifs_cancelled_callback) and
callback execution (mid->callback(mid)) are not atomic, allowing the
network thread to execute the old callback even after cancellation.
Solution:
Add per-mid locking to ensure atomic callback execution:
- Add spinlock_t mid_lock to struct mid_q_entry
- Protect mid_state, callback, and related fields with mid_lock
- Add mid_execute_callback() wrapper for safe callback execution
- Use mid_lock in compound_send_recv() cancellation logic
Key changes:
- Initialize mid_lock in alloc_mid() and smb2_mid_entry_alloc()
- Replace direct mid->callback() calls with mid_execute_callback()
- Protect all mid state changes with appropriate locks
- Update locking documentation
This ensures that either the original callback or the cancellation
callback executes atomically, preventing reference count leaks when
requests are interrupted by signals.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=220404
Fixes: ee258d79159a ("CIFS: Move credit processing to mid callbacks for SMB3")
Signed-off-by: Wang Zhaolong <wangzhaolong@huaweicloud.com>
---
fs/smb/client/cifs_debug.c | 4 ++++
fs/smb/client/cifsglob.h | 4 ++++
fs/smb/client/connect.c | 22 ++++++++++++++++++----
fs/smb/client/smb1ops.c | 6 ++++++
fs/smb/client/smb2ops.c | 18 ++++++++++++------
fs/smb/client/smb2transport.c | 1 +
fs/smb/client/transport.c | 29 ++++++++++++++++++-----------
7 files changed, 63 insertions(+), 21 deletions(-)
diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
index 80d6a51b8c11..4708afc9106c 100644
--- a/fs/smb/client/cifs_debug.c
+++ b/fs/smb/client/cifs_debug.c
@@ -60,10 +60,11 @@ void cifs_dump_mids(struct TCP_Server_Info *server)
return;
cifs_dbg(VFS, "Dump pending requests:\n");
spin_lock(&server->mid_queue_lock);
list_for_each_entry(mid_entry, &server->pending_mid_q, qhead) {
+ spin_lock(&mid_entry->mid_lock);
cifs_dbg(VFS, "State: %d Cmd: %d Pid: %d Cbdata: %p Mid %llu\n",
mid_entry->mid_state,
le16_to_cpu(mid_entry->command),
mid_entry->pid,
mid_entry->callback_data,
@@ -80,10 +81,11 @@ void cifs_dump_mids(struct TCP_Server_Info *server)
if (mid_entry->resp_buf) {
cifs_dump_detail(mid_entry->resp_buf, server);
cifs_dump_mem("existing buf: ",
mid_entry->resp_buf, 62);
}
+ spin_unlock(&mid_entry->mid_lock);
}
spin_unlock(&server->mid_queue_lock);
#endif /* CONFIG_CIFS_DEBUG2 */
}
@@ -672,16 +674,18 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
seq_printf(m, "\n\tServer ConnectionId: 0x%llx",
chan_server->conn_id);
spin_lock(&chan_server->mid_queue_lock);
list_for_each_entry(mid_entry, &chan_server->pending_mid_q, qhead) {
+ spin_lock(&mid_entry->mid_lock);
seq_printf(m, "\n\t\tState: %d com: %d pid: %d cbdata: %p mid %llu",
mid_entry->mid_state,
le16_to_cpu(mid_entry->command),
mid_entry->pid,
mid_entry->callback_data,
mid_entry->mid);
+ spin_unlock(&mid_entry->mid_lock);
}
spin_unlock(&chan_server->mid_queue_lock);
}
spin_unlock(&ses->chan_lock);
seq_puts(m, "\n--\n");
diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 536dff5b4a9c..486744adfc72 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -1730,10 +1730,11 @@ struct mid_q_entry {
unsigned int resp_buf_size;
int mid_state; /* wish this were enum but can not pass to wait_event */
int mid_rc; /* rc for MID_RC */
__le16 command; /* smb command code */
unsigned int optype; /* operation type */
+ spinlock_t mid_lock;
bool wait_cancelled:1; /* Cancelled while waiting for response */
bool deleted_from_q:1; /* Whether Mid has been dequeued frem pending_mid_q */
bool large_buf:1; /* if valid response, is pointer to large buf */
bool multiRsp:1; /* multiple trans2 responses for one request */
bool multiEnd:1; /* both received */
@@ -2034,10 +2035,13 @@ require use of the stronger protocol */
* init_cached_dir
* cifsFileInfo->fh_mutex cifsFileInfo cifs_new_fileinfo
* cifsFileInfo->file_info_lock cifsFileInfo->count cifs_new_fileinfo
* ->invalidHandle initiate_cifs_search
* ->oplock_break_cancelled
+ * mid_q_entry->mid_lock mid_q_entry->mid_state alloc_mid
+ * mid_q_entry->callback smb2_mid_entry_alloc
+ * (Ensure that mid->callback is executed atomically)
****************************************************************************/
#ifdef DECLARE_GLOBALS_HERE
#define GLOBAL_EXTERN
#else
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 587845a2452d..57c2ebf64ef0 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -288,10 +288,18 @@ cifs_mark_tcp_ses_conns_for_reconnect(struct TCP_Server_Info *server,
}
}
spin_unlock(&cifs_tcp_ses_lock);
}
+static inline void mid_execute_callback(struct mid_q_entry *mid)
+{
+ spin_lock(&mid->mid_lock);
+ if (mid->callback)
+ mid->callback(mid);
+ spin_unlock(&mid->mid_lock);
+}
+
static void
cifs_abort_connection(struct TCP_Server_Info *server)
{
struct mid_q_entry *mid, *nmid;
struct list_head retry_list;
@@ -322,22 +330,24 @@ cifs_abort_connection(struct TCP_Server_Info *server)
INIT_LIST_HEAD(&retry_list);
cifs_dbg(FYI, "%s: moving mids to private list\n", __func__);
spin_lock(&server->mid_queue_lock);
list_for_each_entry_safe(mid, nmid, &server->pending_mid_q, qhead) {
kref_get(&mid->refcount);
+ spin_lock(&mid->mid_lock);
if (mid->mid_state == MID_REQUEST_SUBMITTED)
mid->mid_state = MID_RETRY_NEEDED;
+ spin_unlock(&mid->mid_lock);
list_move(&mid->qhead, &retry_list);
mid->deleted_from_q = true;
}
spin_unlock(&server->mid_queue_lock);
cifs_server_unlock(server);
cifs_dbg(FYI, "%s: issuing mid callbacks\n", __func__);
list_for_each_entry_safe(mid, nmid, &retry_list, qhead) {
list_del_init(&mid->qhead);
- mid->callback(mid);
+ mid_execute_callback(mid);
release_mid(mid);
}
if (cifs_rdma_enabled(server)) {
cifs_server_lock(server);
@@ -917,11 +927,11 @@ is_smb_response(struct TCP_Server_Info *server, unsigned char type)
*/
list_for_each_entry_safe(mid, nmid, &dispose_list, qhead) {
list_del_init(&mid->qhead);
mid->mid_rc = mid_rc;
mid->mid_state = MID_RC;
- mid->callback(mid);
+ mid_execute_callback(mid);
release_mid(mid);
}
/*
* If reconnect failed then wait two seconds. In most
@@ -956,14 +966,16 @@ dequeue_mid(struct mid_q_entry *mid, bool malformed)
{
#ifdef CONFIG_CIFS_STATS2
mid->when_received = jiffies;
#endif
spin_lock(&mid->server->mid_queue_lock);
+ spin_lock(&mid->mid_lock);
if (!malformed)
mid->mid_state = MID_RESPONSE_RECEIVED;
else
mid->mid_state = MID_RESPONSE_MALFORMED;
+ spin_unlock(&mid->mid_lock);
/*
* Trying to handle/dequeue a mid after the send_recv()
* function has finished processing it is a bug.
*/
if (mid->deleted_from_q == true) {
@@ -1104,22 +1116,24 @@ clean_demultiplex_info(struct TCP_Server_Info *server)
spin_lock(&server->mid_queue_lock);
list_for_each_safe(tmp, tmp2, &server->pending_mid_q) {
mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
cifs_dbg(FYI, "Clearing mid %llu\n", mid_entry->mid);
kref_get(&mid_entry->refcount);
+ spin_lock(&mid_entry->mid_lock);
mid_entry->mid_state = MID_SHUTDOWN;
+ spin_unlock(&mid_entry->mid_lock);
list_move(&mid_entry->qhead, &dispose_list);
mid_entry->deleted_from_q = true;
}
spin_unlock(&server->mid_queue_lock);
/* now walk dispose list and issue callbacks */
list_for_each_safe(tmp, tmp2, &dispose_list) {
mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
cifs_dbg(FYI, "Callback mid %llu\n", mid_entry->mid);
list_del_init(&mid_entry->qhead);
- mid_entry->callback(mid_entry);
+ mid_execute_callback(mid_entry);
release_mid(mid_entry);
}
/* 1/8th of sec is more than enough time for them to exit */
msleep(125);
}
@@ -1392,11 +1406,11 @@ cifs_demultiplex_thread(void *p)
"Share deleted. Reconnect needed");
}
}
if (!mids[i]->multiRsp || mids[i]->multiEnd)
- mids[i]->callback(mids[i]);
+ mid_execute_callback(mids[i]);
release_mid(mids[i]);
} else if (server->ops->is_oplock_break &&
server->ops->is_oplock_break(bufs[i],
server)) {
diff --git a/fs/smb/client/smb1ops.c b/fs/smb/client/smb1ops.c
index 13f600a3d0c4..6a6b09cfcefa 100644
--- a/fs/smb/client/smb1ops.c
+++ b/fs/smb/client/smb1ops.c
@@ -95,17 +95,20 @@ cifs_find_mid(struct TCP_Server_Info *server, char *buffer)
struct smb_hdr *buf = (struct smb_hdr *)buffer;
struct mid_q_entry *mid;
spin_lock(&server->mid_queue_lock);
list_for_each_entry(mid, &server->pending_mid_q, qhead) {
+ spin_lock(&mid->mid_lock);
if (compare_mid(mid->mid, buf) &&
mid->mid_state == MID_REQUEST_SUBMITTED &&
le16_to_cpu(mid->command) == buf->Command) {
+ spin_unlock(&mid->mid_lock);
kref_get(&mid->refcount);
spin_unlock(&server->mid_queue_lock);
return mid;
}
+ spin_unlock(&mid->mid_lock);
}
spin_unlock(&server->mid_queue_lock);
return NULL;
}
@@ -198,16 +201,19 @@ cifs_get_next_mid(struct TCP_Server_Info *server)
num_mids = 0;
spin_lock(&server->mid_queue_lock);
list_for_each_entry(mid_entry, &server->pending_mid_q, qhead) {
++num_mids;
+ spin_lock(&mid_entry->mid_lock);
if (mid_entry->mid == cur_mid &&
mid_entry->mid_state == MID_REQUEST_SUBMITTED) {
+ spin_unlock(&mid_entry->mid_lock);
/* This mid is in use, try a different one */
collision = true;
break;
}
+ spin_unlock(&mid_entry->mid_lock);
}
spin_unlock(&server->mid_queue_lock);
/*
* if we have more than 32k mids in the list, then something
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 2643d86a5b5f..dc0f1ba70e61 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -4803,27 +4803,33 @@ static void smb2_decrypt_offload(struct work_struct *work)
#endif
if (dw->server->ops->is_network_name_deleted)
dw->server->ops->is_network_name_deleted(dw->buf,
dw->server);
- mid->callback(mid);
+ spin_lock(&mid->mid_lock);
+ if (mid->callback)
+ mid->callback(mid);
+ spin_unlock(&mid->mid_lock);
} else {
spin_lock(&dw->server->srv_lock);
if (dw->server->tcpStatus == CifsNeedReconnect) {
- spin_lock(&dw->server->mid_queue_lock);
- mid->mid_state = MID_RETRY_NEEDED;
- spin_unlock(&dw->server->mid_queue_lock);
spin_unlock(&dw->server->srv_lock);
- mid->callback(mid);
+ spin_lock(&mid->mid_lock);
+ mid->mid_state = MID_RETRY_NEEDED;
+ if (mid->callback)
+ mid->callback(mid);
+ spin_unlock(&mid->mid_lock);
} else {
+ spin_unlock(&dw->server->srv_lock);
spin_lock(&dw->server->mid_queue_lock);
+ spin_lock(&mid->mid_lock);
mid->mid_state = MID_REQUEST_SUBMITTED;
+ spin_unlock(&mid->mid_lock);
mid->deleted_from_q = false;
list_add_tail(&mid->qhead,
&dw->server->pending_mid_q);
spin_unlock(&dw->server->mid_queue_lock);
- spin_unlock(&dw->server->srv_lock);
}
}
release_mid(mid);
}
diff --git a/fs/smb/client/smb2transport.c b/fs/smb/client/smb2transport.c
index ff9ef7fcd010..bc0e92eb2b64 100644
--- a/fs/smb/client/smb2transport.c
+++ b/fs/smb/client/smb2transport.c
@@ -769,10 +769,11 @@ smb2_mid_entry_alloc(const struct smb2_hdr *shdr,
}
temp = mempool_alloc(cifs_mid_poolp, GFP_NOFS);
memset(temp, 0, sizeof(struct mid_q_entry));
kref_init(&temp->refcount);
+ spin_lock_init(&temp->mid_lock);
temp->mid = le64_to_cpu(shdr->MessageId);
temp->credits = credits > 0 ? credits : 1;
temp->pid = current->pid;
temp->command = shdr->Command; /* Always LE */
temp->when_alloc = jiffies;
diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
index ca9358c24ceb..8bbcecf2225d 100644
--- a/fs/smb/client/transport.c
+++ b/fs/smb/client/transport.c
@@ -52,10 +52,11 @@ alloc_mid(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
}
temp = mempool_alloc(cifs_mid_poolp, GFP_NOFS);
memset(temp, 0, sizeof(struct mid_q_entry));
kref_init(&temp->refcount);
+ spin_lock_init(&temp->mid_lock);
temp->mid = get_mid(smb_buffer);
temp->pid = current->pid;
temp->command = cpu_to_le16(smb_buffer->Command);
cifs_dbg(FYI, "For smb_command %d\n", smb_buffer->Command);
/* easier to use jiffies */
@@ -875,17 +876,17 @@ SendReceiveNoRsp(const unsigned int xid, struct cifs_ses *ses,
static int
cifs_sync_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
{
int rc = 0;
+ spin_lock(&mid->mid_lock);
cifs_dbg(FYI, "%s: cmd=%d mid=%llu state=%d\n",
__func__, le16_to_cpu(mid->command), mid->mid, mid->mid_state);
- spin_lock(&server->mid_queue_lock);
switch (mid->mid_state) {
case MID_RESPONSE_READY:
- spin_unlock(&server->mid_queue_lock);
+ spin_unlock(&mid->mid_lock);
return rc;
case MID_RETRY_NEEDED:
rc = -EAGAIN;
break;
case MID_RESPONSE_MALFORMED:
@@ -896,21 +897,25 @@ cifs_sync_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
break;
case MID_RC:
rc = mid->mid_rc;
break;
default:
+ cifs_server_dbg(VFS, "%s: invalid mid state mid=%llu state=%d\n",
+ __func__, mid->mid, mid->mid_state);
+ spin_unlock(&mid->mid_lock);
+
+ spin_lock(&server->mid_queue_lock);
if (mid->deleted_from_q == false) {
list_del_init(&mid->qhead);
mid->deleted_from_q = true;
}
spin_unlock(&server->mid_queue_lock);
- cifs_server_dbg(VFS, "%s: invalid mid state mid=%llu state=%d\n",
- __func__, mid->mid, mid->mid_state);
+
rc = -EIO;
goto sync_mid_done;
}
- spin_unlock(&server->mid_queue_lock);
+ spin_unlock(&mid->mid_lock);
sync_mid_done:
release_mid(mid);
return rc;
}
@@ -1212,17 +1217,19 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
for (; i < num_rqst; i++) {
cifs_server_dbg(FYI, "Cancelling wait for mid %llu cmd: %d\n",
midQ[i]->mid, le16_to_cpu(midQ[i]->command));
send_cancel(server, &rqst[i], midQ[i]);
spin_lock(&server->mid_queue_lock);
+ spin_lock(&midQ[i]->mid_lock);
midQ[i]->wait_cancelled = true;
if (midQ[i]->mid_state == MID_REQUEST_SUBMITTED ||
midQ[i]->mid_state == MID_RESPONSE_RECEIVED) {
midQ[i]->callback = cifs_cancelled_callback;
cancelled_mid[i] = true;
credits[i].value = 0;
}
+ spin_unlock(&midQ[i]->mid_lock);
spin_unlock(&server->mid_queue_lock);
}
}
for (i = 0; i < num_rqst; i++) {
@@ -1421,20 +1428,20 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses,
goto out;
rc = wait_for_response(server, midQ);
if (rc != 0) {
send_cancel(server, &rqst, midQ);
- spin_lock(&server->mid_queue_lock);
+ spin_lock(&midQ->mid_lock);
if (midQ->mid_state == MID_REQUEST_SUBMITTED ||
midQ->mid_state == MID_RESPONSE_RECEIVED) {
/* no longer considered to be "in-flight" */
midQ->callback = release_mid;
- spin_unlock(&server->mid_queue_lock);
+ spin_unlock(&midQ->mid_lock);
add_credits(server, &credits, 0);
return rc;
}
- spin_unlock(&server->mid_queue_lock);
+ spin_unlock(&midQ->mid_lock);
}
rc = cifs_sync_mid_result(midQ, server);
if (rc != 0) {
add_credits(server, &credits, 0);
@@ -1603,19 +1610,19 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifs_tcon *tcon,
}
rc = wait_for_response(server, midQ);
if (rc) {
send_cancel(server, &rqst, midQ);
- spin_lock(&server->mid_queue_lock);
+ spin_lock(&midQ->mid_lock);
if (midQ->mid_state == MID_REQUEST_SUBMITTED ||
midQ->mid_state == MID_RESPONSE_RECEIVED) {
/* no longer considered to be "in-flight" */
midQ->callback = release_mid;
- spin_unlock(&server->mid_queue_lock);
+ spin_unlock(&midQ->mid_lock);
return rc;
}
- spin_unlock(&server->mid_queue_lock);
+ spin_unlock(&midQ->mid_lock);
}
/* We got the response - restart system call. */
rstart = 1;
spin_lock(&server->srv_lock);
--
2.39.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client
2025-08-05 6:47 [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client Wang Zhaolong
` (3 preceding siblings ...)
2025-08-05 6:47 ` [PATCH V2 4/4] smb: client: fix mid_q_entry memleak leak with per-mid locking Wang Zhaolong
@ 2025-08-05 12:47 ` Enzo Matsumiya
2025-08-05 16:54 ` Wang Zhaolong
2025-08-05 16:39 ` Steve French
5 siblings, 1 reply; 14+ messages in thread
From: Enzo Matsumiya @ 2025-08-05 12:47 UTC (permalink / raw)
To: Wang Zhaolong
Cc: sfrench, pshilov, linux-cifs, samba-technical, linux-kernel,
chengzhihao1, yi.zhang, yangerkun
Hi Wang,
On 08/05, Wang Zhaolong 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.
Do you have a reproducer for this? mids are allocated from kmem cache,
and a leak should certainly be visible (WARN on rmmod), even without any
debugging facilities enabled.
However, I do know that the following problem is quite common in cifs:
thread 0 | thread 1
----------------|----------------
| lock
| check data
| data is valid
| unlock
lock |
invalidate data | lock (spins)
unlock | ...
| // assumes data still valid
| use invalid data (not really freed)
| unlock
You see that no matter how many locks you add to protect data, there's
still a chance of having this "race condition" feeling.
So, personally, I'm skeptical about having yet another spinlock with
questionable or no effect at all.
But again, if I can reproduce this bug myself, it'll be much easier to
analyse effectiveness/review your patches.
Apart from that, cleanup patches always get my +1 :)
Cheers,
Enzo
>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
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client
2025-08-05 6:47 [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client Wang Zhaolong
` (4 preceding siblings ...)
2025-08-05 12:47 ` [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client Enzo Matsumiya
@ 2025-08-05 16:39 ` Steve French
2025-08-05 18:16 ` Enzo Matsumiya
2025-08-06 0:55 ` Wang Zhaolong
5 siblings, 2 replies; 14+ messages in thread
From: Steve French @ 2025-08-05 16:39 UTC (permalink / raw)
To: Wang Zhaolong
Cc: pshilov, linux-cifs, samba-technical, linux-kernel, chengzhihao1,
yi.zhang, yangerkun, Enzo Matsumiya
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
[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@huaweicloud.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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client
2025-08-05 12:47 ` [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client Enzo Matsumiya
@ 2025-08-05 16:54 ` Wang Zhaolong
0 siblings, 0 replies; 14+ messages in thread
From: Wang Zhaolong @ 2025-08-05 16:54 UTC (permalink / raw)
To: Enzo Matsumiya
Cc: sfrench, pshilov, linux-cifs, samba-technical, linux-kernel,
chengzhihao1, yi.zhang, yangerkun
>
> Do you have a reproducer for this? mids are allocated from kmem cache,
> and a leak should certainly be visible (WARN on rmmod), even without any
> debugging facilities enabled.
Thanks for your reply and review!
Yes, I have the reproducer. I have listed it in the commit message.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=220404
>
> However, I do know that the following problem is quite common in cifs:
>
> thread 0 | thread 1
> ----------------|----------------
> | lock
> | check data
> | data is valid
> | unlock
> lock |
> invalidate data | lock (spins)
> unlock | ...
> | // assumes data still valid
> | use invalid data (not really freed)
> | unlock
>
> You see that no matter how many locks you add to protect data, there's
> still a chance of having this "race condition" feeling.
>
> So, personally, I'm skeptical about having yet another spinlock with
> questionable or no effect at all.
>
> But again, if I can reproduce this bug myself, it'll be much easier to
> analyse effectiveness/review your patches.
>
> Apart from that, cleanup patches always get my +1 :)
>
The data is interrelated, and updates to the data are protected by per-mid locks
to avoid `race condition`.
For example, in the following scenario, the update and check of
mid->mid_state are protected by the lock spin_lock(&midQ[i]->mid_lock),
which can prevent leakage issues.
```
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 */
/* Signal interrupts wait: rc = -ERESTARTSYS */
spin_lock(&midQ[i]->mid_lock);
mids[0]->callback
cifs_compound_callback
/* The change comes too late */
mid->mid_state = MID_RESPONSE_READY
spin_lock(&midQ[i]->mid_lock);
spin_lock(&midQ[i]->mid_lock);
/* if (... || midQ[i]->mid_state == MID_RESPONSE_RECEIVED) Not satisfied *?
spin_unlock(&midQ[i]->mid_lock);
release_mid // -1
release_mid // -1
```
Or, in the case where the callback is replaced
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 */
/* Signal interrupts wait: rc = -ERESTARTSYS */
spin_lock(&midQ[i]->mid_lock);
/* if (... || midQ[i]->mid_state == MID_RESPONSE_RECEIVED) Satisfied *?
midQ[0]->callback = cifs_cancelled_callback;
cancelled_mid[i] = true;
spin_unlock(&midQ[i]->mid_lock);
spin_lock(&midQ[i]->mid_lock);
mids[0]->callback
cifs_cancelled_callback
cifs_compound_callback
/* The change comes too late */
mid->mid_state = MID_RESPONSE_READY
release_mid // -1
spin_lock(&midQ[i]->mid_lock);
release_mid // -1
I know this approach is quite ugly, but it is a relatively reliable
method that preserves the historical bugfix logic.
Relevant patches:
d527f51331ca ("cifs: Fix UAF in cifs_demultiplex_thread()")
ee258d79159a ("CIFS: Move credit processing to mid callbacks for SMB3")
8544f4aa9dd1 ("CIFS: Fix credit computation for compounded requests")
d7d7a66aacd6 ("cifs: avoid use of global locks for high contention data")
Thanks again for your reply. I’m looking forward to any further
suggestions you may have to improve the patch.
Best regards,
Wang Zhaolong
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client
2025-08-05 16:39 ` Steve French
@ 2025-08-05 18:16 ` Enzo Matsumiya
2025-08-05 19:17 ` Enzo Matsumiya
2025-08-06 0:55 ` Wang Zhaolong
1 sibling, 1 reply; 14+ messages in thread
From: Enzo Matsumiya @ 2025-08-05 18:16 UTC (permalink / raw)
To: Steve French
Cc: Wang Zhaolong, pshilov, linux-cifs, samba-technical, linux-kernel,
chengzhihao1, yi.zhang, yangerkun
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@huaweicloud.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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client
2025-08-05 18:16 ` Enzo Matsumiya
@ 2025-08-05 19:17 ` Enzo Matsumiya
0 siblings, 0 replies; 14+ messages in thread
From: Enzo Matsumiya @ 2025-08-05 19:17 UTC (permalink / raw)
To: Wang Zhaolong
Cc: Steve French, linux-cifs, samba-technical, linux-kernel,
chengzhihao1, yi.zhang, yangerkun
Hi Wang,
On 08/05, Enzo Matsumiya wrote:
>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!
So, strangely, your poc.c doesn't give that backtrace (invalid wait
context) that Steve mentions.
And while I could confirm the original mid leak and that your fix works,
now I get this in kmemleak:
...
unreferenced object 0xffff8881064edf00 (size 192):
comm "poc", pid 36032, jiffies 4294813121
hex dump (first 32 bytes):
00 df 4e 06 81 88 ff ff 00 df 4e 06 81 88 ff ff ..N.......N.....
01 00 00 00 00 00 00 00 00 50 6d 03 81 88 ff ff .........Pm.....
backtrace (crc fc0a60b2):
kmem_cache_alloc_noprof+0x2f4/0x3f0
mempool_alloc_noprof+0x6e/0x1c0
generate_smb3signingkey+0x17f/0x2c0 [cifs]
smb2_verify_signature+0x110/0x170 [cifs]
compound_send_recv+0x11/0xb90 [cifs]
compound_send_recv+0x983/0xb90 [cifs]
SMB2_close_init+0x9f/0xc0 [cifs]
cifs_readdir+0x44/0x1450 [cifs]
iterate_dir+0x8a/0x160
__x64_sys_getdents+0x7b/0x120
do_syscall_64+0x6a/0x2d0
entry_SYSCALL_64_after_hwframe+0x76/0x7e
From a quick look at the code, I couldn't make sense of it, and even
less so _how_ your patch could be causing this. But it's certainly
doing something that is impacting the signing crypto TFM.
Cheers,
Enzo
>>[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@huaweicloud.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
>>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client
2025-08-05 16:39 ` Steve French
2025-08-05 18:16 ` Enzo Matsumiya
@ 2025-08-06 0:55 ` Wang Zhaolong
2025-08-06 12:24 ` Enzo Matsumiya
2025-08-07 14:43 ` Wang Zhaolong
1 sibling, 2 replies; 14+ messages in thread
From: Wang Zhaolong @ 2025-08-06 0:55 UTC (permalink / raw)
To: Steve French
Cc: pshilov, linux-cifs, samba-technical, linux-kernel, chengzhihao1,
yi.zhang, yangerkun, Enzo Matsumiya
> 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
>
I am unable to view any information in the link above. Is this information
only visible to logged-in users?
>
> [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@huaweicloud.com> wrote:
It's quite strange that the lock reported in the stack trace is an internal
lock of the crypto module, which only protects the internal logic of crypto.
Moreover, I have not yet found a path where the callback for cifs registration
is executed within the scope of this lock.
```c
// crypto/api.c
static struct crypto_alg *crypto_alg_lookup(const char *name, u32 type,
u32 mask)
{
const u32 fips = CRYPTO_ALG_FIPS_INTERNAL;
struct crypto_alg *alg;
u32 test = 0;
if (!((type | mask) & CRYPTO_ALG_TESTED))
test |= CRYPTO_ALG_TESTED;
down_read(&crypto_alg_sem);
...
up_read(&crypto_alg_sem);
return alg;
```
More information is needed to confirm this issue. Could you please provide it?
Best regards,
Wang Zhaolong
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client
2025-08-06 0:55 ` Wang Zhaolong
@ 2025-08-06 12:24 ` Enzo Matsumiya
2025-08-07 14:43 ` Wang Zhaolong
1 sibling, 0 replies; 14+ messages in thread
From: Enzo Matsumiya @ 2025-08-06 12:24 UTC (permalink / raw)
To: Wang Zhaolong
Cc: Steve French, pshilov, linux-cifs, samba-technical, linux-kernel,
chengzhihao1, yi.zhang, yangerkun
On 08/06, Wang Zhaolong 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
>>
>
>I am unable to view any information in the link above. Is this information
>only visible to logged-in users?
That one is publicly visible.
If you're using a Chrome-based browser, you might need to whitelist the
website though as it doesn't use HTTPS.
> ...
>>
>>(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@huaweicloud.com> wrote:
>
>
>It's quite strange that the lock reported in the stack trace is an internal
>lock of the crypto module, which only protects the internal logic of crypto.
>Moreover, I have not yet found a path where the callback for cifs registration
>is executed within the scope of this lock.
>
>```c
>// crypto/api.c
>static struct crypto_alg *crypto_alg_lookup(const char *name, u32 type,
> u32 mask)
>{
> const u32 fips = CRYPTO_ALG_FIPS_INTERNAL;
> struct crypto_alg *alg;
> u32 test = 0;
>
> if (!((type | mask) & CRYPTO_ALG_TESTED))
> test |= CRYPTO_ALG_TESTED;
>
> down_read(&crypto_alg_sem);
> ...
> up_read(&crypto_alg_sem);
> return alg;
>```
>More information is needed to confirm this issue. Could you please provide it?
In summary the problem is in mid_execute_callback() when callback is
smb2_writev_callback.
Cleaning it up for clarity:
---- begin ----
cifsd/24912 is trying to lock crypto_alg_sem at crypto_alg_lookup+0x40/0x120
cifsd/24912 is holding &temp->mid_lock at mid_execute_callback+0x19/0x40
Reversed call trace:
cifs_demultiplex_thread
mid_execute_callback
<lock mid_lock>
smb2_writev_callback
smb2_check_receive
smb2_verify_signature
smb3_calc_signature
cifs_alloc_hash
crypto_alloc_tfm_node
crypto_alg_mod_lookup
crypto_alg_lookup
down_read
lock_acquire
__lock_acquire
>>> BUG() here <<<
...
<unlock mid_lock>
---- end ----
Before the patch I sent you privately yesterday, I confirmed this
by locking mid_lock directly only for cifs_compound*callback and
cifs_wake_up_task.
Also you need to make sure to use a reproducer/tester that uses writev,
which your poc.c from original bug report doesn't.
HTH
Cheers,
Enzo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client
2025-08-06 0:55 ` Wang Zhaolong
2025-08-06 12:24 ` Enzo Matsumiya
@ 2025-08-07 14:43 ` Wang Zhaolong
2025-08-07 16:02 ` Steve French
1 sibling, 1 reply; 14+ messages in thread
From: Wang Zhaolong @ 2025-08-07 14:43 UTC (permalink / raw)
To: Steve French
Cc: pshilov, linux-cifs, samba-technical, linux-kernel, chengzhihao1,
yi.zhang, yangerkun, Enzo Matsumiya
Sorry for the delayed response. I can see exactly what went wrong now.
The issue is that my implementation holds a spinlock (mid_lock) while
executing the callback, but the callback path can eventually lead to
crypto_alg_lookup() which tries to acquire a semaphore. This violates
the kernel's locking rules - we cannot sleep while holding a spinlock.
Perhaps I should consider a more ingenious solution that can safely
handle these cross-subsystem interactions.
I'll rework the patch to fix this locking issue and send a v3. I'll
probably need to rethink the whole locking strategy to be more aware
of what the callbacks actually do and what they might need to sleep for.
Best regards,
Wang Zhaolong
>
>> 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
>>
>
> I am unable to view any information in the link above. Is this information
> only visible to logged-in users?
>
>
>>
>> [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@huaweicloud.com> wrote:
>
>
> It's quite strange that the lock reported in the stack trace is an internal
> lock of the crypto module, which only protects the internal logic of crypto.
> Moreover, I have not yet found a path where the callback for cifs registration
> is executed within the scope of this lock.
>
> ```c
> // crypto/api.c
> static struct crypto_alg *crypto_alg_lookup(const char *name, u32 type,
> u32 mask)
> {
> const u32 fips = CRYPTO_ALG_FIPS_INTERNAL;
> struct crypto_alg *alg;
> u32 test = 0;
>
> if (!((type | mask) & CRYPTO_ALG_TESTED))
> test |= CRYPTO_ALG_TESTED;
>
> down_read(&crypto_alg_sem);
> ...
> up_read(&crypto_alg_sem);
> return alg;
> ```
> More information is needed to confirm this issue. Could you please provide it?
>
> Best regards,
> Wang Zhaolong
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client
2025-08-07 14:43 ` Wang Zhaolong
@ 2025-08-07 16:02 ` Steve French
0 siblings, 0 replies; 14+ messages in thread
From: Steve French @ 2025-08-07 16:02 UTC (permalink / raw)
To: Wang Zhaolong
Cc: pshilov, linux-cifs, samba-technical, linux-kernel, chengzhihao1,
yi.zhang, yangerkun, Enzo Matsumiya
presumably the first three cleanup are ok - but if objections let me know
On Thu, Aug 7, 2025 at 9:43 AM Wang Zhaolong
<wangzhaolong@huaweicloud.com> wrote:
>
>
> Sorry for the delayed response. I can see exactly what went wrong now.
>
> The issue is that my implementation holds a spinlock (mid_lock) while
> executing the callback, but the callback path can eventually lead to
> crypto_alg_lookup() which tries to acquire a semaphore. This violates
> the kernel's locking rules - we cannot sleep while holding a spinlock.
>
> Perhaps I should consider a more ingenious solution that can safely
> handle these cross-subsystem interactions.
>
> I'll rework the patch to fix this locking issue and send a v3. I'll
> probably need to rethink the whole locking strategy to be more aware
> of what the callbacks actually do and what they might need to sleep for.
>
> Best regards,
> Wang Zhaolong
>
>
> >
> >> 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
> >>
> >
> > I am unable to view any information in the link above. Is this information
> > only visible to logged-in users?
> >
> >
> >>
> >> [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@huaweicloud.com> wrote:
> >
> >
> > It's quite strange that the lock reported in the stack trace is an internal
> > lock of the crypto module, which only protects the internal logic of crypto.
> > Moreover, I have not yet found a path where the callback for cifs registration
> > is executed within the scope of this lock.
> >
> > ```c
> > // crypto/api.c
> > static struct crypto_alg *crypto_alg_lookup(const char *name, u32 type,
> > u32 mask)
> > {
> > const u32 fips = CRYPTO_ALG_FIPS_INTERNAL;
> > struct crypto_alg *alg;
> > u32 test = 0;
> >
> > if (!((type | mask) & CRYPTO_ALG_TESTED))
> > test |= CRYPTO_ALG_TESTED;
> >
> > down_read(&crypto_alg_sem);
> > ...
> > up_read(&crypto_alg_sem);
> > return alg;
> > ```
> > More information is needed to confirm this issue. Could you please provide it?
> >
> > Best regards,
> > Wang Zhaolong
> >
>
>
>
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2025-08-07 16:02 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-05 6:47 [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client Wang Zhaolong
2025-08-05 6:47 ` [PATCH V2 1/4] smb: client: rename server mid_lock to mid_queue_lock Wang Zhaolong
2025-08-05 6:47 ` [PATCH V2 2/4] smb: client: add mid_counter_lock to protect the mid counter counter Wang Zhaolong
2025-08-05 6:47 ` [PATCH V2 3/4] smb: client: smb: client: eliminate mid_flags field Wang Zhaolong
2025-08-05 6:47 ` [PATCH V2 4/4] smb: client: fix mid_q_entry memleak leak with per-mid locking Wang Zhaolong
2025-08-05 12:47 ` [PATCH V2 0/4] Fix mid_q_entry memory leaks in SMB client Enzo Matsumiya
2025-08-05 16:54 ` Wang Zhaolong
2025-08-05 16:39 ` Steve French
2025-08-05 18:16 ` Enzo Matsumiya
2025-08-05 19:17 ` Enzo Matsumiya
2025-08-06 0:55 ` Wang Zhaolong
2025-08-06 12:24 ` Enzo Matsumiya
2025-08-07 14:43 ` Wang Zhaolong
2025-08-07 16:02 ` Steve French
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).