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

[Dust Li <dust.li@linux.alibaba.com>]

On 2026-09-17 13:15:23, Mahanta Jambigi wrote:
>
>
>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)
>

I checked the code again, and I'm afraid adding SMC_CLOSED is still not right.

smc_listen_decline(): smc_switch_to_fallback(), smc_clc_send_decline() then
fails, and smc_listen_out_err() sets the socket to SMC_CLOSED. The socket
stays on the accept queue, hashed, until accept() reaches smc_accept_dequeue().

But the early return sits after nlmsg_put() but before diag_mode,
smc_diag_msg_attrs_fill() and the SMC_DIAG_FALLBACK attribute are filled.
The record is still emitted, but diag_mode is left at 0 -- which is
SMC_DIAG_MODE_SMCR, not "unknown" -- and the fallback reason attribute is gone.

Why don't you use conn->free instead of sk_state ? It is set unconditionally
at the top of smc_conn_free() before any lgr/lnk reference is dropped.


So maybe something like this ? Please double check.

diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
index bf0beaa23bdb6..a19f16881ae7e 100644
--- a/net/smc/smc_diag.c
+++ b/net/smc/smc_diag.c
@@ -103,6 +103,9 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
        if (nla_put(skb, SMC_DIAG_FALLBACK, sizeof(fallback), &fallback) < 0)
                goto errout;

+       if (smc->conn.freed)
+               goto out;
+
        if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
            smc->conn.alert_token_local) {
                struct smc_connection *conn = &smc->conn;
@@ -185,6 +188,7 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
                        goto errout;
        }

+out:
        nlmsg_end(skb, nlh);
        return 0;

BTW, as we talked in the previous threads, the teardown path is messy and lots
of hidden holes, so I think we will finnally refine those.
As for now, if this works, I think we can go with it.

Best regards,
Dust