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

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

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.

So the correct placement is after smc_diag_msg_attrs_fill(), which fills
diag_mode, diag_uid, and diag_inode, but before SMC_DIAG_FALLBACK:

    r->diag_state = sk->sk_state;
    if (smc->use_fallback)
        r->diag_mode = SMC_DIAG_MODE_FALLBACK_TCP;
    else if (...)
        ...
    if (smc_diag_msg_attrs_fill(sk, skb, r, user_ns))
        goto errout;

+   if (r->diag_state == SMC_INIT || r->diag_state == SMC_CLOSED)
+       goto out;

    fallback.reason = smc->fallback_rsn;
    ...

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

At this position all fields smcss reads for these two states are already
filled correctly, including diag_mode for --smcr/--smcd filtering.  The
CONNINFO, LGRINFO, and DMBINFO blocks — which contain the unsafe lgr/lnk
dereferences — are never reached.

What is your opinion on this?
> 
> 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;
> +

Regarding conn->freed: it does not fully close the race. The check and
the subsequent lgr/lnk dereferences in the LGRINFO/DMBINFO blocks are
not atomic. The diag reader can pass the conn->freed == 0 check, then
smc_conn_free() runs concurrently and calls smc_lgr_put() which may drop
the lgr refcount to zero and free lgr, and then the reader resumes and
dereferences conn->lgr — a UAF.

>         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.

I agree, as of now I am trying to fix the UAF bug with minimal changes.