[PATCH net v3 0/3] net/smc: fix link group scheduling, registration and work lifetime

[PATCH net v3 0/3] net/smc: fix link group scheduling, registration and work lifetime

[Chengfeng Ye]

This revises the two-patch v2 series and adds a separate fix for the
running-free_work lifetime issue identified during review.

Patch 1 serializes delayed-work admission with the freeing transition.
Patch 2 gives early cleanup a single teardown owner, pins its callers,
and rejects first registration after teardown has claimed a new group.
Patch 3 gives queued/running free work its own group reference while
retaining asynchronous cancellation to avoid the existing wait cycle.

Each patch is a complete logical change. The work-reference acquisition,
cancellation and release changes all appear together in patch 3. Patches
2 and 3 record their prerequisites in their permanent commit messages.
The pre-existing callback issue remains until patch 3; no earlier commit
acquires a work reference that needs a later patch to release it.

The original KASAN excerpts in patches 1 and 2 came from instrumented
runs. The registration and running-callback gaps were identified by
source review. No new runtime reproduction is claimed.

Changes since v2:
- Serialize first registration with the freeing claim, and clear conn->lgr
  when registration is rejected before it acquires a connection reference.
- Add the running-work lifetime fix as a separate third patch.
- Correct the cancellation comment and document commit dependencies.
- Rebase onto current net/main and validate each cumulative revision.

Validation:
- Applied the exported patches one at a time in a clean worktree; each
  resulting tree and commit message matches the corresponding source commit.
- Built net/smc/ with W=1 after each patch, with CONFIG_IPV6=y and disabled
  (six successful builds; CONFIG_SMC, DIBS and INFINIBAND enabled).
- Strict checkpatch: zero errors, warnings and checks for all three patches.
- Runtime and SMC hardware testing have not been performed.

v2: https://lore.kernel.org/r/20261003183237.2284245-1-nicoyip.dev@gmail.com/
v1 scheduling: https://lore.kernel.org/r/20260927074520.3694663-1-nicoyip.dev@gmail.com/
v1 early cleanup: https://lore.kernel.org/r/20260927063607.3691520-1-nicoyip.dev@gmail.com/

Chengfeng Ye (3):
  net/smc: serialize link group free work scheduling
  net/smc: serialize early link group cleanup with termination
  net/smc: hold a link group reference for free work

 net/smc/af_smc.c   |  8 +++++--
 net/smc/smc_core.c | 59 +++++++++++++++++++++++++++++++++++-----------
 net/smc/smc_core.h |  2 +-
 3 files changed, 52 insertions(+), 17 deletions(-)


base-commit: aaaaf87ea99b8766c9a8aa0e71aa42e6bc8a5320
-- 
2.43.0

[PATCH net v3 1/3] 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 v3:
- Rebase onto current net/main.
- Correct the cancellation comment without implying that checking freeing
  protects an already-running callback. Patch 3 supplies that protection.

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/

v2: https://lore.kernel.org/r/20261003183237.2284245-2-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..660cd41ec720 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 */
 	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 v3 1/3] 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.
> 
> [ ... ]
> 
> 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>

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

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

[PATCH net v3 2/3] 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.

A new group is published before its first connection is registered. Hold
it before publication until registration or failed-registration cleanup
finishes. The reference keeps the allocation alive, but teardown can still
destroy the group's resources before registration. Take the list lock
around the freeing check and first registration, nesting conns_lock as in
the reuse path. Reject a group already claimed for teardown and clear
conn->lgr, since that connection never acquired a group reference.

On success, acquire the connection reference before dropping the temporary
hold. A later terminator sees the registered connection and must acquire
its socket lock before removing it. On failure, keep the temporary hold
through early cleanup and then release it. Reused groups retain their
existing reference protocol.

This depends on "net/smc: serialize link group free work scheduling" for
the standalone freeing bool and the locked early-cleanup transition.

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 v3:
- Serialize first registration with the teardown claim under the group list
  lock, nesting conns_lock in the same order as connection reuse.
- Reject groups already being freed and clear conn->lgr on that path.
- Document the scheduling fix prerequisite in the permanent commit message.

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/

v2: https://lore.kernel.org/r/20261003183237.2284245-3-nicoyip.dev@gmail.com/

 net/smc/af_smc.c   |  8 ++++++--
 net/smc/smc_core.c | 21 ++++++++++++++++++---
 2 files changed, 24 insertions(+), 5 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 660cd41ec720..c1bb284c4233 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);
@@ -2049,15 +2054,25 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini)
 		if (rc)
 			goto out;
 		lgr = conn->lgr;
