Re: [PATCH net] net/smc: Serialize early link group cleanup with termination

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 1 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch covers only one of the two callers of
  smc_lgr_cleanup_early().
- [Medium] Early cleanup can free the link group while a
  smc_lgr_free_work() callback that has already started is still using…

Pre-existing issues:
- [Low] In __smc_lgr_terminate() (net/smc/smc_core.c:1573-1574), the
  comment says "cancel free_work sync, will terminate when lgr->freeing…

--

net/smc: Serialize early link group cleanup with termination

This commit makes smc_lgr_cleanup_early() check and claim lgr->freeing
under lgr_lock before it calls __smc_lgr_terminate(). This stops early
cleanup from racing with the other teardown paths. It also makes
smc_conn_abort() hold a temporary link group reference across
smc_conn_free() and early cleanup.

> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index e9f93b3ab435b..97b0a0b510664 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -1011,12 +1011,16 @@ static void smc_conn_abort(struct smc_sock *smc, int local_first)
>  	struct smc_link_group *lgr = conn->lgr;
>  	bool lgr_valid = false;
>  
> -	if (smc_conn_lgr_valid(conn))
> +	if (local_first && smc_conn_lgr_valid(conn)) {
>  		lgr_valid = true;
> +		smc_lgr_hold(lgr);
> +	}

[Severity: High]
This adds a reference for the smc_conn_abort() caller, because the new
lgr->freeing check needs the link group to still be allocated. Does the
other caller of smc_lgr_cleanup_early() need the same protection?

net/smc/smc_core.c:smc_conn_create() {
    ...
		write_lock_bh(&lgr->conns_lock);
		rc = smc_lgr_register_conn(conn, true);
		write_unlock_bh(&lgr->conns_lock);
		if (rc) {
			smc_lgr_cleanup_early(lgr);
			goto out;
		}
	}
	smc_lgr_hold(conn->lgr); /* lgr_put in smc_conn_free() */
    ...
}

The hold is only taken after registration succeeds. On the failure path,
smc_lgr_cleanup_early() runs without a reference of its own.

For SMC-R first contact, smc_lgr_register_conn() fails through
smcr_lgr_conn_assign_link() only when the new link is no longer
SMC_LNK_ACTIVATING. As far as I can tell, that happens only when
__smcr_link_clear() resets the link inside a teardown's smc_lgr_free(). That
teardown drops the link's lgr reference and then the initial one. For
example:

smc_smcr_terminate_all(NULL)   /* reboot notifier via smc_lgrs_shutdown() */
    list_splice_init(&smc_lgr_list.list, &lgr_free_list);
    lgr->freeing = 1;
    __smc_lgr_terminate(lgr, false)
        smc_lgr_free()
            smcr_link_clear()->__smcr_link_clear()  /* link UNUSED, lgr put */
            smc_lgr_put(lgr); /* theoretically last lgr_put */
                __smc_lgr_free()->kfree(lgr)

The connecting thread (connect() or the smc_hs_wq listen worker) then sees
registration fail and calls smc_lgr_cleanup_early() on the freed lgr. There,
smc_lgr_list_head() reads lgr->is_smcd and the new check reads
lgr->freeing.

The commit message says:

    The extra reference keeps the group alive until early cleanup has
    checked teardown ownership.

The smc_conn_create() caller has the same lifetime requirement. Is a
use-after-free from early cleanup racing with termination still reachable
through that path? The window is narrow. It looks like it needs a teardown
such as a reboot to run during an SMC-R first-contact handshake.

>  
>  	smc_conn_free(conn);
> -	if (local_first && lgr_valid)
> +	if (lgr_valid) {
>  		smc_lgr_cleanup_early(lgr);
> +		smc_lgr_put(lgr);
> +	}
>  }
>  
>  /* check if there is a rdma device available for this connection. */
> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 9974149659c2f..b166a379753c7 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -688,6 +688,11 @@ void smc_lgr_cleanup_early(struct smc_link_group *lgr)
>  
>  	smc_lgr_list_head(lgr, &lgr_lock);
>  	spin_lock_bh(lgr_lock);
> +	if (lgr->freeing) {
> +		spin_unlock_bh(lgr_lock);
> +		return;
> +	}
> +	lgr->freeing = 1;

[Severity: Medium]
Can early cleanup free the link group while a smc_lgr_free_work() callback
that has already started is still using it?

After freeing is claimed here, __smc_lgr_terminate() only does a
non-synchronous cancel:

net/smc/smc_core.c:__smc_lgr_terminate() {
    ...
	/* cancel free_work sync, will terminate when lgr->freeing is set */
	cancel_delayed_work(&lgr->free_work);
	lgr->terminating = 1;
    ...
}

free_work is queued on system_percpu_wq, not lgr->tx_wq. So
destroy_workqueue(lgr->tx_wq) in smc_lgr_free() does not flush it. The
callback also holds no lgr reference.

Suppose the callback has started but has not yet reached the freeing check:

net/smc/smc_core.c:smc_lgr_free_work() {
    ...
	smc_lgr_list_head(lgr, &lgr_lock);
	spin_lock_bh(lgr_lock);
	if (lgr->freeing) {
    ...
}

Teardown can then run to completion. smc_lgr_free() drops the initial
reference, and the new smc_lgr_put() in smc_conn_abort() drops the last
one. __smc_lgr_free() then frees the lgr and its embedded free_work, and
the callback goes on to dereference freed memory.

The commit message says:

    This also prevents early cleanup from racing with the delayed free
    worker

It looks like the freeing flag only prevents a second teardown while the
object is still allocated. It does not keep the lgr alive for a worker that
lost the race. Is the commit message accurate here?

To trigger this, the SMC_LGR_FREE_DELAY_* timer has to expire during the
creator's abort. That looks like it needs an unusual stall or a jiffies
jump.

[Severity: Low]
This isn't a bug introduced by this patch, but the comment in
__smc_lgr_terminate() quoted above says "cancel free_work sync". The code
calls cancel_delayed_work(), which does not wait for a callback that is
already running.

What protects a pending smc_lgr_free_work() is its own re-check of
lgr->freeing under lgr_lock, not a synchronous cancel. Early cleanup now
reaches this code after claiming freeing. Could the comment be updated to
match the code?

>  	/* do not use this link group for new connections */
>  	if (!list_empty(&lgr->list))
>  		list_del_init(&lgr->list);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927063607.3691520-1-nicoyip.dev%40gmail.com