[PATCH net v2 0/2] net/smc: fix link group teardown races

[PATCH net v2 0/2] net/smc: fix link group teardown races

[Chengfeng Ye]

These two fixes were posted separately, but both change the link group's
freeing protocol. Resend them together so scheduling and early cleanup
use the same locked teardown transition.

Patch 1 serializes the freeing check and delayed-work rearm with teardown.
It gives freeing its own storage and sets it under the list lock during
early cleanup, so another connection cannot rearm after cancellation.
It also corrects the cancellation comment: cancel_delayed_work() cancels
pending work but does not synchronize with an already-running callback.

Patch 2 excludes competing early teardown and pins both early-cleanup
callers through their ownership checks, including failed registration of
a new link group. Each patch retains its original KASAN evidence and
Fixes tag. The evidence comes from earlier instrumented runs.

The separate pre-existing race in which an already-running free_work
callback outlives the group is outside this series. The callback reference
and cancellation mechanisms are unchanged. The socket-lock versus
abort-work wait cycle also remains separate.

The series is based on net/main 71a77ab76e74. Local validation results are
recorded alongside the exported patches; no runtime test is claimed.

Chengfeng Ye (2):
  net/smc: serialize link group free work scheduling
  net/smc: serialize early link group cleanup with termination

 net/smc/af_smc.c   |  8 ++++++--
 net/smc/smc_core.c | 16 +++++++++++++++-
 net/smc/smc_core.h |  2 +-
 3 files changed, 22 insertions(+), 4 deletions(-)


base-commit: 71a77ab76e74131a101f4d2d2afb0dcbf81b4e3c
-- 
2.43.0

[PATCH net v2 1/2] 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.

  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(). Set freeing under that lock in early cleanup as well,
before __smc_lgr_terminate() cancels the work. An SMC-D server releases
smc_server_lgr_pending before receiving CONFIRM, so another connection can
reuse the group and race its removal with early cleanup.

Give freeing its own storage. The neighbouring sync_err and terminating
bitfields are written without the list lock; their read-modify-write
updates must not overwrite the freeing transition.

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
Assisted-by: GPT-6.1-Sol
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
Changes in v2:
- Give freeing its own storage so adjacent bitfield updates cannot
  overwrite the list-lock-protected transition.
- Set freeing under the list lock during early cleanup to prevent a
  concurrent SMC-D connection from rearming after cancellation.
- Keep asynchronous cancellation without adding work-owned references.
- Send with the early-cleanup ownership fix as an ordered two-patch series.

v1: https://lore.kernel.org/r/20260927074520.3694663-1-nicoyip.dev@gmail.com/

 net/smc/smc_core.c | 8 +++++++-
 net/smc/smc_core.h | 2 +-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 9974149659c2..478c8bc2759a 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.
@@ -691,6 +696,7 @@ void smc_lgr_cleanup_early(struct smc_link_group *lgr)
 	/* do not use this link group for new connections */
 	if (!list_empty(&lgr->list))
 		list_del_init(&lgr->list);
+	lgr->freeing = 1;
 	spin_unlock_bh(lgr_lock);
 	__smc_lgr_terminate(lgr, true);
 }
@@ -1565,7 +1571,7 @@ static void __smc_lgr_terminate(struct smc_link_group *lgr, bool soft)
 
 	if (lgr->terminating)
 		return;	/* lgr already terminating */
-	/* cancel free_work sync, will terminate when lgr->freeing is set */
+	/* cancel pending free_work; a running instance rechecks freeing */
 	cancel_delayed_work(&lgr->free_work);
 	lgr->terminating = 1;
 
diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h
index 5c18f08a4c8a..b8c67f277337 100644
--- a/net/smc/smc_core.h
+++ b/net/smc/smc_core.h
@@ -303,7 +303,7 @@ struct smc_link_group {
 	struct workqueue_struct	*tx_wq;		/* wq for conn. tx workers */
 	u8			sync_err : 1;	/* lgr no longer fits to peer */
 	u8			terminating : 1;/* lgr is terminating */
-	u8			freeing : 1;	/* lgr is being freed */
+	bool			freeing;	/* lgr is being freed */
 
 	refcount_t		refcnt;		/* lgr reference count */
 	bool			is_smcd;	/* SMC-R or SMC-D */
-- 
2.43.0

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

[netdev-bot+sashiko]

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

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

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] __smc_lgr_terminate() (net/smc/smc_core.c) still cancels
  free_work with cancel_delayed_work(), which does not wait for a…

Pre-existing issues:
- [Low] Baseline decision: not a valid finding for the final state of the
  series.
- [Low] Baseline decision: not reported (a pre-existing, low-impact race
  the patch does not claim to fix).
- [Low] Baseline decision: not a valid finding for the final state of the
  series.

--

net/smc: serialize link group free work scheduling

This patch holds the link group list lock across the freeing check and
mod_delayed_work() in smc_lgr_schedule_free_work(). It also sets
lgr->freeing under that lock in smc_lgr_cleanup_early(). Finally, it moves
freeing out of the shared bitfield byte and gives it its own bool.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 9974149659c2f..478c8bc2759a0 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c

[ ... ]

> @@ -691,6 +696,7 @@ void smc_lgr_cleanup_early(struct smc_link_group *lgr)
>  	/* do not use this link group for new connections */
>  	if (!list_empty(&lgr->list))
>  		list_del_init(&lgr->list);
> +	lgr->freeing = 1;
>  	spin_unlock_bh(lgr_lock);
>  	__smc_lgr_terminate(lgr, true);
>  }

[Severity: Low]
This is a pre-existing issue, but is lgr still guaranteed to be alive when
smc_lgr_cleanup_early() is reached from smc_conn_abort()?

net/smc/af_smc.c:smc_conn_abort() {
    ...
	smc_conn_free(conn);
	if (local_first && lgr_valid)
		smc_lgr_cleanup_early(lgr);
}

smc_conn_free() drops the connection's reference:

	smc_lgr_put(lgr); /* lgr_hold in smc_conn_create() */

After that, smc_lgr_list_head(), spin_lock_bh(), list_del_init() and the
new freeing store all run without a reference held.

The unreferenced access was already there before this patch. The next
patch in the series, "net/smc: serialize early link group cleanup with
termination", adds smc_lgr_hold()/smc_lgr_put() around smc_conn_free()
and smc_lgr_cleanup_early(). It does the same on the registration
failure path in smc_conn_create().

Both patches carry Cc: stable. Could this changelog say that the
follow-up patch has to be backported with it?

[Severity: Low]
This isn't a bug introduced by this patch, but should lgr->freeing be
tested before it is set here?

At this commit, smc_lgr_cleanup_early() sets freeing unconditionally and
always calls __smc_lgr_terminate(). Suppose smc_lgr_free_work() has
already claimed the group:

smc_lgr_free_work()
    lgr->freeing = 1; /* this instance does the freeing, no new schedule */
    ...
    smc_lgr_free(lgr);

__smc_lgr_terminate() only checks lgr->terminating, and
smc_lgr_free_work() never sets it. The second teardown would then go into
smc_lgr_free() again. The same applies to a group already claimed by
smc_lgr_terminate_sched() or smc_smcd_terminate().

Can this end in a second destroy_workqueue() of lgr->tx_wq, a second
put_device(), and an lgr refcount underflow?

The baseline smc_lgr_cleanup_early() did not test freeing either. The
follow-up patch "net/smc: serialize early link group cleanup with
termination" adds the check under lgr_lock:

	if (lgr->freeing) {
		spin_unlock_bh(lgr_lock);
		return;
	}

[ ... ]

> @@ -1565,7 +1571,7 @@ static void __smc_lgr_terminate(struct smc_link_group *lgr, bool soft)
>  
>  	if (lgr->terminating)
>  		return;	/* lgr already terminating */
> -	/* cancel free_work sync, will terminate when lgr->freeing is set */
> +	/* cancel pending free_work; a running instance rechecks freeing */
>  	cancel_delayed_work(&lgr->free_work);
>  	lgr->terminating = 1;

