Re: [PATCH net 3/4] net/smc: fix the failover catch-up across the cursor wrap rollover

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] net/smc: clamping diff in smc_switch_cursor() leaves prod
  behind cons
--

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

net/smc: fix the failover catch-up across the cursor wrap rollover

This commit updates smc_switch_cursor() to compute the failover catch-up
distance using smc_curs_sdiff(), bound it by the number of sent bytes,
and advance the RMB cursors across multiple ring wraps.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index fb9f6a4eb6835..76a577a20e4ae 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -1054,22 +1054,28 @@ static int smc_switch_cursor(struct smc_sock *smc, struct smc_cdc_tx_pend *pend,
>  			     struct smc_wr_buf *wr_buf)
>  {
>  	struct smc_connection *conn = &smc->conn;
> -	union smc_host_cursor cons, fin;
> +	union smc_host_cursor cons, fin, sent;
>  	int rc = 0;
> -	int diff;
> +	s64 diff;
>  
>  	conn->tx_switch_gen++;	/* completions of earlier sends: no credit */
> +	smc_curs_copy(&sent, &conn->tx_curs_sent, conn);
>  	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() */
>  	smc_curs_copy(&conn->local_tx_ctrl.prod, &fin, conn);
>  	smc_curs_copy(&cons, &conn->local_rx_ctrl.cons, conn);
>  
> -	if (smc_curs_comp(conn->peer_rmbe_size, &cons, &fin) < 0) {
> +	/* not past what was sent, whatever cons says; that also keeps
> +	 * diff within one send buffer for smc_curs_add()
> +	 */
> +	diff = min(smc_curs_sdiff(conn->peer_rmbe_size, &fin, &cons),
> +		   smc_curs_sdiff(conn->sndbuf_desc->len,
> +				  &conn->tx_curs_fin, &sent));
> +	if (diff > 0) {
>  		/* cons cursor advanced more than fin, and prod was set
>  		 * fin above, so now prod is smaller than cons. Fix that.
>  		 */
> -		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,
> @@ -1079,10 +1085,10 @@ static int smc_switch_cursor(struct smc_sock *smc, struct smc_cdc_tx_pend *pend,
>  		atomic_add(diff, &conn->sndbuf_space);
>  		smp_mb__after_atomic();
>  
> -		smc_curs_add(conn->peer_rmbe_size,
> -			     &conn->local_tx_ctrl.prod, diff);
> -		smc_curs_add(conn->peer_rmbe_size,
> -			     &conn->local_tx_ctrl_fin, diff);
> +		smc_curs_advance(conn->peer_rmbe_size,
> +				 &conn->local_tx_ctrl.prod, diff);
> +		smc_curs_advance(conn->peer_rmbe_size,
> +				 &conn->local_tx_ctrl_fin, diff);
>  	}

[Severity: High]
Can clamping diff to tx_curs_fin..sent leave conn->local_tx_ctrl.prod
behind conn->local_rx_ctrl.cons?

Whenever fin..cons exceeds tx_curs_fin..sent, local_tx_ctrl.prod is only
advanced by diff (to fin + diff), which remains strictly behind cons.
That happens if a peer advertises cons ahead of tx_curs_sent, or across
two link switches when data sent on the first link reaches the peer and is
consumed after the first smc_switch_cursor() already rewound tx_curs_sent
to tx_curs_fin (making tx_curs_fin..sent zero on the second switch before
tx_work resends).

Both smc_write_space() and smc_tx_rdma_writes() assume cons <= prod:

net/smc/smc_tx.c:smc_tx_rdma_writes() {
	...
	if (prod.wrap == cons.wrap) {
		dst_len = min_t(size_t,
				conn->peer_rmbe_size - prod.count, len);
	} else {
		dst_len = len;
	}
	...
}

When prod < cons, smc_write_space() can compute a negative
peer_rmbe_space (stalling the connection), or when prod.wrap != cons.wrap
with prod.count <= cons.count, smc_tx_rdma_writes() skips clamping
dst_len to conn->peer_rmbe_size - prod.count and can issue an RDMA write
past the end of the peer RMBE.

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