linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] smb:client: fix possible use after free problems
@ 2025-08-04 12:10 Stefan Metzmacher
  2025-08-04 12:10 ` [PATCH 1/5] smb: client: let send_done() cleanup before calling smbd_disconnect_rdma_connection() Stefan Metzmacher
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Stefan Metzmacher @ 2025-08-04 12:10 UTC (permalink / raw)
  To: linux-cifs, samba-technical; +Cc: metze, Steve French, Tom Talpey, Long Li

While refactoring the client and server smbdirect code I
noticed a few problems where we might hit use after free
style problems.

In order to allow backports I decided to fix the problems
before trying to move things to common code.

The server has similar problems, I'll send a separate
patchset for the server as they are independed.

Stefan Metzmacher (5):
  smb: client: let send_done() cleanup before calling
    smbd_disconnect_rdma_connection()
  smb: client: remove separate empty_packet_queue
  smb: client: make sure we call ib_dma_unmap_single() only if we called
    ib_dma_map_single already
  smb: client: let recv_done() cleanup before notifying the callers.
  smb: client: let recv_done() avoid touching data_transfer after
    cleanup/move

 fs/smb/client/cifs_debug.c |   6 +-
 fs/smb/client/smbdirect.c  | 124 ++++++++++++-------------------------
 fs/smb/client/smbdirect.h  |   4 --
 3 files changed, 42 insertions(+), 92 deletions(-)

-- 
2.43.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/5] smb: client: let send_done() cleanup before calling smbd_disconnect_rdma_connection()
  2025-08-04 12:10 [PATCH 0/5] smb:client: fix possible use after free problems Stefan Metzmacher
@ 2025-08-04 12:10 ` Stefan Metzmacher
  2025-08-06 11:39   ` Stefan Metzmacher
  2025-08-04 12:10 ` [PATCH 2/5] smb: client: remove separate empty_packet_queue Stefan Metzmacher
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 8+ messages in thread
From: Stefan Metzmacher @ 2025-08-04 12:10 UTC (permalink / raw)
  To: linux-cifs, samba-technical; +Cc: metze, Steve French, Tom Talpey, Long Li

We should call ib_dma_unmap_single() and mempool_free() before calling
smbd_disconnect_rdma_connection().

And smbd_disconnect_rdma_connection() needs to be the last function to
call as all other state might already be gone after it returns.

Cc: Steve French <smfrench@gmail.com>
Cc: Tom Talpey <tom@talpey.com>
Cc: Long Li <longli@microsoft.com>
Cc: linux-cifs@vger.kernel.org
Cc: samba-technical@lists.samba.org
Fixes: f198186aa9bb ("CIFS: SMBD: Establish SMB Direct connection")
Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 fs/smb/client/smbdirect.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
index 754e94a0e07f..b6c369088479 100644
--- a/fs/smb/client/smbdirect.c
+++ b/fs/smb/client/smbdirect.c
@@ -281,18 +281,20 @@ static void send_done(struct ib_cq *cq, struct ib_wc *wc)
 	log_rdma_send(INFO, "smbd_request 0x%p completed wc->status=%d\n",
 		request, wc->status);
 
-	if (wc->status != IB_WC_SUCCESS || wc->opcode != IB_WC_SEND) {
-		log_rdma_send(ERR, "wc->status=%d wc->opcode=%d\n",
-			wc->status, wc->opcode);
-		smbd_disconnect_rdma_connection(request->info);
-	}
-
 	for (i = 0; i < request->num_sge; i++)
 		ib_dma_unmap_single(sc->ib.dev,
 			request->sge[i].addr,
 			request->sge[i].length,
 			DMA_TO_DEVICE);
 
