[PATCH v6] scsi: libiscsi_tcp: zero the unsent part of a short read

[PATCH v6] scsi: libiscsi_tcp: zero the unsent part of a short read

[Yehyeong Lee]

A target can finish a read without sending the data.  iscsi_tcp_data_in()
bounds each Data-In PDU against the command buffer but never adds them up,
so a completion that declares no underflow is believed and the command
ends with DID_OK.  The pages keep whatever they held.

Over a 1 MiB pread of a file that had never been read, 999424 bytes came
back as the contents of an unrelated file the same process had written
earlier.  pread() returned 1048576 and errno was 0.

Count the Data-In payload per task and, when the command completes, zero
the tail of the read buffer that was never received.  Whatever the
midlayer reports as transferred the caller then sees zeroes instead of
stale page contents, so no completion status has to be special-cased.
Writes are untouched.

Fixes: a081c13e39b5 ("[SCSI] iscsi_tcp: split module into lib and lld")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
v6: change approach.  Instead of deciding from the status, the sense, the
    service response and the residual whether the target promised the whole
    buffer, zero the bytes that did not arrive.  The previous versions kept
    growing because every completion shape that reports a full transfer has
    to be enumerated - SAM_STAT_GOOD, CHECK_CONDITION with RECOVERED_ERROR,
    and whatever a future upper-level driver decides - and missing one hands
    the caller the previous contents of those pages.  Zeroing the tail is
    independent of all of it: whatever the midlayer reports as transferred,
    the caller sees zeroes.  That also removes the need to fail the command,
    so a legitimate error reply no longer costs the connection.  The
    predictor and its three call sites go away; the per-task counter, its
    reset and the continuity check stay.
v5: do not check a response that did not complete at the target
    (ISCSI_STATUS_CMD_COMPLETED), since iscsi_scsi_cmd_rsp() already fails
    it with DID_ERROR, and treat a residual larger than the command buffer
    as a protocol error rather than a reason to skip the check (noted by
    Sashiko AI review).  The remaining case it raised, CHECK_CONDITION with
    a data segment shorter than two bytes, needs no change: libiscsi fails
    that with DID_BAD_TARGET, so the data is never handed to the caller.
v4: also validate coverage when the status is CHECK_CONDITION with a sense
    key of RECOVERED_ERROR.  sd_done() counts RECOVERED_ERROR as the whole
    buffer transferred, so v3's "skip every non-GOOD status" left a target
    able to short a read and still have it reported as a full success
    (noted by Sashiko AI review).
    The Data-In continuity check requires in-order Data-In and so does not
    interoperate with a target that negotiates DataPDUInOrder=No; that is
    deliberate, as making a safety check conditional on a peer-negotiated
    value would let the peer turn it off.
v3: only validate coverage for SAM_STAT_GOOD completions; a non-GOOD reply
    such as CHECK_CONDITION that omits the underflow flag must not be
    treated as a short read (noted by Sashiko AI review).
v2: take back_lock once on the SCSI Response path and make the counter
    uint32_t, both per Mike Christie's review.

On "was it built over another patch?" - no, v1 was generated against
v7.2-rc5 directly.  It stopped applying because c1dea15f819cd ("scsi:
libiscsi_tcp: Bound SCSI Response data segment to the connection
buffer") folded ISCSI_OP_SCSI_CMD_RSP into the shared response case,
which is the block v1's last hunk patched.

v1: https://lore.kernel.org/all/20260814121428.359541-1-yhlee@isslab.korea.ac.kr/

 drivers/scsi/libiscsi_tcp.c | 85 +++++++++++++++++++++++++++++++++++--
 include/scsi/libiscsi_tcp.h |  1 +
 2 files changed, 83 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
index d35f93451ee9b..00bdbcc6e27e3 100644
--- a/drivers/scsi/libiscsi_tcp.c
+++ b/drivers/scsi/libiscsi_tcp.c
@@ -397,6 +397,61 @@ void iscsi_tcp_hdr_recv_prep(struct iscsi_tcp_conn *tcp_conn)
 }
 EXPORT_SYMBOL_GPL(iscsi_tcp_hdr_recv_prep);
 
