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

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

On 2026-09-22 18:57:24, Mahanta Jambigi wrote:
>
>
>On 21/09/26 3:26 pm, Dust Li wrote:
>> On 2026-09-18 12:57:13, Mahanta Jambigi wrote:
>>>
>>>
>>> On 17/09/26 9:23 pm, Dust Li wrote:
>>>> 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.
>>>
>>> Regarding your concern about wrong output when the early return fires
>>> before diag_mode and SMC_DIAG_FALLBACK are filled: you are right that
>>> diag_mode must be filled before the guard. *smcss.c* reads diag_mode at
>>> lines 177 and 179 for --smcr/--smcd filtering, which happens before the
>>> SMC_INIT and SMC_CLOSED state checks. If diag_mode is left as 0 for
>>> a failed-fallback SMC_CLOSED socket, --smcr would incorrectly include it
>>> and --smcd would incorrectly exclude it.
>>>
>>> *SMC_DIAG_FALLBACK* however is *not* needed before the guard — smcss
>>> only reads it inside the diag_mode == SMC_DIAG_MODE_FALLBACK_TCP branch
>>> which is never reached for *SMC_INIT* or *SMC_CLOSED* sockets, as those
>>> two states hit goto newline before that point.
>> 
>> I don't agree on this.
>> 
>> Here what we are changing is the UAPI behaviour, which should not be
>> constrained by what smcss did.
>> 
>> And SMC_INIT can coexist with fallback. For example:
>> 
>> smc_sendmsg() calls smc_switch_to_fallback(SMC_CLC_DECL_OPTUNSUPP) under
>> MSG_FASTOPEN while sk_state is SMC_INIT and never changes it afterwards --
>> fastopen does not call connect(), so the state never goes to SMC_ACTIVE.
>
>In that case how about moving the check further down, like below? This
>way I am not changing the UAPI behaviour and SMC_INIT can co-exist with
>fallback.

Yes, I think we should do that.

Best regards,
Dust

>
>smc_sendmsg() calls smc_switch_to_fallback(SMC_CLC_DECL_OPTUNSUPP) under
>MSG_FASTOPEN while sk_state is SMC_INIT and it never transitions further
>— connect() is not called in that path, so the fallback reason is real
>diagnostic information that should be reported. The guard after the
>SMC_DIAG_FALLBACK nla_put preserves that.
>
>--- 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 (sk->sk_state == SMC_INIT || sk->sk_state == SMC_CLOSED)
>+		goto out;
>+
> 	if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
> 	    smc->conn.alert_token_local) {
>@@ -185,5 +188,6 @@ static int __smc_diag_dump(struct sock *sk, struct
>sk_buff *skb,
> 		goto errout;
> 	}
>
>+out:
> 	nlmsg_end(skb, nlh);
> 	return 0;