* [PATCH 0/4] Fix mid_q_entry memory leaks in SMB client
@ 2025-08-04 13:40 Wang Zhaolong
2025-08-04 13:40 ` [PATCH 1/4] smb: client: rename server mid_lock to mid_queue_lock Wang Zhaolong
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Wang Zhaolong @ 2025-08-04 13:40 UTC (permalink / raw)
To: sfrench, pshilov
Cc: linux-cifs, samba-technical, linux-kernel, chengzhihao1,
wangzhaolong, 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.
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 | 23 +++++++-----
fs/smb/client/connect.c | 57 +++++++++++++++++-----------
fs/smb/client/smb1ops.c | 23 ++++++++----
fs/smb/client/smb2ops.c | 69 ++++++++++++++++++----------------
fs/smb/client/smb2transport.c | 5 ++-
fs/smb/client/transport.c | 71 +++++++++++++++++++----------------
7 files changed, 150 insertions(+), 110 deletions(-)
--
2.39.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/4] smb: client: rename server mid_lock to mid_queue_lock
2025-08-04 13:40 [PATCH 0/4] Fix mid_q_entry memory leaks in SMB client Wang Zhaolong
@ 2025-08-04 13:40 ` Wang Zhaolong
2025-08-04 13:40 ` [PATCH 2/4] smb: client: add mid_counter_lock to protect the mid counter counter Wang Zhaolong
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Wang Zhaolong @ 2025-08-04 13:40 UTC (permalink / raw)
To: sfrench, pshilov
Cc: linux-cifs, samba-technical, linux-kernel, chengzhihao1,
wangzhaolong, 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] 7+ messages in thread
* [PATCH 2/4] smb: client: add mid_counter_lock to protect the mid counter counter
2025-08-04 13:40 [PATCH 0/4] Fix mid_q_entry memory leaks in SMB client Wang Zhaolong
2025-08-04 13:40 ` [PATCH 1/4] smb: client: rename server mid_lock to mid_queue_lock Wang Zhaolong
@ 2025-08-04 13:40 ` Wang Zhaolong
2025-08-04 13:40 ` [PATCH 3/4] smb: client: smb: client: eliminate mid_flags field Wang Zhaolong
2025-08-04 13:40 ` [PATCH 4/4] smb: client: fix mid_q_entry memleak leak with per-mid locking Wang Zhaolong
3 siblings, 0 replies; 7+ messages in thread
From: Wang Zhaolong @ 2025-08-04 13:40 UTC (permalink / raw)
To: sfrench, pshilov
Cc: linux-cifs, samba-technical, linux-kernel, chengzhihao1,
wangzhaolong, 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] 7+ messages in thread
* [PATCH 3/4] smb: client: smb: client: eliminate mid_flags field
2025-08-04 13:40 [PATCH 0/4] Fix mid_q_entry memory leaks in SMB client Wang Zhaolong
2025-08-04 13:40 ` [PATCH 1/4] smb: client: rename server mid_lock to mid_queue_lock Wang Zhaolong
2025-08-04 13:40 ` [PATCH 2/4] smb: client: add mid_counter_lock to protect the mid counter counter Wang Zhaolong
@ 2025-08-04 13:40 ` Wang Zhaolong
2025-08-04 13:40 ` [PATCH 4/4] smb: client: fix mid_q_entry memleak leak with per-mid locking Wang Zhaolong
3 siblings, 0 replies; 7+ messages in thread
From: Wang Zhaolong @ 2025-08-04 13:40 UTC (permalink / raw)
To: sfrench, pshilov
Cc: linux-cifs, samba-technical, linux-kernel, chengzhihao1,
wangzhaolong, 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] 7+ messages in thread
* [PATCH 4/4] smb: client: fix mid_q_entry memleak leak with per-mid locking
2025-08-04 13:40 [PATCH 0/4] Fix mid_q_entry memory leaks in SMB client Wang Zhaolong
` (2 preceding siblings ...)
2025-08-04 13:40 ` [PATCH 3/4] smb: client: smb: client: eliminate mid_flags field Wang Zhaolong
@ 2025-08-04 13:40 ` Wang Zhaolong
2025-08-05 2:53 ` Steve French
3 siblings, 1 reply; 7+ messages in thread
From: Wang Zhaolong @ 2025-08-04 13:40 UTC (permalink / raw)
To: sfrench, pshilov
Cc: linux-cifs, samba-technical, linux-kernel, chengzhihao1,
wangzhaolong, 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 | 5 +++++
fs/smb/client/connect.c | 22 ++++++++++++++++++----
fs/smb/client/smb1ops.c | 6 ++++++
fs/smb/client/smb2ops.c | 15 +++++++++------
fs/smb/client/smb2transport.c | 1 +
fs/smb/client/transport.c | 29 ++++++++++++++++++-----------
7 files changed, 61 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..430163a878d9 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -1357,10 +1357,11 @@ struct tcon_link {
struct cifs_tcon *tl_tcon;
};
extern struct tcon_link *cifs_sb_tlink(struct cifs_sb_info *cifs_sb);
extern void smb3_free_compound_rqst(int num_rqst, struct smb_rqst *rqst);
+extern inline void mid_execute_callback(struct mid_q_entry *mid);
static inline struct cifs_tcon *
tlink_tcon(struct tcon_link *tlink)
{
return tlink->tl_tcon;
@@ -1730,10 +1731,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 +2036,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..2d85554b8041 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);
}
+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..14c572e04894 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -4803,27 +4803,30 @@ 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);
+ mid_execute_callback(mid);
} 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] 7+ messages in thread
* Re: [PATCH 4/4] smb: client: fix mid_q_entry memleak leak with per-mid locking
2025-08-04 13:40 ` [PATCH 4/4] smb: client: fix mid_q_entry memleak leak with per-mid locking Wang Zhaolong
@ 2025-08-05 2:53 ` Steve French
2025-08-05 6:08 ` Wang Zhaolong
0 siblings, 1 reply; 7+ messages in thread
From: Steve French @ 2025-08-05 2:53 UTC (permalink / raw)
To: Wang Zhaolong
Cc: pshilov, linux-cifs, samba-technical, linux-kernel, chengzhihao1,
yi.zhang, yangerkun
minor sparse warning when compiling this:
CHECK smb2ops.c
smb2ops.c: note: in included file:
cifsglob.h:1362:40: error: marked inline, but without a definition
On Mon, Aug 4, 2025 at 9:00 AM Wang Zhaolong
<wangzhaolong@huaweicloud.com> wrote:
>
> 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 | 5 +++++
> fs/smb/client/connect.c | 22 ++++++++++++++++++----
> fs/smb/client/smb1ops.c | 6 ++++++
> fs/smb/client/smb2ops.c | 15 +++++++++------
> fs/smb/client/smb2transport.c | 1 +
> fs/smb/client/transport.c | 29 ++++++++++++++++++-----------
> 7 files changed, 61 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..430163a878d9 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -1357,10 +1357,11 @@ struct tcon_link {
> struct cifs_tcon *tl_tcon;
> };
>
> extern struct tcon_link *cifs_sb_tlink(struct cifs_sb_info *cifs_sb);
> extern void smb3_free_compound_rqst(int num_rqst, struct smb_rqst *rqst);
> +extern inline void mid_execute_callback(struct mid_q_entry *mid);
>
> static inline struct cifs_tcon *
> tlink_tcon(struct tcon_link *tlink)
> {
> return tlink->tl_tcon;
> @@ -1730,10 +1731,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 +2036,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..2d85554b8041 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);
> }
>
> +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..14c572e04894 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -4803,27 +4803,30 @@ 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);
> + mid_execute_callback(mid);
> } 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
>
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 4/4] smb: client: fix mid_q_entry memleak leak with per-mid locking
2025-08-05 2:53 ` Steve French
@ 2025-08-05 6:08 ` Wang Zhaolong
0 siblings, 0 replies; 7+ messages in thread
From: Wang Zhaolong @ 2025-08-05 6:08 UTC (permalink / raw)
To: Steve French
Cc: pshilov, linux-cifs, samba-technical, linux-kernel, chengzhihao1,
yi.zhang, yangerkun
> minor sparse warning when compiling this:
>
> CHECK smb2ops.c
> smb2ops.c: note: in included file:
> cifsglob.h:1362:40: error: marked inline, but without a definition
>
> On Mon, Aug 4, 2025 at 9:00 AM Wang Zhaolong
> <wangzhaolong@huaweicloud.com> wrote:
This seems to be a warning caused by the static analysis
tool analyzing the smb2ops.c file individually. The
implementation of mid_execute_callback() is actually in
connect.c, and the compiler can handle this situation
correctly.
Perhaps I should inline it directly in smb2ops.c to
eliminate the warning issue in the header file. I will
send the V2 version patch later.
Best regards,
Wang Zhaolong
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-08-05 6:08 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-04 13:40 [PATCH 0/4] Fix mid_q_entry memory leaks in SMB client Wang Zhaolong
2025-08-04 13:40 ` [PATCH 1/4] smb: client: rename server mid_lock to mid_queue_lock Wang Zhaolong
2025-08-04 13:40 ` [PATCH 2/4] smb: client: add mid_counter_lock to protect the mid counter counter Wang Zhaolong
2025-08-04 13:40 ` [PATCH 3/4] smb: client: smb: client: eliminate mid_flags field Wang Zhaolong
2025-08-04 13:40 ` [PATCH 4/4] smb: client: fix mid_q_entry memleak leak with per-mid locking Wang Zhaolong
2025-08-05 2:53 ` Steve French
2025-08-05 6:08 ` Wang Zhaolong
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).