[PATCH 0/5] smb: client: don't use server->smbd_conn without a reference or lock

[PATCH 0/5] smb: client: don't use server->smbd_conn without a reference or lock

[Stefan Metzmacher]

Hi Paulo/Namjae,

The SMB Direct client keeps the transport in server->smbd_conn and
accesses it from several places without any locking:

  - smbd_get_parameters()
  - smbd_register_mr()
  - smbd_debug_proc_show()

At the same time cifs_abort_connection() (and the reconnect path) can
call smbd_destroy() at any time, which frees the smbd_connection and
releases the underlying smbdirect socket. So the readers above can race
with the teardown and end up using a freed connection/socket, a
use-after-free.

While looking at this, smbd_get_parameters() also turned out to
dereference server->smbd_conn without checking it, which is a NULL
pointer dereference in smb2_negotiate_wsize()/smb2_negotiate_rsize()
when it is called during a reconnect window.

This series closes the races:

  - server->smbd_conn is only ever changed under server->srv_lock, so
    readers can safely look at it while holding that lock.

  - smbdirect gets smbdirect_socket_get()/smbdirect_socket_put(), which
    take and drop a reference that keeps the socket memory alive (but
    does not keep it connected). smbd_register_mr() uses them to hold
    the socket across smbdirect_connection_register_mr_io(); once the
    connection is gone the socket is no longer connected and the call
    fails gracefully. A registered MR has its own reference counting.

  - smbd_get_parameters() no longer returns a pointer into the socket
    (which cannot be kept valid for the callers). It copies the current
    parameters into the new server->smbd_params under server->srv_lock
    and returns a pointer to that; with no connection it returns zeros.

  - smbd_debug_proc_show() runs with cifs_tcp_ses_lock held, so it takes
    server->srv_lock instead of a socket reference.

The two "pass server to ..." patches are mechanical prototype changes
with no behaviour change, split out so the final fix stays small and
each step builds on its own.

All of this is a use-after-free that goes back to the switch to the
smbdirect socket API, so the whole series is tagged for stable via

  Fixes: b8aef8c8808c ("smb: client: make use of smbdirect_socket_create_kern()/smbdirect_socket_release()")

which is in mainline since v7.1-rc1.

I run various xfstests with this.

I'm unsure if this should go via cifs-next or ksmbd-for-next,
for consistency with the series I just send I guess both
could go via ksmbd-for-next?

Stefan Metzmacher (5):
  smb: client: change server->smbd_conn only under server->srv_lock
  smb: smbdirect: add smbdirect_socket_get() and smbdirect_socket_put()
  smb: client: pass server to smbd_get_parameters()
  smb: client: pass server to smbd_register_mr()
  smb: client: don't use server->smbd_conn without a reference or lock

 fs/smb/client/cifsglob.h  |  12 ++++
 fs/smb/client/connect.c   |  12 +++-
 fs/smb/client/file.c      |   4 +-
 fs/smb/client/smb2ops.c   |   4 +-
 fs/smb/client/smb2pdu.c   |   4 +-
 fs/smb/client/smbdirect.c | 124 +++++++++++++++++++++++++++++++++-----
 fs/smb/client/smbdirect.h |   8 ++-
 fs/smb/smbdirect/socket.c |  27 +++++++++
 include/linux/smbdirect.h |   3 +
 9 files changed, 171 insertions(+), 27 deletions(-)

-- 
2.43.0

[PATCH 1/5] smb: client: change server->smbd_conn only under server->srv_lock

[Stefan Metzmacher]

smbd_get_parameters(), smbd_register_mr() and smbd_debug_proc_show()
use server->smbd_conn without holding cifs_server_lock(), while
cifs_abort_connection() can destroy it via smbd_destroy() at any time,
which frees the smbd_connection and releases the smbdirect socket.
smbd_debug_proc_show() is called with cifs_tcp_ses_lock held, so
cifs_server_lock() can't be used there.

Change server->smbd_conn only under server->srv_lock, via the new
smbd_set_connection() and in smbd_destroy(), which now clears the
pointer under the lock before releasing the socket. This allows
readers to use server->smbd_conn under server->srv_lock.

cifs_get_tcp_session() calls smbd_destroy() on failure, also for
early failures before server->srv_lock is initialized, so only call
it if server->smbd_conn is set.