-		write_lock_bh(&lgr->conns_lock);
-		rc = smc_lgr_register_conn(conn, true);
-		write_unlock_bh(&lgr->conns_lock);
+		spin_lock_bh(lgr_lock);
+		if (lgr->freeing) {
+			conn->lgr = NULL;
+			rc = SMC_CLC_DECL_SYNCERR;
+		} else {
+			write_lock_bh(&lgr->conns_lock);
+			rc = smc_lgr_register_conn(conn, true);
+			write_unlock_bh(&lgr->conns_lock);
+		}
+		spin_unlock_bh(lgr_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 v3 2/3] net/smc: serialize early link group cleanup with termination

[sashiko-bot]

> 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
> [ ... ]
> 
> 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>

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

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (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).

- [Medium] This patch adds a new source of SMC_CLC_DECL_SYNCERR, and it
  uses that code for a purely local event.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  smc_lgr_cleanup_early() now always hands teardown to the existing…

--

net/smc: serialize early link group cleanup with termination

With this patch, smc_lgr_cleanup_early() returns early if another teardown
path has already set lgr->freeing. It also adds temporary link group
references in smc_conn_abort() and around first-contact registration in
smc_conn_create(), and checks lgr->freeing under the list lock before the
first connection of a new link group is registered.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 660cd41ec720a..c1bb284c42334 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: Medium]
This is a pre-existing issue, not one introduced by this patch. With this
early return, the existing teardown owner is always the one that finishes
teardown. On some paths that means it kills a connection that
smc_conn_free() left registered.

smc_conn_free() only unregisters the connection while the lgr is still on
its list:

net/smc/smc_core.c:smc_conn_free() {
    ...
	if (!list_empty(&lgr->list)) {
		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
		smc_lgr_unregister_conn(conn);
	}
    ...
}

When they claim freeing, smc_lgr_terminate_sched() and the second loop of
smc_smcr_terminate_all() and smc_smcd_terminate_all() call list_del_init()
on lgr->list.

Take a client first-contact connect that has already registered its conn.
A terminator then claims the group. The trigger could be a link down, an
LLC delete link request, or a CLC decline with the first-contact bit
received in smcr_clnt_conf_first_link()->smc_clc_wait_msg():

smc_connect_rdma()
  connect_abort:
  smc_conn_abort(smc, 1)
    smc_conn_free()
      conn->freed = 1, link and lgr references dropped
      lgr->list is empty, smc_lgr_unregister_conn() skipped
    smc_lgr_cleanup_early()
      lgr->freeing is set, returns
__smc_connect()
  falls back to TCP, sk_state = SMC_ACTIVE, socket lock released

terminate worker:
__smc_lgr_terminate()
  conn still found in conns_all
  lock_sock(&smc->sk);
  smc_conn_kill(conn, soft);
    smc_close_active_abort()
      tcp_abort(smc->clcsock->sk, ECONNABORTED);

Could this reset a TCP fallback connection that userspace already sees as
established? The server side looks able to hit the same thing through
smc_listen_decline().

Before this patch, the early cleanup thread either ended up in this same
state, or ran __smc_lgr_terminate() itself and blocked in lock_sock() on
its own socket. The hang is gone, but the stale registration remains.

The commit message says:

  "Once the connection is unregistered, a termination worker can finish
  without taking its socket lock."

Does that still hold on this path, where smc_conn_free() skips the
unregistration?

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

[ ... ]

