[PATCH net] net/smc: free stashed qentry before overwrite in REQ_ADD_LINK to ADD_LINK transition

[PATCH net] net/smc: free stashed qentry before overwrite in REQ_ADD_LINK to ADD_LINK transition

[Mahanta Jambigi]

When smc_llc_event_handler() transitions the local LLC flow from
SMC_LLC_FLOW_REQ_ADD_LINK to SMC_LLC_FLOW_ADD_LINK on arrival of an ADD_LINK
request, it calls smc_llc_flow_qentry_set() unconditionally:

	if (lgr->llc_flow_lcl.type == SMC_LLC_FLOW_REQ_ADD_LINK) {
		lgr->llc_flow_lcl.type = SMC_LLC_FLOW_ADD_LINK;
		smc_llc_flow_qentry_set(&lgr->llc_flow_lcl, qentry);
		...
	}

A CONFIRM_LINK or ADD_LINK_CONT arriving while flow->type is
SMC_LLC_FLOW_REQ_ADD_LINK is stashed into flow->qentry via the
SMC_LLC_CONFIRM_LINK / SMC_LLC_ADD_LINK_CONT handler (which stores into
flow->qentry for any non-NONE flow type).  When the subsequent ADD_LINK
arrives, the REQ_ADD_LINK branch overwrites flow->qentry with the new pointer
without first freeing the stashed allocation, leaking one kmalloc object.

The stashed entry has no consumer: smc_llc_wait() is only called from
llc_add_link_work, which is not yet scheduled while the flow type remains
REQ_ADD_LINK.  No waiter is sleeping on llc_msg_waiter at this point.
It is safe to unconditionally free any stashed qentry before
the overwrite.

Call smc_llc_flow_qentry_del() before smc_llc_flow_qentry_set() in the
REQ_ADD_LINK branch.  smc_llc_flow_qentry_del() already checks flow->qentry
before freeing, so the normal path where no entry is stashed is a no-op.

Fixes: b4ba4652b3f8 ("net/smc: extend LLC layer for SMC-Rv2")
Reviewed-by: Hidayath Khan <hidayath@linux.ibm.com>
Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
---
 net/smc/smc_llc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
