[PATCH 0/2] nvmet-tcp: fix receive path error handling and state machine

[PATCH 0/2] nvmet-tcp: fix receive path error handling and state machine

[Maurizio Lombardi]

Patch 1 fixes a potential issue where network data could be read into an
uninitialized iterator. Currently, nvmet_tcp_build_pdu_iovec() returns void,
meaning callers are unaware if an out-of-bounds PDU length or offset triggers
an early return. Consequently, callers blindly overwrite the queue state to
NVMET_TCP_RECV_DATA. This patch modifies the function to return an error code,
shifting the handling responsibility to the callers to ensure proper socket
teardown.

Patch 2 cleans up redundant, localized calls to nvmet_tcp_fatal_error() scattered
across the receive path. It delegates the responsibility of executing the fatal
error function to the top-level caller by bubbling up the error codes.

Maurizio Lombardi (2):
  nvmet-tcp: propagate nvmet_tcp_build_pdu_iovec() errors to its callers
  nvmet-tcp: remove redundant calls to nvmet_tcp_fatal_error()

 drivers/nvme/target/tcp.c | 75 +++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 38 deletions(-)

-- 
2.53.0

[PATCH 1/2] nvmet-tcp: propagate nvmet_tcp_build_pdu_iovec() errors to its callers

[Maurizio Lombardi]

Currently, when nvmet_tcp_build_pdu_iovec() detects an out-of-bounds
PDU length or offset, it triggers nvmet_tcp_fatal_error(cmd->queue)
and returns early. However, because the function returns void, the
callers are entirely unaware that a fatal error has occurred and
that the cmd->recv_msg.msg_iter was left uninitialized.

Callers such as nvmet_tcp_handle_h2c_data_pdu() proceed to blindly
overwrite the queue state with queue->rcv_state = NVMET_TCP_RECV_DATA
Consequently, the socket receiving loop may attempt to read incoming
network data into the uninitialized iterator.

Fix this by shifting the error handling responsibility to the callers.

Fixes: 52a0a9854934 ("nvmet-tcp: add bounds checks in nvmet_tcp_build_pdu_iovec")
Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 drivers/nvme/target/tcp.c | 53 +++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 21 deletions(-)

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index acc71a26733f..dc1311cde04c 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -351,7 +351,7 @@ static void nvmet_tcp_free_cmd_buffers(struct nvmet_tcp_cmd *cmd)
 
 static void nvmet_tcp_fatal_error(struct nvmet_tcp_queue *queue);
 
