Re: [PATCH net] net/smc: free pending qentry in smc_llc_flow_stop() before memset

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

Pre-existing issues:
- [High] Incomplete fix for memory leak of late-arriving LLC messages due to a TOCTOU data race.
- [High] Preexisting memory leak in `smc_llc_lgr_clear()` where canceled asynchronous flow tasks leak dynamically allocated `qentry` objects.
--

--- Patch [1]: [PATCH net] net/smc: free pending qentry in smc_llc_flow_stop() before memset ---
commit 4bed80c54874ec213bfc62c4cef2f581d5c51aa6
Author: Mahanta Jambigi <[email protected]>

net/smc: free pending qentry in smc_llc_flow_stop() before memset

This commit addresses a memory leak by freeing the flow->qentry pointer inside
smc_llc_flow_stop() before the flow structure is cleared with memset.

> 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
> @@ -320,6 +320,7 @@ int smc_llc_flow_initiate(struct smc_link_group *lgr,
>  void smc_llc_flow_stop(struct smc_link_group *lgr, struct smc_llc_flow *flow)
>  {
>  	spin_lock_bh(&lgr->llc_flow_lock);
> +	smc_llc_flow_qentry_del(flow);
>  	memset(flow, 0, sizeof(*flow));

[Severity: High]
This isn't a bug introduced by this patch, but does this fix fully resolve
the leak?

Looking at smc_llc_rx_response() and smc_llc_event_handler(), they appear
to read flow->type and write to flow->qentry locklessly:

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

Could a remote peer sending duplicate or late-arriving messages cause
smc_llc_rx_response() on one CPU to locklessly assign a new qentry to
flow->qentry either during or immediately after the memset() executes here?

If that happens, wouldn't the newly assigned qentry either be zeroed out
or left dangling and overwritten when the next flow starts, resulting in
another leak?

>  	flow->type = SMC_LLC_FLOW_NONE;
>  	spin_unlock_bh(&lgr->llc_flow_lock);

[Severity: High]
This is a pre-existing issue, but does smc_llc_lgr_clear() also leak these
dynamically allocated qentry objects?

When an LLC flow starts, a qentry is stored in lgr->llc_flow_lcl.qentry or
lgr->llc_flow_rmt.qentry. If the link group is suddenly terminated,
smc_llc_lgr_clear() cancels the background tasks:

net/smc/smc_llc.c:smc_llc_lgr_clear() {
    ...
    cancel_work_sync(&lgr->llc_event_work);
    cancel_work_sync(&lgr->llc_add_link_work);
    cancel_work_sync(&lgr->llc_del_link_work);
    if (lgr->delayed_event) {
        kfree(lgr->delayed_event);
        lgr->delayed_event = NULL;
    }
    ...
}

Since these canceled works never process or free the stored qentries, and
smc_llc_lgr_clear() only frees lgr->delayed_event, will
lgr->llc_flow_lcl.qentry and lgr->llc_flow_rmt.qentry be permanently leaked
when the link group is freed?

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