smb: client: fix TCP timers deadlock after rmmod

Commit ef7134c7fc48 ("smb: client: Fix use-after-free of network namespace.")
fixed a netns UAF by manually enabled socket refcounting
(sk->sk_net_refcnt=1 and sock_inuse_add(net, 1)).

The reason the patch worked for that bug was because we now hold
references to the netns (get_net_track() gets a ref internally)
and they're properly released (internally, on __sk_destruct()),
but only because sk->sk_net_refcnt was set.

Problem:
(this happens regardless of CONFIG_NET_NS_REFCNT_TRACKER and regardless
if init_net or other)

Setting sk->sk_net_refcnt=1 *manually* and *after* socket creation is not
only out of cifs scope, but also technically wrong -- it's set conditionally
based on user (=1) vs kernel (=0) sockets.  And net/ implementations
seem to base their user vs kernel space operations on it.

e.g. upon TCP socket close, the TCP timers are not cleared because
sk->sk_net_refcnt=1:
(cf. commit 151c9c724d05 ("tcp: properly terminate timers for kernel sockets"))

net/ipv4/tcp.c:
    void tcp_close(struct sock *sk, long timeout)
    {
    	lock_sock(sk);
    	__tcp_close(sk, timeout);
    	release_sock(sk);
    	if (!sk->sk_net_refcnt)
    		inet_csk_clear_xmit_timers_sync(sk);
    	sock_put(sk);
    }

Which will throw a lockdep warning and then, as expected, deadlock on
tcp_write_timer().

A way to reproduce this is by running the reproducer from ef7134c7fc48
and then 'rmmod cifs'.  A few seconds later, the deadlock/lockdep
warning shows up.

Fix:
We shouldn't mess with socket internals ourselves, so do not set
sk_net_refcnt manually.

Also change __sock_create() to sock_create_kern() for explicitness.

As for non-init_net network namespaces, we deal with it the best way
we can -- hold an extra netns reference for server->ssocket and drop it
when it's released.  This ensures that the netns still exists whenever
we need to create/destroy server->ssocket, but is not directly tied to
it.

Fixes: ef7134c7fc48 ("smb: client: Fix use-after-free of network namespace.")
Cc: stable@vger.kernel.org
Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
Signed-off-by: Steve French <stfrench@microsoft.com>

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 2372538a12118980a3bffb6f5ba531b8e71c3aea..ddcc9e514a0ed55de8d61fad662f6509ba273b8a 100644 (file)
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -987,9 +987,13 @@ 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)) {
@@ -1037,6 +1041,7 @@ 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);
@@ -1713,6 +1718,8 @@ 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->conn_id = atomic_inc_return(&tcpSesNextId);
@@ -1844,6 +1851,7 @@ smbd_connected:
 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:
@@ -1852,8 +1860,10 @@ out_err:
                        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);
@@ -3131,20 +3141,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(net, sfamily, SOCK_STREAM,
-                                  IPPROTO_TCP, &server->ssocket, 1);
+               rc = sock_create_kern(net, sfamily, SOCK_STREAM, IPPROTO_TCP, &server->ssocket);
                if (rc < 0) {
                        cifs_server_dbg(VFS, "Error %d creating socket\n", rc);
                        return rc;
                }
 
-               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);
+               /*
+                * Grab netns reference for the socket.
+                *
+                * It'll be released here, on error, or in clean_demultiplex_info() upon server
+                * teardown.
+                */
+               get_net(net);
 
                /* BB other socket options to set KEEPALIVE, NODELAY? */
                cifs_dbg(FYI, "Socket created\n");
@@ -3158,8 +3168,10 @@ generic_ip_connect(struct TCP_Server_Info *server)
        }
 
        rc = bind_socket(server);
-       if (rc < 0)
+       if (rc < 0) {
+               put_net(cifs_net_ns(server));
                return rc;
+       }
 
        /*
         * Eventually check for other socket options to change from
@@ -3196,6 +3208,7 @@ 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;
@@ -3204,6 +3217,9 @@ 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;
 }