-static void nvmet_tcp_build_pdu_iovec(struct nvmet_tcp_cmd *cmd)
+static int nvmet_tcp_build_pdu_iovec(struct nvmet_tcp_cmd *cmd)
 {
 	struct bio_vec *iov = cmd->iov;
 	struct scatterlist *sg;
@@ -364,22 +364,19 @@ static void nvmet_tcp_build_pdu_iovec(struct nvmet_tcp_cmd *cmd)
 	offset = cmd->rbytes_done;
 	cmd->sg_idx = offset / PAGE_SIZE;
 	sg_offset = offset % PAGE_SIZE;
-	if (!cmd->req.sg_cnt || cmd->sg_idx >= cmd->req.sg_cnt) {
-		nvmet_tcp_fatal_error(cmd->queue);
-		return;
-	}
+	if (!cmd->req.sg_cnt || cmd->sg_idx >= cmd->req.sg_cnt)
+		return -EPROTO;
+
 	sg = &cmd->req.sg[cmd->sg_idx];
 	sg_remaining = cmd->req.sg_cnt - cmd->sg_idx;
 
 	while (length) {
-		if (!sg_remaining) {
-			nvmet_tcp_fatal_error(cmd->queue);
-			return;
-		}
-		if (!sg->length || sg->length <= sg_offset) {
-			nvmet_tcp_fatal_error(cmd->queue);
-			return;
-		}
+		if (!sg_remaining)
+			return -EPROTO;
+
+		if (!sg->length || sg->length <= sg_offset)
+			return -EPROTO;
+
 		u32 iov_len = min_t(u32, length, sg->length - sg_offset);
 
 		bvec_set_page(iov, sg_page(sg), iov_len,
@@ -394,6 +391,7 @@ static void nvmet_tcp_build_pdu_iovec(struct nvmet_tcp_cmd *cmd)
 
 	iov_iter_bvec(&cmd->recv_msg.msg_iter, ITER_DEST, cmd->iov,
 		      nr_pages, cmd->pdu_len);
+	return 0;
 }
 
 static void nvmet_tcp_fatal_error(struct nvmet_tcp_queue *queue)
@@ -931,7 +929,7 @@ static int nvmet_tcp_handle_icreq(struct nvmet_tcp_queue *queue)
 	return 0;
 }
 
-static void nvmet_tcp_handle_req_failure(struct nvmet_tcp_queue *queue,
+static int nvmet_tcp_handle_req_failure(struct nvmet_tcp_queue *queue,
 		struct nvmet_tcp_cmd *cmd, struct nvmet_req *req)
 {
 	size_t data_len = le32_to_cpu(req->cmd->common.dptr.sgl.length);
@@ -947,19 +945,25 @@ static void nvmet_tcp_handle_req_failure(struct nvmet_tcp_queue *queue,
 	if (!nvme_is_write(cmd->req.cmd) || !data_len ||
 	    data_len > cmd->req.port->inline_data_size) {
 		nvmet_prepare_receive_pdu(queue);
-		return;
+		return 0;
 	}
 
 	ret = nvmet_tcp_map_data(cmd);
 	if (unlikely(ret)) {
 		pr_err("queue %d: failed to map data\n", queue->idx);
 		nvmet_tcp_fatal_error(queue);
-		return;
+		return -EPROTO;
 	}
 
 	queue->rcv_state = NVMET_TCP_RECV_DATA;
-	nvmet_tcp_build_pdu_iovec(cmd);
 	cmd->flags |= NVMET_TCP_F_INIT_FAILED;
+	ret = nvmet_tcp_build_pdu_iovec(cmd);
+	if (unlikely(ret)) {
+		pr_err("queue %d: failed to build PDU iovec", queue->idx);
+		return ret;
+	}
+
+	return 0;
 }
 
 static int nvmet_tcp_handle_h2c_data_pdu(struct nvmet_tcp_queue *queue)
@@ -1011,7 +1015,10 @@ static int nvmet_tcp_handle_h2c_data_pdu(struct nvmet_tcp_queue *queue)
 		goto err_proto;
 	}
 	cmd->pdu_recv = 0;
-	nvmet_tcp_build_pdu_iovec(cmd);
+	if (unlikely(nvmet_tcp_build_pdu_iovec(cmd))) {
+		pr_err("queue %d: failed to build PDU iovec\n", queue->idx);
+		goto err_proto;
+	}
 	queue->cmd = cmd;
 	queue->rcv_state = NVMET_TCP_RECV_DATA;
 
@@ -1074,8 +1081,7 @@ static int nvmet_tcp_done_recv_pdu(struct nvmet_tcp_queue *queue)
 			le32_to_cpu(req->cmd->common.dptr.sgl.length),
 			le16_to_cpu(req->cqe->status));
 
-		nvmet_tcp_handle_req_failure(queue, queue->cmd, req);
-		return 0;
+		return nvmet_tcp_handle_req_failure(queue, queue->cmd, req);
 	}
 
 	ret = nvmet_tcp_map_data(queue->cmd);
