[PATCH net] net/smc: serialize link group free work scheduling

[PATCH net] net/smc: serialize link group free work scheduling

[Chengfeng Ye]

smc_lgr_schedule_free_work() checks lgr->freeing without the link group
list lock held by the teardown paths. A concurrent smc_lgr_free_work()
can set freeing and cancel the delayed work between that check and
mod_delayed_work(), leaving a timer pending on a freed link group:

  CPU 0 (smc_conn_free)            CPU 1 (smc_lgr_free_work)
  observe lgr->freeing == 0
                                  set lgr->freeing under lgr_lock
                                  cancel_delayed_work()
                                  smc_lgr_free(): drop lgr reference
  mod_delayed_work()
  smc_lgr_put(): drop last reference and free lgr

When the timer expires, the timer core accesses the freed memory.
KASAN reported:

  BUG: KASAN: use-after-free in __run_timers+0x86d/0x8d0
  Write of size 8 at addr ffff8881186902a8 by task swapper/2/0
  Call Trace:
   <IRQ>
   __run_timers+0x86d/0x8d0
   timer_expire_remote+0xd3/0x120
   tmigr_handle_remote_up+0x4f4/0xab0
   __walk_groups_from+0x40/0x150
   tmigr_handle_remote+0x229/0x2c0
   run_timer_softirq+0x1f5/0x250
   handle_softirqs+0x18d/0x5b0
   </IRQ>

Hold the existing link group list lock across the freeing check and
mod_delayed_work(). Rearming then either precedes the freeing transition
and is caught by the subsequent cancellation, or sees freeing set and
is skipped. smc_conn_free() retains its link group reference until after
scheduling, keeping the group alive while acquiring the lock.

Fixes: 8e316b9e7260 ("net/smc: improve link group freeing")
Cc: stable@vger.kernel.org
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
 net/smc/smc_core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 9974149659c2..8feb486ada56 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -80,6 +80,10 @@ static void smc_ibdev_cnt_dec(struct smc_link *lnk)
 
 static void smc_lgr_schedule_free_work(struct smc_link_group *lgr)
 {
+	spinlock_t *lgr_lock; /* protects lgr->freeing */
+
+	smc_lgr_list_head(lgr, &lgr_lock);
+	spin_lock_bh(lgr_lock);
 	/* client link group creation always follows the server link group
 	 * creation. For client use a somewhat higher removal delay time,
 	 * otherwise there is a risk of out-of-sync link groups.
@@ -90,6 +94,7 @@ static void smc_lgr_schedule_free_work(struct smc_link_group *lgr)
 						SMC_LGR_FREE_DELAY_CLNT :
 						SMC_LGR_FREE_DELAY_SERV);
 	}
+	spin_unlock_bh(lgr_lock);
 }
 
 /* Register connection's alert token in our lookup structure.
-- 
2.43.0

Re: [PATCH net] net/smc: serialize link group free work scheduling

[sashiko-bot]

> smc_lgr_schedule_free_work() checks lgr->freeing without the link group
> list lock held by the teardown paths. A concurrent smc_lgr_free_work()
> can set freeing and cancel the delayed work between that check and
> mod_delayed_work(), leaving a timer pending on a freed link group:
> 
>   CPU 0 (smc_conn_free)            CPU 1 (smc_lgr_free_work)
>   observe lgr->freeing == 0
>                                   set lgr->freeing under lgr_lock
>                                   cancel_delayed_work()
>                                   smc_lgr_free(): drop lgr reference
>   mod_delayed_work()
>   smc_lgr_put(): drop last reference and free lgr
> 
> When the timer expires, the timer core accesses the freed memory.
> KASAN reported:
> [ ... ]
> scheduling, keeping the group alive while acquiring the lock.
> 
> Fixes: 8e316b9e7260 ("net/smc: improve link group freeing")
> Cc: stable@vger.kernel.org
> Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260927074520.3694663-1-nicoyip.dev@gmail.com?part=1

Re: [PATCH net] net/smc: serialize link group free work scheduling

[netdev-bot+sashiko]

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

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

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 fix is incomplete.
- [Medium] The patch treats lgr_lock as the lock for lgr->freeing (the
  new comment reads 'protects lgr->freeing').

Pre-existing issues:
- [High] Separately from the pending-timer rearm this patch fixes, a
  free_work callback that has already started can race with lgr teardown…

--

net/smc: serialize link group free work scheduling

This patch takes the link group list lock in smc_lgr_schedule_free_work()
around the lgr->freeing check and the mod_delayed_work() call. The goal is
to stop a concurrent smc_lgr_free_work() from setting freeing and cancelling
the work between the check and the rearm, which would leave a timer pending
on a freed link group.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 9974149659c2f..8feb486ada562 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -80,6 +80,10 @@ static void smc_ibdev_cnt_dec(struct smc_link *lnk)
>  
>  static void smc_lgr_schedule_free_work(struct smc_link_group *lgr)
>  {
> +	spinlock_t *lgr_lock; /* protects lgr->freeing */

[Severity: Medium]
Is lgr_lock enough to protect lgr->freeing? freeing is a 1-bit bitfield,
and it shares a u8 with sync_err and terminating:

net/smc/smc_core.h:struct smc_link_group {
	...
	u8			sync_err : 1;	/* lgr no longer fits to peer */
	u8			terminating : 1;/* lgr is terminating */
	u8			freeing : 1;	/* lgr is being freed */
	...
}

Both neighbouring bits are written without lgr_lock. One example is
smc_clc_wait_msg():

	smc->conn.lgr->sync_err = 1;
	smc_lgr_terminate_sched(smc->conn.lgr);

Another is __smc_lgr_terminate():

	lgr->terminating = 1;

Each of these stores is a read-modify-write of the whole byte.
smc_smcd_terminate() and smc_lgr_free_work() set freeing = 1 under
lgr_lock. Can a concurrent sync_err or terminating store write back a stale
byte and lose that freeing = 1?

In the sync_err case, smc_lgr_terminate_sched() then returns early because
the lgr is already unlinked, so freeing is never set again.

Consider a later smc_conn_free() that passed its unlocked list_empty()
check before the unlink. Once __smc_lgr_terminate() has cancelled
free_work, that call could reach smc_lgr_schedule_free_work() with
conns_num == 0. It would read freeing == 0 under the new lock and rearm the
timer on an lgr that is about to be freed.

Would it work better to give freeing its own storage (for example a bool),
or to make every writer of that byte hold lgr_lock?

> +
> +	smc_lgr_list_head(lgr, &lgr_lock);
> +	spin_lock_bh(lgr_lock);
>  	/* client link group creation always follows the server link group
>  	 * creation. For client use a somewhat higher removal delay time,
>  	 * otherwise there is a risk of out-of-sync link groups.
> @@ -90,6 +94,7 @@ static void smc_lgr_schedule_free_work(struct smc_link_group *lgr)
>  						SMC_LGR_FREE_DELAY_CLNT :
>  						SMC_LGR_FREE_DELAY_SERV);
>  	}
> +	spin_unlock_bh(lgr_lock);
>  }

[Severity: High]
Does this fully close the rearm race? The commit message says a rearm
"either precedes the freeing transition and is caught by the subsequent
cancellation, or sees freeing set and is skipped". That only holds if every
teardown path sets lgr->freeing under lgr_lock before it cancels free_work.

smc_lgr_cleanup_early() unlinks the lgr under lgr_lock but never sets
freeing:

net/smc/smc_core.c:smc_lgr_cleanup_early() {
	...
	spin_lock_bh(lgr_lock);
	/* do not use this link group for new connections */
	if (!list_empty(&lgr->list))
		list_del_init(&lgr->list);
	spin_unlock_bh(lgr_lock);
	__smc_lgr_terminate(lgr, true);
}