+	if (wc->status != IB_WC_SUCCESS || wc->opcode != IB_WC_SEND) {
+		log_rdma_send(ERR, "wc->status=%d wc->opcode=%d\n",
+			wc->status, wc->opcode);
+		mempool_free(request, request->info->request_mempool);
+		smbd_disconnect_rdma_connection(request->info);
+		return;
+	}
+
 	if (atomic_dec_and_test(&request->info->send_pending))
 		wake_up(&request->info->wait_send_pending);
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/5] smb: client: remove separate empty_packet_queue
  2025-08-04 12:10 [PATCH 0/5] smb:client: fix possible use after free problems Stefan Metzmacher
  2025-08-04 12:10 ` [PATCH 1/5] smb: client: let send_done() cleanup before calling smbd_disconnect_rdma_connection() Stefan Metzmacher
@ 2025-08-04 12:10 ` Stefan Metzmacher
  2025-08-04 12:10 ` [PATCH 3/5] smb: client: make sure we call ib_dma_unmap_single() only if we called ib_dma_map_single already Stefan Metzmacher
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Stefan Metzmacher @ 2025-08-04 12:10 UTC (permalink / raw)
  To: linux-cifs, samba-technical; +Cc: metze, Steve French, Tom Talpey, Long Li

There's no need to maintain two lists, we can just
have a single list of receive buffers, which are free to use.

It just added unneeded complexity and resulted in
ib_dma_unmap_single() not being called from recv_done()
for empty keepalive packets.

Cc: Steve French <smfrench@gmail.com>
Cc: Tom Talpey <tom@talpey.com>
Cc: Long Li <longli@microsoft.com>
Cc: linux-cifs@vger.kernel.org
Cc: samba-technical@lists.samba.org
Fixes: f198186aa9bb ("CIFS: SMBD: Establish SMB Direct connection")
Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 fs/smb/client/cifs_debug.c |  6 ++--
 fs/smb/client/smbdirect.c  | 62 +++-----------------------------------
 fs/smb/client/smbdirect.h  |  4 ---
 3 files changed, 7 insertions(+), 65 deletions(-)

diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
index f1cea365b6f1..ca7c2265c25f 100644
--- a/fs/smb/client/cifs_debug.c
+++ b/fs/smb/client/cifs_debug.c
@@ -481,10 +481,8 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
 			server->smbd_conn->receive_credit_target);
 		seq_printf(m, "\nPending send_pending: %x ",
 			atomic_read(&server->smbd_conn->send_pending));
-		seq_printf(m, "\nReceive buffers count_receive_queue: %x "
-			"count_empty_packet_queue: %x",
-			server->smbd_conn->count_receive_queue,
-			server->smbd_conn->count_empty_packet_queue);
+		seq_printf(m, "\nReceive buffers count_receive_queue: %x ",
+			server->smbd_conn->count_receive_queue);
 		seq_printf(m, "\nMR responder_resources: %x "
 			"max_frmr_depth: %x mr_type: %x",
 			server->smbd_conn->responder_resources,
diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
index b6c369088479..b18e2bc6c8ed 100644
--- a/fs/smb/client/smbdirect.c
+++ b/fs/smb/client/smbdirect.c
@@ -13,8 +13,6 @@
 #include "cifsproto.h"
 #include "smb2proto.h"
 
-static struct smbd_response *get_empty_queue_buffer(
-		struct smbd_connection *info);
 static struct smbd_response *get_receive_buffer(
 		struct smbd_connection *info);
 static void put_receive_buffer(
@@ -23,8 +21,6 @@ static void put_receive_buffer(
 static int allocate_receive_buffers(struct smbd_connection *info, int num_buf);
 static void destroy_receive_buffers(struct smbd_connection *info);
 
-static void put_empty_packet(
-		struct smbd_connection *info, struct smbd_response *response);
 static void enqueue_reassembly(
 		struct smbd_connection *info,
 		struct smbd_response *response, int data_length);
@@ -393,7 +389,6 @@ static bool process_negotiation_response(
 static void smbd_post_send_credits(struct work_struct *work)
 {
 	int ret = 0;
-	int use_receive_queue = 1;
 	int rc;
 	struct smbd_response *response;
 	struct smbd_connection *info =
@@ -409,18 +404,9 @@ static void smbd_post_send_credits(struct work_struct *work)
 	if (info->receive_credit_target >
 		atomic_read(&info->receive_credits)) {
 		while (true) {
-			if (use_receive_queue)
-				response = get_receive_buffer(info);
-			else
-				response = get_empty_queue_buffer(info);
-			if (!response) {
-				/* now switch to empty packet queue */
-				if (use_receive_queue) {
-					use_receive_queue = 0;
-					continue;
-				} else
-					break;
-			}
+			response = get_receive_buffer(info);
+			if (!response)
+				break;
 
 			response->type = SMBD_TRANSFER_DATA;
 			response->first_segment = false;
@@ -511,7 +497,7 @@ static void recv_done(struct ib_cq *cq, struct ib_wc *wc)
 				response,
 				data_length);
 		} else
-			put_empty_packet(info, response);
+			put_receive_buffer(info, response);
 
 		if (data_length)
 			wake_up_interruptible(&info->wait_reassembly_queue);
@@ -1115,17 +1101,6 @@ static int smbd_negotiate(struct smbd_connection *info)
 	return rc;
 }
 
-static void put_empty_packet(
-		struct smbd_connection *info, struct smbd_response *response)
-{
-	spin_lock(&info->empty_packet_queue_lock);
-	list_add_tail(&response->list, &info->empty_packet_queue);
-	info->count_empty_packet_queue++;
-	spin_unlock(&info->empty_packet_queue_lock);
-
-	queue_work(info->workqueue, &info->post_send_credits_work);
-}
-
 /*
  * Implement Connection.FragmentReassemblyBuffer defined in [MS-SMBD] 3.1.1.1
  * This is a queue for reassembling upper layer payload and present to upper
@@ -1174,25 +1149,6 @@ static struct smbd_response *_get_first_reassembly(struct smbd_connection *info)
 	return ret;
 }
 
-static struct smbd_response *get_empty_queue_buffer(
-		struct smbd_connection *info)
-{
-	struct smbd_response *ret = NULL;
-	unsigned long flags;
-
-	spin_lock_irqsave(&info->empty_packet_queue_lock, flags);
-	if (!list_empty(&info->empty_packet_queue)) {
-		ret = list_first_entry(
-			&info->empty_packet_queue,
-			struct smbd_response, list);
-		list_del(&ret->list);
-		info->count_empty_packet_queue--;
-	}
-	spin_unlock_irqrestore(&info->empty_packet_queue_lock, flags);
-
-	return ret;
-}
-
 /*
  * Get a receive buffer
  * For each remote send, we need to post a receive. The receive buffers are
@@ -1257,10 +1213,6 @@ static int allocate_receive_buffers(struct smbd_connection *info, int num_buf)
 	spin_lock_init(&info->receive_queue_lock);
 	info->count_receive_queue = 0;
 
-	INIT_LIST_HEAD(&info->empty_packet_queue);
-	spin_lock_init(&info->empty_packet_queue_lock);
-	info->count_empty_packet_queue = 0;
-
 	init_waitqueue_head(&info->wait_receive_queues);
 
 	for (i = 0; i < num_buf; i++) {
@@ -1294,9 +1246,6 @@ static void destroy_receive_buffers(struct smbd_connection *info)
 
 	while ((response = get_receive_buffer(info)))
 		mempool_free(response, info->response_mempool);
-
-	while ((response = get_empty_queue_buffer(info)))
-		mempool_free(response, info->response_mempool);
 }
 
 /* Implement idle connection timer [MS-SMBD] 3.1.6.2 */
@@ -1383,8 +1332,7 @@ void smbd_destroy(struct TCP_Server_Info *server)
 
 	log_rdma_event(INFO, "free receive buffers\n");
 	wait_event(info->wait_receive_queues,
-		info->count_receive_queue + info->count_empty_packet_queue
-			== sp->recv_credit_max);
+		info->count_receive_queue == sp->recv_credit_max);
 	destroy_receive_buffers(info);
 
 	/*
diff --git a/fs/smb/client/smbdirect.h b/fs/smb/client/smbdirect.h
index 75b3f491c3ad..ea04ce8a9763 100644
--- a/fs/smb/client/smbdirect.h
+++ b/fs/smb/client/smbdirect.h
@@ -110,10 +110,6 @@ struct smbd_connection {
 	int count_receive_queue;
 	spinlock_t receive_queue_lock;
 
-	struct list_head empty_packet_queue;
-	int count_empty_packet_queue;
-	spinlock_t empty_packet_queue_lock;
-
 	wait_queue_head_t wait_receive_queues;
 
 	/* Reassembly queue */
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/5] smb: client: make sure we call ib_dma_unmap_single() only if we called ib_dma_map_single already
  2025-08-04 12:10 [PATCH 0/5] smb:client: fix possible use after free problems Stefan Metzmacher
  2025-08-04 12:10 ` [PATCH 1/5] smb: client: let send_done() cleanup before calling smbd_disconnect_rdma_connection() Stefan Metzmacher
  2025-08-04 12:10 ` [PATCH 2/5] smb: client: remove separate empty_packet_queue Stefan Metzmacher
@ 2025-08-04 12:10 ` Stefan Metzmacher
  2025-08-04 12:10 ` [PATCH 4/5] smb: client: let recv_done() cleanup before notifying the callers Stefan Metzmacher
  2025-08-04 12:10 ` [PATCH 5/5] smb: client: let recv_done() avoid touching data_transfer after cleanup/move Stefan Metzmacher
  4 siblings, 0 replies; 8+ messages in thread
