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

[Chengfeng Ye]

On Wed, Sep 30, 2026 at 8:38 AM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> 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

The issues (incomplete fix and comment consistency) identified are
addressed in v2
https://lore.kernel.org/netdev/179105277608.1402591.16173087244761956438@xxxxxxxxxx/T/#t

Best regards,
Chengfeng