Re: [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-13 07:33:28
Also in:
linux-s390, sashiko-reviews
On 08/08/26 1:46 pm, sashiko-bot@kernel.org wrote:
quoted hunk
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: New issues: - [High] Dropping the hash lock per-socket and unconditionally restarting the list walk from the head using a logical index introduces O(N^2) list traversal complexity and causes active sockets to be skipped in the diag dump. Pre-existing issues: - [Critical] The patch attempts to fix a Use-After-Free race on `smc->clcsock` by adding `clcsock_release_lock` in `smc_diag_msg_common_fill()`, but misses a concurrent teardown path in `smc_accept_dequeue()` that frees the socket without acquiring the mutex. ----- Patch [1]: [PATCH net v3] net/smc: fix clcsock and lgr/lnk races in smc_diag dump path ---commit a16d6de2c1851166c340508a037e5292d66a0907 Author: Mahanta Jambigi [off-list ref] net/smc: fix clcsock and lgr/lnk races in smc_diag dump path This commit attempts to fix two races in the SMC diag dump path by holding clcsock_release_lock in smc_diag_msg_common_fill() and lock_sock() in __smc_diag_dump(). It also modifies the dump iteration to pin sockets, drop the hash lock, and restart the traversal from the head of the list.quoted
diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c index bf0beaa23bdb6..72d77360e504f 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_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);[Severity: Critical] This is a pre-existing issue, but does this mutex fully protect against concurrent teardowns? Looking at smc_accept_dequeue() in net/smc/af_smc.c, if an unaccepted socket is closed (SMC_CLOSED), it unhashes it and frees isk->clcsock without holding clcsock_release_lock or the child's socket lock:
I'll address this issue separately as this is a pre-existing one.
net/smc/af_smc.c:smc_accept_dequeue() { ... if (new_sk->sk_state == SMC_CLOSED) { new_sk->sk_prot->unhash(new_sk); if (isk->clcsock) { sock_release(isk->clcsock); isk->clcsock = NULL; } ... } If the netlink diag dump thread concurrently iterates the hash table, grabs the socket reference, and calls smc_diag_msg_common_fill(), could it acquire the mutex, see a non-NULL clcsock, and dereference it while it is being freed by smc_accept_dequeue()? [ ... ]quoted
@@ -204,25 +228,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;[Severity: High] Does restarting the list walk from the head using a logical index (snum) introduce an O(N^2) list traversal complexity? If any previously seen sockets are concurrently unhashed while the lock is dropped, could stepping over (num < snum) elements skip active sockets that shifted to earlier positions in the list?
I got couple of reviews from Sashiko AI & I have addressed them here. Q1: O(N²) complexity? Yes, this is O(N²) in the worst case — on each restart we skip already-dumped entries from the head. This is an accepted trade-off: the same pattern is used in inet_diag and unix_diag. The lock cannot be held across __smc_diag_dump() since it now takes sleeping locks (mutex_lock, lock_sock), so drop-and-restart is unavoidable. The skip itself is cheap (counter comparison only), and the SMC hash is small in practice. Q2: Can sockets shift to earlier positions and get skipped? No. SMC uses an hlist where new sockets are always inserted at the head via hlist_add_head(). Removal does not reorder remaining nodes. So a socket that existed before an unlock cannot move to an earlier position — its ordinal index across restarts is stable. A socket inserted during the unlock will appear at position 0 on the next restart and will be skipped by num < snum, but that is correct and consistent behaviour — netlink dumps are not guaranteed to be atomic snapshots. Sashiko AI review · https://sashiko.dev/#/patchset/20260807081606.3200128-1-mjambigi@linux.ibm.com?part=1