Re: [PATCH net v7 2/3] net/smc: bound the peer rkey counts in SMC-Rv2 LLC messages

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Data Race and DMA Buffer Overwrite in Shared Rx Buffer
- [High] Incomplete Bounds Check allows Stale Data Read
- [High] Functional Protocol Breakage and Silent Truncation for max_recv_sge == 1
- [High] Concurrent modification of lgr->rtokens without holding rmbs_lock
- [High] DMA Cache Coherency and API Violation
--

--- Patch [2]: [PATCH net v7 2/3] net/smc: bound the peer rkey counts in SMC-Rv2 LLC messages ---
commit d101a926b1d8bf4e6a72617cd433aee546b7917f
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

net/smc: bound the peer rkey counts in SMC-Rv2 LLC messages

This commit bounds the remote key count loops in SMC-Rv2 LLC message
processing by the size of the buffer being read. It also prevents reading
past the inline message bytes when there is no shared v2 receive buffer.

> diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
> index 7282f8ad7593d..f04e0392ec96c 100644
> --- a/net/smc/smc_llc.c
> +++ b/net/smc/smc_llc.c
> @@ -1000,13 +1000,21 @@ static void smc_llc_save_add_link_rkeys(struct smc_link *link,
>  					struct smc_link *link_new,
>  					u8 *llc_msg)
>  {
> +	const u32 rt_off = offsetof(struct smc_llc_msg_add_link_v2_ext, rt);
>  	struct smc_llc_msg_add_link_v2_ext *ext;
>  	struct smc_link_group *lgr = link->lgr;
>  	int max, i;
>  
> +	/* Without a shared v2 receive buffer the extension is not copied
> +	 * into the queue entry, so not even ext->num_rkeys is there.
> +	 */
> +	if (!smc_link_shared_v2_rxbuf(link))
> +		return;

[Severity: High]
This isn't a bug introduced by this patch, but does this early return cause a
silent protocol failure?

By skipping the processing of the peer's new remote memory region keys, it
seems this could lead to persistent connection failures for SMC-Rv2 links when
max_recv_sge == 1. The underlying issue appears to be that smc_llc_enqueue()
only copies the 44-byte inline header and discards the extension.

>  	ext = (struct smc_llc_msg_add_link_v2_ext *)(llc_msg +
>  						     SMC_WR_TX_SIZE);

[Severity: Critical]
This is a pre-existing issue, but can reading from the asynchronously modified
shared receive buffer cause memory corruption?

If a peer sends multiple LLC messages with extensions back-to-back, the
hardware might DMA a new incoming extension over the existing one before it is
processed in llc_event_work. Could this lead to reading overwritten state?

>  	max = min_t(u8, ext->num_rkeys, SMC_LLC_RKEYS_PER_MSG_V2);
> +	max = min_t(u32, max, (SMC_WR_BUF_V2_SIZE - SMC_WR_TX_SIZE - rt_off) /
> +			      sizeof(ext->rt[0]));

[Severity: High]
This isn't a bug introduced by this patch, but does using the statically
allocated maximum buffer size here, rather than the dynamically received
message length, permit stale data reads?

If a peer sends a short packet but specifies a large num_rkeys, the bounds
check might allow the loop to iterate through stale data left over from
previous messages.

>  	down_write(&lgr->rmbs_lock);
>  	for (i = 0; i < max; i++) {
>  		smc_rtoken_set(lgr, link->link_idx, link_new->link_idx,

[ ... ]

> @@ -1811,17 +1819,25 @@ static void smc_llc_rmt_delete_rkey(struct smc_link_group *lgr)
>  	link = qentry->link;
>  
>  	if (lgr->smc_version == SMC_V2) {
> +		const u32 rkey_off =
> +			offsetof(struct smc_llc_msg_delete_rkey_v2, rkey);
>  		struct smc_llc_msg_delete_rkey_v2 *llcv2;
> +		u32 buf_len;
>  
>  		if (smc_link_shared_v2_rxbuf(link)) {
>  			memcpy(lgr->wr_rx_buf_v2, llc, sizeof(*llc));

[Severity: High]
This isn't a bug introduced by this patch, but does this explicit memcpy to a
DMA_FROM_DEVICE mapped buffer violate the DMA API?

Writing directly to this buffer could dirty the cache line shared with the
hardware's concurrent DMA of the message extension, which might cause the CPU
cache writeback to clobber the incoming hardware DMA.

>  			llcv2 = (struct smc_llc_msg_delete_rkey_v2 *)lgr->wr_rx_buf_v2;

[Severity: Critical]
This is a pre-existing issue, but similar to the addition flow above, can
reading directly from the shared receive buffer here result in data races
if the hardware performs a DMA overwrite before the extension is fully
processed?

> +			buf_len = SMC_WR_BUF_V2_SIZE;
>  		} else {
>  			llcv2 = (struct smc_llc_msg_delete_rkey_v2 *)llc;
> +			buf_len = sizeof(qentry->msg);
>  		}
>  		llcv2->num_inval_rkeys = 0;
>  
>  		max = min_t(u8, llcv2->num_rkeys, SMC_LLC_RKEYS_PER_MSG_V2);
> +		/* bound by the buffer llcv2 points at */
> +		max = min_t(u32, max, (buf_len - rkey_off) /
> +				      sizeof(llcv2->rkey[0]));

[Severity: High]
This is a pre-existing issue, but could bounding against the static buffer
capacity rather than the actual packet length cause stale data to be processed
if the packet length is smaller than the capacity?

>  		for (i = 0; i < max; i++) {
>  			if (smc_rtoken_delete(link, llcv2->rkey[i]))

[Severity: High]
This is a pre-existing issue, but does calling smc_rtoken_delete() here
modify the shared token structures without holding the required rmbs_lock?

In smc_llc_save_add_link_rkeys(), modifications to the tokens are protected by
down_write(&lgr->rmbs_lock). Modifying them concurrently without the lock could
potentially corrupt the rtokens list.

>  				llcv2->num_inval_rkeys++;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819023306.644849-1-yhlee@isslab.korea.ac.kr?part=2