+/**
+ * iscsi_tcp_zero_unread - zero the part of a read buffer the target never sent
+ * @task: scsi command task
+ *
+ * A target can end a read without sending all of the data, in any of the ways
+ * a completion can be reported.  Rather than predict from the status, the
+ * sense and the residual whether the buffer was promised in full, zero the
+ * bytes that did not arrive.  Whatever the midlayer then reports as
+ * transferred, the caller sees zeroes and not the previous contents of those
+ * pages.
+ *
+ * The continuity check in iscsi_tcp_data_in() means the bytes that did not
+ * arrive are exactly the tail, so one range covers them.
+ */
+static void iscsi_tcp_zero_unread(struct iscsi_task *task)
+{
+	struct iscsi_tcp_task *tcp_task = task->dd_data;
+	struct scsi_cmnd *sc = task->sc;
+
+	if (!sc || sc->sc_data_direction != DMA_FROM_DEVICE ||
+	    tcp_task->data_in_bytes >= sc->sdb.length)
+		return;
+
+	sg_zero_buffer(sc->sdb.table.sgl, sc->sdb.table.nents,
+		       sc->sdb.length - tcp_task->data_in_bytes,
+		       tcp_task->data_in_bytes);
+}
+
+/**
+ * iscsi_tcp_complete_cmd - zero any unread tail, then complete the command
+ * @conn: iscsi connection
+ * @hdr: the PDU carrying the status
+ * @data: data segment of that PDU, if it has one
+ * @datalen: length of @data
+ *
+ * back_lock is taken once for the lookup, the fill and the completion.
+ */
+static int iscsi_tcp_complete_cmd(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
+				  char *data, int datalen)
+{
+	struct iscsi_task *task;
+	int rc;
+
+	spin_lock(&conn->session->back_lock);
+	task = iscsi_itt_to_ctask(conn, hdr->itt);
+	if (!task) {
+		spin_unlock(&conn->session->back_lock);
+		return ISCSI_ERR_BAD_ITT;
+	}
+	iscsi_tcp_zero_unread(task);
+	rc = __iscsi_complete_pdu(conn, hdr, data, datalen);
+	spin_unlock(&conn->session->back_lock);
+	return rc;
+}
+
 /*
  * Handle incoming reply to any other type of command
  */
@@ -410,8 +465,13 @@ iscsi_tcp_data_recv_done(struct iscsi_tcp_conn *tcp_conn,
 	if (!iscsi_tcp_dgst_verify(tcp_conn, segment))
 		return ISCSI_ERR_DATA_DGST;
 
-	rc = iscsi_complete_pdu(conn, tcp_conn->in.hdr,
-			conn->data, tcp_conn->in.datalen);
+	if ((tcp_conn->in.hdr->opcode & ISCSI_OPCODE_MASK) ==
+	    ISCSI_OP_SCSI_CMD_RSP)
+		rc = iscsi_tcp_complete_cmd(conn, tcp_conn->in.hdr, conn->data,
+					    tcp_conn->in.datalen);
+	else
+		rc = iscsi_complete_pdu(conn, tcp_conn->in.hdr, conn->data,
+					tcp_conn->in.datalen);
 	if (rc)
 		return rc;
 
@@ -509,6 +569,11 @@ static int iscsi_tcp_data_in(struct iscsi_conn *conn, struct iscsi_task *task)
 		return ISCSI_ERR_DATA_OFFSET;
 	}
 
+	if (tcp_task->data_offset != tcp_task->data_in_bytes)
+		return ISCSI_ERR_DATA_OFFSET;
+
+	tcp_task->data_in_bytes += tcp_conn->in.datalen;
+
 	conn->datain_pdus_cnt++;
 	return 0;
 }