> @@ -2049,15 +2054,25 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini)
>  		if (rc)
>  			goto out;
>  		lgr = conn->lgr;
> -		write_lock_bh(&lgr->conns_lock);
> -		rc = smc_lgr_register_conn(conn, true);
> -		write_unlock_bh(&lgr->conns_lock);
> +		spin_lock_bh(lgr_lock);
> +		if (lgr->freeing) {
> +			conn->lgr = NULL;
> +			rc = SMC_CLC_DECL_SYNCERR;

[Severity: Medium]
Is SMC_CLC_DECL_SYNCERR the right code for a purely local teardown race?

smc_clc.h defines it as a synchronization error. The comment earlier in
smc_conn_create() limits it to the case where the server reuses a link
group but the client wants a new one.

It also changes what goes on the wire. smc_clc_send_decline() sets
SMC_FIRST_CONTACT_MASK only for this code:

	dclc.hdr.typev2 = (peer_diag_info == SMC_CLC_DECL_SYNCERR) ?
					SMC_FIRST_CONTACT_MASK : 0;

and the peer reacts to it in smc_clc_wait_msg():

	if ((dclc->hdr.typev2 & SMC_FIRST_CONTACT_MASK) &&
	    smc->conn.lgr) {
		smc->conn.lgr->sync_err = 1;
		smc_lgr_terminate_sched(smc->conn.lgr);
	}

In the client role, a local teardown can set freeing after smc_lgr_create()
has published the group and before this check. That teardown could be
smc_smcr_terminate_all(NULL), smc_smcd_terminate() from an ISM peer
shutdown event, smc_smcd_terminate_all() or smc_lgr_terminate_sched().
The result is:

__smc_connect()
  smc_connect_decline_fallback(..., SMC_CLC_DECL_SYNCERR, ...)
    smc_clc_send_decline()   /* sets SMC_FIRST_CONTACT_MASK */

The server is waiting for the CLC confirm in smc_listen_work(), with
conn.lgr pointing at its new first-contact group. It then sets sync_err on
that group and schedules its termination, which empties lgr->list and sets
freeing.

>From there, smc_listen_decline()->smc_conn_abort(new_smc, 1)->smc_conn_free()
skips smc_lgr_unregister_conn() because lgr->list is empty.
smc_lgr_cleanup_early() then returns early because freeing is set.

The server then falls back to TCP, and smc_listen_out() drops the socket
lock. Wouldn't the terminate worker's __smc_lgr_terminate() then find the
still-registered conn and call smc_conn_kill()? That ends in tcp_abort()
on the server's fallback clcsock.

Would a code without the first-contact side effect, such as
SMC_CLC_DECL_INTERR, avoid resetting the peer's fallback connection here?

In the server-role case the peer's conn.lgr is still NULL, so there it
would only give the peer the wrong diagnosis and statistics.

> +		} else {
> +			write_lock_bh(&lgr->conns_lock);
> +			rc = smc_lgr_register_conn(conn, true);
> +			write_unlock_bh(&lgr->conns_lock);
> +		}
> +		spin_unlock_bh(lgr_lock);
>  		if (rc) {
>  			smc_lgr_cleanup_early(lgr);
> +			smc_lgr_put(lgr); /* lgr_hold in smc_lgr_create() */
>  			goto out;
>  		}
>  	}

[ ... ]

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

[PATCH net v3 3/3] net/smc: hold a link group reference for free work

[Chengfeng Ye]

__smc_lgr_terminate() cancels free_work asynchronously. If the callback
has already started, it can still access the group after the terminator
drops the last reference. The callback has to dereference the group to
find its list lock and check freeing, so that check cannot protect its
own access to the allocation. This lifetime gap was identified during
code review of the scheduling and early-cleanup fixes.

Take a group reference before mod_delayed_work(). Keep it when a new
execution is queued, including when a previous callback is still running.
If the work was already pending, release the extra reference because that
pending execution already owns one. Release the queued reference when
cancellation succeeds, or at every callback exit when it executes.

Use the reference-aware cancellation helper for connection reuse,
termination and cancellation of a rearmed instance by the free worker.
The callback's reference keeps the group alive even after smc_lgr_free()
drops the teardown owner's reference.

Keep cancellation asynchronous. Waiting for free_work from connection
setup can deadlock: the free worker drains tx_wq, whose worker needs the
socket lock held by setup.

This builds on "net/smc: serialize link group free work scheduling" and
"net/smc: serialize early link group cleanup with termination". Together
they close admission before cancellation and give resource teardown a
single owner; the work reference then protects callbacks that are already
running when that owner cancels pending work.

Link: https://lore.kernel.org/r/179113886433.434549.10069135669585050618@kernel.org/
Fixes: 13085e1b5cab ("net/smc: fix deadlock triggered by cancel_delayed_work_syn()")
Cc: stable@vger.kernel.org
Assisted-by: GPT-6.1-Sol
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
New in v3:
- Address the independent running-free_work lifetime issue reported against
  both v2 patches by sashiko, with the complete reference protocol in this commit.
- Cover new scheduling, rearming, all three cancellation sites and every
  callback exit while retaining asynchronous cancellation.

Review: https://lore.kernel.org/r/179113886433.434549.10069135669585050618@kernel.org/
Review: https://lore.kernel.org/r/179113886579.434549.12546097168507601074@kernel.org/

 net/smc/smc_core.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index c1bb284c4233..9b54f873c6a9 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -89,14 +89,22 @@ static void smc_lgr_schedule_free_work(struct smc_link_group *lgr)
 	 * otherwise there is a risk of out-of-sync link groups.
 	 */
 	if (!lgr->freeing) {
-		mod_delayed_work(system_percpu_wq, &lgr->free_work,
-				 (!lgr->is_smcd && lgr->role == SMC_CLNT) ?
-						SMC_LGR_FREE_DELAY_CLNT :
-						SMC_LGR_FREE_DELAY_SERV);
+		smc_lgr_hold(lgr); /* released by the worker or cancellation */
+		if (mod_delayed_work(system_percpu_wq, &lgr->free_work,
+				     (!lgr->is_smcd && lgr->role == SMC_CLNT) ?
+					     SMC_LGR_FREE_DELAY_CLNT :
+					     SMC_LGR_FREE_DELAY_SERV))
+			smc_lgr_put(lgr); /* pending work already owns a reference */
 	}
 	spin_unlock_bh(lgr_lock);
 }
 
