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

[netdev-bot+sashiko@kernel.org]

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?

[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?

>  	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?

>  	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
list during failover, so the !list_empty(&smc->conn.lgr->list) check does
not stop the reader.

Suppose a reader loads the old link pointer just before the switch. Can it
then see link->smcibdev == NULL and oops? That is the same signature that
9dbe086c69b8 fixed. It could also read the name of an ibdev that has
already been released.

Unhashing before smc_conn_free() does not cover this path. The v2
serialisation of the conn->lnk swap against the reader was dropped in this
version, and nothing replaces it.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923061716.1970059-1-mjambigi%40linux.ibm.com