Re: [PATCH net v4] net/smc: fix lgr/lnk lifetime vs diag reader race

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

On 15/09/26 8:43 pm, Dust Li wrote:
> On 2026-09-11 11:09:06, Mahanta Jambigi wrote:
>> The diag dump walks the socket hash table under a read_lock and
>> dereferences conn->lgr and conn->lnk.  Two terminal teardown paths
>> drop those references via smc_conn_free() while the socket is still
>> hashed:
>> 
>> - smc_conn_kill() -> smc_close_active_abort() -> smc_conn_free() -
>> smc_close_passive_work() -> smc_conn_free()
>> 
>> This allows the diag reader to dereference a freed lgr or lnk.
>> 
>> Fix it by unhashing the socket before smc_conn_free() is called at
>> each of these two sites.  Any socket visible to the diag reader
>> under the hash read_lock then has valid conn->lgr and conn->lnk
>> pointers.
>> 
>> Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets") Fixes:
>> 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R
>> connections") Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
>> --- Changes in v4: - dropped smc_conn_unhash() wrapper, conn-
>> >unhashed flag, and all changes to af_smc.c, smc.h, smc_core.c and
>> smc_core.h; smc_unhash_sk() is already idempotent via sk_hashed(),
>> so direct calls at the two teardown sites in smc_close.c are
>> sufficient - dropped the __smc_release() hunk: it needs no change
>> since the subsequent unhash there is already a safe no-op - fixed
>> premature-unhash issue present in v3: smc_conn_free() must not unhash
>> because smc_conn_abort() calls it before smc_switch_to_fallback() in
>> both smc_listen_decline() and smc_connect_rdma() error paths; unhashing
>> there would make live fallback sockets invisible to smcss -
>> likewise, the ISM/RDMA retry loops (smc_find_ism_v2_device_serv(), 
>> smc_find_rdma_v2_device_serv()) call smc_conn_abort() on a failed attempt
>> and then smc_conn_create() on the next device; unhashing in 
>> smc_conn_free() would permanently hide the established connection
>> from smc_diag since smc_conn_create() does not re-hash the socket
> 
> Hi Mahanta,
> 
> This version looks clean. And you explained why we can't call unhash
> in smc_conn_abort() well. But smc_conn_abort() still calls
> smc_conn_free(), when the smc_sk is still hashed, is there still a
> race window with dump ?

Hi Dust,

Thank you for catching this corner case!

During early handshake setup (when sk_state is *SMC_INIT*),
smc_conn_abort() can be called on connection failure/fallback and
invokes smc_conn_free() while the socket remains hashed, leaving a
window where a concurrent diag dump could evaluate smc_conn_lgr_valid()
and dereference conn->lgr / conn->lnk.

Since sockets in *SMC_INIT* are in a transient embryonic handshake phase
and userspace (smcss) *skips displaying link-group, DMB, and connection
details for INIT state sockets anyway*, we could have __smc_diag_dump()
skip inspecting connection/link-group extensions when r->diag_state ==
SMC_INIT:

diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
--- a/net/smc/smc_diag.c
+++ b/net/smc/smc_diag.c
@@ -90,6 +90,9 @@ static int __smc_diag_dump(struct sock *sk, struct
sk_buff *skb,
 	r->diag_state = sk->sk_state;
+	if (r->diag_state == SMC_INIT)
+		return 0;
+
 	if (smc->use_fallback)
 		r->diag_mode = SMC_DIAG_MODE_FALLBACK_TCP;
 	else if (smc_conn_lgr_valid(&smc->conn) && smc->conn.lgr->is_smcd)

Together with unhashing before smc_conn_free() in smc_close.c for
established and closing sockets, this cleanly closes the race window
across all socket states without touching the hash table mechanics
during fallback/retry.

Does this approach look good to you? If you agree, I will prepare and
submit v5 with this change. Please let me know if you have any other
suggestions or alternative approaches, and I'll be happy to look into them.

Best regards,
Mahant