[Severity: High]
Can a running free_work instance safely do that recheck?

smc_lgr_free_work() holds no reference on lgr, and the recheck itself
dereferences lgr:

smc_lgr_free_work() {
    ...
	smc_lgr_list_head(lgr, &lgr_lock);
	spin_lock_bh(lgr_lock);
	if (lgr->freeing) {
    ...
}

cancel_delayed_work() does not wait for a callback that is already
running. smc_lgr_free() only drains tx_wq before it drops the base
reference.

Could this sequence free lgr while the worker is still using it?

CPU 0 (kworker)                     CPU 1 (terminator)
smc_lgr_free_work() starts
  (PENDING already cleared)
  preempted before
  smc_lgr_list_head()
                                    set lgr->freeing under lgr_lock
                                    __smc_lgr_terminate()
                                      cancel_delayed_work() returns false
                                      smc_lgr_cleanup()
                                      smc_lgr_free()
                                        smc_lgr_put()
                                          __smc_lgr_free()
                                            kfree(lgr)
  smc_lgr_list_head(lgr, ...)
  spin_lock_bh(lgr_lock)
  reads lgr->freeing

free_work is only scheduled when the group has no connections. So the put
in smc_lgr_free() drops the last reference.

The terminator could be any of these:

  terminate_work after a peer-triggered smc_smcd_terminate()
  smc_lgr_terminate_sched() after a CLC DECLINE with the first contact flag
  smc_smcr_terminate_all() or smc_smcd_terminate_all()

If the freed memory reads freeing == 0, the worker also goes on to
list_del_init() and a second smc_lgr_free() on the freed object.

The asynchronous cancel predates this patch. However, the new comment
describes running instances as safe, and the v2 notes say the patch keeps
"asynchronous cancellation without adding work-owned references".

Does this need cancel_delayed_work_sync() on the terminate paths that can
drop the last reference, or a reference owned by the work item?

> diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h
> index 5c18f08a4c8a5..b8c67f277337a 100644
> --- a/net/smc/smc_core.h
> +++ b/net/smc/smc_core.h
> @@ -303,7 +303,7 @@ struct smc_link_group {
>  	struct workqueue_struct	*tx_wq;		/* wq for conn. tx workers */
>  	u8			sync_err : 1;	/* lgr no longer fits to peer */
>  	u8			terminating : 1;/* lgr is terminating */
> -	u8			freeing : 1;	/* lgr is being freed */
> +	bool			freeing;	/* lgr is being freed */

[Severity: Low]
This is a pre-existing issue, but sync_err and terminating still share a
byte after freeing moves out.

smc_clc_wait_msg() does:

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

and __smc_lgr_terminate() does:

	lgr->terminating = 1;

The two writes have no common lock. Can one read-modify-write overwrite
the other?

The impact looks limited. freeing and list membership still prevent
reuse or a double teardown. If terminating = 1 is lost, the main effect
is that smc_wr_tx_get_free_slot() waits instead of failing fast on:

	if (in_softirq() || lgr->terminating) {

That wait ends when the link stops being sendable or after
SMC_WR_TX_WAIT_FREE_SLOT_TIME. The changelog only claims to protect
freeing, so this could be handled in a separate patch.

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

[PATCH net v2 2/2] net/smc: serialize early link group cleanup with termination

[Chengfeng Ye]

smc_lgr_cleanup_early() enters __smc_lgr_terminate() even when another
teardown path has already claimed lgr->freeing. Both callers can observe
lgr->terminating clear before either sets it, then tear down the same
link group, causing use-after-free and duplicate resource release.

  BUG: KASAN: use-after-free in __smc_lgr_terminate+0x393/0x3a0
  Write of size 1 at addr ffff8880bf2c0300 by task poc/106
  Call Trace:
   __smc_lgr_terminate+0x393/0x3a0
   __smc_connect+0x2e3d/0x4930
   smc_connect+0x42c/0x580
   __sys_connect+0xfc/0x130
   __x64_sys_connect+0x6d/0xb0

Check freeing under the link group list lock before unlinking the group
or entering termination. The existing locked freeing transition then
claims exclusive teardown ownership; an early cleanup that loses the
claim leaves teardown to the existing owner and does not remove the group
from a device teardown's private list.

Keep a temporary link group reference across smc_conn_free() and early
cleanup in smc_conn_abort(). Once the connection is unregistered, a
termination worker can finish without taking its socket lock. The extra
reference keeps the allocation alive until the ownership check returns.

The registration failure path in smc_conn_create() needs the same
protection. A new group is published before registration, but the
connection reference is taken only on success. Concurrent termination
can release the group after conns_lock is dropped and before early
cleanup. Hold the new group before publication and release that temporary
reference after failed-registration cleanup. On success, acquire the
ordinary connection reference before releasing the temporary hold.
Reused groups retain their existing reference protocol.

Fixes: f9aab6f2ce57 ("net/smc: immediate freeing in smc_lgr_cleanup_early()")
Cc: stable@vger.kernel.org
Assisted-by: GPT-6.1-Sol
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
Changes in v2:
- Pin newly created link groups before publication, covering the
  registration failure handoff to early cleanup. Drop the temporary hold
  after cleanup on failure, or after the connection hold on success.
- Send after the free-work scheduling fix and reuse its standalone
  freeing bool and locked early-cleanup transition.
- Pin the link group across the smc_conn_abort() early-cleanup ownership
  check.

v1: https://lore.kernel.org/r/20260927063607.3691520-1-nicoyip.dev@gmail.com/

 net/smc/af_smc.c   | 8 ++++++--
 net/smc/smc_core.c | 8 ++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index e9f93b3ab435..97b0a0b51066 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);
+	}
 
 	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 478c8bc2759a..1918a79ff60e 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -693,6 +693,10 @@ 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;
+	}
 	/* do not use this link group for new connections */
 	if (!list_empty(&lgr->list))
 		list_del_init(&lgr->list);