@@ -1092,7 +1098,12 @@ static int nvmet_tcp_done_recv_pdu(struct nvmet_tcp_queue *queue)
 	if (nvmet_tcp_need_data_in(queue->cmd)) {
 		if (nvmet_tcp_has_inline_data(queue->cmd)) {
 			queue->rcv_state = NVMET_TCP_RECV_DATA;
-			nvmet_tcp_build_pdu_iovec(queue->cmd);
+			ret = nvmet_tcp_build_pdu_iovec(queue->cmd);
+			if (unlikely(ret)) {
+				pr_err("queue %d: failed to build PDU iovec\n",
+					queue->idx);
+				return ret;
+			}
 			return 0;
 		}
 		/* send back R2T */
-- 
2.53.0

Re: [PATCH 1/2] nvmet-tcp: propagate nvmet_tcp_build_pdu_iovec() errors to its callers

[Keith Busch]

On Wed, Mar 11, 2026 at 05:09:19PM +0100, Maurizio Lombardi wrote:
> Currently, when nvmet_tcp_build_pdu_iovec() detects an out-of-bounds
> PDU length or offset, it triggers nvmet_tcp_fatal_error(cmd->queue)
> and returns early. However, because the function returns void, the
> callers are entirely unaware that a fatal error has occurred and
> that the cmd->recv_msg.msg_iter was left uninitialized.
> 
> Callers such as nvmet_tcp_handle_h2c_data_pdu() proceed to blindly
> overwrite the queue state with queue->rcv_state = NVMET_TCP_RECV_DATA
> Consequently, the socket receiving loop may attempt to read incoming
> network data into the uninitialized iterator.
> 
> Fix this by shifting the error handling responsibility to the callers.

This looks good to me. Just some minor comments below.

> +	ret = nvmet_tcp_build_pdu_iovec(cmd);
> +	if (unlikely(ret)) {
> +		pr_err("queue %d: failed to build PDU iovec", queue->idx);

Missing the '\n' in this print.

> @@ -1092,7 +1098,12 @@ static int nvmet_tcp_done_recv_pdu(struct nvmet_tcp_queue *queue)
>  	if (nvmet_tcp_need_data_in(queue->cmd)) {
>  		if (nvmet_tcp_has_inline_data(queue->cmd)) {
>  			queue->rcv_state = NVMET_TCP_RECV_DATA;
> -			nvmet_tcp_build_pdu_iovec(queue->cmd);
> +			ret = nvmet_tcp_build_pdu_iovec(queue->cmd);
> +			if (unlikely(ret)) {
> +				pr_err("queue %d: failed to build PDU iovec\n",
> +					queue->idx);
> +				return ret;
> +			}
>  			return 0;

Minor nit here, you can squash the two rets into a single 'return ret;'.

[PATCH 2/2] nvmet-tcp: remove redundant calls to nvmet_tcp_fatal_error()

[Maurizio Lombardi]

Executing nvmet_tcp_fatal_error() is generally the responsibility
of the caller (nvmet_tcp_try_recv); all other functions should
just return the error code.

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 drivers/nvme/target/tcp.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index dc1311cde04c..3e70fafd9359 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -885,7 +885,6 @@ static int nvmet_tcp_handle_icreq(struct nvmet_tcp_queue *queue)
 	if (le32_to_cpu(icreq->hdr.plen) != sizeof(struct nvme_tcp_icreq_pdu)) {
 		pr_err("bad nvme-tcp pdu length (%d)\n",
 			le32_to_cpu(icreq->hdr.plen));
-		nvmet_tcp_fatal_error(queue);
 		return -EPROTO;
 	}
 
@@ -951,7 +950,6 @@ static int nvmet_tcp_handle_req_failure(struct nvmet_tcp_queue *queue,
 	ret = nvmet_tcp_map_data(cmd);
 	if (unlikely(ret)) {
 		pr_err("queue %d: failed to map data\n", queue->idx);
-		nvmet_tcp_fatal_error(queue);
 		return -EPROTO;
 	}
 
@@ -1026,7 +1024,6 @@ static int nvmet_tcp_handle_h2c_data_pdu(struct nvmet_tcp_queue *queue)
 
 err_proto:
 	/* FIXME: use proper transport errors */
-	nvmet_tcp_fatal_error(queue);
 	return -EPROTO;
 }
 
@@ -1041,7 +1038,6 @@ static int nvmet_tcp_done_recv_pdu(struct nvmet_tcp_queue *queue)
 		if (hdr->type != nvme_tcp_icreq) {
 			pr_err("unexpected pdu type (%d) before icreq\n",
 				hdr->type);
-			nvmet_tcp_fatal_error(queue);
 			return -EPROTO;
 		}
 		return nvmet_tcp_handle_icreq(queue);
@@ -1050,7 +1046,6 @@ static int nvmet_tcp_done_recv_pdu(struct nvmet_tcp_queue *queue)
 	if (unlikely(hdr->type == nvme_tcp_icreq)) {
 		pr_err("queue %d: received icreq pdu in state %d\n",
 			queue->idx, queue->state);
-		nvmet_tcp_fatal_error(queue);
 		return -EPROTO;
 	}
 
@@ -1067,7 +1062,6 @@ static int nvmet_tcp_done_recv_pdu(struct nvmet_tcp_queue *queue)
 		pr_err("queue %d: out of commands (%d) send_list_len: %d, opcode: %d",
 			queue->idx, queue->nr_cmds, queue->send_list_len,
 			nvme_cmd->common.opcode);
-		nvmet_tcp_fatal_error(queue);
 		return -ENOMEM;
 	}
 
@@ -1088,9 +1082,9 @@ static int nvmet_tcp_done_recv_pdu(struct nvmet_tcp_queue *queue)
 	if (unlikely(ret)) {
 		pr_err("queue %d: failed to map data\n", queue->idx);
 		if (nvmet_tcp_has_inline_data(queue->cmd))
-			nvmet_tcp_fatal_error(queue);
-		else
-			nvmet_req_complete(req, ret);
+			return -EPROTO;
+
+		nvmet_req_complete(req, ret);
 		ret = -EAGAIN;
 		goto out;
 	}
@@ -1215,7 +1209,6 @@ static int nvmet_tcp_try_recv_pdu(struct nvmet_tcp_queue *queue)
 
 		if (unlikely(!nvmet_tcp_pdu_valid(hdr->type))) {
 			pr_err("unexpected pdu type %d\n", hdr->type);
-			nvmet_tcp_fatal_error(queue);
 			return -EIO;
 		}
 
@@ -1229,16 +1222,12 @@ static int nvmet_tcp_try_recv_pdu(struct nvmet_tcp_queue *queue)
 	}
 
 	if (queue->hdr_digest &&
-	    nvmet_tcp_verify_hdgst(queue, &queue->pdu, hdr->hlen)) {
-		nvmet_tcp_fatal_error(queue); /* fatal */
+	    nvmet_tcp_verify_hdgst(queue, &queue->pdu, hdr->hlen))
 		return -EPROTO;
-	}
 
 	if (queue->data_digest &&
-	    nvmet_tcp_check_ddgst(queue, &queue->pdu)) {
-		nvmet_tcp_fatal_error(queue); /* fatal */
+	    nvmet_tcp_check_ddgst(queue, &queue->pdu))
 		return -EPROTO;
-	}
 
 	return nvmet_tcp_done_recv_pdu(queue);
 }
