[PATCH net v3] net/smc: fix clcsock and lgr/lnk races in smc_diag dump path
From: Mahanta Jambigi <mjambigi@linux.ibm.com>
Date: 2026-08-07 08:16:29
Also in:
linux-s390
Subsystem:
networking [general], shared memory communications (smc) sockets, the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, D. Wythe, Dust Li, Sidraya Jayagond, Mahanta Jambigi, Linus Torvalds
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.
Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets")
Fixes: 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R connections")
Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>
Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
---
net/smc/smc_diag.c | 78 +++++++++++++++++++++++++++++++++++----------
1 file changed, 61 insertions(+), 17 deletions(-)
diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
index bf0beaa23bdb..000000000000 100644
--- a/net/smc/smc_diag.c
+++ b/net/smc/smc_diag.c@@ -39,22 +39,34 @@ 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 @@ 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;
@@ -182,13 +203,16 @@ dinfo.peer_token = conn->peer_token; 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 +228,43 @@ 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