@@ -658,7 +723,7 @@ iscsi_tcp_process_data_in(struct iscsi_tcp_conn *tcp_conn,
 
 	/* check for non-exceptional status */
 	if (hdr->flags & ISCSI_FLAG_DATA_STATUS) {
-		rc = iscsi_complete_pdu(conn, tcp_conn->in.hdr, NULL, 0);
+		rc = iscsi_tcp_complete_cmd(conn, tcp_conn->in.hdr, NULL, 0);
 		if (rc)
 			return rc;
 	}
@@ -752,6 +817,14 @@ iscsi_tcp_hdr_dissect(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
 			spin_unlock(&conn->session->back_lock);
 			return rc;
 		}
+		/*
+		 * A Data-In with no data segment can still carry the status,
+		 * and it completes the command here rather than from
+		 * iscsi_tcp_process_data_in(), so fill the tail on this path
+		 * too.
+		 */
+		if (hdr->flags & ISCSI_FLAG_DATA_STATUS)
+			iscsi_tcp_zero_unread(task);
 		rc = __iscsi_complete_pdu(conn, hdr, NULL, 0);
 		spin_unlock(&conn->session->back_lock);
 		break;
@@ -783,6 +856,11 @@ iscsi_tcp_hdr_dissect(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
 			break;
 		}
 
+		if (opcode == ISCSI_OP_SCSI_CMD_RSP && !tcp_conn->in.datalen) {
+			rc = iscsi_tcp_complete_cmd(conn, hdr, NULL, 0);
+			break;
+		}
+
 		/* If there's data coming in with the response,
 		 * receive it to the connection's buffer.
 		 */
@@ -995,6 +1073,7 @@ int iscsi_tcp_task_init(struct iscsi_task *task)
 
 	BUG_ON(kfifo_len(&tcp_task->r2tqueue));
 	tcp_task->exp_datasn = 0;
+	tcp_task->data_in_bytes = 0;
 
 	/* Prepare PDU, optionally w/ immediate data */
 	ISCSI_DBG_TCP(conn, "task deq [itt 0x%x imm %d unsol %d]\n",
diff --git a/include/scsi/libiscsi_tcp.h b/include/scsi/libiscsi_tcp.h
index ef53d4bea28a0..a6e63db75d674 100644
--- a/include/scsi/libiscsi_tcp.h
+++ b/include/scsi/libiscsi_tcp.h
@@ -66,6 +66,7 @@ struct iscsi_tcp_conn {
 
 struct iscsi_tcp_task {
 	uint32_t		exp_datasn;	/* expected target's R2TSN/DataSN */
+	uint32_t		data_in_bytes;	/* Data-In payload received */
 	int			data_offset;
 	struct iscsi_r2t_info	*r2t;		/* in progress solict R2T */
 	struct iscsi_pool	r2tpool;

base-commit: 2c3418fffa9d037b2038a6db48be63f9e2291806
-- 
2.43.0

Re: [PATCH v6] scsi: libiscsi_tcp: zero the unsent part of a short read

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: libiscsi_tcp: breaks cxgbi hardware offload and violates DMA API
--

commit a677da403d374784c9e9272a697dea8be970518e
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: libiscsi_tcp: zero the unsent part of a short read

A target can finish a read without sending the data. iscsi_tcp_data_in()
bounds each Data-In PDU against the command buffer but never adds them up,
so a completion that declares no underflow is believed and the command
ends with DID_OK.

> diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
> index d35f93451ee9b..00bdbcc6e27e3 100644
> --- a/drivers/scsi/libiscsi_tcp.c
> +++ b/drivers/scsi/libiscsi_tcp.c
> @@ -397,6 +397,61 @@ void iscsi_tcp_hdr_recv_prep(struct iscsi_tcp_conn *tcp_conn)
>  }
>  EXPORT_SYMBOL_GPL(iscsi_tcp_hdr_recv_prep);
>  
> +/**
> + * iscsi_tcp_zero_unread - zero the part of a read buffer the target never sent
> + * @task: scsi command task
> + *
> + * A target can end a read without sending all of the data, in any of the ways
> + * a completion can be reported.  Rather than predict from the status, the
> + * sense and the residual whether the buffer was promised in full, zero the
> + * bytes that did not arrive.  Whatever the midlayer then reports as
> + * transferred, the caller sees zeroes and not the previous contents of those
> + * pages.
> + *
> + * The continuity check in iscsi_tcp_data_in() means the bytes that did not
> + * arrive are exactly the tail, so one range covers them.
> + */
> +static void iscsi_tcp_zero_unread(struct iscsi_task *task)
> +{
> +	struct iscsi_tcp_task *tcp_task = task->dd_data;
> +	struct scsi_cmnd *sc = task->sc;
> +
> +	if (!sc || sc->sc_data_direction != DMA_FROM_DEVICE ||
> +	    tcp_task->data_in_bytes >= sc->sdb.length)
> +		return;
> +
> +	sg_zero_buffer(sc->sdb.table.sgl, sc->sdb.table.nents,
> +		       sc->sdb.length - tcp_task->data_in_bytes,
> +		       tcp_task->data_in_bytes);

[Severity: High]
Does this code violate the DMA API and overwrite hardware-placed data for
offloaded transfers?

For hardware offload drivers like cxgbi that use iscsi_tcp_recv_skb() for
header processing but Direct Data Placement (DDP) for data payloads,
intermediate Data-In PDUs bypass the host stack. This means
tcp_task->data_in_bytes will not be incremented for DDP'd data.

When the final completion PDU is processed and iscsi_tcp_zero_unread() is
invoked, could it zero the entire buffer (sc->sdb.length - 0), destroying the
valid data already placed by the hardware?

Since the buffer is actively mapped DMA_FROM_DEVICE (e.g. via dma_map_sg()
in cxgbi_ddp_reserve()), zeroing it from the CPU without calling
dma_sync_sg_for_cpu() or prior unmapping is a strict DMA API violation. On
non-coherent architectures, cache invalidation during scsi_dma_unmap() would
further discard the CPU's zeroes.

> +}
> +

[ ... ]

> @@ -509,6 +569,11 @@ static int iscsi_tcp_data_in(struct iscsi_conn *conn, struct iscsi_task *task)
>  		return ISCSI_ERR_DATA_OFFSET;
>  	}
>  
> +	if (tcp_task->data_offset != tcp_task->data_in_bytes)
> +		return ISCSI_ERR_DATA_OFFSET;

[Severity: High]
Does this strict continuity check incorrectly fail hardware offloaded
transfers?

If the final PDU contains a data segment after intermediate headers bypassed
the software stack (due to offload), tcp_task->data_in_bytes will not account
for the offloaded data. Will tcp_task->data_offset != tcp_task->data_in_bytes
then incorrectly evaluate to true, causing the connection to fail with
ISCSI_ERR_DATA_OFFSET?

> +
> +	tcp_task->data_in_bytes += tcp_conn->in.datalen;
> +
>  	conn->datain_pdus_cnt++;
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008040129.747538-1-yhlee@isslab.korea.ac.kr?part=1