Re: [syzbot ci] Re: net/smc: fix clcsock and lgr/lnk races in smc_diag dump path

[Mahanta Jambigi <mjambigi@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 246 bytes --]

#syz test

v4: fixes two races in the smc_diag dump path (clcsock NULL race and
lgr/lnk teardown race) plus moves mutex_init(&clcsock_release_lock)
before sk->sk_prot->hash(sk) in smc_sk_init() to fix the uninitialized
mutex warning you reported.

[-- Attachment #2: v4-0001-net-smc-fix-clcsock-lgr-lnk-races-and-mutex-init-ordering.patch --]
[-- Type: text/plain, Size: 8436 bytes --]

From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Mahanta Jambigi <mjambigi@linux.ibm.com>
Date: Mon, 11 Aug 2026 00:00:00 +0000
Subject: [PATCH net v4] net/smc: fix clcsock and lgr/lnk races in smc_diag
 dump path

Two races in the SMC diag dump path:

Race 1: smc_diag_msg_common_fill() reads smc->clcsock fields after a
NULL check, but smc_clcsock_release() can set clcsock = NULL under
clcsock_release_lock between the check and the reads.  Hold the same
mutex in the read path to make the check and reads atomic.

Race 2: __smc_diag_dump() dereferences conn->lgr and conn->lnk with no
protection against concurrent teardown.  smc_close_active_abort() calls
smc_conn_free() -- which drops lgr and link refcounts -- without first
unhashing the socket, leaving stale pointers visible to the dump.  The
teardown path holds lock_sock(sk) across smc_conn_free(); take the same
lock in __smc_diag_dump() to serialise fully.

Both fixes require sleeping locks, which are illegal under the
read_lock(&smc_hash->lock) held by smc_diag_dump_proto().  Pin each
socket with refcount_inc_not_zero() before dropping the hash lock, call
__smc_diag_dump() locklessly, then release the pin.  Restart sk_for_each()
from head after each unlock rather than resuming mid-walk:
smc_unhash_sk() nulls sk->sk_node.next via sk_del_node_init(), so
resuming an interrupted walk silently truncates the dump.

Additionally, smc_sk_init() called sk->sk_prot->hash(sk) before
mutex_init(&clcsock_release_lock), creating a window where a concurrent
diag dump could call mutex_lock() on an uninitialized mutex and trigger:

    DEBUG_LOCKS_WARN_ON(lock->magic != lock)
    WARNING: kernel/locking/mutex.c:625

Fix by moving mutex_init() before hash() so the mutex is fully
initialized before the socket becomes visible to concurrent readers.

Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets")
Fixes: 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R connections")
Reported-by: syzbot+cia7ddf4f17dfab266@syzkaller.appspotmail.com
Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
---
Changes in v4:
- Move mutex_init(&clcsock_release_lock) before sk->sk_prot->hash(sk)
  in smc_sk_init() to close a window where a concurrent diag dump could
  call mutex_lock() on an uninitialized mutex (syzbot).

Changes in v3:
- Restart sk_for_each() from head after each unlock rather than
  resuming mid-walk. sk_del_node_init() nulls sk->sk_node.next so
  continuing an interrupted hlist walk silently truncates the dump
  (Hidayath Khan).
- Move sock_put() to before read_lock() so a last-reference drop
  cannot run socket teardown under the rwlock (Hidayath Khan).
- Move r->diag_state = sk->sk_state after lock_sock() so diag_state
  and the lgr/lnk fields form a consistent snapshot (Sidraya Jayagond).

Changes in v2:
- Replaces all three patches with a single patch and a simpler
  approach, based on Hidayath's comments.
- Drop read_lock(&smc_hash->lock) before calling __smc_diag_dump()
  and re-acquire it afterwards, using refcount_inc_not_zero() to pin
  the socket across the unlock window (same pattern as tcp_diag).
- With no spinlock held, take lock_sock(sk) in __smc_diag_dump()
  before any lgr or lnk dereference.
- With the spinlock dropped, smc_diag_msg_common_fill() can use
  mutex_lock(&clcsock_release_lock) instead of mutex_trylock().

 net/smc/af_smc.c   |  2 +-
 net/smc/smc_diag.c | 76 +++++++++++++++++++++++++++++++++++-----------

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index b5db69073e20..38a4c009363c 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -409,8 +409,8 @@ void smc_sk_init(struct net *net, struct sock *sk, int protocol)
 				      "sk_lock-AF_SMC", &smc_key);
 	spin_lock_init(&smc->accept_q_lock);
 	spin_lock_init(&smc->conn.send_lock);
-	sk->sk_prot->hash(sk);
 	mutex_init(&smc->clcsock_release_lock);
+	sk->sk_prot->hash(sk);
 	smc_init_saved_callbacks(smc);
 	smc->limit_smc_hs = net->smc.limit_smc_hs;
 	smc->use_fallback = false; /* assume rdma capability first */
diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
index bf0beaa23bdb..f3bad0cb09b1 100644
--- a/net/smc/smc_diag.c
+++ b/net/smc/smc_diag.c
@@ -39,22 +39,34 @@ static void smc_diag_msg_common_fill(struct smc_diag_msg *r, struct sock *sk)
 	memset(r, 0, sizeof(*r));
 	r->diag_family = sk->sk_family;
 	sock_diag_save_cookie(sk, r->id.idiag_cookie);