__smc_lgr_terminate() then cancels only once, outside the lock. It sets
terminating, not freeing:

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

On an SMC-D server, smc_listen_work() drops smc_server_lgr_pending after
it sends the ACCEPT. A second connection (conn2) from the same peer can
then join the first-contact lgr.

If the first connection then fails (no CONFIRM, or a DECLINE), teardown
goes through:

smc_listen_work()->smc_listen_decline()->smc_conn_abort()->
    smc_lgr_cleanup_early()->__smc_lgr_terminate()

Meanwhile conn2 can be freed on another CPU:

CPU0 (conn1 abort)                CPU1 (smc_conn_free(conn2))
                                  !list_empty(&lgr->list) is true
                                  smc_lgr_unregister_conn(conn2)
smc_lgr_cleanup_early()
  list_del_init(&lgr->list)
  __smc_lgr_terminate()
    cancel_delayed_work()
    lgr->terminating = 1
                                  lgr->conns_num == 0
                                  smc_lgr_schedule_free_work()
                                    spin_lock_bh(lgr_lock)
                                    lgr->freeing == 0
                                    mod_delayed_work()
    smc_lgr_free()
                                  smc_lgr_put() drops last reference

This would leave free_work's timer pending on a kfree'd lgr, which is the
same __run_timers use-after-free as in the KASAN report.

Should smc_lgr_cleanup_early() set lgr->freeing under lgr_lock? Or should
smc_lgr_schedule_free_work() also skip the rearm when the lgr is unlinked
or terminating?

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Can a
smc_lgr_free_work() callback that has already started running race with
lgr teardown?

smc_lgr_free_work() holds no reference on the lgr and dereferences it
straight away:

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

