Re: [PATCH net v7 3/4] net/smc: bound the send length to the send buffer in smc_tx_sendmsg()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 (2) · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] smc: flow control bypass and data corruption on negative
  sndbuf_space
- [Low] The new comment in smc_tx_sendmsg() (net/smc/smc_tx.c:236-240)
  says sndbuf_space 'can be out of range' because of late SMC-R…

Pre-existing issues:
- [High] The clamp bounds the size of each copy, not the flow control.
- [High] smc_switch_cursor() (net/smc/smc_core.c:1033) reads tx_curs_fin
  and local_tx_ctrl_fin, rewrites tx_curs_sent, tx_curs_fin and…

--

net/smc: bound the send length to the send buffer in smc_tx_sendmsg()

The patch clamps writespace in smc_tx_sendmsg() to sndbuf_desc->len. It
also maps negative values to len. This keeps the copy inside the send
buffer when conn->sndbuf_space has been over-credited, either after an
SMC-R link failover or through the SMC-D DMB merge path.

> diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c
> index 3144b4b1fe290..58d9dfdcdf33e 100644
> --- a/net/smc/smc_tx.c
> +++ b/net/smc/smc_tx.c
> @@ -233,6 +233,14 @@ int smc_tx_sendmsg(struct smc_sock *smc, struct msghdr *msg, size_t len)
>  		/* initialize variables for 1st iteration of subsequent loop */
>  		/* could be just 1 byte, even after smc_tx_wait above */
>  		writespace = atomic_read(&conn->sndbuf_space);
> +		/* sndbuf_space can be out of range: late completions of an
> +		 * old SMC-R link credit it again after a failover, and with
> +		 * SMC-D DMB merge it follows the peer's consumer cursor.

[Severity: Low]
This isn't a bug, but the new comment says sndbuf_space can be out of
range. A few lines further down in the same loop, the existing comment
still says the opposite:

net/smc/smc_tx.c:smc_tx_sendmsg() {
    ...
	atomic_sub(copylen, &conn->sndbuf_space);
	/* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */
	smp_mb__after_atomic();
    ...
}

The same "guarantee" comment also appears in the credit paths that the
new comment names: smc_cdc_tx_handler(), smcd_cdc_msg_send() and the DMB
merge branch of smc_cdc_msg_recv_action().

The clamped subtraction doesn't bring the value back into that range. If
sndbuf_space starts at 3 * len, subtracting copylen <= len leaves at
least 2 * len. A negative value is mapped to len and then pushed further
below zero.

Should those comments be updated or dropped so they don't contradict the
new one?

> +		 * Bound writespace, which sizes the copy; a negative value
> +		 * would become huge in the size_t min_t() below.
> +		 */
> +		if (writespace < 0 || writespace > conn->sndbuf_desc->len)
> +			writespace = conn->sndbuf_desc->len;

[Severity: High]
Mapping a negative writespace to sndbuf_desc->len is new behaviour from
this patch. It turns a negative sndbuf_space into a full-ring copy on
every pass of the loop. Is that intended?