From: Stefan Metzmacher @ 2025-08-04 12:10 UTC (permalink / raw)
  To: linux-cifs, samba-technical; +Cc: metze, Steve French, Tom Talpey, Long Li

In case of failures either ib_dma_map_single() might not be called yet
or ib_dma_unmap_single() was already called.

We should make sure put_receive_buffer() only calls
ib_dma_unmap_single() if needed.

Cc: Steve French <smfrench@gmail.com>
Cc: Tom Talpey <tom@talpey.com>
Cc: Long Li <longli@microsoft.com>
Cc: linux-cifs@vger.kernel.org
Cc: samba-technical@lists.samba.org
Fixes: f198186aa9bb ("CIFS: SMBD: Establish SMB Direct connection")
Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 fs/smb/client/smbdirect.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
index b18e2bc6c8ed..a32ebb4d48a2 100644
--- a/fs/smb/client/smbdirect.c
+++ b/fs/smb/client/smbdirect.c
@@ -1057,6 +1057,7 @@ static int smbd_post_recv(
 	if (rc) {
 		ib_dma_unmap_single(sc->ib.dev, response->sge.addr,
 				    response->sge.length, DMA_FROM_DEVICE);
+		response->sge.length = 0;
 		smbd_disconnect_rdma_connection(info);
 		log_rdma_recv(ERR, "ib_post_recv failed rc=%d\n", rc);
 	}
@@ -1186,8 +1187,13 @@ static void put_receive_buffer(
 	struct smbdirect_socket *sc = &info->socket;
 	unsigned long flags;
 
-	ib_dma_unmap_single(sc->ib.dev, response->sge.addr,
-		response->sge.length, DMA_FROM_DEVICE);
+	if (likely(response->sge.length != 0)) {
+		ib_dma_unmap_single(sc->ib.dev,
+				    response->sge.addr,
+				    response->sge.length,
+				    DMA_FROM_DEVICE);
+		response->sge.length = 0;
+	}
 
 	spin_lock_irqsave(&info->receive_queue_lock, flags);
 	list_add_tail(&response->list, &info->receive_queue);
@@ -1221,6 +1227,7 @@ static int allocate_receive_buffers(struct smbd_connection *info, int num_buf)
 			goto allocate_failed;
 
 		response->info = info;
+		response->sge.length = 0;
 		list_add_tail(&response->list, &info->receive_queue);
 		info->count_receive_queue++;
 	}
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 4/5] smb: client: let recv_done() cleanup before notifying the callers.
  2025-08-04 12:10 [PATCH 0/5] smb:client: fix possible use after free problems Stefan Metzmacher
                   ` (2 preceding siblings ...)
  2025-08-04 12:10 ` [PATCH 3/5] smb: client: make sure we call ib_dma_unmap_single() only if we called ib_dma_map_single already Stefan Metzmacher
