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

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

Pre-existing issues:
- [High] Remote DoS via memory leak of dynamically allocated `qentry` during LLC flow termination
--

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

This commit modifies how oversized SMC-Rv2 LLC messages are handled by
copying the tail of the message directly into the queue entry. It avoids
rebuilding the message layout in multiple consumers and bounds the
DELETE_RKEY_V2 processing by the actual message length that arrived.

> diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
> index c9bf938f1ee2a..ba60cc0f1690b 100644
> --- a/net/smc/smc_llc.c
> +++ b/net/smc/smc_llc.c

[ ... ]

> @@ -2083,18 +2098,35 @@ static void smc_llc_rx_response(struct smc_link *link,
>  	wake_up(&link->lgr->llc_msg_waiter);
>  }

[Severity: High]
This is a pre-existing issue, but can this leak dynamically allocated qentry
objects?

If a peer sends a duplicate or delayed LLC response (like SMC_LLC_ADD_LINK)
while the local LLC flow is still active but after the local node has already
cleared flow->qentry, smc_llc_rx_response() will assign the newly allocated
qentry to flow->qentry via smc_llc_flow_qentry_set() without taking a lock:

net/smc/smc_llc.c:smc_llc_rx_response() {
    ...
assign:
    /* assign responses to the local flow, we requested them */
    smc_llc_flow_qentry_set(&link->lgr->llc_flow_lcl, qentry);
    wake_up(&link->lgr->llc_msg_waiter);
}

When the flow eventually terminates, smc_llc_flow_stop() clears the flow:

net/smc/smc_llc.c:smc_llc_flow_stop() {
    ...
    spin_lock_bh(&lgr->llc_flow_lock);
    memset(flow, 0, sizeof(*flow));
    ...
}

This memset() erases the flow->qentry pointer without calling kfree(),
permanently leaking the buffer. Could this allow a remote peer to cause OOM
by intentionally sending delayed or duplicate responses?

> -static void smc_llc_enqueue(struct smc_link *link, union smc_llc_msg *llc)
> +static void smc_llc_enqueue(struct smc_link *link, union smc_llc_msg *llc,
> +			    u32 byte_len)
>  {
>  	struct smc_link_group *lgr = link->lgr;
>  	struct smc_llc_qentry *qentry;
>  	unsigned long flags;
> +	u16 body_len = 0;
>  
> -	qentry = kmalloc_obj(*qentry, GFP_ATOMIC);
> +	/* V2 messages can be longer than the inline union smc_llc_msg. Carry
> +	 * the remainder in the qentry itself, so that its lifetime and its
> +	 * length match the message the peer actually sent.
> +	 */

[ ... ]

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