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

[sashiko-bot@kernel.org]

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