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

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

Pre-existing issues:
- [High] Non-atomic clearing of `wr_tx_v2_pend` before `wr_tx_buf_v2` creates a TOCTOU race that prematurely signals slot availability, corrupting concurrent V2 messages and control data.
- [High] An error completion for a V1 work request unconditionally zeroes out the link group-wide V2 buffer, corrupting concurrent V2 messages on other links.
--

--- Patch [1]: [PATCH net-next v1 1/2] net/smc: clear the correct v2 slot and buffer in smc_wr_tx_put_slot() ---
commit 4fe2881d4ba7f681ee22c596087bf040cfe5c6f0
Author: D. Wythe <[email protected]>

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

This patch fixes an issue where 8-byte pointer variables were being zeroed
instead of the structures they point to. It passes the pointers directly
and uses sizeof(*pointer) to properly clear the intended structures.

> diff --git a/net/smc/smc_wr.c b/net/smc/smc_wr.c
> --- 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->lgr->wr_tx_buf_v2, 0,
> -		       sizeof(link->lgr->wr_tx_buf_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));

[Severity: High]
This isn't a bug introduced by this patch, but it looks like this lockless
synchronization pattern might have a race condition.

Because memset() is not atomic, could this zero out the state tracking struct
(which contains idx) before the data buffer is fully cleared?

If a concurrent thread calls smc_wr_tx_get_v2_slot():

    if (link->wr_tx_v2_pend->idx == link->wr_tx_cnt)
        return -EBUSY;

It checks the idx to locklessly determine slot availability. If idx gets
zeroed early by the first memset(), could the concurrent thread claim the
slot and write new data, only for it to be destroyed by the remainder of
the memset() operations?

For V1 buffers, this appears to be safely handled using a dedicated
link->wr_tx_mask bitmask that is cleared only after the memsets.

[Severity: High]
This is another pre-existing issue in the same file, but could the error
logic in smc_wr_tx_process_cqe() corrupt shared V2 payloads?

smc_wr_tx_process_cqe() {
    ...
    if (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 a V1 work request completes with an error, the error path unconditionally
zeroes out link->lgr->wr_tx_buf_v2 just because the link group version is
SMC_V2.

Since this buffer is shared across all links within the link group, wouldn't
zeroing it out from a failing link silently corrupt the payload of any
concurrent V2 message on healthy, surviving links in the same group?

>  		return 1;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1