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

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

On 16/09/26 8:54 pm, Dust Li wrote:
> On 2026-09-16 14:04:42, Mahanta Jambigi wrote:
>>
>>
>> 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.
> 
> What about this path ?
> 
> smc_listen_decline() -> smc_listen_out_err(), where smc_conn_abort() has
> already run, and the sock state is then changed from SMC_INIT to SMC_CLOSED.

Hi Dust,

Good catch on the smc_listen_out_err() path as well!

In smc_listen_decline() -> smc_listen_out_err(), the server socket fails
handshake and is transitioned to SMC_CLOSED while remaining in the hash
table until smc_accept_dequeue() runs.

Since userspace (smcss) skips displaying all connection, link-group, and
DMB details for both SMC_INIT and SMC_CLOSED sockets (smcss.c:186 and
smcss.c:199), we can update __smc_diag_dump() to check for both states:

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 || r->diag_state == SMC_CLOSED)
+		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)

This guarantees that __smc_diag_dump() never attempts to inspect
connection or link-group fields for sockets that are either still in
embryonic setup (SMC_INIT) or have aborted/closed (SMC_CLOSED).

Combined with unhashing before smc_conn_free() in smc_close.c for
established connections during active/passive close, this cleanly closes
the race window across all tear-down and abort paths.

Does this approach look good to you? If you agree, I will prepare and
submit v5 with this update.

Best regards,
Mahanta