Skip to content

Commit 95d2b9f

Browse files
q2venSteve French
authored andcommitted
Revert "smb: client: fix TCP timers deadlock after rmmod"
This reverts commit e9f2517. Commit e9f2517 ("smb: client: fix TCP timers deadlock after rmmod") is intended to fix a null-ptr-deref in LOCKDEP, which is mentioned as CVE-2024-54680, but is actually did not fix anything; The issue can be reproduced on top of it. [0] Also, it reverted the change by commit ef7134c ("smb: client: Fix use-after-free of network namespace.") and introduced a real issue by reviving the kernel TCP socket. When a reconnect happens for a CIFS connection, the socket state transitions to FIN_WAIT_1. Then, inet_csk_clear_xmit_timers_sync() in tcp_close() stops all timers for the socket. If an incoming FIN packet is lost, the socket will stay at FIN_WAIT_1 forever, and such sockets could be leaked up to net.ipv4.tcp_max_orphans. Usually, FIN can be retransmitted by the peer, but if the peer aborts the connection, the issue comes into reality. I warned about this privately by pointing out the exact report [1], but the bogus fix was finally merged. So, we should not stop the timers to finally kill the connection on our side in that case, meaning we must not use a kernel socket for TCP whose sk->sk_net_refcnt is 0. The kernel socket does not have a reference to its netns to make it possible to tear down netns without cleaning up every resource in it. For example, tunnel devices use a UDP socket internally, but we can destroy netns without removing such devices and let it complete during exit. Otherwise, netns would be leaked when the last application died. However, this is problematic for TCP sockets because TCP has timers to close the connection gracefully even after the socket is close()d. The lifetime of the socket and its netns is different from the lifetime of the underlying connection. If the socket user does not maintain the netns lifetime, the timer could be fired after the socket is close()d and its netns is freed up, resulting in use-after-free. Actually, we have seen so many similar issues and converted such sockets to have a reference to netns. That's why I converted the CIFS client socket to have a reference to netns (sk->sk_net_refcnt == 1), which is somehow mentioned as out-of-scope of CIFS and technically wrong in e9f2517, but **is in-scope and right fix**. Regarding the LOCKDEP issue, we can prevent the module unload by bumping the module refcount when switching the LOCKDDEP key in sock_lock_init_class_and_name(). [2] For a while, let's revert the bogus fix. Note that now we can use sk_net_refcnt_upgrade() for the socket conversion, but I'll do so later separately to make backport easy. Link: https://lore.kernel.org/all/20250402020807.28583-1-kuniyu@amazon.com/ #[0] Link: https://lore.kernel.org/netdev/c08bd5378da647a2a4c16698125d180a@huawei.com/ #[1] Link: https://lore.kernel.org/lkml/20250402005841.19846-1-kuniyu@amazon.com/ #[2] Fixes: e9f2517 ("smb: client: fix TCP timers deadlock after rmmod") Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> Cc: stable@vger.kernel.org Signed-off-by: Steve French <stfrench@microsoft.com>
1 parent c707193 commit 95d2b9f

File tree

1 file changed

+10
-26
lines changed

1 file changed

+10
-26
lines changed

fs/smb/client/connect.c

Lines changed: 10 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1073,13 +1073,9 @@ clean_demultiplex_info(struct TCP_Server_Info *server)
10731073
msleep(125);
10741074
if (cifs_rdma_enabled(server))
10751075
smbd_destroy(server);
1076-
10771076
if (server->ssocket) {
10781077
sock_release(server->ssocket);
10791078
server->ssocket = NULL;
1080-
1081-
/* Release netns reference for the socket. */
1082-
put_net(cifs_net_ns(server));
10831079
}
10841080

10851081
if (!list_empty(&server->pending_mid_q)) {
@@ -1127,7 +1123,6 @@ clean_demultiplex_info(struct TCP_Server_Info *server)
11271123
*/
11281124
}
11291125

1130-
/* Release netns reference for this server. */
11311126
put_net(cifs_net_ns(server));
11321127
kfree(server->leaf_fullpath);
11331128
kfree(server->hostname);
@@ -1773,8 +1768,6 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx,
17731768

