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

[Dust Li <dust.li@linux.alibaba.com>]

On 2026-10-13 17:17:15, Chengfeng Ye wrote:
>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;

This should not be SMC_CLC_DECL_SYNCERR, maybe SMC_CLC_DECL_INTERR or add
a new error code.

>+		} 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() */

smc_lgr_put() may call smcr_buf_free() -> vfree(), which may sleep.
And you put it in the spinlock.

Best regards,
Dust