@@ -999,6 +1003,7 @@ static int smc_lgr_create(struct smc_sock *smc, struct smc_init_info *ini)
 		lgr->buf_type = lgr->net->smc.sysctl_smcr_buf_type;
 		atomic_inc(&lgr_cnt);
 	}
+	smc_lgr_hold(lgr); /* lgr_put in smc_conn_create() */
 	smc->conn.lgr = lgr;
 	spin_lock_bh(lgr_lock);
 	list_add_tail(&lgr->list, lgr_list);
@@ -2054,10 +2059,13 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini)
 		write_unlock_bh(&lgr->conns_lock);
 		if (rc) {
 			smc_lgr_cleanup_early(lgr);
+			smc_lgr_put(lgr); /* lgr_hold in smc_lgr_create() */
 			goto out;
 		}
 	}
 	smc_lgr_hold(conn->lgr); /* lgr_put in smc_conn_free() */
+	if (ini->first_contact_local)
+		smc_lgr_put(conn->lgr); /* lgr_hold in smc_lgr_create() */
 	if (!conn->lgr->is_smcd)
 		smcr_link_hold(conn->lnk); /* link_put in smc_conn_free() */
 	conn->freed = 0;
-- 
2.43.0

Re: [PATCH net v2 2/2] net/smc: serialize early link group cleanup with termination

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 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 new temporary hold in smc_lgr_create()/smc_conn_create()
  keeps only the struct smc_link_group memory alive across the window…
- [Low] The fix only works if the preceding series patch "net/smc:
  serialize link group free work scheduling" is also applied, and this…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the exclusive-
  teardown protocol this patch builds on is unsafe against an already-…

--

net/smc: serialize early link group cleanup with termination

This patch makes smc_lgr_cleanup_early() check lgr->freeing under the link
group list lock, so it does not tear down a group that another path
already owns. It also takes a temporary lgr reference in smc_conn_abort().
For newly created groups, it takes another temporary reference from
publication in smc_lgr_create() until registration in smc_conn_create()
finishes.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 478c8bc2759a0..1918a79ff60e4 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -693,6 +693,10 @@ 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;
> +	}

