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

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

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.

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;