@@ -1323,7 +1312,6 @@ static int nvmet_tcp_try_recv_ddgst(struct nvmet_tcp_queue *queue)
 			le32_to_cpu(cmd->exp_ddgst));
 		nvmet_req_uninit(&cmd->req);
 		nvmet_tcp_free_cmd_buffers(cmd);
-		nvmet_tcp_fatal_error(queue);
 		ret = -EPROTO;
 		goto out;
 	}
-- 
2.53.0

Re: [PATCH 2/2] nvmet-tcp: remove redundant calls to nvmet_tcp_fatal_error()

[Keith Busch]

On Wed, Mar 11, 2026 at 05:09:20PM +0100, Maurizio Lombardi wrote:
> Executing nvmet_tcp_fatal_error() is generally the responsibility
> of the caller (nvmet_tcp_try_recv); all other functions should
> just return the error code.

Looks good.

Reviewed-by: Keith Busch <kbusch@kernel.org>

Re: [PATCH 0/2] nvmet-tcp: fix receive path error handling and state machine

[yunje shin]

On Thu, Mar 12, 2026 at 1:09 AM Maurizio Lombardi <mlombard@redhat.com> wrote:
>
> Patch 1 fixes a potential issue where network data could be read into an
> uninitialized iterator. Currently, nvmet_tcp_build_pdu_iovec() returns void,
> meaning callers are unaware if an out-of-bounds PDU length or offset triggers
> an early return. Consequently, callers blindly overwrite the queue state to
> NVMET_TCP_RECV_DATA. This patch modifies the function to return an error code,
> shifting the handling responsibility to the callers to ensure proper socket
> teardown.