[Severity: Low]

This early return only gives exclusive teardown ownership in combination
with two other pieces: the lgr->freeing = 1 write later in
smc_lgr_cleanup_early(), and freeing being a standalone bool instead of a
bitfield that shares storage with sync_err and terminating.

Both of those come from the earlier series patch "net/smc: serialize link
group free work scheduling". The baseline smc_lgr_cleanup_early() never
sets freeing.

The commit body only says:

    The existing locked freeing transition then claims exclusive teardown
    ownership

The explicit dependency is stated only in the v2 changelog below the ---
line, and git am drops that part.

The context lines of this hunk don't include the lgr->freeing = 1 line,
so the hunk applies cleanly to a tree without the prerequisite. The two
patches also carry different Fixes: tags (8e316b9e7260 and f9aab6f2ce57).

Suppose a stable tree takes only this patch. smc_lgr_cleanup_early()
would then check freeing but never claim it. Couldn't
smc_lgr_terminate_sched(), smc_smcd_terminate() or
smc_smcr_terminate_all() still claim the group after the check, so that
both paths enter __smc_lgr_terminate()?

Could the body mention the dependency? Another option is a "# needs ..."
annotation on the Cc: stable line.

[Severity: High]

This is a pre-existing issue and wasn't introduced by this patch. Still,
the exclusive teardown protocol that this check builds on doesn't seem
safe against a free_work instance that is already running.

smc_lgr_free_work() holds no lgr reference. It dereferences the group
before it can see that freeing was claimed:

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

__smc_lgr_terminate() only uses the non-synchronous cancel:

	/* cancel pending free_work; a running instance rechecks freeing */
	cancel_delayed_work(&lgr->free_work);

Neither smc_lgr_free() nor __smc_lgr_free() waits for free_work before
kfree(lgr).

Can this sequence happen on an idle group (conns_num == 0)?

CPU1 (free_work timer expired)
smc_lgr_free_work()
  <preempted before spin_lock_bh(lgr_lock)>

CPU2 (peer DELETE_LINK with SMC_LLC_FLAG_DEL_LINK_ALL)
smc_llc_process_cli_delete_link()
  smc_lgr_terminate_sched()
    lgr->freeing = 1;
    schedule_work(&lgr->terminate_work);
smc_lgr_terminate_work()
  __smc_lgr_terminate()
    cancel_delayed_work(&lgr->free_work);   <- does not wait
    smc_lgr_free()
      smc_lgr_put()
        __smc_lgr_free()
          kfree(lgr);

CPU1 resumes
  smc_lgr_list_head(lgr, &lgr_lock);   <- reads freed lgr
  spin_lock_bh(lgr_lock);

The resumed worker would take a spinlock through a pointer loaded from
freed memory. If the slab has been reused and reads freeing == 0 with an
empty tree, the worker could also run list_del_init() and smc_lgr_free()
on foreign memory.

Other terminators would reach the same window, for example an LLC protocol
violation, or smc_smcd_terminate() from an ISM event.

The smc_conn_abort()->smc_lgr_cleanup_early() path doesn't look like a
realistic trigger here. smc_lgr_schedule_free_work() is only called once
conns_num reaches zero, and the creating connection stays registered
until its own abort.

Could free_work hold its own lgr reference, taken when it is armed and
dropped when the work exits or is cancelled successfully? Alternatively,
the final put could use cancel_delayed_work_sync() when it is not running
from free_work itself.

