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

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

On 02/09/26 5:46 pm, Dust Li wrote:
> 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 ?
Hi Dust,

After debugging further, I believe that __smcr_link_clear() cannot be
called from smc_switch_link_and_count() because
refcount_dec_and_test(&lnk->refcnt) is always false on that path — the
initial ref (set in smcr_link_init(), released only in
smcr_link_clear()) is always held while the switch executes. So there is
no UAF from that path. The only consequence is that the diag reader may
observe a stale conn->lnk pointer value — pointing to the old link which
is still fully live — and read the old link's ibport/link_id/ibname from it.

The real UAF is via smc_conn_free(): it drops both the connection-owned
conn->lnk reference via smcr_link_put() and the connection-owned
conn->lgr reference via smc_lgr_put(), while the socket is still visible
in the hash table. For conn->lgr this means kfree(lgr) can race with the
diag reader dereferencing lgr->is_smcd, lgr->role, lgr->list etc. For
conn->lnk this means memset(lnk, 0, ...) in __smcr_link_clear() can race
with the diag reader dereferencing link->smcibdev->ibdev->name. The
current patch tries to fix this using lgr_lnk_lock. Since you suggested
we fix it with *unhash-before-free*, I thought about it and here is my
proposal.

Proposed fix (high level):

Introduce a smc_conn_unhash() helper with a per-connection unhashed flag
that ensures the socket is removed from the hash table exactly once.
Call it at the top of smc_conn_free(), before any lgr/lnk references are
dropped.

This establishes the invariant: any socket still visible to the diag
reader under read_lock(hash->lock) has valid conn->lgr and conn->lnk
pointers. The mutual exclusion between write_lock_bh(hash->lock) inside
smc_conn_unhash() and the diag reader's read_lock(hash->lock) ensures
that by the time smcr_link_put() runs in smc_conn_free(), the socket is
already gone from the hash — the diag reader either completes before the
unhash or never sees the socket at all.

The unhashed flag is needed because smc_conn_free() can be called from
multiple paths (e.g. smc_conn_kill() ahead of __smc_release()), so we
need to guarantee the unhash happens exactly once.

Does this design look reasonable to you?

There is a separate minor issue — smc_switch_link_and_count() updates
conn->lnk under send_lock while the diag reader loads it without any
lock, which can cause stale ibport/link_id/ibname in *smcss -R* during a
failover event. This is a data race but not a safety issue since the
link struct is always live at that point.

This race is very rare in practice — it requires a concurrent smcss -R
dump to hit the exact CPU-cycle window during a conn->lnk pointer write,
which itself only happens during an exceptional link failover event.
Even when it occurs, the effect is transient: one dump may show the old
link's ibport/link_id/ibname, and the next dump will show the correct
values.