Re: [PATCH net-next v4 1/3] net/smc: clear the correct v2 slot and buffer in smc_wr_tx_put_slot()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Data race in V2 slot cleanup allowing a concurrent sender's data to be overwritten.
- [High] Unconditional zeroing of the shared `lgr->wr_tx_buf_v2` in `smc_wr_tx_process_cqe()` on any link error corrupts in-progress LLC messages on other links.
--

--- Patch [1]: [PATCH net-next v4 1/3] net/smc: clear the correct v2 slot and buffer in smc_wr_tx_put_slot() ---
commit eb37bf5f30fd47da384e2535c48a0ad5576aa908
Author: D. Wythe <alibuda@linux.alibaba.com>

net/smc: clear the correct v2 slot and buffer in smc_wr_tx_put_slot()

smc_wr_tx_put_slot() tries to reset the v2 pending slot and buffer with
memset(&link->wr_tx_v2_pend, 0, sizeof(link->wr_tx_v2_pend)) and the
equivalent for wr_tx_buf_v2. Both are pointers, so this zeroes the 8-byte
pointer variable instead of the structure it points to. The pending slot
and buffer are therefore never actually cleared, and the pointers get
overwritten with NULL.

Pass the pointers directly and use sizeof(*pointer) so the intended
structures are cleared.

> diff --git a/net/smc/smc_wr.c b/net/smc/smc_wr.c
> index 59c92b46945c5..6b5add9229937 100644
> --- a/net/smc/smc_wr.c
> +++ b/net/smc/smc_wr.c
> @@ -288,10 +288,10 @@ int smc_wr_tx_put_slot(struct smc_link *link,
>  	} else if (link->lgr->smc_version == SMC_V2 &&
>  		   pend->idx == link->wr_tx_cnt) {
>  		/* Large v2 buffer */
> -		memset(&link->wr_tx_v2_pend, 0,
> -		       sizeof(link->wr_tx_v2_pend));
> +		memset(link->wr_tx_v2_pend, 0,
> +		       sizeof(*link->wr_tx_v2_pend));

[Severity: High]
This isn't a bug introduced by this patch, but does clearing the control
structure before the data buffer create a race window?

If a thread sends an LLC message and waits for a response, the Rx tasklet 
might process the Rx CQE first, waking up the LLC flow thread which proceeds
to send the next message by calling smc_wr_tx_get_v2_slot().

Concurrently, the Tx tasklet could process the Tx CQE and clear the slot
control structure here. The zeroed idx immediately publishes the slot as
free, so the LLC thread could acquire it and begin formatting its message in
link->lgr->wr_tx_buf_v2.

The same sequence appears to occur in smc_wr_tx_process_cqe().

Would the following memset then wipe out the newly written message?

> -		memset(&link->lgr->wr_tx_buf_v2, 0,
> -		       sizeof(link->lgr->wr_tx_buf_v2));
> +		memset(link->lgr->wr_tx_buf_v2, 0,
> +		       sizeof(*link->lgr->wr_tx_buf_v2));

[Severity: High]
This is a pre-existing issue, but could unconditionally zeroing the shared
link->lgr->wr_tx_buf_v2 cause issues for other links in the LGR?

It looks like the same zeroing pattern happens in the error path of
smc_wr_tx_process_cqe():

net/smc/smc_wr.c:smc_wr_tx_process_cqe() {
    ...
    if (unlikely(wc->status)) {
        ...
        if (link->lgr->smc_version == SMC_V2) {
            memset(link->wr_tx_v2_pend, 0, sizeof(*link->wr_tx_v2_pend));
            memset(link->lgr->wr_tx_buf_v2, 0, sizeof(*link->lgr->wr_tx_buf_v2));
        }
    }
}

If Link B is actively formatting an LLC message in this shared LGR buffer,
and a normal data work request concurrently fails on Link A, would Link A's
tasklet incorrectly wipe out Link B's in-progress LLC message?

>  		return 1;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716113745.65234-1-alibuda@linux.alibaba.com?part=1