Re: [PATCH net 2/4] net/smc: do not credit stale CDC completions after a link switch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] net/smc: non-atomic in-place cursor updates in
  smc_switch_cursor() [1]
--

commit eb6e0c5a04a6c58fa63f96a2d68145215e6a20f0
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/smc: do not credit stale CDC completions after a link switch

This commit tags pending CDC transmissions with a link-switch generation
counter and checks the completion cursor against tx_curs_fin so stale or
out-of-order completions after a link switch do not credit send buffer space.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 73e86100e6db2..fb9f6a4eb6835 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -1058,6 +1058,7 @@ static int smc_switch_cursor(struct smc_sock *smc, struct smc_cdc_tx_pend *pend,
>  	int rc = 0;
>  	int diff;
>  
> +	conn->tx_switch_gen++;	/* completions of earlier sends: no credit */
>  	smc_curs_copy(&conn->tx_curs_sent, &conn->tx_curs_fin, conn);
>  	smc_curs_copy(&fin, &conn->local_tx_ctrl_fin, conn);
>  	/* set prod cursor to old state, enforce tx_rdma_writes() */

[Severity: Medium]
This problem wasn't introduced by this patch, but can a concurrent reader
observe a torn conn->tx_curs_sent when smc_switch_cursor() advances it in
place with smc_curs_add() [1]?

net/smc/smc_core.c:smc_switch_cursor() {
	...
	if (smc_curs_comp(conn->peer_rmbe_size, &cons, &fin) < 0) {
		diff = smc_curs_diff(conn->peer_rmbe_size, &fin, &cons);
		smc_curs_add(conn->sndbuf_desc->len,
			     &conn->tx_curs_sent, diff);
		smc_curs_add(conn->sndbuf_desc->len,
			     &conn->tx_curs_fin, diff);
	...
}

smc_switch_cursor() holds conn->send_lock, not the socket lock.  Unlike
smc_curs_copy(), smc_curs_add() mutates curs->count and curs->wrap in place
Across three steps:

CPU0                                  CPU1
smc_switch_cursor()                   smc_close_stream_wait()
  smc_curs_add(..., &conn->tx_curs_sent, diff)
    curs->count += value;
    curs->wrap++;
                                        smc_tx_prepared_sends()
                                          smc_curs_copy(&sent,
                                            &conn->tx_curs_sent, conn)
                                          smc_curs_diff(..., &sent, &prep)
                                            <-- torn cursor, returns 0
    curs->count -= size;

When conn->tx_curs_sent wraps to match conn->tx_curs_prep.wrap, a reader that
copies conn->tx_curs_sent before curs->count is reduced sees
sent.wrap == prep.wrap with sent.count > prep.count.  smc_curs_diff() then
clamps prep.count - sent.count to 0, which lets smc_close_stream_wait() stop
waiting for prepared sends and close the connection before queued data is
transmitted.

Advancing local cursor copies with smc_curs_add() and publishing them via
smc_curs_copy() would avoid exposing the intermediate state.

[1] https://sashiko.dev/#/bug/linux-9856806b-d08f-4d48-bb6c-9f1dcc135c1e

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-b4-disp-1a9ab9b0-v1-0-4a7c8d85d849@proton.me?part=2