+static void smc_lgr_cancel_free_work(struct smc_link_group *lgr)
+{
+	if (cancel_delayed_work(&lgr->free_work))
+		smc_lgr_put(lgr); /* cancelled work will not release its reference */
+}
+
 /* Register connection's alert token in our lookup structure.
  * To use rbtrees we have to implement our own insert core.
  * Requires @conns_lock
@@ -733,19 +741,19 @@ static void smc_lgr_free_work(struct work_struct *work)
 	spin_lock_bh(lgr_lock);
 	if (lgr->freeing) {
 		spin_unlock_bh(lgr_lock);
-		return;
+		goto out;
 	}
 	read_lock_bh(&lgr->conns_lock);
 	conns = RB_EMPTY_ROOT(&lgr->conns_all);
 	read_unlock_bh(&lgr->conns_lock);
 	if (!conns) { /* number of lgr connections is no longer zero */
 		spin_unlock_bh(lgr_lock);
-		return;
+		goto out;
 	}
 	list_del_init(&lgr->list); /* remove from smc_lgr_list */
 	lgr->freeing = 1; /* this instance does the freeing, no new schedule */
 	spin_unlock_bh(lgr_lock);
-	cancel_delayed_work(&lgr->free_work);
+	smc_lgr_cancel_free_work(lgr);
 
 	if (!lgr->is_smcd && !lgr->terminating)
 		smc_llc_send_link_delete_all(lgr, true,
@@ -755,6 +763,8 @@ static void smc_lgr_free_work(struct work_struct *work)
 	if (!lgr->is_smcd)
 		smcr_lgr_link_deactivate_all(lgr);
 	smc_lgr_free(lgr);
+out:
+	smc_lgr_put(lgr); /* smc_lgr_hold in smc_lgr_schedule_free_work() */
 }
 
 static void smc_lgr_terminate_work(struct work_struct *work)
@@ -1576,8 +1586,8 @@ static void __smc_lgr_terminate(struct smc_link_group *lgr, bool soft)
 
 	if (lgr->terminating)
 		return;	/* lgr already terminating */
-	/* cancel pending free_work */
-	cancel_delayed_work(&lgr->free_work);
+	/* A running free_work holds its own link group reference. */
+	smc_lgr_cancel_free_work(lgr);
 	lgr->terminating = 1;
 
 	/* kill remaining link group connections */
@@ -2029,8 +2039,8 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini)
 			conn->lgr = lgr;
 			rc = smc_lgr_register_conn(conn, false);
 			write_unlock_bh(&lgr->conns_lock);
-			if (!rc && delayed_work_pending(&lgr->free_work))
-				cancel_delayed_work(&lgr->free_work);
+			if (!rc)
+				smc_lgr_cancel_free_work(lgr);
 			break;
 		}
 		write_unlock_bh(&lgr->conns_lock);
-- 
2.43.0

Re: [PATCH net v3 3/3] net/smc: hold a link group reference for free work

[sashiko-bot]

> __smc_lgr_terminate() cancels free_work asynchronously. If the callback
> has already started, it can still access the group after the terminator
> drops the last reference. The callback has to dereference the group to
> find its list lock and check freeing, so that check cannot protect its
> own access to the allocation. This lifetime gap was identified during
> code review of the scheduling and early-cleanup fixes.
> 
> Take a group reference before mod_delayed_work(). Keep it when a new
> execution is queued, including when a previous callback is still running.
> If the work was already pending, release the extra reference because that
> pending execution already owns one. Release the queued reference when
> cancellation succeeds, or at every callback exit when it executes.
> 
> Use the reference-aware cancellation helper for connection reuse,
> termination and cancellation of a rearmed instance by the free worker.
> [ ... ]
> Link: https://lore.kernel.org/r/179113886433.434549.10069135669585050618@kernel.org/
> Fixes: 13085e1b5cab ("net/smc: fix deadlock triggered by cancel_delayed_work_syn()")
> Cc: stable@vger.kernel.org
> Assisted-by: GPT-6.1-Sol
> 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/20261013091716.1210420-1-nicoyip.dev@gmail.com?part=3