Re: [PATCH net v2 0/2] net/smc: fix diag dump lifetime races

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

On 2026-09-01 20:43:11, Mahanta Jambigi wrote:
>
>
>On 01/09/26 6:32 pm, Dust Li wrote:
>> On 2026-08-31 20:27:38, Mahanta Jambigi wrote:
>>>
>>>
>>> On 31/08/26 7:12 pm, Dust Li wrote:
>>>> On 2026-08-28 08:54:37, Mahanta Jambigi wrote:
>>>>> This series fixes multiple lifetime races in the SMC diag dump path.
>>>>>
>>>>> The first patch adds the basic infrastructure needed to synchronize diag readers
>>>>> against connection-owned conn->lgr/conn->lnk updates. It introduces a
>>>>> per-connection spinlock and uses it in the link switch and connection free
>>>>> handoff paths. conn->lgr and conn->lnk are NULLed under the lock before the
>>>>> borrowed references are released, so a non-NULL conn->lgr seen under the lock
>>>>> guarantees the lgr object is alive. The diag reader can rely on this invariant
>>>>> without borrowing any extra reference.
>>>>>
>>>>> The second patch fixes two races in smc_diag itself:
>>>>>
>>>>> - serialize clcsock field access against smc_clcsock_release() with
>>>>>  mutex_trylock()
>>>>> - take conn->lgr_lnk_lock when reading conn->lgr and conn->lnk; use
>>>>>  smc_conn_lgr_valid() inside the lock to check that the connection is
>>>>>  still registered, then snapshot all required fields and call nla_put()
>>>>>  after releasing the lock
>>>>
>>>> Hi Mahanta,
>>>>
>>>> As discussed in the other thread, I think we should defer the release of
>>>> smc->clcsock and remove clcsock_release_lock.
>>>>
>>>> In that case, we should no longer need these two patches. Also,
>>>> introducing more locks in SMC is the last thing I want to do :)
>>>
>>> Thanks for the new series "[RFC net-next 0/7] net/smc: tie clcsock
>>> lifetime to the smc socket and remove clcsock_release_lock" — once it
>>> lands, we can drop the mutex_trylock() fix for Race 1 (clcsock).
>>>
>>> However, Race 2 remains open. Your series does not touch smc_core.c or
>>> smc_cdc.c, so smc_conn_free(), smc_switch_link_and_count(), and
>>> smc_cdc_msg_validate() still write conn->lgr/conn->lnk with no
>>> synchronization against the diag reader.
>> 
>> Hi Mahanta,
>> 
>> Thanks for the detailed explanation. You are right that a per-connection
>> spinlock can work here, and I agree none of the lock sites are on the
>> per-message hot path. But I think we can also do the same thing we did with
>> clcsock_release_lock: instead of adding a lock, tie the lifetime of
>> conn->lgr/conn->lnk to the point where the connection stops being observable,
>> and remove the need for synchronization altogether.
>> 
>> For lgr/lnk, that point is the hash table. Once the connection is unhashed, the
>> diag dump (which iterates under the hash read_lock) can no longer reach it. So
>> if we make sure the connection-owned references are only dropped after unhash,
>> the invariant becomes: holding the hash read_lock and seeing a non-NULL
>> conn->lgr implies it is safe to dereference. The diag path then reduces to
>> `hold hash read_lock -> read conn->lgr -> if non-NULL, use it -> done` with no
>> new lock, no extra reference, and no trylock. The invariant is carried by
>> object lifetime rather than by a lock, which I find easier to keep correct over
>> time.
>
>I looked carefully at the new design and found one remaining gap.
>
>The *unhash* invariant — "any socket in the hash has its
>connection-owned lgr/lnk refs held" — protects against smc_conn_free()
>dropping refs while the socket is still hashed. However it does not
>protect the conn->lnk->smcibdev->ibdev->name access in the
>SMC_DIAG_LGRINFO block in smc_diag.c file.
>
>The gap is in *smc_switch_link_and_count*(). It is called under
>send_lock (not under any hash-related lock) and calls
>smcr_link_put(conn->lnk) on the old link before reassigning conn->lnk.
>If that put drops the last reference, __smcr_link_clear() runs
>immediately, doing memset(lnk, 0, sizeof(struct smc_link)) which zeroes
>lnk->smcibdev. The socket remains hashed throughout — so the *unhash*
>invariant is not violated — but the diag reader can hold a stale pointer
>to the old link and race this memset. The timeline:
>
>diag reader [hash read_lock held]:
>    conn->lnk → old_lnk (non-NULL, socket hashed ✓)
>    [about to read old_lnk->smcibdev->ibdev->name]
>
>smc_switch_link_and_count() [send_lock held]:
>    smcr_link_put(old_lnk) → last ref → __smcr_link_clear()
>        memset(old_lnk, 0, ...) ← smcibdev = NULL
>
>diag reader:
>    old_lnk->smcibdev->ibdev->name ← NULL deref
>
>The *unhash* invariant says nothing about the link a connection used to
>point at before *smc_switch_link_and_count*() swapped it. Hash
>membership of the socket provides no protection here because the socket
>is still hashed — the link pointer simply changed underneath the diag
>reader.
>
>Any ideas on this?

Good catch ! You are right that the "unhash invariant" as stated
does not cover the stale-conn->lnk race through a link switch + clear.

I think we can close this window by holding the SMC hash table lock in
smcr_link_clear()? As the diag walker captures and dereferences the stale
pointer within a single read_lock hold on the SMC hash table. Since
smc_link_clear() is a cold path, and the region where we hold the SMC hash
table lock is small, the overhead should be negligible.


Something like this:

```diff
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 5af55abece57..ca3030f02e70 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1361,6 +1361,18 @@ static void __smcr_link_clear(struct smc_link *lnk)
        struct smc_ib_device *smcibdev;
 
        smc_wr_free_link_mem(lnk);
 
+       write_lock_bh(&smc_v4_hashinfo.lock);
+       write_lock_bh(&smc_v6_hashinfo.lock);
        smc_ibdev_cnt_dec(lnk);
        put_device(&lnk->smcibdev->ibdev->dev);
        smcibdev = lnk->smcibdev;
@@ -1368,6 +1380,9 @@ static void __smcr_link_clear(struct smc_link *lnk)
        lnk->state = SMC_LNK_UNUSED;
        if (!atomic_dec_return(&smcibdev->lnk_cnt))
                wake_up(&smcibdev->lnks_deleted);
+       write_unlock_bh(&smc_v6_hashinfo.lock);
+       write_unlock_bh(&smc_v4_hashinfo.lock);
```

What do you think ?

Best regards,
Dust