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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

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).

- [Medium] The commit message says the conn/lgr/lnk blocks of
  __smc_diag_dump() are guarded for SMC_INIT and SMC_CLOSED sockets.
- [Low] In __smc_diag_dump() (net/smc/smc_diag.c:93-97) the new
  `r->diag_state != SMC_INIT &&` check in the diag_mode selection sends…

--

net/smc: fix lgr/lnk lifetime vs diag reader race

The patch closes races between the SMC diag dump and connection teardown.
It skips the conn/lgr/lnk attribute blocks for SMC_INIT and SMC_CLOSED
sockets, and pairs smp_load_acquire()/smp_store_release() on sk_state. It
also unhashes the socket before smc_conn_free() or smcd_buf_detach() in
several teardown paths, and copies the cached link->ibname.

> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
> index bf0beaa23bdb6..36a3b8f31f28d 100644
> --- a/net/smc/smc_diag.c
> +++ b/net/smc/smc_diag.c
> @@ -87,10 +87,11 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
>  
>  	r = nlmsg_data(nlh);
>  	smc_diag_msg_common_fill(r, sk);
> -	r->diag_state = sk->sk_state;
> +	r->diag_state = smp_load_acquire(&sk->sk_state);
>  	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)
> +	else if (r->diag_state != SMC_INIT &&
> +		 smc_conn_lgr_valid(&smc->conn) && smc->conn.lgr->is_smcd)
>  		r->diag_mode = SMC_DIAG_MODE_SMCD;
>  	else
>  		r->diag_mode = SMC_DIAG_MODE_SMCR;

[Severity: Low]
Is this change to diag_mode intended? With the new r->diag_state !=
SMC_INIT check, every non-fallback SMC_INIT socket falls into the else
branch and is reported as SMC_DIAG_MODE_SMCR.

Before this patch, an SMC_INIT socket with a registered SMC-D link group
was reported as SMC_DIAG_MODE_SMCD.

On the client side, this window runs from smc_conn_create() in
smc_connect_ism() until the smp_store_release() to SMC_ACTIVE. On the
server side it lasts until smc_listen_out_connected(), which includes
waiting for the peer's CLC CONFIRM. During that time, sock_diag users see
an SMC-D connection labelled as SMC-R. The "Mode of a connection" enum in
include/uapi/linux/smc_diag.h has no "unknown" value for this case.

The commit message says:

  "Guard the conn/lgr/lnk blocks in __smc_diag_dump() for SMC_INIT and
   SMC_CLOSED sockets; smcss already suppresses these attributes for
   closed/unconnected sockets so there is no observability regression."

It doesn't mention the diag_mode change, which only appears in the v6
changelog below the --- line.

Could the transport type be recorded in a field of struct smc_sock when
the lgr is assigned? That would avoid both the lgr dereference and the
wrong mode.

[ ... ]

> @@ -103,6 +104,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 (r->diag_state == SMC_INIT || r->diag_state == SMC_CLOSED)
> +		goto out;
> +

[Severity: Medium]
The commit message says the conn/lgr/lnk blocks are guarded for both
SMC_INIT and SMC_CLOSED. The diag_mode selection above excludes only
SMC_INIT, and it runs before this early exit.

For an SMC_CLOSED socket, can __smc_diag_dump() still call
smc_conn_lgr_valid() and then dereference smc->conn.lgr->is_smcd?

In the server handshake error path, a CLOSED socket stays hashed after
its conn has been freed:

smc_listen_decline()
  smc_conn_abort()
    smc_conn_free()
      smc_lgr_unregister_conn()
        write_lock_bh(&lgr->conns_lock);
        __smc_lgr_unregister_conn()
          conn->alert_token_local = 0;
        write_unlock_bh(&lgr->conns_lock);
      smcr_link_put(conn->lnk);
      smc_lgr_put(lgr);          <-- conn->lgr is left set
  smc_listen_out_err()
    newsmcsk->sk_state = SMC_CLOSED;

The SMC_CLOSED store in smc_listen_out_err() is a plain store, not the
smp_store_release() this patch adds for SMC_ACTIVE.

Between the alert_token_local reset and the CLOSED store, the only
operations are release-ordered ones: write_unlock_bh(),
refcount_dec_and_test() via smcr_link_put()/smc_lgr_put(), and
sock_put(). None of them orders the later CLOSED store.

On a weakly ordered CPU such as arm64, could the reader's
smp_load_acquire() see SMC_CLOSED while alert_token_local still reads as
non-zero? The reader would then dereference an lgr that the connection no
longer holds a reference to. If link-group freeing runs concurrently,
that read is a use-after-free.

This needs a non-local-first lgr with a negative reason code, so that
smc_lgr_cleanup_early() is not called. The window is narrow.

Would it help to exclude SMC_CLOSED from the diag_mode condition as well,
or to move this early exit ahead of the diag_mode selection? Publishing
SMC_CLOSED in smc_listen_out_err() with smp_store_release() would also
close the gap.

>  	if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
>  	    smc->conn.alert_token_local) {
>  		struct smc_connection *conn = &smc->conn;

[ ... ]

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