Skip to content

Commit e383573

Browse files
Wang Zhaolongsmfrench
authored andcommitted
smb: client: fix mid_q_entry memleak leak with per-mid locking
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 root cause is that there's a race between callback assignment and execution. Fix this by introducing per-mid locking: - Add spinlock_t mid_lock to struct mid_q_entry - Add mid_execute_callback() for atomic callback execution - Use mid_lock in cancellation paths to ensure atomicity 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: ee258d7 ("CIFS: Move credit processing to mid callbacks for SMB3") Signed-off-by: Wang Zhaolong <[email protected]> Signed-off-by: Steve French <[email protected]>
1 parent 7d34ec3 commit e383573

File tree

6 files changed

+40
-20
lines changed

6 files changed

+40
-20
lines changed

fs/smb/client/cifsglob.h

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1732,6 +1732,7 @@ struct mid_q_entry {
17321732
int mid_rc; /* rc for MID_RC */
17331733
__le16 command; /* smb command code */
17341734
unsigned int optype; /* operation type */
1735+
spinlock_t mid_lock;
17351736
bool wait_cancelled:1; /* Cancelled while waiting for response */
17361737
bool deleted_from_q:1; /* Whether Mid has been dequeued frem pending_mid_q */
17371738
bool large_buf:1; /* if valid response, is pointer to large buf */
@@ -2036,6 +2037,9 @@ require use of the stronger protocol */
20362037
* cifsFileInfo->file_info_lock cifsFileInfo->count cifs_new_fileinfo
20372038
* ->invalidHandle initiate_cifs_search
20382039
* ->oplock_break_cancelled
2040+
* mid_q_entry->mid_lock mid_q_entry->callback alloc_mid
2041+
* smb2_mid_entry_alloc
2042+
* (Any fields of mid_q_entry that will need protection)
20392043
****************************************************************************/
20402044

20412045
#ifdef DECLARE_GLOBALS_HERE
@@ -2375,6 +2379,23 @@ static inline bool cifs_netbios_name(const char *name, size_t namelen)
23752379
return ret;
23762380
}
23772381

