Re: [PATCH net v6 3/3] net/smc: carry oversized SMC-Rv2 LLC messages in the queue entry

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

Pre-existing issues:
- [High] Data race due to lockless modifications of rtoken state in the remote LLC flow.
- [High] Hardware-level data corruption due to sharing a single DMA spillover buffer across all receive WQEs.
--

--- Patch [3]: [PATCH net v6 3/3] net/smc: carry oversized SMC-Rv2 LLC messages in the queue entry ---
commit 189076dfcd2735a0769a31ecfff64742ead50fcf
Author: Yehyeong Lee <[email protected]>

net/smc: carry oversized SMC-Rv2 LLC messages in the queue entry

This commit updates SMC-Rv2 to copy the tail of oversized LLC messages
into the queue entry itself instead of relying on a shared receive buffer
bound by the arrival time. This allows operations like ADD_LINK and
DELETE_RKEY_V2 to process all declared rkeys correctly without being limited
by the buffer size.

Cc: [email protected]

> diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
> --- a/net/smc/smc_llc.c
> +++ b/net/smc/smc_llc.c

[ ... ]

> @@ -1819,27 +1835,28 @@ static void smc_llc_rmt_delete_rkey(struct smc_link_group *lgr)
>  		max = min_t(u32, max, SMC_LLC_DEL_RKEY_V2_INLINE +
>  				      qentry->body_len / sizeof(__be32));
>  		for (i = 0; i < max; i++) {
> -			if (smc_rtoken_delete(link, llcv2->rkey[i]))
> +			__be32 rkey;
> +
> +			if (i < SMC_LLC_DEL_RKEY_V2_INLINE)
> +				rkey = llcv2->rkey[i];
> +			else
> +				memcpy(&rkey, qentry->body +
> +					      (i - SMC_LLC_DEL_RKEY_V2_INLINE) *
> +					      sizeof(rkey), sizeof(rkey));
> +			if (smc_rtoken_delete(link, rkey))

[Severity: High]
This isn't a bug introduced by this patch, but does calling
smc_rtoken_delete() here introduce a data race?

Looking at smc_rtoken_delete() in net/smc/smc_core.c, it zeroes the
lgr->rtokens array and calls clear_bit() on lgr->rtokens_used_mask without
holding the lgr->rmbs_lock.

Concurrently, another thread executing the local flow like
smc_llc_srv_add_link() -> smc_rtoken_set() will acquire
down_write(&lgr->rmbs_lock) to modify these exact same structures.

Could this lack of locking in the remote flow lead to rtoken state
corruption?

>  				llcv2->num_inval_rkeys++;
>  		}

[ ... ]

> @@ -2081,18 +2098,52 @@ static void smc_llc_rx_response(struct smc_link *link,
>  	qentry->link = link;
>  	INIT_LIST_HEAD(&qentry->list);
>  	memcpy(&qentry->msg, llc, sizeof(union smc_llc_msg));
> +	if (body_len) {
> +		u8 *src = smc_link_shared_v2_rxbuf(link) ?
> +			(u8 *)lgr->wr_rx_buf_v2 : (u8 *)llc;
> +
> +		memcpy(qentry->body, src + SMC_WR_TX_SIZE, body_len);
> +	}

[Severity: High]
This is a pre-existing issue, but is it safe to copy from this shared buffer
if multiple oversized messages arrive back-to-back?

In smc_wr_init_sge() (in net/smc/smc_wr.c), every receive WQE has its second
SGE mapped to the exact same shared DMA address (lnk->wr_rx_v2_dma_addr +
SMC_WR_TX_SIZE).

Because the hardware HCA processes incoming packets at wire speed
independently of CPU polling, if a remote peer sends multiple oversized LLC
messages (like back-to-back DELETE_RKEY_V2 messages), won't the hardware DMA
the tails of both messages into this exact same shared spillover buffer?

Could this cause the earlier message's tail to be overwritten before this
memcpy executes, leading to state corruption from reading incorrect body data?

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