@ 2025-08-04 12:10 ` Stefan Metzmacher
  2025-08-04 12:10 ` [PATCH 5/5] smb: client: let recv_done() avoid touching data_transfer after cleanup/move Stefan Metzmacher
  4 siblings, 0 replies; 8+ messages in thread
From: Stefan Metzmacher @ 2025-08-04 12:10 UTC (permalink / raw)
  To: linux-cifs, samba-technical; +Cc: metze, Steve French, Tom Talpey, Long Li

We should call put_receive_buffer() before waking up the callers.

For the internal error case of response->type being unexpected,
we now also call smbd_disconnect_rdma_connection() instead
of not waking up the callers at all.

Note that the SMBD_TRANSFER_DATA case still has problems,
which will be addressed in the next commit in order to make
it easier to review this one.

Cc: Steve French <smfrench@gmail.com>
Cc: Tom Talpey <tom@talpey.com>
Cc: Long Li <longli@microsoft.com>
Cc: linux-cifs@vger.kernel.org
Cc: samba-technical@lists.samba.org
Fixes: f198186aa9bb ("CIFS: SMBD: Establish SMB Direct connection")
Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 fs/smb/client/smbdirect.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
index a32ebb4d48a2..21a12e08082f 100644
--- a/fs/smb/client/smbdirect.c
+++ b/fs/smb/client/smbdirect.c
@@ -454,7 +454,6 @@ static void recv_done(struct ib_cq *cq, struct ib_wc *wc)
 	if (wc->status != IB_WC_SUCCESS || wc->opcode != IB_WC_RECV) {
 		log_rdma_recv(INFO, "wc->status=%d opcode=%d\n",
 			wc->status, wc->opcode);
-		smbd_disconnect_rdma_connection(info);
 		goto error;
 	}
 
