Skip to content

Commit

Permalink
Revert "smb: client: fix TCP timers deadlock after rmmod"
Browse files Browse the repository at this point in the history
commit 95d2b9f upstream.

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>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  • Loading branch information
Kuniyuki Iwashima authored and Greg Kroah-Hartman committed Apr 25, 2025
1 parent 2aa10d2 commit f761eee
Showing 1 changed file with 10 additions and 26 deletions.
36 changes: 10 additions & 26 deletions fs/smb/client/connect.c
Original file line number Diff line number Diff line change
Expand Up @@ -987,13 +987,9 @@ clean_demultiplex_info(struct TCP_Server_Info *server)
msleep(125);
if (cifs_rdma_enabled(server))
smbd_destroy(server);

if (server->ssocket) {
sock_release(server->ssocket);
server->ssocket = NULL;

/* Release netns reference for the socket. */
put_net(cifs_net_ns(server));
}

if (!list_empty(&server->pending_mid_q)) {
Expand Down Expand Up @@ -1041,7 +1037,6 @@ clean_demultiplex_info(struct TCP_Server_Info *server)
*/
}

/* Release netns reference for this server. */
put_net(cifs_net_ns(server));
kfree(server->leaf_fullpath);
kfree(server->hostname);
Expand Down Expand Up @@ -1717,8 +1712,6 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx,

tcp_ses->ops = ctx->ops;
tcp_ses->vals = ctx->vals;

/* Grab netns reference for this server. */
cifs_set_net_ns(tcp_ses, get_net(current->nsproxy->net_ns));

tcp_ses->sign = ctx->sign;
Expand Down Expand Up @@ -1851,7 +1844,6 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx,
out_err_crypto_release:
cifs_crypto_secmech_release(tcp_ses);

/* Release netns reference for this server. */
put_net(cifs_net_ns(tcp_ses));

out_err:
Expand All @@ -1860,10 +1852,8 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx,
cifs_put_tcp_session(tcp_ses->primary_server, false);
kfree(tcp_ses->hostname);
kfree(tcp_ses->leaf_fullpath);
if (tcp_ses->ssocket) {
if (tcp_ses->ssocket)
sock_release(tcp_ses->ssocket);
put_net(cifs_net_ns(tcp_ses));
}
kfree(tcp_ses);
}
return ERR_PTR(rc);
Expand Down Expand Up @@ -3131,20 +3121,20 @@ generic_ip_connect(struct TCP_Server_Info *server)
socket = server->ssocket;
} else {
struct net *net = cifs_net_ns(server);
struct sock *sk;

rc = sock_create_kern(net, sfamily, SOCK_STREAM, IPPROTO_TCP, &server->ssocket);
rc = __sock_create(net, sfamily, SOCK_STREAM,
IPPROTO_TCP, &server->ssocket, 1);
if (rc < 0) {
cifs_server_dbg(VFS, "Error %d creating socket\n", rc);
return rc;
}

/*
* Grab netns reference for the socket.
*
* It'll be released here, on error, or in clean_demultiplex_info() upon server
* teardown.
*/
get_net(net);
sk = server->ssocket->sk;
__netns_tracker_free(net, &sk->ns_tracker, false);
sk->sk_net_refcnt = 1;
get_net_track(net, &sk->ns_tracker, GFP_KERNEL);
sock_inuse_add(net, 1);

/* BB other socket options to set KEEPALIVE, NODELAY? */
cifs_dbg(FYI, "Socket created\n");
Expand All @@ -3158,10 +3148,8 @@ generic_ip_connect(struct TCP_Server_Info *server)
}

rc = bind_socket(server);
if (rc < 0) {
put_net(cifs_net_ns(server));
if (rc < 0)
return rc;
}

/*
* Eventually check for other socket options to change from
Expand Down Expand Up @@ -3198,7 +3186,6 @@ generic_ip_connect(struct TCP_Server_Info *server)
if (rc < 0) {
cifs_dbg(FYI, "Error %d connecting to server\n", rc);
trace_smb3_connect_err(server->hostname, server->conn_id, &server->dstaddr, rc);
put_net(cifs_net_ns(server));
sock_release(socket);
server->ssocket = NULL;
return rc;
Expand All @@ -3207,9 +3194,6 @@ generic_ip_connect(struct TCP_Server_Info *server)
if (sport == htons(RFC1001_PORT))
rc = ip_rfc1001_connect(server);

if (rc < 0)
put_net(cifs_net_ns(server));

return rc;
}

Expand Down

0 comments on commit f761eee

Please sign in to comment.