Thank you for the patchset — the error propagation cleanup looks
really clean and makes the receive path error handling much clearer.

I've been trying to reproduce this crash scenario on my end [1],
but I haven't been able to trigger it reliably in my test
environment so far.

If you have a crash log or call trace available, could you share it?
It would really help me verify that my test setup is exercising
the right code path.

[1] https://lore.kernel.org/linux-nvme/DGUSGT9WLRH8.GBZ0CM62IV9T@arkamax.eu/

Best regards,
Yunje

Re: [PATCH 0/2] nvmet-tcp: fix receive path error handling and state machine

[Maurizio Lombardi]

On Fri Mar 13, 2026 at 6:58 AM CET, yunje shin wrote:
> On Thu, Mar 12, 2026 at 1:09 AM Maurizio Lombardi <mlombard@redhat.com> wrote:
>>
>> Patch 1 fixes a potential issue where network data could be read into an
>> uninitialized iterator. Currently, nvmet_tcp_build_pdu_iovec() returns void,
>> meaning callers are unaware if an out-of-bounds PDU length or offset triggers
>> an early return. Consequently, callers blindly overwrite the queue state to
>> NVMET_TCP_RECV_DATA. This patch modifies the function to return an error code,
>> shifting the handling responsibility to the callers to ensure proper socket
>> teardown.
>
> Thank you for the patchset — the error propagation cleanup looks
> really clean and makes the receive path error handling much clearer.
>
> I've been trying to reproduce this crash scenario on my end [1],
> but I haven't been able to trigger it reliably in my test
> environment so far.

I guess that the msg_data_left(&cmd->recv_msg) check in
nvmet_tcp_try_recv_data() prevents the derefence of the uninitialized
iterator, so this is why it doesn't crash.

I see however that it tries to parse junk, so the patchset is useful
anyway, I will correct the commit message.


[  114.122436] nvmet_tcp: unexpected pdu type 75
[  114.122487] nvmet: ctrl 1 fatal error occurred!
[  124.660886] nvmet: Created nvm controller 1 for subsystem test-nqn for NQN nqn.2014-08.org.nvmexpress:uuid:eb574a60-a657-4daa-8a7d-c3c1d25033f6.
[  132.872857] nvmet_tcp: unexpected pdu type 129
[  132.872871] nvmet_tcp: unexpected pdu type 236
[  132.872880] nvmet: ctrl 1 fatal error occurred!


Maurizio

Re: [PATCH 0/2] nvmet-tcp: fix receive path error handling and state machine

[Maurizio Lombardi]

On Fri Mar 13, 2026 at 12:17 PM CET, Maurizio Lombardi wrote:
> On Fri Mar 13, 2026 at 6:58 AM CET, yunje shin wrote:
>
> I guess that the msg_data_left(&cmd->recv_msg) check in
> nvmet_tcp_try_recv_data() prevents the derefence of the uninitialized
> iterator, so this is why it doesn't crash.

Ah!! I got it!

The trick is to enable the data digests:

[ 4486.731644] nvmet_tcp: queue 2: cmd 12292 pdu (6) data digest error: recv 0x1ee9aab3 expected 0x4d76611a
[ 4486.791455] ------------[ cut here ]------------
[ 4486.791464] percpu ref (nvmet_sq_free [nvmet]) <= 0 (0) after switching to atomic
[ 4486.791491] WARNING: lib/percpu-refcount.c:197 at percpu_ref_switch_to_atomic_rcu+0x200/0x210, CPU#3: swapper/3/0
[ 4486.793089] Modules linked in: nvmet_tcp nvmet nvme_keyring nvme_auth hkdf rfkill vfat fat fuse loop nfnetlink vsock_loopback vmw_vsock_virtio_transport_common vmw_vsock_vmci_transport vmw_vmci vsock xfs virtio_net ghash_ce net_failover virtio_blk failover virtio_console dm_mirror dm_region_hash dm_log dm_mod i2c_dev
[ 4486.798659] CPU: 3 UID: 0 PID: 0 Comm: swapper/3 Not tainted 7.0.0-rc3+ #1 PREEMPT(full)
[ 4486.799172] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
[ 4486.799579] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 4486.800009] pc : percpu_ref_switch_to_atomic_rcu+0x200/0x210
[ 4486.800345] lr : percpu_ref_switch_to_atomic_rcu+0x200/0x210
[ 4486.800691] sp : ffff80008001bdf0
[ 4486.800888] x29: ffff80008001bdf0 x28: ffff0000cecc8ba0 x27: ffff00112c16a2c0
[ 4486.801320] x26: ffffb00ad9f97990 x25: ffffb00adc031c80 x24: ffff0000cecc8b80
[ 4486.801797] x23: ffff0000cecc8ba0 x22: ffffb00adc036e60 x21: ffffb00adb06b208
[ 4486.802271] x20: 0000adf96ef8a620 x19: 7fffffffffffffff x18: 00000000ffffffff
[ 4486.802746] x17: ffff5006507c9000 x16: ffffb00ad9958998 x15: ffffb00adc71b709
[ 4486.803219] x14: ffffffffffffffff x13: 0000000000000008 x12: 0101010101010101
[ 4486.803695] x11: 7f7f7f7f7f7f7f7f x10: fefefefefefeff32 x9 : ffffb00ad997f5c8
[ 4486.804167] x8 : ffffb00adc0865d4 x7 : 0000000000000000 x6 : 000000000000000f
[ 4486.804641] x5 : ffff00112c154588 x4 : 00000000ffff82e9 x3 : ffff5006507c9000
[ 4486.805114] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000c0a78000
[ 4486.805584] Call trace:
[ 4486.805748]  percpu_ref_switch_to_atomic_rcu+0x200/0x210 (P)
[ 4486.806125]  rcu_do_batch+0x188/0x7b8
[ 4486.806372]  rcu_core+0x138/0x2f0
[ 4486.806595]  rcu_core_si+0x18/0x30
[ 4486.806821]  handle_softirqs+0x114/0x490
[ 4486.807083]  __do_softirq+0x1c/0x28
[ 4486.807320]  ____do_softirq+0x18/0x30
[ 4486.807566]  call_on_irq_stack+0x30/0x48
[ 4486.807827]  do_softirq_own_stack+0x24/0x50
[ 4486.808105]  __irq_exit_rcu+0x130/0x168
[ 4486.808360]  irq_exit_rcu+0x18/0x30
[ 4486.808592]  el1_interrupt+0x50/0xb8
[ 4486.808834]  el1h_64_irq_handler+0x18/0x28
[ 4486.809113]  el1h_64_irq+0x80/0x88
[ 4486.809340]  default_idle_call+0x38/0x340 (P)
[ 4486.809629]  cpuidle_idle_call+0x184/0x200
[ 4486.809904]  do_idle+0xa4/0x120
[ 4486.810114]  cpu_startup_entry+0x40/0x50
[ 4486.810377]  secondary_start_kernel+0x12c/0x170
[ 4486.810681]  __secondary_switched+0xc0/0xc8
[ 4486.810958] ---[ end trace 0000000000000000 ]---
[ 4486.811288] percpu_ref_switch_to_atomic_rcu: percpu_ref_switch_to_atomic_rcu(): percpu_ref underflow slab kmalloc-64 start ffff0000cecc8b80 pointer offset 0 size 64

Maurizio