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

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

On 25/09/26 2:50 am, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 2 (1) · Medium: 1 · Low: 0
> 
> 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] __smc_diag_dump() (net/smc/smc_diag.c:91-96) still reads
>   `smc->conn.lgr->is_smcd` to work out r->diag_mode.
> - [High] In the SMC_DIAG_LGRINFO block, __smc_diag_dump() does `struct
>   smc_link *link = smc->conn.lnk;` and then dereferences…
> - [Medium] The commit message says the socket is unhashed "before
>   smc_conn_free() is called at the two terminal teardown sites".
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the patch
>   leaves it open while adding an unhash that runs too late on the same…
> 
> --
> 
> net/smc: fix lgr/lnk lifetime vs diag reader race
> 
> This patch unhashes SMC sockets in smc_close_active_abort() and
> smc_close_passive_work() before smc_conn_free() drops the lgr/lnk
> references. It also makes __smc_diag_dump() skip the conn/lgr/lnk
> attribute blocks when the socket is in SMC_INIT or SMC_CLOSED state. The
> goal is to keep the diag reader from dereferencing a freed link group or
> link.
> 
>> diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
>> index bb0313ef5f7c1..3940098c651d9 100644
>> --- a/net/smc/smc_close.c
>> +++ b/net/smc/smc_close.c
>> @@ -130,6 +130,11 @@ void smc_close_active_abort(struct smc_sock *smc)
>>  	struct sock *sk = &smc->sk;
>>  	bool release_clcsock = false;
>>  
>> +	/*
>> +	 * Unhash before any branch calls smc_conn_free() and drops lgr/lnk refs;
>> +	 * the diag reader must not see a hashed socket with freed lgr/lnk.
>> +	 */
>> +	sk->sk_prot->unhash(sk);
> 
> [Severity: Medium]
> The commit message says the socket is unhashed "before smc_conn_free() is
> called at the two terminal teardown sites". This unhash, though, runs
> unconditionally for every state. Is that intended?
> 
> The SMC_ACTIVE, SMC_APPCLOSEWAIT1 and SMC_APPCLOSEWAIT2 branches never call
> smc_conn_free(). They only set SMC_CLOSED and call sock_put(). SMC_INIT,
> SMC_PEERABORTWAIT and SMC_CLOSED don't call it either. Neither does the early
> break taken when smc_close_cancel_work() changes the state.
> 
> On link group termination, smc_conn_kill() calls smc_close_active_abort()
> for every connection:
> 
> net/smc/smc_core.c:smc_conn_kill() {
>     ...
>     smc_lgr_unregister_conn(conn);
>     smc_close_active_abort(smc);
> }
> 
> Doesn't this make killed SMC_ACTIVE sockets vanish from smc_diag/smcss
> right away instead of at close? The application still has them open, and
> their lgr/lnk references are still held.
> 
> These sockets reach smc_conn_free() later in __smc_release(), which already
> unhashes first. smc_release() is also a third caller of
> smc_close_active_abort(), and the commit message doesn't mention it.
> 
> Would it be narrower to unhash only right before the two smc_conn_free()
> calls in the SMC_PEERCLOSEWAIT1/2/SMC_PEERFINCLOSEWAIT and
> SMC_PROCESSABORT/SMC_APPFINCLOSEWAIT branches?

You are right. The unconditional unhash at the top of
smc_close_active_abort() was placed there because v5 traced the
smc_conn_kill() → smc_close_active_abort() → smc_conn_free() chain and
stopped at smc_close_active_abort() as the insertion point. This was too
broad — it also fires for the SMC_ACTIVE/APPCLOSEWAIT branches which
never call smc_conn_free(), and for the smc_release() call site where
the socket is a live connection the application still has open.

Fix in v6: remove the unconditional unhash from the top of
smc_close_active_abort() and place it scoped, immediately before each of
the two smc_conn_free() calls — in the PEERCLOSEWAIT1/2/PEERFINCLOSEWAIT
branch and the PROCESSABORT/APPFINCLOSEWAIT branch.

> 
> [Severity: High]
> This is a pre-existing issue, not one introduced by this patch. Still, on
> the smc_conn_kill() path this new unhash runs too late to protect against
> it.
> 
> On SMC-D with dmb_nocopy (for example loopback-ism), smc_conn_kill() frees
> the sndbuf descriptor before it unregisters the conn and before it calls
> smc_close_active_abort():
> 
> net/smc/smc_core.c:smc_conn_kill() {
>     ...
>     if (smc_ism_support_dmb_nocopy(conn->lgr->smcd))
>         smcd_buf_detach(conn);
>     ...
>     smc_lgr_unregister_conn(conn);
>     smc_close_active_abort(smc);
> }
> 
> smcd_buf_detach() does this:
> 
>     buf_desc = conn->sndbuf_desc;
>     conn->sndbuf_desc = NULL;
>     kfree(buf_desc);
> 
> During that window the socket is still hashed and still in SMC_ACTIVE, so
> it passes the new diag guard. alert_token_local is also still set. That
> means __smc_diag_dump() enters the SMC_DIAG_CONNINFO block and evaluates:
> 
>     .sndbuf_size = conn->sndbuf_desc ?
>         conn->sndbuf_desc->len : 0,
> 
> These are plain loads, and only the hash read_lock is held. Could this
> dereference NULL, or read ->len from an smc_buf_desc that has been kfree'd?
> The lock_sock() in __smc_lgr_terminate() does not exclude the diag reader.
> 
> Would unhashing at the start of smc_conn_kill(), before smcd_buf_detach(),
> close this as well?

