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

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

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.

Best regards,
Dust