Suppose the idle timer fires and the worker starts, but is delayed before
this point (by preemption, or on PREEMPT_RT). A teardown can then set
freeing and reach __smc_lgr_terminate(). Examples are smc_smcd_terminate()
from an ISM peer shutdown event in smcd_handle_sw_event(),
smc_lgr_terminate_sched(), and smc_smcd_terminate_all() or
smc_smcr_terminate_all(). The only cancellation there is:

	cancel_delayed_work(&lgr->free_work);

That call neither cancels nor waits for a callback that is already
running.

With no connections left, smc_lgr_free() destroys lgr->tx_wq. free_work is
on system_percpu_wq, so this does not drain it. smc_lgr_put() then calls
__smc_lgr_free(), which frees the lgr.

When the worker resumes, it takes spin_lock_bh() on a lock pointer loaded
from freed memory. If the stale freeing bit reads 0, it also calls
list_del_init() and smc_lgr_free() a second time.

Should teardown use cancel_delayed_work_sync() where the context allows,
or should the queued work hold its own lgr reference?

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

Re: [PATCH net] net/smc: serialize link group free work scheduling

[Chengfeng Ye]

On Wed, Sep 30, 2026 at 8:45 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 (1) · Medium: 1 · Low: 0
>
> 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 fix is incomplete.
> - [Medium] The patch treats lgr_lock as the lock for lgr->freeing (the
>   new comment reads 'protects lgr->freeing').
>
> Pre-existing issues:
> - [High] Separately from the pending-timer rearm this patch fixes, a
>   free_work callback that has already started can race with lgr teardown…
>
> --
>
> net/smc: serialize link group free work scheduling
>
> This patch takes the link group list lock in smc_lgr_schedule_free_work()
> around the lgr->freeing check and the mod_delayed_work() call. The goal is
> to stop a concurrent smc_lgr_free_work() from setting freeing and cancelling
> the work between the check and the rearm, which would leave a timer pending
> on a freed link group.
>
> > diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> > index 9974149659c2f..8feb486ada562 100644
> > --- a/net/smc/smc_core.c
> > +++ b/net/smc/smc_core.c
> > @@ -80,6 +80,10 @@ static void smc_ibdev_cnt_dec(struct smc_link *lnk)
> >
> >  static void smc_lgr_schedule_free_work(struct smc_link_group *lgr)
> >  {
> > +     spinlock_t *lgr_lock; /* protects lgr->freeing */
>
> [Severity: Medium]
> Is lgr_lock enough to protect lgr->freeing? freeing is a 1-bit bitfield,
> and it shares a u8 with sync_err and terminating:
>
> net/smc/smc_core.h:struct smc_link_group {
>         ...
>         u8                      sync_err : 1;   /* lgr no longer fits to peer */
>         u8                      terminating : 1;/* lgr is terminating */
>         u8                      freeing : 1;    /* lgr is being freed */
>         ...
> }
>
> Both neighbouring bits are written without lgr_lock. One example is
> smc_clc_wait_msg():
>
>         smc->conn.lgr->sync_err = 1;
>         smc_lgr_terminate_sched(smc->conn.lgr);
>
> Another is __smc_lgr_terminate():
>
>         lgr->terminating = 1;
>
> Each of these stores is a read-modify-write of the whole byte.
> smc_smcd_terminate() and smc_lgr_free_work() set freeing = 1 under
> lgr_lock. Can a concurrent sync_err or terminating store write back a stale
> byte and lose that freeing = 1?
>
> In the sync_err case, smc_lgr_terminate_sched() then returns early because
> the lgr is already unlinked, so freeing is never set again.
>
> Consider a later smc_conn_free() that passed its unlocked list_empty()
> check before the unlink. Once __smc_lgr_terminate() has cancelled
> free_work, that call could reach smc_lgr_schedule_free_work() with
> conns_num == 0. It would read freeing == 0 under the new lock and rearm the
> timer on an lgr that is about to be freed.
>
> Would it work better to give freeing its own storage (for example a bool),
> or to make every writer of that byte hold lgr_lock?
>
> > +
> > +     smc_lgr_list_head(lgr, &lgr_lock);
> > +     spin_lock_bh(lgr_lock);
> >       /* client link group creation always follows the server link group
> >        * creation. For client use a somewhat higher removal delay time,
> >        * otherwise there is a risk of out-of-sync link groups.
> > @@ -90,6 +94,7 @@ static void smc_lgr_schedule_free_work(struct smc_link_group *lgr)
> >                                               SMC_LGR_FREE_DELAY_CLNT :
> >                                               SMC_LGR_FREE_DELAY_SERV);
> >       }
> > +     spin_unlock_bh(lgr_lock);
> >  }
>
> [Severity: High]
> Does this fully close the rearm race? The commit message says a rearm
> "either precedes the freeing transition and is caught by the subsequent
> cancellation, or sees freeing set and is skipped". That only holds if every
> teardown path sets lgr->freeing under lgr_lock before it cancels free_work.
>
> smc_lgr_cleanup_early() unlinks the lgr under lgr_lock but never sets
> freeing:
>
> net/smc/smc_core.c:smc_lgr_cleanup_early() {
>         ...
>         spin_lock_bh(lgr_lock);
>         /* do not use this link group for new connections */
>         if (!list_empty(&lgr->list))
>                 list_del_init(&lgr->list);
>         spin_unlock_bh(lgr_lock);
>         __smc_lgr_terminate(lgr, true);
> }
>
> __smc_lgr_terminate() then cancels only once, outside the lock. It sets
> terminating, not freeing:
>
> net/smc/smc_core.c:__smc_lgr_terminate() {
>         if (lgr->terminating)
>                 return; /* lgr already terminating */
>         /* cancel free_work sync, will terminate when lgr->freeing is set */
>         cancel_delayed_work(&lgr->free_work);
>         lgr->terminating = 1;
>         ...
>         smc_lgr_cleanup(lgr);
>         smc_lgr_free(lgr);
> }
>
> On an SMC-D server, smc_listen_work() drops smc_server_lgr_pending after
> it sends the ACCEPT. A second connection (conn2) from the same peer can
> then join the first-contact lgr.
>
> If the first connection then fails (no CONFIRM, or a DECLINE), teardown
> goes through:
>
> smc_listen_work()->smc_listen_decline()->smc_conn_abort()->
>     smc_lgr_cleanup_early()->__smc_lgr_terminate()
>
> Meanwhile conn2 can be freed on another CPU:
>
> CPU0 (conn1 abort)                CPU1 (smc_conn_free(conn2))
>                                   !list_empty(&lgr->list) is true
>                                   smc_lgr_unregister_conn(conn2)
> smc_lgr_cleanup_early()
>   list_del_init(&lgr->list)
>   __smc_lgr_terminate()
>     cancel_delayed_work()
>     lgr->terminating = 1
>                                   lgr->conns_num == 0
>                                   smc_lgr_schedule_free_work()
>                                     spin_lock_bh(lgr_lock)
>                                     lgr->freeing == 0
>                                     mod_delayed_work()
>     smc_lgr_free()
>                                   smc_lgr_put() drops last reference
>
> This would leave free_work's timer pending on a kfree'd lgr, which is the
> same __run_timers use-after-free as in the KASAN report.
>
> Should smc_lgr_cleanup_early() set lgr->freeing under lgr_lock? Or should
> smc_lgr_schedule_free_work() also skip the rearm when the lgr is unlinked
> or terminating?
>
> [Severity: High]
> This is a pre-existing issue and was not introduced by this patch. Can a
> smc_lgr_free_work() callback that has already started running race with
> lgr teardown?
>
> smc_lgr_free_work() holds no reference on the lgr and dereferences it
> straight away:
>
> net/smc/smc_core.c:smc_lgr_free_work() {
>         ...
>         smc_lgr_list_head(lgr, &lgr_lock);
>         spin_lock_bh(lgr_lock);
>         if (lgr->freeing) {
>         ...
> }
>
> Suppose the idle timer fires and the worker starts, but is delayed before
> this point (by preemption, or on PREEMPT_RT). A teardown can then set
> freeing and reach __smc_lgr_terminate(). Examples are smc_smcd_terminate()
> from an ISM peer shutdown event in smcd_handle_sw_event(),
> smc_lgr_terminate_sched(), and smc_smcd_terminate_all() or
> smc_smcr_terminate_all(). The only cancellation there is:
>
>         cancel_delayed_work(&lgr->free_work);
>
> That call neither cancels nor waits for a callback that is already
> running.
>
> With no connections left, smc_lgr_free() destroys lgr->tx_wq. free_work is
> on system_percpu_wq, so this does not drain it. smc_lgr_put() then calls
> __smc_lgr_free(), which frees the lgr.
>
> When the worker resumes, it takes spin_lock_bh() on a lock pointer loaded
> from freed memory. If the stale freeing bit reads 0, it also calls
> list_del_init() and smc_lgr_free() a second time.
>
> Should teardown use cancel_delayed_work_sync() where the context allows,
> or should the queued work hold its own lgr reference?
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927074520.3694663-1-nicoyip.dev%40gmail.com

v2 (in a patch series 1/2, fixed together with another issue net/smc:
Serialize early link group cleanup with termination) has just been
sent to address the issues reported by sashiko.
https://lore.kernel.org/netdev/179105277608.1402591.16173087244761956438@kernel.org/T/#m30bd40e302bc951b76e8507324f6457502e4512c.

Best regards,
Chengfeng