@@ -471,8 +470,9 @@ static void recv_done(struct ib_cq *cq, struct ib_wc *wc)
 		info->full_packet_received = true;
 		info->negotiate_done =
 			process_negotiation_response(response, wc->byte_len);
+		put_receive_buffer(info, response);
 		complete(&info->negotiate_completion);
-		break;
+		return;
 
 	/* SMBD data transfer packet */
 	case SMBD_TRANSFER_DATA:
@@ -529,14 +529,16 @@ static void recv_done(struct ib_cq *cq, struct ib_wc *wc)
 		}
 
 		return;
-
-	default:
-		log_rdma_recv(ERR,
-			"unexpected response type=%d\n", response->type);
 	}
 
+	/*
+	 * This is an internal error!
+	 */
+	log_rdma_recv(ERR, "unexpected response type=%d\n", response->type);
+	WARN_ON_ONCE(response->type != SMBD_TRANSFER_DATA);
 error:
 	put_receive_buffer(info, response);
+	smbd_disconnect_rdma_connection(info);
 }
 
 static struct rdma_cm_id *smbd_create_id(
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 5/5] smb: client: let recv_done() avoid touching data_transfer after cleanup/move
  2025-08-04 12:10 [PATCH 0/5] smb:client: fix possible use after free problems Stefan Metzmacher
                   ` (3 preceding siblings ...)
  2025-08-04 12:10 ` [PATCH 4/5] smb: client: let recv_done() cleanup before notifying the callers Stefan Metzmacher