-	if (!smc->clcsock)
-		return;
-	r->id.idiag_sport = htons(smc->clcsock->sk->sk_num);
-	r->id.idiag_dport = smc->clcsock->sk->sk_dport;
-	r->id.idiag_if = smc->clcsock->sk->sk_bound_dev_if;
-	if (sk->sk_protocol == SMCPROTO_SMC) {
-		r->id.idiag_src[0] = smc->clcsock->sk->sk_rcv_saddr;
-		r->id.idiag_dst[0] = smc->clcsock->sk->sk_daddr;
+	/*
+	 * smc_clcsock_release() sets smc->clcsock = NULL under
+	 * clcsock_release_lock before freeing the socket. Hold the same
+	 * mutex here to make the NULL check and all field reads atomic
+	 * with that writer. mutex_lock() is safe: this function is called
+	 * only after the hash spinlock has been dropped by
+	 * smc_diag_dump_proto().
+	 */
+	mutex_lock(&smc->clcsock_release_lock);
+	if (smc->clcsock) {
+		r->id.idiag_sport = htons(smc->clcsock->sk->sk_num);
+		r->id.idiag_dport = smc->clcsock->sk->sk_dport;
+		r->id.idiag_if = smc->clcsock->sk->sk_bound_dev_if;
+		if (sk->sk_protocol == SMCPROTO_SMC) {
+			r->id.idiag_src[0] = smc->clcsock->sk->sk_rcv_saddr;
+			r->id.idiag_dst[0] = smc->clcsock->sk->sk_daddr;
 #if IS_ENABLED(CONFIG_IPV6)
-	} else if (sk->sk_protocol == SMCPROTO_SMC6) {
-		memcpy(&r->id.idiag_src, &smc->clcsock->sk->sk_v6_rcv_saddr,
-		       sizeof(smc->clcsock->sk->sk_v6_rcv_saddr));
-		memcpy(&r->id.idiag_dst, &smc->clcsock->sk->sk_v6_daddr,
-		       sizeof(smc->clcsock->sk->sk_v6_daddr));
+		} else if (sk->sk_protocol == SMCPROTO_SMC6) {
+			memcpy(&r->id.idiag_src,
+			       &smc->clcsock->sk->sk_v6_rcv_saddr,
+			       sizeof(smc->clcsock->sk->sk_v6_rcv_saddr));
+			memcpy(&r->id.idiag_dst,
+			       &smc->clcsock->sk->sk_v6_daddr,
+			       sizeof(smc->clcsock->sk->sk_v6_daddr));
 #endif
+		}
 	}
+	mutex_unlock(&smc->clcsock_release_lock);
 }
 
 static int smc_diag_msg_attrs_fill(struct sock *sk, struct sk_buff *skb,
@@ -87,6 +99,15 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
 
 	r = nlmsg_data(nlh);
 	smc_diag_msg_common_fill(r, sk);
+	/*
+	 * Take the socket lock to serialise against smc_conn_free(),
+	 * which drops lgr and link refcounts under lock_sock(). Without
+	 * this, a concurrent close can free lgr->lnk[] memory between
+	 * our smc_conn_lgr_valid() check and the subsequent lgr/lnk
+	 * dereferences. lock_sock() is safe here because the hash
+	 * spinlock has been dropped by smc_diag_dump_proto().
+	 */
+	lock_sock(sk);
 	r->diag_state = sk->sk_state;
 	if (smc->use_fallback)
 		r->diag_mode = SMC_DIAG_MODE_FALLBACK_TCP;
@@ -184,11 +205,12 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
 		if (nla_put(skb, SMC_DIAG_DMBINFO, sizeof(dinfo), &dinfo) < 0)
 			goto errout;
 	}
-
+	release_sock(sk);
 	nlmsg_end(skb, nlh);
 	return 0;
 
 errout:
+	release_sock(sk);
 	nlmsg_cancel(skb, nlh);
 	return -EMSGSIZE;
 }
@@ -204,25 +226,43 @@ static int smc_diag_dump_proto(struct proto *prot, struct sk_buff *skb,
 	int rc = 0, num = 0;
 	struct sock *sk;
 
-	read_lock(&prot->h.smc_hash->lock);
 	head = &prot->h.smc_hash->ht;
+restart:
+	num = 0;
+	read_lock(&prot->h.smc_hash->lock);
 	if (hlist_empty(head))
 		goto out;
-
 	sk_for_each(sk, head) {
 		if (!net_eq(sock_net(sk), net))
 			continue;
 		if (num < snum)
 			goto next;
+		/*
+		 * Pin sk before dropping the lock. refcount_inc_not_zero()
+		 * skips sockets already past their last reference.
+		 * smc_unhash_sk() nulls sk->sk_node.next via sk_del_node_init()
+		 * so resuming an interrupted sk_for_each() would terminate
+		 * early if a socket is unhashed while the lock is dropped.
+		 * Restart from head after each unlock, using snum to skip
+		 * already-dumped entries.
+		 */
+		if (!refcount_inc_not_zero(&sk->sk_refcnt))
+			goto next;
+		read_unlock(&prot->h.smc_hash->lock);
+
 		rc = __smc_diag_dump(sk, skb, cb, nlmsg_data(cb->nlh), bc);
+		sock_put(sk);
+
 		if (rc < 0)
-			goto out;
+			goto out_nolock;
+		snum = num + 1;
+		goto restart;
 next:
 		num++;
 	}
-
 out:
 	read_unlock(&prot->h.smc_hash->lock);
+out_nolock:
 	cb_ctx->pos[p_type] = num;
 	return rc;
 }
-- 
2.50.1