Fixes: b8aef8c8808c ("smb: client: make use of smbdirect_socket_create_kern()/smbdirect_socket_release()")
Cc: Paulo Alcantara <pc@manguebit.org>
Cc: Namjae Jeon <linkinjeon@kernel.org>
Cc: Tom Talpey <tom@talpey.com>
Cc: linux-cifs@vger.kernel.org
Cc: samba-technical@lists.samba.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 fs/smb/client/connect.c   | 12 +++++++++---
 fs/smb/client/smbdirect.c | 29 +++++++++++++++++++++++++----
 fs/smb/client/smbdirect.h |  4 ++++
 3 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 28e1ddeb6182..a45411037253 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -1858,8 +1858,9 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx,
 		rc = -ENOENT;
 		goto out_err_crypto_release;
 #endif
-		tcp_ses->smbd_conn = smbd_get_connection(
-			tcp_ses, (struct sockaddr *)&ctx->dstaddr);
+		smbd_set_connection(tcp_ses,
+				    smbd_get_connection(tcp_ses,
+							(struct sockaddr *)&ctx->dstaddr));
 		if (tcp_ses->smbd_conn) {
 			cifs_dbg(VFS, "RDMA transport established\n");
 			rc = 0;
@@ -1934,7 +1935,12 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx,
 		kfree(tcp_ses->leaf_fullpath);
 		if (tcp_ses->ssocket)
 			sock_release(tcp_ses->ssocket);
-		smbd_destroy(tcp_ses);
+		/*
+		 * srv_lock is not initialized yet for
+		 * the early gotos, but smbd_conn is NULL then.
+		 */
+		if (tcp_ses->smbd_conn)
+			smbd_destroy(tcp_ses);
 		kfree(tcp_ses);
 	}
 	return ERR_PTR(rc);
diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
index 563ef488a225..3ecf34c7379a 100644
--- a/fs/smb/client/smbdirect.c
+++ b/fs/smb/client/smbdirect.c
@@ -201,7 +201,17 @@ static int smbd_post_send_full_iter(struct smbdirect_socket *sc,
  */
 void smbd_destroy(struct TCP_Server_Info *server)
 {
-	struct smbd_connection *info = server->smbd_conn;
+	struct smbd_connection *info;
+
+	/*
+	 * server->smbd_conn is changed under server->srv_lock,
+	 * so that readers which don't hold cifs_server_lock()
+	 * can use it under server->srv_lock.
+	 */
+	spin_lock(&server->srv_lock);
+	info = server->smbd_conn;
+	server->smbd_conn = NULL;
+	spin_unlock(&server->srv_lock);
 
 	if (!info) {
 		log_rdma_event(INFO, "rdma session already destroyed\n");
@@ -211,7 +221,17 @@ void smbd_destroy(struct TCP_Server_Info *server)
 	smbdirect_socket_release(info->socket);
 
 	kfree(info);
-	server->smbd_conn = NULL;
+}
+
+/*
+ * Publish a new connection, see smbd_destroy()
+ */
+void smbd_set_connection(struct TCP_Server_Info *server,
+			 struct smbd_connection *info)
+{
+	spin_lock(&server->srv_lock);
+	server->smbd_conn = info;
+	spin_unlock(&server->srv_lock);
 }
 
 /*
@@ -236,8 +256,9 @@ int smbd_reconnect(struct TCP_Server_Info *server)
 
 create_conn:
 	log_rdma_event(INFO, "creating rdma session\n");
-	server->smbd_conn = smbd_get_connection(
-		server, (struct sockaddr *) &server->dstaddr);
+	smbd_set_connection(server,
+			    smbd_get_connection(server,
+						(struct sockaddr *) &server->dstaddr));
 
 	if (server->smbd_conn) {
 		cifs_dbg(VFS, "RDMA transport re-established\n");
diff --git a/fs/smb/client/smbdirect.h b/fs/smb/client/smbdirect.h
index be205ec02077..54ee49f9cfc6 100644
--- a/fs/smb/client/smbdirect.h
+++ b/fs/smb/client/smbdirect.h
@@ -30,6 +30,8 @@ struct smbd_connection {
 /* Create a SMBDirect session */
 struct smbd_connection *smbd_get_connection(
 	struct TCP_Server_Info *server, struct sockaddr *dstaddr);
+void smbd_set_connection(struct TCP_Server_Info *server,
+			 struct smbd_connection *info);
 
 const struct smbdirect_socket_parameters *smbd_get_parameters(struct smbd_connection *conn);
 
@@ -58,6 +60,8 @@ void smbd_debug_proc_show(struct TCP_Server_Info *server, struct seq_file *m);
 struct smbd_connection {};
 static inline void *smbd_get_connection(
 	struct TCP_Server_Info *server, struct sockaddr *dstaddr) {return NULL;}
+static inline void smbd_set_connection(struct TCP_Server_Info *server,
+				       struct smbd_connection *info) {}
 static inline int smbd_reconnect(struct TCP_Server_Info *server) {return -1; }
 static inline void smbd_destroy(struct TCP_Server_Info *server) {}
 static inline int smbd_recv(struct smbd_connection *info, struct msghdr *msg) {return -1; }
-- 
2.43.0

[PATCH 2/5] smb: smbdirect: add smbdirect_socket_get() and smbdirect_socket_put()

[Stefan Metzmacher]

These take and drop a reference on sc->refs.destroy, which only keeps
the memory of the socket alive. smbdirect_socket_release() still
disconnects and destroys the socket, after that it is no longer
connected and all functions fail gracefully.

This will be used by the smb client in order to use the socket,
e.g. for smbdirect_connection_register_mr_io(), while
cifs_abort_connection() may release it at the same time.

Fixes: b8aef8c8808c ("smb: client: make use of smbdirect_socket_create_kern()/smbdirect_socket_release()")
Cc: Paulo Alcantara <pc@manguebit.org>
Cc: Namjae Jeon <linkinjeon@kernel.org>
Cc: Tom Talpey <tom@talpey.com>
Cc: linux-cifs@vger.kernel.org
Cc: samba-technical@lists.samba.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 fs/smb/smbdirect/socket.c | 27 +++++++++++++++++++++++++++
 include/linux/smbdirect.h |  3 +++
 2 files changed, 30 insertions(+)

diff --git a/fs/smb/smbdirect/socket.c b/fs/smb/smbdirect/socket.c
index e270e36b3145..13cf50b96bc9 100644
--- a/fs/smb/smbdirect/socket.c
+++ b/fs/smb/smbdirect/socket.c
@@ -851,6 +851,33 @@ void smbdirect_socket_release(struct smbdirect_socket *sc)
 }
 EXPORT_SYMBOL_GPL(smbdirect_socket_release);
 
+/*
+ * smbdirect_socket_get() and smbdirect_socket_put() only keep
+ * the memory of the socket alive, they don't prevent
+ * smbdirect_socket_release() from disconnecting and destroying
+ * it, after that the socket is no longer connected and
+ * all functions fail gracefully.
+ *
+ * This allows a caller to use the socket, e.g. for
+ * smbdirect_connection_register_mr_io(), while another
+ * thread may call smbdirect_socket_release() at the same time.
+ *
+ * smbdirect_socket_put() may free the socket,
+ * so it must be called from process context.
+ */
+void smbdirect_socket_get(struct smbdirect_socket *sc)
+{
+	kref_get(&sc->refs.destroy);
+}
+EXPORT_SYMBOL_GPL(smbdirect_socket_get);
+
+void smbdirect_socket_put(struct smbdirect_socket *sc)
+{
+	might_sleep();
+	kref_put(&sc->refs.destroy, smbdirect_socket_release_destroy);
+}
+EXPORT_SYMBOL_GPL(smbdirect_socket_put);
+
 int smbdirect_socket_wait_for_credits(struct smbdirect_socket *sc,
 				      enum smbdirect_socket_status expected_status,
 				      int unexpected_errno,
diff --git a/include/linux/smbdirect.h b/include/linux/smbdirect.h
index 97f5ba730fa7..7f0b1ed6cf16 100644
--- a/include/linux/smbdirect.h
+++ b/include/linux/smbdirect.h
@@ -111,6 +111,9 @@ void smbdirect_socket_shutdown(struct smbdirect_socket *sc);
 
 void smbdirect_socket_release(struct smbdirect_socket *sc);
 
+void smbdirect_socket_get(struct smbdirect_socket *sc);
+void smbdirect_socket_put(struct smbdirect_socket *sc);
+
 int smbdirect_connection_send_batch_flush(struct smbdirect_socket *sc,
 					  struct smbdirect_send_batch *batch,
 					  bool is_last);
-- 
2.43.0

[PATCH 3/5] smb: client: pass server to smbd_get_parameters()

[Stefan Metzmacher]

This is a mechanical change of the prototype of smbd_get_parameters()
to take a struct TCP_Server_Info *server instead of a
struct smbd_connection *, deriving server->smbd_conn inside the
function, with no change in behaviour. A following change uses this to
access server->smbd_conn under server->srv_lock.

smbd_get_connection() is called before the connection is published in
server->smbd_conn, so it uses
smbdirect_socket_get_current_parameters() directly.

Fixes: b8aef8c8808c ("smb: client: make use of smbdirect_socket_create_kern()/smbdirect_socket_release()")
Cc: Paulo Alcantara <pc@manguebit.org>
Cc: Namjae Jeon <linkinjeon@kernel.org>
Cc: Tom Talpey <tom@talpey.com>
Cc: linux-cifs@vger.kernel.org
Cc: samba-technical@lists.samba.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 fs/smb/client/file.c      | 4 ++--
 fs/smb/client/smb2ops.c   | 4 ++--
 fs/smb/client/smbdirect.c | 8 +++++---
 fs/smb/client/smbdirect.h | 2 +-
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
index 0d428517f454..632d1f24e36b 100644
--- a/fs/smb/client/file.c
+++ b/fs/smb/client/file.c
@@ -98,7 +98,7 @@ static void cifs_prepare_write(struct netfs_io_subrequest *subreq)
 #ifdef CONFIG_CIFS_SMB_DIRECT
 	if (server->smbd_conn) {
 		const struct smbdirect_socket_parameters *sp =
-			smbd_get_parameters(server->smbd_conn);
+			smbd_get_parameters(server);
 
 		stream->sreq_max_segs = sp->max_frmr_depth;
 	}
@@ -192,7 +192,7 @@ static int cifs_prepare_read(struct netfs_io_subrequest *subreq)
 #ifdef CONFIG_CIFS_SMB_DIRECT
 	if (server->smbd_conn) {
 		const struct smbdirect_socket_parameters *sp =
-			smbd_get_parameters(server->smbd_conn);
+			smbd_get_parameters(server);
 
 		rreq->io_streams[0].sreq_max_segs = sp->max_frmr_depth;
 	}
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 3464470d3297..1b2ef128810f 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -516,7 +516,7 @@ smb3_negotiate_wsize(struct cifs_tcon *tcon, struct smb3_fs_context *ctx)
 #ifdef CONFIG_CIFS_SMB_DIRECT
 	if (server->rdma) {
 		const struct smbdirect_socket_parameters *sp =
-			smbd_get_parameters(server->smbd_conn);
+			smbd_get_parameters(server);
 
 		if (server->sign)
 			/*
@@ -567,7 +567,7 @@ smb3_negotiate_rsize(struct cifs_tcon *tcon, struct smb3_fs_context *ctx)
 #ifdef CONFIG_CIFS_SMB_DIRECT
 	if (server->rdma) {
 		const struct smbdirect_socket_parameters *sp =
-			smbd_get_parameters(server->smbd_conn);
+			smbd_get_parameters(server);
 
 		if (server->sign)
 			/*
diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
index 3ecf34c7379a..41856261c372 100644
--- a/fs/smb/client/smbdirect.c
+++ b/fs/smb/client/smbdirect.c
@@ -361,8 +361,10 @@ static struct smbd_connection *_smbd_get_connection(
 	return NULL;
 }
 
-const struct smbdirect_socket_parameters *smbd_get_parameters(struct smbd_connection *conn)
+const struct smbdirect_socket_parameters *smbd_get_parameters(struct TCP_Server_Info *server)
 {
+	struct smbd_connection *conn = server->smbd_conn;
+
 	if (unlikely(!conn->socket)) {
 		static const struct smbdirect_socket_parameters zero_params;
 
@@ -390,7 +392,7 @@ struct smbd_connection *smbd_get_connection(
 	if (!ret)
 		return NULL;
 
-	sp = smbd_get_parameters(ret);
+	sp = smbdirect_socket_get_current_parameters(ret->socket);
 
 	server->rdma_readwrite_threshold =
 		rdma_readwrite_threshold > sp->max_fragmented_send_size ?
@@ -435,7 +437,7 @@ int smbd_send(struct TCP_Server_Info *server,
 {
 	struct smbd_connection *info = server->smbd_conn;
 	struct smbdirect_socket *sc = info->socket;
-	const struct smbdirect_socket_parameters *sp = smbd_get_parameters(info);
+	const struct smbdirect_socket_parameters *sp = smbd_get_parameters(server);
 	struct smb_rqst *rqst;
 	struct iov_iter iter;
 	struct smbdirect_send_batch_storage bstorage;
diff --git a/fs/smb/client/smbdirect.h b/fs/smb/client/smbdirect.h
index 54ee49f9cfc6..84fab8b6d1f0 100644
--- a/fs/smb/client/smbdirect.h
+++ b/fs/smb/client/smbdirect.h
@@ -33,7 +33,7 @@ struct smbd_connection *smbd_get_connection(
 void smbd_set_connection(struct TCP_Server_Info *server,
 			 struct smbd_connection *info);
 
-const struct smbdirect_socket_parameters *smbd_get_parameters(struct smbd_connection *conn);
+const struct smbdirect_socket_parameters *smbd_get_parameters(struct TCP_Server_Info *server);
 
 /* Reconnect SMBDirect session */
 int smbd_reconnect(struct TCP_Server_Info *server);
-- 
2.43.0

[PATCH 4/5] smb: client: pass server to smbd_register_mr()

[Stefan Metzmacher]

This is a mechanical change of the prototype of smbd_register_mr()
to take a struct TCP_Server_Info *server instead of a
struct smbd_connection *, deriving server->smbd_conn inside the
function, with no change in behaviour. A following change uses this to
take a reference on server->smbd_conn's socket under server->srv_lock.

Fixes: b8aef8c8808c ("smb: client: make use of smbdirect_socket_create_kern()/smbdirect_socket_release()")
Cc: Paulo Alcantara <pc@manguebit.org>
Cc: Namjae Jeon <linkinjeon@kernel.org>
Cc: Tom Talpey <tom@talpey.com>
Cc: linux-cifs@vger.kernel.org
Cc: samba-technical@lists.samba.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 fs/smb/client/smb2pdu.c   | 4 ++--
 fs/smb/client/smbdirect.c | 3 ++-
 fs/smb/client/smbdirect.h | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index 880ce12f50c4..aa5a33ca5d08 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -4598,7 +4598,7 @@ smb2_new_read_req(void **buf, unsigned int *total_len,
 		struct smbdirect_buffer_descriptor_v1 *v1;
 		bool need_invalidate = server->dialect == SMB30_PROT_ID;
 
-		rdata->mr = smbd_register_mr(server->smbd_conn, &rdata->subreq.io_iter,
+		rdata->mr = smbd_register_mr(server, &rdata->subreq.io_iter,
 					     true, need_invalidate);
 		if (!rdata->mr) {
 			rc = -EAGAIN;
@@ -5200,7 +5200,7 @@ smb2_async_writev(struct cifs_io_subrequest *wdata)
 		struct smbdirect_buffer_descriptor_v1 *v1;
 		bool need_invalidate = server->dialect == SMB30_PROT_ID;
 
-		wdata->mr = smbd_register_mr(server->smbd_conn, &wdata->subreq.io_iter,
+		wdata->mr = smbd_register_mr(server, &wdata->subreq.io_iter,
 					     false, need_invalidate);
 		if (!wdata->mr) {
 			rc = -EAGAIN;
diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
index 41856261c372..942763789335 100644
--- a/fs/smb/client/smbdirect.c
+++ b/fs/smb/client/smbdirect.c
@@ -537,10 +537,11 @@ int smbd_send(struct TCP_Server_Info *server,
  * need_invalidate: true if this MR needs to be locally invalidated after I/O
  * return value: the MR registered, NULL if failed.
  */
-struct smbdirect_mr_io *smbd_register_mr(struct smbd_connection *info,
+struct smbdirect_mr_io *smbd_register_mr(struct TCP_Server_Info *server,
 				 struct iov_iter *iter,
 				 bool writing, bool need_invalidate)
 {
+	struct smbd_connection *info = server->smbd_conn;
 	struct smbdirect_socket *sc = info->socket;
 
 	if (!smbdirect_connection_is_connected(sc))
diff --git a/fs/smb/client/smbdirect.h b/fs/smb/client/smbdirect.h
index 84fab8b6d1f0..1e1cf9a0104c 100644
--- a/fs/smb/client/smbdirect.h
+++ b/fs/smb/client/smbdirect.h
@@ -47,7 +47,7 @@ int smbd_send(struct TCP_Server_Info *server,
 
 /* Interfaces to register and deregister MR for RDMA read/write */
 struct smbdirect_mr_io *smbd_register_mr(
-	struct smbd_connection *info, struct iov_iter *iter,
+	struct TCP_Server_Info *server, struct iov_iter *iter,
 	bool writing, bool need_invalidate);
 void smbd_mr_fill_buffer_descriptor(struct smbdirect_mr_io *mr,
 				    struct smbdirect_buffer_descriptor_v1 *v1);
-- 
2.43.0

[PATCH 5/5] smb: client: don't use server->smbd_conn without a reference or lock

[Stefan Metzmacher]

smbd_get_parameters(), smbd_register_mr() and smbd_debug_proc_show()
used server->smbd_conn without any locking, while
cifs_abort_connection() can destroy it via smbd_destroy() at any
time, which frees the smbd_connection and releases the smbdirect
socket.

smbd_register_mr() now uses the new smbd_get_socket() and
smbd_put_socket(), which take and drop a reference on the socket of
server->smbd_conn under server->srv_lock via smbdirect_socket_get()
and smbdirect_socket_put(). Once the connection is destroyed, the
socket is no longer connected, so
smbdirect_connection_register_mr_io() fails gracefully. A registered
mr has its own reference counting.

smbd_debug_proc_show() is called with cifs_tcp_ses_lock held, so it
holds server->srv_lock instead, which prevents server->smbd_conn from
being destroyed.

smbd_get_parameters() returned a pointer into the socket, which can't
be kept valid for the callers. Now it copies the parameters into the
new server->smbd_params under server->srv_lock and returns a pointer
to that. If there's no connection (anymore), server->smbd_params is
set to zeros. This also fixes a possible NULL pointer dereference in
smb2_negotiate_wsize() and smb2_negotiate_rsize(), which called
smbd_get_parameters(server->smbd_conn) without checking
server->smbd_conn, e.g. during a reconnect.

smbd_send() is called under cifs_server_lock(), so it uses
smbdirect_socket_get_current_parameters() directly.

Fixes: b8aef8c8808c ("smb: client: make use of smbdirect_socket_create_kern()/smbdirect_socket_release()")
Cc: Paulo Alcantara <pc@manguebit.org>
Cc: Namjae Jeon <linkinjeon@kernel.org>
Cc: Tom Talpey <tom@talpey.com>
Cc: linux-cifs@vger.kernel.org
Cc: samba-technical@lists.samba.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 fs/smb/client/cifsglob.h  | 12 +++++
 fs/smb/client/smbdirect.c | 94 +++++++++++++++++++++++++++++++++------
 2 files changed, 93 insertions(+), 13 deletions(-)

diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 79e4e84f8985..c3efbc3d1f2a 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -30,6 +30,9 @@
 #include "smb2pdu.h"
 #include "smb1pdu.h"
 #include <linux/filelock.h>
+#ifdef CONFIG_CIFS_SMB_DIRECT
+#include <linux/smbdirect.h>
+#endif
 
 #define SMB_PATH_MAX 260
 #define CIFS_PORT 445
@@ -761,6 +764,15 @@ struct TCP_Server_Info {
 	bool	rdma;
 	/* point to the SMBD connection if RDMA is used instead of socket */
 	struct smbd_connection *smbd_conn;
+#ifdef CONFIG_CIFS_SMB_DIRECT
+	/*
+	 * A copy of the current smbdirect socket parameters,
+	 * updated by smbd_get_parameters(). smbd_conn and
+	 * its socket can be destroyed at any time, so callers
+	 * can't get a pointer into it.
+	 */
+	struct smbdirect_socket_parameters smbd_params;
+#endif
 	struct delayed_work	echo; /* echo ping workqueue job */
 	char	*smallbuf;	/* pointer to current "small" buffer */
 	char	*bigbuf;	/* pointer to current "big" buffer */
diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
index 942763789335..e59bf9a48ea0 100644
--- a/fs/smb/client/smbdirect.c
+++ b/fs/smb/client/smbdirect.c
@@ -361,17 +361,61 @@ static struct smbd_connection *_smbd_get_connection(
 	return NULL;
 }
 
-const struct smbdirect_socket_parameters *smbd_get_parameters(struct TCP_Server_Info *server)
+/*
+ * server->smbd_conn can be destroyed by cifs_abort_connection()
+ * at any time, so we take a reference on its socket while we use
+ * it. server->smbd_conn is changed under server->srv_lock,
+ * see smbd_set_connection() and smbd_destroy().
+ *
+ * Once the connection is destroyed, the socket is no longer
+ * connected, so the smbdirect functions fail gracefully.
+ *
+ * smbd_put_socket() may free the socket, so this
+ * can only be used in process context.
+ */
+static struct smbdirect_socket *smbd_get_socket(struct TCP_Server_Info *server)
 {
-	struct smbd_connection *conn = server->smbd_conn;
+	struct smbdirect_socket *sc = NULL;
 
-	if (unlikely(!conn->socket)) {
-		static const struct smbdirect_socket_parameters zero_params;
-
-		return &zero_params;
+	spin_lock(&server->srv_lock);
+	if (server->smbd_conn && server->smbd_conn->socket) {
+		sc = server->smbd_conn->socket;
+		smbdirect_socket_get(sc);
 	}
+	spin_unlock(&server->srv_lock);
 
-	return smbdirect_socket_get_current_parameters(conn->socket);
+	return sc;
+}
+
+static void smbd_put_socket(struct smbdirect_socket *sc)
+{
+	if (sc)
+		smbdirect_socket_put(sc);
+}
+
+/*
+ * We can't return a pointer into the socket, as it may
+ * be destroyed at any time, so we copy the parameters
+ * into server->smbd_params and return a pointer to that.
+ *
+ * The copy is done under server->srv_lock, which also
+ * prevents server->smbd_conn from being destroyed,
+ * see smbd_destroy().
+ *
+ * If there's no connection (anymore), we fill server->smbd_params
+ * with zeros.
+ */
+const struct smbdirect_socket_parameters *smbd_get_parameters(struct TCP_Server_Info *server)
+{
+	spin_lock(&server->srv_lock);
+	if (server->smbd_conn && server->smbd_conn->socket)
+		server->smbd_params =
+			*smbdirect_socket_get_current_parameters(server->smbd_conn->socket);
+	else
+		memset(&server->smbd_params, 0, sizeof(server->smbd_params));
+	spin_unlock(&server->srv_lock);
+
+	return &server->smbd_params;
 }
 
 struct smbd_connection *smbd_get_connection(
@@ -392,6 +436,9 @@ struct smbd_connection *smbd_get_connection(
 	if (!ret)
 		return NULL;
 
+	/*
+	 * ret is not published in server->smbd_conn yet
+	 */
 	sp = smbdirect_socket_get_current_parameters(ret->socket);
 
 	server->rdma_readwrite_threshold =
@@ -437,7 +484,13 @@ int smbd_send(struct TCP_Server_Info *server,
 {
 	struct smbd_connection *info = server->smbd_conn;
 	struct smbdirect_socket *sc = info->socket;
-	const struct smbdirect_socket_parameters *sp = smbd_get_parameters(server);
+	/*
+	 * We're called under cifs_server_lock(), so
+	 * server->smbd_conn can't be destroyed while
+	 * we use it.
+	 */
+	const struct smbdirect_socket_parameters *sp =
+		smbdirect_socket_get_current_parameters(sc);
 	struct smb_rqst *rqst;
 	struct iov_iter iter;
 	struct smbdirect_send_batch_storage bstorage;
@@ -541,13 +594,19 @@ struct smbdirect_mr_io *smbd_register_mr(struct TCP_Server_Info *server,
 				 struct iov_iter *iter,
 				 bool writing, bool need_invalidate)
 {
-	struct smbd_connection *info = server->smbd_conn;
-	struct smbdirect_socket *sc = info->socket;
+	struct smbdirect_socket *sc = smbd_get_socket(server);
+	struct smbdirect_mr_io *mr = NULL;
 
-	if (!smbdirect_connection_is_connected(sc))
-		return NULL;
+	/*
+	 * The mr has its own reference counting
+	 * and doesn't need the socket reference
+	 * once registered.
+	 */
+	if (sc && smbdirect_connection_is_connected(sc))
+		mr = smbdirect_connection_register_mr_io(sc, iter, writing, need_invalidate);
+	smbd_put_socket(sc);
 
-	return smbdirect_connection_register_mr_io(sc, iter, writing, need_invalidate);
+	return mr;
 }
 
 void smbd_mr_fill_buffer_descriptor(struct smbdirect_mr_io *mr,
@@ -572,7 +631,15 @@ void smbd_debug_proc_show(struct TCP_Server_Info *server, struct seq_file *m)
 	if (!server->rdma)
 		return;
 
+	/*
+	 * We're called with cifs_tcp_ses_lock held, so we
+	 * can't use smbd_get_socket(), but holding
+	 * server->srv_lock prevents server->smbd_conn
+	 * from being destroyed, see smbd_destroy().
+	 */
+	spin_lock(&server->srv_lock);
 	if (!server->smbd_conn) {
+		spin_unlock(&server->srv_lock);
 		seq_puts(m, "\nSMBDirect transport not available");
 		return;
 	}
@@ -580,6 +647,7 @@ void smbd_debug_proc_show(struct TCP_Server_Info *server, struct seq_file *m)
 	smbdirect_connection_legacy_debug_proc_show(server->smbd_conn->socket,
 						    server->rdma_readwrite_threshold,
 						    m);
+	spin_unlock(&server->srv_lock);
 }
 
 MODULE_IMPORT_NS("SMBDIRECT");
-- 
2.43.0

Re: [PATCH 0/5] smb: client: don't use server->smbd_conn without a reference or lock

[Stefan Metzmacher]

Hi Paulo/Namjae,

I'll address these https://sashiko.dev/#/patchset/cover.1791225453.git.metze%40samba.org
and resubmit v2 later...

metze

> The SMB Direct client keeps the transport in server->smbd_conn and
> accesses it from several places without any locking:
> 
>    - smbd_get_parameters()
>    - smbd_register_mr()
>    - smbd_debug_proc_show()
> 
> At the same time cifs_abort_connection() (and the reconnect path) can
> call smbd_destroy() at any time, which frees the smbd_connection and
> releases the underlying smbdirect socket. So the readers above can race
> with the teardown and end up using a freed connection/socket, a
> use-after-free.
> 
> While looking at this, smbd_get_parameters() also turned out to
> dereference server->smbd_conn without checking it, which is a NULL
> pointer dereference in smb2_negotiate_wsize()/smb2_negotiate_rsize()
> when it is called during a reconnect window.
> 
> This series closes the races:
> 
>    - server->smbd_conn is only ever changed under server->srv_lock, so
>      readers can safely look at it while holding that lock.
> 
>    - smbdirect gets smbdirect_socket_get()/smbdirect_socket_put(), which
>      take and drop a reference that keeps the socket memory alive (but
>      does not keep it connected). smbd_register_mr() uses them to hold
>      the socket across smbdirect_connection_register_mr_io(); once the
>      connection is gone the socket is no longer connected and the call
>      fails gracefully. A registered MR has its own reference counting.
> 
>    - smbd_get_parameters() no longer returns a pointer into the socket
>      (which cannot be kept valid for the callers). It copies the current
>      parameters into the new server->smbd_params under server->srv_lock
>      and returns a pointer to that; with no connection it returns zeros.
> 
>    - smbd_debug_proc_show() runs with cifs_tcp_ses_lock held, so it takes
>      server->srv_lock instead of a socket reference.
> 
> The two "pass server to ..." patches are mechanical prototype changes
> with no behaviour change, split out so the final fix stays small and
> each step builds on its own.
> 
> All of this is a use-after-free that goes back to the switch to the
> smbdirect socket API, so the whole series is tagged for stable via
> 
>    Fixes: b8aef8c8808c ("smb: client: make use of smbdirect_socket_create_kern()/smbdirect_socket_release()")
> 
> which is in mainline since v7.1-rc1.
> 
> I run various xfstests with this.
> 
> I'm unsure if this should go via cifs-next or ksmbd-for-next,
> for consistency with the series I just send I guess both
> could go via ksmbd-for-next?
> 
> Stefan Metzmacher (5):
>    smb: client: change server->smbd_conn only under server->srv_lock
>    smb: smbdirect: add smbdirect_socket_get() and smbdirect_socket_put()
>    smb: client: pass server to smbd_get_parameters()
>    smb: client: pass server to smbd_register_mr()
>    smb: client: don't use server->smbd_conn without a reference or lock
> 
>   fs/smb/client/cifsglob.h  |  12 ++++
>   fs/smb/client/connect.c   |  12 +++-
>   fs/smb/client/file.c      |   4 +-
>   fs/smb/client/smb2ops.c   |   4 +-
>   fs/smb/client/smb2pdu.c   |   4 +-
>   fs/smb/client/smbdirect.c | 124 +++++++++++++++++++++++++++++++++-----
>   fs/smb/client/smbdirect.h |   8 ++-
>   fs/smb/smbdirect/socket.c |  27 +++++++++
>   include/linux/smbdirect.h |   3 +
>   9 files changed, 171 insertions(+), 27 deletions(-)
> 

Re: [PATCH 0/5] smb: client: don't use server->smbd_conn without a reference or lock

[Paulo Alcantara]

Stefan Metzmacher <metze@samba.org> writes:

> I'll address these https://sashiko.dev/#/patchset/cover.1791225453.git.metze%40samba.org
> and resubmit v2 later...

Sounds good.  Thanks for the fixes.

Looking forward to seeing the test results from Tom.