@ 2025-08-04 12:10 ` Stefan Metzmacher
  4 siblings, 0 replies; 8+ messages in thread
From: Stefan Metzmacher @ 2025-08-04 12:10 UTC (permalink / raw)
  To: linux-cifs, samba-technical; +Cc: metze, Steve French, Tom Talpey, Long Li

Calling enqueue_reassembly() and wake_up_interruptible(&info->wait_reassembly_queue)
or put_receive_buffer() means the reponse/data_transfer pointer might
get re-used by another thread, which means these should be
the last operations before calling return.

Cc: Steve French <smfrench@gmail.com>
Cc: Tom Talpey <tom@talpey.com>
Cc: Long Li <longli@microsoft.com>
Cc: linux-cifs@vger.kernel.org
Cc: samba-technical@lists.samba.org
Fixes: f198186aa9bb ("CIFS: SMBD: Establish SMB Direct connection")
Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
 fs/smb/client/smbdirect.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
index 21a12e08082f..58321e483a1a 100644
--- a/fs/smb/client/smbdirect.c
+++ b/fs/smb/client/smbdirect.c
@@ -479,10 +479,6 @@ static void recv_done(struct ib_cq *cq, struct ib_wc *wc)
 		data_transfer = smbd_response_payload(response);
 		data_length = le32_to_cpu(data_transfer->data_length);
 
-		/*
-		 * If this is a packet with data playload place the data in
-		 * reassembly queue and wake up the reading thread
-		 */
 		if (data_length) {
 			if (info->full_packet_received)
 				response->first_segment = true;
@@ -491,16 +487,7 @@ static void recv_done(struct ib_cq *cq, struct ib_wc *wc)
 				info->full_packet_received = false;
 			else
 				info->full_packet_received = true;
-
-			enqueue_reassembly(
-				info,
-				response,
-				data_length);
-		} else
-			put_receive_buffer(info, response);
-
-		if (data_length)
-			wake_up_interruptible(&info->wait_reassembly_queue);
+		}
 
 		atomic_dec(&info->receive_credits);
 		info->receive_credit_target =
@@ -528,6 +515,16 @@ static void recv_done(struct ib_cq *cq, struct ib_wc *wc)
 			info->keep_alive_requested = KEEP_ALIVE_PENDING;
 		}
 
+		/*
+		 * If this is a packet with data playload place the data in
+		 * reassembly queue and wake up the reading thread
+		 */
+		if (data_length) {
+			enqueue_reassembly(info, response, data_length);
+			wake_up_interruptible(&info->wait_reassembly_queue);
+		} else
+			put_receive_buffer(info, response);
+
 		return;
 	}
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/5] smb: client: let send_done() cleanup before calling smbd_disconnect_rdma_connection()
  2025-08-04 12:10 ` [PATCH 1/5] smb: client: let send_done() cleanup before calling smbd_disconnect_rdma_connection() Stefan Metzmacher
@ 2025-08-06 11:39   ` Stefan Metzmacher
  2025-08-06 17:36     ` Steve French
  0 siblings, 1 reply; 8+ messages in thread
From: Stefan Metzmacher @ 2025-08-06 11:39 UTC (permalink / raw)
  To: Steve French
  Cc: Steve French, Tom Talpey, Long Li, linux-cifs@vger.kernel.org,
	Samba Technical

Hi Steve,

can you please squash this into the commit? Otherwise it introduces
as new use-after-free problem with request->info.

diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
index 7377216e033d..5d1fa83583f6 100644
--- a/fs/smb/client/smbdirect.c
+++ b/fs/smb/client/smbdirect.c
@@ -286,8 +286,8 @@ static void send_done(struct ib_cq *cq, struct ib_wc *wc)
  	if (wc->status != IB_WC_SUCCESS || wc->opcode != IB_WC_SEND) {
  		log_rdma_send(ERR, "wc->status=%d wc->opcode=%d\n",
  			wc->status, wc->opcode);
-		mempool_free(request, request->info->request_mempool);
-		smbd_disconnect_rdma_connection(request->info);
+		mempool_free(request, info->request_mempool);
+		smbd_disconnect_rdma_connection(info);
  		return;
  	}

Thanks!
metze

Am 04.08.25 um 14:10 schrieb Stefan Metzmacher:
> We should call ib_dma_unmap_single() and mempool_free() before calling
> smbd_disconnect_rdma_connection().
> 
> And smbd_disconnect_rdma_connection() needs to be the last function to
> call as all other state might already be gone after it returns.
> 
> Cc: Steve French <smfrench@gmail.com>
> Cc: Tom Talpey <tom@talpey.com>
> Cc: Long Li <longli@microsoft.com>
> Cc: linux-cifs@vger.kernel.org
> Cc: samba-technical@lists.samba.org
> Fixes: f198186aa9bb ("CIFS: SMBD: Establish SMB Direct connection")
> Signed-off-by: Stefan Metzmacher <metze@samba.org>
> ---
>   fs/smb/client/smbdirect.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
> index 754e94a0e07f..b6c369088479 100644
> --- a/fs/smb/client/smbdirect.c
> +++ b/fs/smb/client/smbdirect.c
> @@ -281,18 +281,20 @@ static void send_done(struct ib_cq *cq, struct ib_wc *wc)
>   	log_rdma_send(INFO, "smbd_request 0x%p completed wc->status=%d\n",
>   		request, wc->status);
>   
> -	if (wc->status != IB_WC_SUCCESS || wc->opcode != IB_WC_SEND) {
> -		log_rdma_send(ERR, "wc->status=%d wc->opcode=%d\n",
> -			wc->status, wc->opcode);
> -		smbd_disconnect_rdma_connection(request->info);
> -	}
> -
>   	for (i = 0; i < request->num_sge; i++)
>   		ib_dma_unmap_single(sc->ib.dev,
>   			request->sge[i].addr,
>   			request->sge[i].length,
>   			DMA_TO_DEVICE);
>   
> +	if (wc->status != IB_WC_SUCCESS || wc->opcode != IB_WC_SEND) {
> +		log_rdma_send(ERR, "wc->status=%d wc->opcode=%d\n",
> +			wc->status, wc->opcode);
> +		mempool_free(request, request->info->request_mempool);
> +		smbd_disconnect_rdma_connection(request->info);
> +		return;
> +	}
> +
>   	if (atomic_dec_and_test(&request->info->send_pending))
>   		wake_up(&request->info->wait_send_pending);
>   


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/5] smb: client: let send_done() cleanup before calling smbd_disconnect_rdma_connection()
  2025-08-06 11:39   ` Stefan Metzmacher
@ 2025-08-06 17:36     ` Steve French
  0 siblings, 0 replies; 8+ messages in thread
From: Steve French @ 2025-08-06 17:36 UTC (permalink / raw)
  To: Stefan Metzmacher
  Cc: Steve French, Tom Talpey, Long Li, linux-cifs@vger.kernel.org,
	Samba Technical

Done