17741769
tcp_ses->ops = ctx->ops;
17751770
tcp_ses->vals = ctx->vals;
1776-
1777-
/* Grab netns reference for this server. */
17781771
cifs_set_net_ns(tcp_ses, get_net(current->nsproxy->net_ns));
17791772

17801773
tcp_ses->sign = ctx->sign;
@@ -1902,7 +1895,6 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx,
19021895
out_err_crypto_release:
19031896
cifs_crypto_secmech_release(tcp_ses);
19041897

1905-
/* Release netns reference for this server. */
19061898
put_net(cifs_net_ns(tcp_ses));
19071899

19081900
out_err:
@@ -1911,10 +1903,8 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx,
19111903
cifs_put_tcp_session(tcp_ses->primary_server, false);
19121904
kfree(tcp_ses->hostname);
19131905
kfree(tcp_ses->leaf_fullpath);
1914-
if (tcp_ses->ssocket) {
1906+
if (tcp_ses->ssocket)
19151907
sock_release(tcp_ses->ssocket);
1916-
put_net(cifs_net_ns(tcp_ses));
1917-
}
19181908
kfree(tcp_ses);
19191909
}
19201910
return ERR_PTR(rc);
@@ -3358,20 +3348,20 @@ generic_ip_connect(struct TCP_Server_Info *server)
33583348
socket = server->ssocket;
33593349
} else {
33603350
struct net *net = cifs_net_ns(server);
3351+
struct sock *sk;
33613352

3362-
rc = sock_create_kern(net, sfamily, SOCK_STREAM, IPPROTO_TCP, &server->ssocket);
3353+
rc = __sock_create(net, sfamily, SOCK_STREAM,
3354+
IPPROTO_TCP, &server->ssocket, 1);
33633355
if (rc < 0) {
33643356
cifs_server_dbg(VFS, "Error %d creating socket\n", rc);
33653357
return rc;
33663358
}
33673359

3368-
/*
3369-
* Grab netns reference for the socket.
3370-
*
3371-
* It'll be released here, on error, or in clean_demultiplex_info() upon server
3372-
* teardown.
3373-
*/
3374-
get_net(net);
3360+
sk = server->ssocket->sk;
3361+
__netns_tracker_free(net, &sk->ns_tracker, false);
3362+
sk->sk_net_refcnt = 1;
3363+
get_net_track(net, &sk->ns_tracker, GFP_KERNEL);
3364+
sock_inuse_add(net, 1);
33753365

33763366
/* BB other socket options to set KEEPALIVE, NODELAY? */
33773367
cifs_dbg(FYI, "Socket created\n");
@@ -3385,10 +3375,8 @@ generic_ip_connect(struct TCP_Server_Info *server)
33853375
}
33863376

33873377
rc = bind_socket(server);
3388-
if (rc < 0) {
3389-
put_net(cifs_net_ns(server));
3378+
if (rc < 0)
33903379
return rc;
3391-
}
33923380

33933381
/*
33943382
* Eventually check for other socket options to change from
@@ -3425,7 +3413,6 @@ generic_ip_connect(struct TCP_Server_Info *server)
34253413
if (rc < 0) {
34263414
cifs_dbg(FYI, "Error %d connecting to server\n", rc);
34273415
trace_smb3_connect_err(server->hostname, server->conn_id, &server->dstaddr, rc);
3428-
put_net(cifs_net_ns(server));
34293416
sock_release(socket);
34303417
server->ssocket = NULL;
34313418
return rc;
@@ -3443,9 +3430,6 @@ generic_ip_connect(struct TCP_Server_Info *server)
34433430
(server->rfc1001_sessinit == -1 && sport == htons(RFC1001_PORT)))
34443431
rc = ip_rfc1001_connect(server);
34453432

3446-
if (rc < 0)
3447-
put_net(cifs_net_ns(server));
3448-
34493433
return rc;
34503434
}
34513435

0 commit comments

Comments
 (0)