The SMC diag dump path reads conn->lgr and conn->lnk while iterating the socket hash table under a read_lock. Concurrently, RDMA link failure teardown (__smc_lgr_terminate -> smc_conn_kill -> smc_close_active_abort -> smc_conn_free) and the passive close workqueue path (smc_close_passive_work -> smc_conn_free) can drop the connection-owned references to lgr and lnk while the socket is still visible in the hash, allowing the diag reader to dereference a freed lgr or lnk. Fix this by ensuring that for all non-fallback paths the socket is removed from the hash table before the lgr/lnk references are dropped in smc_conn_free(). Introduce smc_conn_unhash() with a per-connection 'unhashed' flag so that the unhash executes exactly once regardless of which path reaches smc_conn_free() first. smc_conn_free() calls smc_conn_unhash() before the lgr_put/link_put sequence, establishing the invariant: any socket still visible to the diag reader under the hash read_lock has valid conn->lgr and conn->lnk pointers. __smc_release() is updated to use smc_conn_unhash() for non-fallback sockets (so the flag is honoured when smc_conn_free() already ran, e.g. via smc_conn_kill() ahead of the user-space close()), and keeps the direct sk->sk_prot->unhash() call only for fallback sockets, which never call smc_conn_free(). The diag path therefore reduces to: hold hash read_lock -> read conn->lgr -> if non-NULL, dereference -> done with no new lock, no extra reference count, and no trylock. 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 Signed-off-by: Mahanta Jambigi --- Changes in v3: - redesigned as a single patch; dropped the lgr_lnk_lock spinlock approach and the 2-patch split - fix is now at the socket hash layer: introduce smc_conn_unhash() with a per-connection unhashed flag; smc_conn_free() unhashes before dropping lgr/lnk refs, so any socket visible to the diag reader under the hash read_lock has valid conn->lgr and conn->lnk pointers - __smc_release() updated to call smc_conn_unhash() for non-fallback sockets so the flag is honoured when smc_conn_free() already ran first - smc_diag.c needs no changes; the hash read_lock invariant is sufficient without any per-connection lock in the dump path - dropped the clcsock/mutex_trylock fix as that will be addressed separately Changes in v2: - this is v2 of the 2-patch series; the earlier submission was mislabelled [PATCH v3] but was in fact the first version sent to the list - split into a 2-patch series; patch 1/2 adds per-connection lgr_lnk_lock infrastructure to smc_core, patch 2/2 fixes the diag dump path using it - dropped lock_sock()/release_sock() from __smc_diag_dump(); v1 held the socket lock across all lgr/lnk dereferences, requiring the hash read_lock to be dropped and re-acquired around each socket - dropped the restart-from-head loop in smc_diag_dump_proto(); the new design does not drop the hash read_lock mid-walk so the hlist truncation concern no longer applies - dropped refcount_inc_not_zero() socket pinning from the dump loop for the same reason: the hash read_lock is now held for the full walk - added per-connection lgr_lnk_lock spinlock to struct smc_connection; conn->lgr and conn->lnk are NULLed under this lock in smc_conn_free() before borrowed references are released, establishing the invariant: a non-NULL conn->lgr seen under lgr_lnk_lock guarantees the lgr is alive - added lgr_lnk_lock to smc_switch_link_and_count() to protect the conn->lnk pointer swap from concurrent diag readers - replaced mutex_lock() on clcsock_release_lock in smc_diag_msg_common_fill() with mutex_trylock(); mutex_lock() was valid in v1 because the hash spinlock had been dropped, but the new design holds the hash read_lock throughout so only a non-sleeping trylock is safe; a failed trylock leaves address fields zeroed, which is acceptable for a monitoring tool - all conn->lgr and conn->lnk accesses in __smc_diag_dump() use a snapshot-then-use pattern: fields are copied into local stack variables under lgr_lnk_lock and nla_put() is called after releasing the lock, avoiding any sleeping operation under the spinlock net/smc/af_smc.c | 10 +++++++++- net/smc/smc.h | 1 + net/smc/smc_core.c | 20 ++++++++++++++++++++ net/smc/smc_core.h | 1 + 4 files changed, 32 insertions(+), 1 deletion(-) diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c index e9f93b3ab435..8c781a4a4485 100644 --- a/net/smc/af_smc.c +++ b/net/smc/af_smc.c @@ -310,7 +310,15 @@ static int __smc_release(struct smc_sock *smc) smc_restore_fallback_changes(smc); } - sk->sk_prot->unhash(sk); + /* Fallback sockets never call smc_conn_free(), so unhash directly. + * Non-fallback sockets use smc_conn_unhash() so that the conn->unhashed + * flag keeps the unhash exactly once even when smc_conn_free() already ran + * first (e.g. via smc_conn_kill()). + */ + if (smc->use_fallback) + sk->sk_prot->unhash(sk); + else + smc_conn_unhash(&smc->conn); if (sk->sk_state == SMC_CLOSED) { if (smc->clcsock) { diff --git a/net/smc/smc.h b/net/smc/smc.h index 427b6d63b993..075312278835 100644 --- a/net/smc/smc.h +++ b/net/smc/smc.h @@ -279,6 +279,7 @@ struct smc_connection { u64 peer_token; /* SMC-D token of peer */ u8 killed; /* abnormal termination */ u8 freed; /* normal termination */ + u8 unhashed; /* removed from sock hash */ u8 out_of_sync; /* out of sync with peer */ }; diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c index 04aedd957543..e302221c35e3 100644 --- a/net/smc/smc_core.c +++ b/net/smc/smc_core.c @@ -1251,6 +1251,20 @@ static void smc_buf_unuse(struct smc_connection *conn, } } +/* unhash the socket once; owns the single unhash for all non-fallback paths. + * Every caller holds lock_sock for this socket, so conn->unhashed is protected + * by that lock and no separate synchronisation is needed. + */ +void smc_conn_unhash(struct smc_connection *conn) +{ + struct smc_sock *smc = container_of(conn, struct smc_sock, conn); + + if (!conn->unhashed) { + conn->unhashed = 1; + smc->sk.sk_prot->unhash(&smc->sk); + } +} + /* remove a finished connection from its link group */ void smc_conn_free(struct smc_connection *conn) { @@ -1263,6 +1277,11 @@ void smc_conn_free(struct smc_connection *conn) return; conn->freed = 1; + /* Unhash before dropping lgr/lnk refs so the diag reader, which + * iterates under the socket hash read_lock, cannot see a connection whose + * lgr or lnk is being freed concurrently. + */ + smc_conn_unhash(conn); if (!smc_conn_lgr_valid(conn)) /* Connection has already unregistered from * link group. @@ -2053,6 +2072,7 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini) if (!conn->lgr->is_smcd) smcr_link_hold(conn->lnk); /* link_put in smc_conn_free() */ conn->freed = 0; + conn->unhashed = 0; conn->local_tx_ctrl.common.type = SMC_CDC_MSG_TYPE; conn->local_tx_ctrl.len = SMC_WR_TX_SIZE; conn->urg_state = SMC_URG_READ; diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h index 5c18f08a4c8a..f23d60aef0b7 100644 --- a/net/smc/smc_core.h +++ b/net/smc/smc_core.h @@ -595,6 +595,7 @@ void smc_sndbuf_sync_sg_for_device(struct smc_connection *conn); void smc_rmb_sync_sg_for_cpu(struct smc_connection *conn); int smc_vlan_by_tcpsk(struct socket *clcsock, struct smc_init_info *ini); +void smc_conn_unhash(struct smc_connection *conn); void smc_conn_free(struct smc_connection *conn); int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini); int smc_core_init(void); -- 2.50.1 (Apple Git-155)