Re: [PATCH v2] scsi: target: tcm_fc: reject overlapping write-data frames

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] tcm_fc: out-of-order writes fail and timeout due to hardcoded FT_WRITE_SEGS limit
--

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

scsi: target: tcm_fc: reject overlapping write-data frames

This commit introduces tracking of FCoE write data segments to prevent
overlapping frames from falsely satisfying the command completion length test,
which could otherwise leave trailing buffer contents unchanged. It tracks
disjoint write ranges in a fixed-size array to reject duplicated payload data.

> diff --git a/drivers/target/tcm_fc/tfc_io.c b/drivers/target/tcm_fc/tfc_io.c
> --- a/drivers/target/tcm_fc/tfc_io.c
> +++ b/drivers/target/tcm_fc/tfc_io.c

[ ... ]

> +/* Record a frame's range; refuse one that repeats data already received. */
> +static bool ft_add_write_range(struct ft_cmd *cmd, u32 off, u32 len)
> +{
> +	u32 end = off + len;
> +	unsigned int i;
> +
> +	if (off < cmd->write_contig_end)
> +		return false;

[ ... ]

> +		if (i == cmd->write_nseg) {
> +			if (cmd->write_nseg >= FT_WRITE_SEGS)
> +				return false;
> +			cmd->write_seg[i].off = off;
> +			cmd->write_seg[i].end = end;
> +			cmd->write_nseg++;
> +		}
> +	}
> +
> +	ft_merge_write_ranges(cmd);
> +	return true;
> +}

[Severity: High]
Does this hardcoded limit reject legitimate, highly out-of-order FCoE frames?

If network packet loss or reordering creates 5 or more disjoint data segments
before they can be coalesced by ft_merge_write_ranges(), this code hits the
FT_WRITE_SEGS cap and returns false for a valid frame.

Additionally, is it possible for concurrent receive threads to call
ft_add_write_range() for the same exchange simultaneously?

Since the array manipulation and cmd->write_nseg increments are performed
without locks, concurrent frames could cause a data race, potentially
leading to array out-of-bounds corruption into cmd->work.

[ ... ]

>  	if (frame_len + rel_off > se_cmd->data_length)
>  		frame_len = se_cmd->data_length - rel_off;
> +	if (!ft_add_write_range(cmd, rel_off, frame_len)) {
> +		pr_err_ratelimited("%s: xid 0x%x: offset %u len %zu repeats data\n",
> +				   __func__, ep->xid, rel_off, frame_len);
> +		goto drop;
> +	}
>  
>  	/*
>  	 * Setup to use first mem list entry, unless no data.

[Severity: High]
If ft_add_write_range() returns false due to hitting the FT_WRITE_SEGS limit,
does this silently drop the frame without aborting the exchange?

Because FCP write data is unacknowledged per-frame, the initiator will not
be aware that the target dropped the data and will wait for command
completion. The target will hang indefinitely waiting for the dropped bytes.

Will this cause the I/O command to permanently hang until it times out under
non-ideal network conditions?

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