index 07e2d7fef7b2..9cc4b3e97699 100644
--- a/net/smc/smc_llc.c
+++ b/net/smc/smc_llc.c
@@ -1911,6 +1911,8 @@ static void smc_llc_event_handler(struct smc_llc_qentry *qentry)
 			if (lgr->llc_flow_lcl.type ==
 					SMC_LLC_FLOW_REQ_ADD_LINK) {
 				/* server started add_link processing */
+				/* free any qentry stashed in REQ_ADD_LINK state */
+				smc_llc_flow_qentry_del(&lgr->llc_flow_lcl);
 				lgr->llc_flow_lcl.type = SMC_LLC_FLOW_ADD_LINK;
 				smc_llc_flow_qentry_set(&lgr->llc_flow_lcl,
 							qentry);
-- 
2.50.1

Re: [PATCH net] net/smc: free stashed qentry before overwrite in REQ_ADD_LINK to ADD_LINK transition

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Use-After-Free and Out-of-Bounds Read in `smc_llc_srv_add_link()` when accessing the `add_llc` payload.
- [High] `smc_llc_flow_stop()` overwrites `flow->qentry` with `memset`, unconditionally leaking any stashed LLC message.
- [High] Unsynchronized access to `lgr->llc_flow_lcl` across workqueue and tasklet contexts causes data races, leading to memory leaks and NULL pointer dereferences.
--

--- Patch [1]: [PATCH net] net/smc: free stashed qentry before overwrite in REQ_ADD_LINK to ADD_LINK transition ---
commit 5dc4ee5e350906dce47b3e2165e175388d750d51
Author: Mahanta Jambigi <mjambigi@linux.ibm.com>
Subject: net/smc: free stashed qentry before overwrite in REQ_ADD_LINK to ADD_LINK transition

When an ADD_LINK request arrives while the flow is in REQ_ADD_LINK state,
the event handler overwrites flow->qentry without freeing any stashed entry.
This leak is fixed by unconditionally calling smc_llc_flow_qentry_del()
before overwriting the qentry pointer.

> diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
> index aa6d83af55eda..c7f0222aff8d0 100644
> --- a/net/smc/smc_llc.c
> +++ b/net/smc/smc_llc.c
[ ... ]
> @@ -1911,6 +1911,8 @@ static void smc_llc_event_handler(struct smc_llc_qentry *qentry)
>  			if (lgr->llc_flow_lcl.type ==
>  					SMC_LLC_FLOW_REQ_ADD_LINK) {
>  				/* server started add_link processing */
> +				/* free any qentry stashed in REQ_ADD_LINK state */
> +				smc_llc_flow_qentry_del(&lgr->llc_flow_lcl);

[Severity: High]
This isn't a bug introduced by this patch, but doesn't this unsynchronized
access to lgr->llc_flow_lcl create a data race between workqueue and tasklet
contexts?

In smc_llc_event_handler() (workqueue context), we modify the flow type and
qentry without holding lgr->llc_flow_lock:

	smc_llc_flow_qentry_del(&lgr->llc_flow_lcl);
	lgr->llc_flow_lcl.type = SMC_LLC_FLOW_ADD_LINK;
	smc_llc_flow_qentry_set(&lgr->llc_flow_lcl, qentry);

If a softirq preempts this immediately after we zero qentry and set the type,
smc_llc_rx_response() could locklessly read the type and assign a new response
to the flow:

smc_llc_rx_response() {
    ...
    /* assign responses to the local flow, we requested them */
    smc_llc_flow_qentry_set(&link->lgr->llc_flow_lcl, qentry);
    ...
}

When the workqueue resumes, it would unconditionally overwrite qentry with its
own message, leaking the softirq's qentry. Also, if the type is changed
concurrently, smc_llc_add_link_work() might falsely trigger
smc_llc_flow_stop() mid-flow, zeroing the state and leading to a subsequent
NULL pointer dereference. Could this be prevented by holding the appropriate
locks?

>  				lgr->llc_flow_lcl.type = SMC_LLC_FLOW_ADD_LINK;
>  				smc_llc_flow_qentry_set(&lgr->llc_flow_lcl,
>  							qentry);

[Severity: High]
This is a pre-existing issue, but while we are looking at memory leaks
related to flow->qentry, does smc_llc_flow_stop() also unconditionally leak
any stashed message?

If an incoming message is stashed into lgr->llc_flow_lcl.qentry while the link
group is in the SMC_LLC_FLOW_REQ_ADD_LINK state, and then the remote peer
sends an SMC_LLC_REQ_ADD_LINK response, smc_llc_event_handler() handles it by
calling smc_llc_flow_stop():

smc_llc_event_handler() {
    ...
    case SMC_LLC_REQ_ADD_LINK:
        if (lgr->role == SMC_CLNT &&
            lgr->llc_flow_lcl.type == SMC_LLC_FLOW_REQ_ADD_LINK &&
            (llc->raw.hdr.flags & SMC_LLC_FLAG_RESP)) {
            smc_llc_flow_stop(link->lgr, &lgr->llc_flow_lcl);
    ...
}

smc_llc_flow_stop() then uses memset to clear the flow state:

smc_llc_flow_stop() {
    ...
    memset(flow, 0, sizeof(*flow));
    flow->type = SMC_LLC_FLOW_NONE;
    ...
}

This zero-initializes the struct, silently wiping the flow->qentry pointer
without freeing its dynamically allocated memory. Could this allow a remote
peer to cause memory exhaustion by sending a CONFIRM_LINK message followed
immediately by a REQ_ADD_LINK response?

[Severity: Critical]
This is another pre-existing issue, but does smc_llc_srv_add_link() have a
use-after-free and out-of-bounds read when accessing the add_llc payload?

In smc_llc_srv_add_link(), qentry is retrieved and explicitly freed via
smc_llc_flow_qentry_del():

smc_llc_srv_add_link() {
    ...
    smc_llc_flow_qentry_del(&lgr->llc_flow_lcl);
    ...
}

Later in the same function, if the connection uses SMC_V2 and the rxbuf is
not shared, add_llc is used as the llc_msg argument:

smc_llc_srv_add_link() {
    ...
    u8 *llc_msg = smc_link_shared_v2_rxbuf(link) ?
        (u8 *)lgr->wr_rx_buf_v2 : (u8 *)add_llc;
    smc_llc_save_add_link_rkeys(link, link_new, llc_msg);
    ...
}

Since add_llc is a pointer inside the previously freed qentry, doesn't this
result in reading from a freed memory allocation? Also, because qentry is
allocated with a fixed size of sizeof(*qentry) and does not hold the extended
V2 rkeys, wouldn't this also cause a heap out-of-bounds read past the slab
chunk boundary?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818073107.466506-1-mjambigi@linux.ibm.com?part=1

Re: [PATCH net] net/smc: free stashed qentry before overwrite in REQ_ADD_LINK to ADD_LINK transition

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 18 Aug 2026 09:31:07 +0200 you wrote:
> When smc_llc_event_handler() transitions the local LLC flow from
> SMC_LLC_FLOW_REQ_ADD_LINK to SMC_LLC_FLOW_ADD_LINK on arrival of an ADD_LINK
> request, it calls smc_llc_flow_qentry_set() unconditionally:
> 
> 	if (lgr->llc_flow_lcl.type == SMC_LLC_FLOW_REQ_ADD_LINK) {
> 		lgr->llc_flow_lcl.type = SMC_LLC_FLOW_ADD_LINK;
> 		smc_llc_flow_qentry_set(&lgr->llc_flow_lcl, qentry);
> 		...
> 	}
> 
> [...]

Here is the summary with links:
  - [net] net/smc: free stashed qentry before overwrite in REQ_ADD_LINK to ADD_LINK transition
    https://git.kernel.org/netdev/net/c/036322025d6e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html