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-27 19:16:26, Chengfeng Ye wrote:
>Hi Dust,
>
>Sorry for the late reply.
>
>On Wed, Oct 21, 2026 at 8:13 PM Dust Li <dust.li@linux.alibaba.com> wrote:
>>
>> 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.
>
>No problem, I will use SMC_CLC_DECL_INTERR in v4.
>
>> >+              } 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.
>
>It seems the smc_lgr_put(lgr) is added after the
>spin_unlock_bh(lgr_lock). Were you referring to another lock held by a
>caller? I will keep this unchanged in v4, let me know if anything
>should be adjusted further.

No, I mean this part in [PATCH net v3 3/3], sorry for replying to the wrong thread.


@@ -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);

Best regards,
Dust