On Wed, Aug 6, 2025 at 6:39 AM Stefan Metzmacher <metze@samba.org> wrote:
>
> Hi Steve,
>
> can you please squash this into the commit? Otherwise it introduces
> as new use-after-free problem with request->info.
>
> diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
> index 7377216e033d..5d1fa83583f6 100644
> --- a/fs/smb/client/smbdirect.c
> +++ b/fs/smb/client/smbdirect.c
> @@ -286,8 +286,8 @@ static void send_done(struct ib_cq *cq, struct ib_wc *wc)
>         if (wc->status != IB_WC_SUCCESS || wc->opcode != IB_WC_SEND) {
>                 log_rdma_send(ERR, "wc->status=%d wc->opcode=%d\n",
>                         wc->status, wc->opcode);
> -               mempool_free(request, request->info->request_mempool);
> -               smbd_disconnect_rdma_connection(request->info);
> +               mempool_free(request, info->request_mempool);
> +               smbd_disconnect_rdma_connection(info);
>                 return;
>         }
>
> Thanks!
> metze
>
> Am 04.08.25 um 14:10 schrieb Stefan Metzmacher:
> > We should call ib_dma_unmap_single() and mempool_free() before calling
> > smbd_disconnect_rdma_connection().
> >
> > And smbd_disconnect_rdma_connection() needs to be the last function to
> > call as all other state might already be gone after it returns.
> >
> > Cc: Steve French <smfrench@gmail.com>
> > Cc: Tom Talpey <tom@talpey.com>
> > Cc: Long Li <longli@microsoft.com>
> > Cc: linux-cifs@vger.kernel.org
> > Cc: samba-technical@lists.samba.org
> > Fixes: f198186aa9bb ("CIFS: SMBD: Establish SMB Direct connection")
> > Signed-off-by: Stefan Metzmacher <metze@samba.org>
> > ---
> >   fs/smb/client/smbdirect.c | 14 ++++++++------
> >   1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
> > index 754e94a0e07f..b6c369088479 100644
> > --- a/fs/smb/client/smbdirect.c
> > +++ b/fs/smb/client/smbdirect.c
> > @@ -281,18 +281,20 @@ static void send_done(struct ib_cq *cq, struct ib_wc *wc)
> >       log_rdma_send(INFO, "smbd_request 0x%p completed wc->status=%d\n",
> >               request, wc->status);
> >
> > -     if (wc->status != IB_WC_SUCCESS || wc->opcode != IB_WC_SEND) {
> > -             log_rdma_send(ERR, "wc->status=%d wc->opcode=%d\n",
> > -                     wc->status, wc->opcode);
> > -             smbd_disconnect_rdma_connection(request->info);
> > -     }
> > -
> >       for (i = 0; i < request->num_sge; i++)
> >               ib_dma_unmap_single(sc->ib.dev,
> >                       request->sge[i].addr,
> >                       request->sge[i].length,
> >                       DMA_TO_DEVICE);
> >
> > +     if (wc->status != IB_WC_SUCCESS || wc->opcode != IB_WC_SEND) {
> > +             log_rdma_send(ERR, "wc->status=%d wc->opcode=%d\n",
> > +                     wc->status, wc->opcode);
> > +             mempool_free(request, request->info->request_mempool);
> > +             smbd_disconnect_rdma_connection(request->info);
> > +             return;
> > +     }
> > +
> >       if (atomic_dec_and_test(&request->info->send_pending))
> >               wake_up(&request->info->wait_send_pending);
> >
>


-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2025-08-06 17:36 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-04 12:10 [PATCH 0/5] smb:client: fix possible use after free problems Stefan Metzmacher
2025-08-04 12:10 ` [PATCH 1/5] smb: client: let send_done() cleanup before calling smbd_disconnect_rdma_connection() Stefan Metzmacher
2025-08-06 11:39   ` Stefan Metzmacher
2025-08-06 17:36     ` Steve French
2025-08-04 12:10 ` [PATCH 2/5] smb: client: remove separate empty_packet_queue Stefan Metzmacher
2025-08-04 12:10 ` [PATCH 3/5] smb: client: make sure we call ib_dma_unmap_single() only if we called ib_dma_map_single already Stefan Metzmacher
2025-08-04 12:10 ` [PATCH 4/5] smb: client: let recv_done() cleanup before notifying the callers Stefan Metzmacher
2025-08-04 12:10 ` [PATCH 5/5] smb: client: let recv_done() avoid touching data_transfer after cleanup/move Stefan Metzmacher

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).