2382+
/*
2383+
* Execute mid callback atomically - ensures callback runs exactly once
2384+
* and prevents sleeping in atomic context.
2385+
*/
2386+
static inline void mid_execute_callback(struct mid_q_entry *mid)
2387+
{
2388+
void (*callback)(struct mid_q_entry *mid);
2389+
2390+
spin_lock(&mid->mid_lock);
2391+
callback = mid->callback;
2392+
mid->callback = NULL; /* Mark as executed, */
2393+
spin_unlock(&mid->mid_lock);
2394+
2395+
if (callback)
2396+
callback(mid);
2397+
}
2398+
23782399
#define CIFS_REPARSE_SUPPORT(tcon) \
23792400
((tcon)->posix_extensions || \
23802401
(le32_to_cpu((tcon)->fsAttrInfo.Attributes) & \

fs/smb/client/cifstransport.c

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ alloc_mid(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
4646
temp = mempool_alloc(cifs_mid_poolp, GFP_NOFS);
4747
memset(temp, 0, sizeof(struct mid_q_entry));
4848
kref_init(&temp->refcount);
49+
spin_lock_init(&temp->mid_lock);
4950
temp->mid = get_mid(smb_buffer);
5051
temp->pid = current->pid;
5152
temp->command = cpu_to_le16(smb_buffer->Command);
@@ -345,16 +346,15 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses,
345346
rc = wait_for_response(server, midQ);
346347
if (rc != 0) {
347348
send_cancel(server, &rqst, midQ);
348-
spin_lock(&server->mid_queue_lock);
349-
if (midQ->mid_state == MID_REQUEST_SUBMITTED ||
350-
midQ->mid_state == MID_RESPONSE_RECEIVED) {
349+
spin_lock(&midQ->mid_lock);
350+
if (midQ->callback) {
351351
/* no longer considered to be "in-flight" */
352352
midQ->callback = release_mid;
353-
spin_unlock(&server->mid_queue_lock);
353+
spin_unlock(&midQ->mid_lock);
354354
add_credits(server, &credits, 0);
355355
return rc;
356356
}
357-
spin_unlock(&server->mid_queue_lock);
357+
spin_unlock(&midQ->mid_lock);
358358
}
359359

360360
rc = cifs_sync_mid_result(midQ, server);
@@ -527,15 +527,14 @@ SendReceiveBlockingLock(const unsigned int xid, struct cifs_tcon *tcon,
527527
rc = wait_for_response(server, midQ);
528528
if (rc) {
529529
send_cancel(server, &rqst, midQ);
530-
spin_lock(&server->mid_queue_lock);
531-
if (midQ->mid_state == MID_REQUEST_SUBMITTED ||
532-
midQ->mid_state == MID_RESPONSE_RECEIVED) {
530+
spin_lock(&midQ->mid_lock);
531+
if (midQ->callback) {
533532
/* no longer considered to be "in-flight" */
534533
midQ->callback = release_mid;
535-
spin_unlock(&server->mid_queue_lock);
534+
spin_unlock(&midQ->mid_lock);
536535
return rc;
537536
}
538-
spin_unlock(&server->mid_queue_lock);
537+
spin_unlock(&midQ->mid_lock);
539538
}
540539

541540
/* We got the response - restart system call. */

fs/smb/client/connect.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -335,7 +335,7 @@ cifs_abort_connection(struct TCP_Server_Info *server)
335335
cifs_dbg(FYI, "%s: issuing mid callbacks\n", __func__);
336336
list_for_each_entry_safe(mid, nmid, &retry_list, qhead) {
337337
list_del_init(&mid->qhead);
338-
mid->callback(mid);
338+
mid_execute_callback(mid);
339339
release_mid(mid);
340340
}
341341

@@ -919,7 +919,7 @@ is_smb_response(struct TCP_Server_Info *server, unsigned char type)
919919
list_del_init(&mid->qhead);
920920
mid->mid_rc = mid_rc;
921921
mid->mid_state = MID_RC;
922-
mid->callback(mid);
922+
mid_execute_callback(mid);
923923
release_mid(mid);
924924
}
925925

@@ -1117,7 +1117,7 @@ clean_demultiplex_info(struct TCP_Server_Info *server)
11171117
mid_entry = list_entry(tmp, struct mid_q_entry, qhead);
11181118
cifs_dbg(FYI, "Callback mid %llu\n", mid_entry->mid);
11191119
list_del_init(&mid_entry->qhead);
1120-
mid_entry->callback(mid_entry);
1120+
mid_execute_callback(mid_entry);
11211121
release_mid(mid_entry);
11221122
}
11231123
/* 1/8th of sec is more than enough time for them to exit */
@@ -1394,7 +1394,7 @@ cifs_demultiplex_thread(void *p)
13941394
}
13951395

13961396
if (!mids[i]->multiRsp || mids[i]->multiEnd)
1397-
mids[i]->callback(mids[i]);
1397+
mid_execute_callback(mids[i]);
13981398

13991399
release_mid(mids[i]);
14001400
} else if (server->ops->is_oplock_break &&

fs/smb/client/smb2ops.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4814,15 +4814,15 @@ static void smb2_decrypt_offload(struct work_struct *work)
48144814
dw->server->ops->is_network_name_deleted(dw->buf,
48154815
dw->server);
48164816

4817-
mid->callback(mid);
4817+
mid_execute_callback(mid);
48184818
} else {
48194819
spin_lock(&dw->server->srv_lock);
48204820
if (dw->server->tcpStatus == CifsNeedReconnect) {
48214821
spin_lock(&dw->server->mid_queue_lock);
48224822
mid->mid_state = MID_RETRY_NEEDED;
48234823
spin_unlock(&dw->server->mid_queue_lock);
48244824
spin_unlock(&dw->server->srv_lock);
4825-
mid->callback(mid);
4825+
mid_execute_callback(mid);
48264826
} else {
48274827
spin_lock(&dw->server->mid_queue_lock);
48284828
mid->mid_state = MID_REQUEST_SUBMITTED;

fs/smb/client/smb2transport.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -771,6 +771,7 @@ smb2_mid_entry_alloc(const struct smb2_hdr *shdr,
771771
temp = mempool_alloc(cifs_mid_poolp, GFP_NOFS);
772772
memset(temp, 0, sizeof(struct mid_q_entry));
773773
kref_init(&temp->refcount);
774+
spin_lock_init(&temp->mid_lock);
774775
temp->mid = le64_to_cpu(shdr->MessageId);
775776
temp->credits = credits > 0 ? credits : 1;
776777
temp->pid = current->pid;

fs/smb/client/transport.c

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1005,15 +1005,14 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
10051005
cifs_server_dbg(FYI, "Cancelling wait for mid %llu cmd: %d\n",
10061006
midQ[i]->mid, le16_to_cpu(midQ[i]->command));
10071007
send_cancel(server, &rqst[i], midQ[i]);
1008-
spin_lock(&server->mid_queue_lock);
1008+
spin_lock(&midQ[i]->mid_lock);
10091009
midQ[i]->wait_cancelled = true;
1010-
if (midQ[i]->mid_state == MID_REQUEST_SUBMITTED ||
1011-
midQ[i]->mid_state == MID_RESPONSE_RECEIVED) {
1010+
if (midQ[i]->callback) {
10121011
midQ[i]->callback = cifs_cancelled_callback;
10131012
cancelled_mid[i] = true;
10141013
credits[i].value = 0;
10151014
}
1016-
spin_unlock(&server->mid_queue_lock);
1015+
spin_unlock(&midQ[i]->mid_lock);
10171016
}
10181017
}
10191018

0 commit comments

Comments
 (0)