Re: [PATCH net v6 3/3] net/smc: skip the DMB nocopy accounting until the send buffer is set

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] net/smc: cursor and sndbuf_space corruption in
  smc_cdc_msg_recv_action()
--

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

net/smc: skip the DMB nocopy accounting until the send buffer is set

This commit publishes conn->sndbuf_desc with smp_store_release() in
smcd_buf_attach() and skips the SMC-D DMB nocopy accounting in
smc_cdc_msg_recv_action() when conn->sndbuf_desc is still NULL.

> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index b3bcc206efe3e..4a4a279b405b1 100644
> --- a/net/smc/smc_cdc.c
> +++ b/net/smc/smc_cdc.c
> @@ -333,6 +333,7 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
>  	union smc_host_cursor cons_old, prod_old;
>  	struct smc_connection *conn = &smc->conn;
>  	int diff_cons, diff_prod, diff_tx;
> +	struct smc_buf_desc *sndbuf_desc;
>  
>  	/* smc_conn_create() can have cleared rmb_desc */
>  	if (!smp_load_acquire(&conn->rmb_desc))
> @@ -357,12 +358,19 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
>  		 * peer RMB, then update tx_curs_fin and sndbuf_space
>  		 * here since peer has already consumed the data.
>  		 */
> +		sndbuf_desc = NULL;
>  		if (conn->lgr->is_smcd &&
>  		    smc_ism_support_dmb_nocopy(conn->lgr->smcd)) {
> +			/* NULL until smcd_buf_attach(), but the old peer of
> +			 * a reused DMB can already signal us.
> +			 */
> +			sndbuf_desc = smp_load_acquire(&conn->sndbuf_desc);
> +		}
> +		if (sndbuf_desc) {
>  			/* Calculate consumed data and
>  			 * increment free send buffer space.
>  			 */
> -			diff_tx = smc_curs_diff(conn->sndbuf_desc->len,
> +			diff_tx = smc_curs_diff(sndbuf_desc->len,
>  						&conn->tx_curs_fin,
>  						&conn->local_rx_ctrl.cons);
>  			/* increase local sndbuf space and fin_curs */

[Severity: High]
Does skipping the if (sndbuf_desc) block while still applying
conn->local_rx_ctrl.cons desynchronize conn->tx_curs_fin from
conn->local_rx_ctrl.cons and corrupt conn->sndbuf_space?

When a CDC message from the old peer of a reused DMB arrives before
smcd_buf_attach() with a non-zero consumer cursor, smc_cdc_msg_to_host()
advances conn->local_rx_ctrl.cons (and adds diff_cons to
conn->peer_rmbe_space, which smc_conn_save_peer_info() already set to
conn->peer_rmbe_size), while skipping the if (sndbuf_desc) block leaves
conn->tx_curs_fin at {0, 0}.

smcd_buf_attach() then initializes conn->sndbuf_space to the full
buf_desc->len without resetting conn->local_rx_ctrl.cons or matching
conn->tx_curs_fin to it.  Because diff_tx is computed against
conn->tx_curs_fin rather than cons_old:

net/smc/smc_cdc.c:smc_cdc_msg_recv_action() {
    ...
			diff_tx = smc_curs_diff(sndbuf_desc->len,
						&conn->tx_curs_fin,
						&conn->local_rx_ctrl.cons);
			/* increase local sndbuf space and fin_curs */
			smp_mb__before_atomic();
			atomic_add(diff_tx, &conn->sndbuf_space);
			/* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */
			smp_mb__after_atomic();
			smc_curs_copy(&conn->tx_curs_fin,
				      &conn->local_rx_ctrl.cons, conn);
    ...
}

any later consumer update that advances past the pre-attach cursor still
credits the pre-attach delta on top of buf_desc->len, pushing
conn->sndbuf_space above sndbuf_desc->len and letting smc_tx_sendmsg()
overwrite unconsumed data or write past the end of the buffer.  Conversely,
when the new peer sends consumer updates at or below the stale
conn->local_rx_ctrl.cons, smc_curs_diff() returns 0 and never credits
conn->peer_rmbe_space or conn->sndbuf_space, stalling transmission.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-b4-disp-1c8d3a53-v6-0-36a38ae35021@proton.me?part=3