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

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.

quoted

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.

quoted

 	if (sk->sk_state != SMC_INIT && smc->clcsock && smc->clcsock->sk) {
 		sk->sk_err = ECONNABORTED;
 		if (smc->clcsock && smc->clcsock->sk)

[ ... ]

quoted

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.

quoted

 	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