You are right. In smc_conn_kill(), smcd_buf_detach() NULLs and frees
conn->sndbuf_desc before smc_close_active_abort() is called, but the
socket is still hashed during that window. The v5 unhash inside
smc_close_active_abort() is too late.

Fix in v6: move the unhash to the top of smc_conn_kill(), before
smcd_buf_detach(). Both call sites (__smc_lgr_terminate() and
smc_conn_abort_work()) hold lock_sock and have no expectation that the
socket remains hashed after smc_conn_kill() returns. smc_unhash_sk() is
idempotent so the subsequent unhash attempts further down become safe
no-ops.

> 
>>  	if (sk->sk_state != SMC_INIT && smc->clcsock && smc->clcsock->sk) {
>>  		sk->sk_err = ECONNABORTED;
>>  		if (smc->clcsock && smc->clcsock->sk)
> 
> [ ... ]
> 
>> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
>> index bf0beaa23bdb6..fe32091e37dab 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 (sk->sk_state == SMC_INIT || sk->sk_state == SMC_CLOSED)
>> +		goto out;
>> +
> 
> [Severity: High]
> Does this guard cover the lgr dereference earlier in __smc_diag_dump() that
> sets r->diag_mode?
> 
> net/smc/smc_diag.c:__smc_diag_dump() {
>     ...
>     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)
>         r->diag_mode = SMC_DIAG_MODE_SMCD;
>     ...
> }
> 
> That load happens before the new SMC_INIT/SMC_CLOSED check, and only the
> smc hash read_lock is held.
> 
> The commit message says this guard handles the smc_conn_abort() SMC_INIT
> case. Take a handshake failure such as smc_listen_decline() ->
> smc_conn_abort(new_smc, local_first). The socket is still hashed in
> SMC_INIT and use_fallback has not been set yet:
> 
> net/smc/af_smc.c:smc_conn_abort() {
>     ...
>     smc_conn_free(conn);
>     if (local_first && lgr_valid)
>         smc_lgr_cleanup_early(lgr);
> }
> 
> The following chain then frees the lgr synchronously:
> 
> smc_lgr_cleanup_early()
>   __smc_lgr_terminate()
>     smc_lgr_free()
>       smc_lgr_put()
>         __smc_lgr_free()
>           kfree(lgr)
> 
> Nothing on this path takes the hash write_lock, and conn->lgr is never
> cleared. smc_conn_lgr_valid() is only plain loads of conn->lgr and
> conn->alert_token_local.
> 
> Can a diag reader pass smc_conn_lgr_valid() just before the conn is
> unregistered, and then read lgr->is_smcd after the kfree()?
> 
> smc_diag_handler_dump() has no capability check, and a remote peer can
> cause the handshake failure. Would checking the state (or use_fallback)
> before any conn.lgr access close this?

You are right. The conn->lgr->is_smcd load in the r->diag_mode
assignment happens before the SMC_INIT guard, leaving a window where a
concurrent smc_lgr_cleanup_early() → kfree(lgr) on another CPU can race
with the reader. This is remotely triggerable since
smc_diag_handler_dump() has no capability check and a remote peer can
cause the handshake failure that leads to smc_conn_abort().

Fix in v6: inline an sk->sk_state != SMC_INIT check directly into the
else if condition, so conn->lgr->is_smcd is never loaded for SMC_INIT
sockets. The SMC_CLOSED check is not needed at that line — on every path
that sets SMC_CLOSED, either the socket is unhashed before
smc_conn_free() runs (__smc_release(), passive work), or conn->lgr is
still live and the dereference is safe.

> 
>>  	if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
>>  	    smc->conn.alert_token_local) {
>>  		struct smc_connection *conn = &smc->conn;
> 
> [Severity: High]
> Sockets in SMC_ACTIVE and the closing states still get past this guard. Is
> the SMC_DIAG_LGRINFO block further down still exposed to a concurrent link
> switch?
> 
> net/smc/smc_diag.c:__smc_diag_dump() {
>     ...
>     struct smc_link *link = smc->conn.lnk;
>     ...
>     memcpy(linfo.lnk[0].ibname, link->smcibdev->ibdev->name,
>            sizeof(link->smcibdev->ibdev->name));
>     ...
> }
> 
> Only the hash read_lock is held here. During SMC-R link failover,
> smcr_link_down() -> smc_switch_conns() -> smc_switch_link_and_count()
> replaces conn->lnk under conn->send_lock and drops the conn's reference on
> the old link:
> 
> net/smc/smc_core.c:smc_switch_link_and_count() {
>     ...
>     smcr_link_put(conn->lnk);
>     conn->lnk = to_lnk;
>     ...
> }
> 
> When smcr_link_clear() drops the last reference, __smcr_link_clear() calls
> put_device() on the ibdev and then memset(lnk, 0). The lgr stays on its

After tracing the refcount accounting I believe this race is not
reachable. The smcr_link_put() in smc_switch_link_and_count() only drops
the per-connection hold taken by smc_conn_create(). The link's base ref
(initialised to 1 in smcr_link_init()) is only released by
smcr_link_clear(), which is called exclusively from the lgr termination
path. Hence __smcr_link_clear() is not called in this path.

By that point the lgr has already been removed from the global list, so
the diag reader's !list_empty(&smc->conn.lgr->list) check already gates
it out of the lgrinfo block.

Will post v6 shortly.

pw-bot: cr