>  	/* do not use this link group for new connections */
>  	if (!list_empty(&lgr->list))
>  		list_del_init(&lgr->list);
> @@ -999,6 +1003,7 @@ static int smc_lgr_create(struct smc_sock *smc, struct smc_init_info *ini)
>  		lgr->buf_type = lgr->net->smc.sysctl_smcr_buf_type;
>  		atomic_inc(&lgr_cnt);
>  	}
> +	smc_lgr_hold(lgr); /* lgr_put in smc_conn_create() */
>  	smc->conn.lgr = lgr;
>  	spin_lock_bh(lgr_lock);
>  	list_add_tail(&lgr->list, lgr_list);
> @@ -2054,10 +2059,13 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini)
>  		write_unlock_bh(&lgr->conns_lock);
>  		if (rc) {
>  			smc_lgr_cleanup_early(lgr);
> +			smc_lgr_put(lgr); /* lgr_hold in smc_lgr_create() */
>  			goto out;
>  		}
>  	}
>  	smc_lgr_hold(conn->lgr); /* lgr_put in smc_conn_free() */
> +	if (ini->first_contact_local)
> +		smc_lgr_put(conn->lgr); /* lgr_hold in smc_lgr_create() */

[Severity: High]

The temporary hold taken in smc_lgr_create() keeps the lgr allocation
alive between list_add_tail() and smc_lgr_register_conn(). Does it also
stop a terminator from tearing the group down in that window?

smc_conn_create() only takes lgr->conns_lock around
smc_lgr_register_conn(conn, true). Nothing rechecks lgr->freeing under
lgr_lock. For SMC-D, this sequence seems possible:

CPU1 (first-contact connect or listen)
smc_conn_create()
  smc_lgr_create()
    smc_lgr_hold(lgr);
    list_add_tail(&lgr->list, lgr_list);

CPU2 (peer ISM GID or shutdown event)
smc_smcd_terminate()
  lgr->peer_shutdown = 1;
  list_move(&lgr->list, &lgr_free_list);
  lgr->freeing = 1;
  schedule_work(&lgr->terminate_work);
smc_lgr_terminate_work()
  __smc_lgr_terminate()
    rb_first(&lgr->conns_all) is NULL, kill loop skipped
    smc_lgr_free()
      destroy_workqueue(lgr->tx_wq);
      smc_ism_put_vlan(); put_device();
      smc_lgr_put(lgr);   <- only the temporary hold remains

CPU1
  write_lock_bh(&lgr->conns_lock);
  smc_lgr_register_conn(conn, true);  <- cannot fail for SMC-D,
                                         sock_hold(), conns_num++
  smc_lgr_hold(conn->lgr);
  smc_lgr_put(conn->lgr);             <- temporary hold dropped
  return 0;

The kill loop never visited this connection, so conn->killed stays clear.
Two problems seem to follow.

First, smc_tx_consumer_update() can get -EPIPE from
smc_cdc_get_slot_and_msg_send() because peer_shutdown is set. It would
then queue work on the destroyed workqueue:

		if ((smc_cdc_get_slot_and_msg_send(conn) < 0) &&
		    !conn->killed) {
			queue_delayed_work(conn->lgr->tx_wq, &conn->tx_work,
					   SMC_TX_WORK_DELAY);

Second, smc_conn_free() sees list_empty(&lgr->list) and skips
smc_lgr_unregister_conn(). That leaks the sock_hold() taken at
registration. The final smc_lgr_put() would then free the lgr while
smc_conn_lgr_valid() still returns true for the connection.

The publication-to-registration window itself predates this patch. Before
it, the later write_lock_bh(&lgr->conns_lock) was a direct
use-after-free.

The commit message says "On success, acquire the ordinary connection
reference before releasing the temporary hold". This handoff doesn't seem
to serialize registration with the freeing claim.

Could first-contact registration take lgr_lock, fail if lgr->freeing is
set, and then take conns_lock and register? That would match the lock
order of the reuse path. Alternatively, could the group be published only
after its first connection is registered?

The SMC-R variant looks mostly blocked. smc_lgr_free() clears the links,
so registration then fails with SMC_CLC_DECL_NOACTLINK and takes the
fixed failure path. It is unclear whether a concurrent smcr_link_clear()
could still hand out a link while it is clearing.

>  	if (!conn->lgr->is_smcd)
>  		smcr_link_hold(conn->lnk); /* link_put in smc_conn_free() */
>  	conn->freed = 0;

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

Re: [PATCH net v2 0/2] net/smc: fix link group teardown races

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.