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

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

On 22/10/26 7:43 pm, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 2
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] The v12 `!lgr->terminating` guard in
>   smc_lgr_schedule_free_work() (net/smc/smc_core.c) does not close the…
> - [Low] The commit message says "Other kill paths leave the socket
>   hashed, since it can stay open until close()".
> - [Low] The commit message's 'Fifth' paragraph gets the mechanism of its
>   own example wrong.
> 
> --
> 
> net/smc: fix lgr/lnk lifetime vs diag reader race
> 
> This patch closes several windows where the SMC sock_diag dump can
> dereference conn/lgr/lnk state that has already been freed. It skips the
> CONNINFO/LGRINFO/DMBINFO attributes for SMC_INIT and SMC_CLOSED sockets,
> orders sk_state with acquire/release, unhashes sockets before teardown in
> several paths, and copies the cached link->ibname. It also unregisters
> connections unconditionally in smc_conn_free() and stops arming free_work
> on a terminating link group.
> 
>> Fifth, on the smc_lgr_cleanup_early() path (e.g. server CLC decline with
>> SYNCERR) __smc_lgr_terminate() sets terminating=1 and unlinks the LGR
>> without setting freeing=1.
> 
> [Severity: Low]
> Is the SYNCERR example here accurate?On the server, smc_clc_wait_msg()

No. On the server, smc_clc_wait_msg() calls smc_lgr_terminate_sched()
for a first-contact DECLINE, which unlinks the LGR and sets freeing
before smc_conn_abort() runs. The skipped unregister comes from there,
not from smc_lgr_cleanup_early().

v13 drops the smc_conn_free() change, so that paragraph is gone. The
SYNCERR case is covered in smc_diag.c instead: a fallback socket is a
plain TCP socket with no SMC connection state, so the CONNINFO/LGRINFO/
DMBINFO blocks are skipped when use_fallback is set.

> [Severity: Low]
> The commit message says:
> 
>   Other kill paths leave the socket hashed, since it can stay open until
>   close().
> 
> Does that match the code? Every smc_conn_kill() ends in

No. Every smc_conn_kill() ends in smc_close_active_abort(), so killed
sockets in the PEERCLOSEWAIT, PROCESSABORT and APPFINCLOSEWAIT states
are unhashed before close(). I removed the sentence, and the commit
message now says that smcss visibility depends on the close state and,
for SMC-D, on the device type.

> [Severity: High]
> Does this check fully stop free_work from being re-armed on a
> terminating link group? The flag check and mod_delayed_work() are not

No. v13 drops the !lgr->terminating check and the unconditional
unregister from v11, so smc_core.c is back to the v10 change only.

The free_work window you describe is independent of the diag reader race
this patch fixes. I'd like to keep it out of this patch.

pw-bot: cr