The gate at the top of the loop treats only zero as no space:

	if (!atomic_read(&conn->sndbuf_space) || conn->urg_tx_pend) {

A negative sndbuf_space is non-zero, so it skips smc_tx_wait()
entirely. The new check then sets writespace to sndbuf_desc->len, so
copylen can be a whole ring.

memcpy_from_msg() writes that at tx_curs_prep. It overwrites whatever
is still unsent in [tx_curs_sent, tx_curs_prep), and whatever is still
unconfirmed in [tx_curs_fin, tx_curs_sent) that an in-flight RDMA write
may still be reading.

The atomic_sub(copylen, &conn->sndbuf_space) afterwards pushes the
value further below zero. This path can therefore never bring it back
to zero. The loop keeps copying until the whole message is consumed,
with no back-pressure. That lasts until CDC completions happen to
credit sndbuf_space back above zero.

smc_tx_wait(), smc_tx_should_cork() and smc_poll() use the same
non-zero test. While the space is negative, a blocked writer would be
woken, corking would still apply, and a poller would see EPOLLOUT.

The note below the --- line says 0 was avoided because the send loop
would spin. That is true with the current !atomic_read() gate. Would it
be better to make the gate treat sndbuf_space <= 0 as no space, along
with the matching checks in smc_tx_wait(), smc_tx_should_cork() and
smc_poll()? A negative value would then block the sender instead of
letting it overwrite data the peer has not received.

Before this patch a negative value caused an out-of-bounds write. With
it, the result is silent stream corruption that sendmsg() reports as
success.

[Severity: High]
This is a pre-existing issue, and this patch didn't introduce it. The
clamp limits the size of each copy but doesn't fix the flow control.
While sndbuf_space is over-credited, can smc_tx_sendmsg() still overwrite
data in the send buffer that hasn't been sent or confirmed yet?

The wait check at the top of the loop counts any non-zero value as free
space:

	if (!atomic_read(&conn->sndbuf_space) || conn->urg_tx_pend) {

smc_tx_wait(), smc_tx_should_cork() and smc_poll() make the same check.
Each pass then copies up to sndbuf_desc->len bytes at tx_curs_prep and
subtracts only copylen. A value of 3 * len stays positive for two more
passes. A negative value never reaches zero.

That lets tx_curs_prep run more than a full ring past tx_curs_sent and
tx_curs_fin. The copy then overwrites unsent bytes in
[tx_curs_sent, tx_curs_prep), plus bytes in [tx_curs_fin, tx_curs_sent)
that in-flight RDMA writes may still be reading.

smc_tx_rdma_writes() then computes:

	to_send = smc_curs_diff(conn->sndbuf_desc->len, &sent, &prep);

That value is now larger than len, so the overwritten ring contents get
sent. sendmsg() reports success, but the peer receives a corrupted
stream.

One trigger is a peer DELETE LINK while the old QP still works:

smc_llc_process_cli_delete_link()
  smc_switch_conns(lgr, lnk_del, false)

A CDC completion still pending on the old link is later handled by
smc_wr_tx_process_cqe()->smc_cdc_tx_handler() with a stale
cdcpend->cursor. If tx_curs_fin is one wrap ahead, smc_curs_diff()
returns nearly 2 * len and tx_curs_fin moves back. The next completion
then credits the same bytes again.

If corking with MSG_MORE or a full peer RMBE keeps tx_curs_sent from
moving between passes, the overwrite is guaranteed.

The note below the --- line already says that "send() can still
overwrite data in the buffer that the peer has not received yet". Before
this patch, the same over-credit caused an out-of-bounds write instead.
Is a fix for the credit in smc_cdc_tx_handler() planned as a follow-up?

[Severity: High]
This is also a pre-existing issue. What serializes smc_switch_cursor()
against smc_cdc_tx_handler()?

smc_switch_conns() holds only conn->send_lock around the switch:

	spin_lock_bh(&conn->send_lock);
	smc_switch_link_and_count(conn, to_lnk);
	rc = smc_switch_cursor(smc, pend, wr_buf);
	spin_unlock_bh(&conn->send_lock);

smc_switch_cursor() rewrites tx_curs_sent, tx_curs_fin,
local_tx_ctrl.prod and local_tx_ctrl_fin, and it adds to sndbuf_space.

smc_cdc_tx_handler() runs from the send CQ tasklet. It does a non-atomic
read, diff, add and copy while holding only bh_lock_sock():

	bh_lock_sock(&smc->sk);
	if (!wc_status && sndbuf_desc) {
		diff = smc_curs_diff(sndbuf_desc->len,
				     &cdcpend->conn->tx_curs_fin,
				     &cdcpend->cursor);
		...
		atomic_add(diff, &cdcpend->conn->sndbuf_space);
		...
		smc_curs_copy(&conn->tx_curs_fin, &cdcpend->cursor, conn);

Neither path takes the other's lock, and the tasklet can run on another
CPU in the middle of the switch. One possible interleaving:

CPU1 (smc_switch_cursor)
  smc_curs_copy(&conn->tx_curs_sent, &conn->tx_curs_fin, conn);
  smc_curs_copy(&fin, &conn->local_tx_ctrl_fin, conn);

CPU2 (smc_cdc_tx_handler)
  atomic_add(diff, &cdcpend->conn->sndbuf_space);
  smc_curs_copy(&conn->tx_curs_fin, &cdcpend->cursor, conn);

CPU1 (smc_switch_cursor, diff computed from the older fin)
  smc_curs_add(conn->sndbuf_desc->len, &conn->tx_curs_fin, diff);
  atomic_add(diff, &conn->sndbuf_space);

Does this credit the same bytes twice and leave tx_curs_fin ahead of
tx_curs_sent?

The patch bounds only the local writespace snapshot. The shared
sndbuf_space would stay inflated, so smc_tx_sendmsg() could still
overwrite unsent data. On SMC-R an RDMA write may still be using that
data. With SMC-D nocopy, the peer may still be reading it from its DMB.

>  		/* not more than what user space asked for */
>  		copylen = min_t(size_t, send_remaining, writespace);
>  		/* determine start of sndbuf */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013-b4-disp-f345196c-v7-0-ef3dddfbfa68%40proton.me