Re: [PATCH net v2 2/2] net/smc: fix races in smc_diag dump path

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete fix: Missing protection against concurrent connection teardown for SMC_DIAG_CONNINFO, leading to Use-After-Free on conn->sndbuf_desc and conn->rmb_desc.
--

commit 7eff481cca86fa2c9b695508a4e20ae6a02805ad
Author: Mahanta Jambigi <mjambigi@linux.ibm.com>

net/smc: fix races in smc_diag dump path

This commit addresses concurrent teardown races in the SMC diag dump path
by adding mutex protection to clcsock reads and spinlock protection to
the LGRINFO and DMBINFO block accesses.

> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
> index bf0beaa23bdb6..06db3d769929a 100644
> --- a/net/smc/smc_diag.c
> +++ b/net/smc/smc_diag.c
[ ... ]
> @@ -143,45 +162,57 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
>  			goto errout;
>  	}
>  

[Severity: High]
This is a pre-existing issue, but does this incomplete fix leave a
use-after-free regression open in the SMC_DIAG_CONNINFO block just above?

The patch successfully adds lgr_lnk_lock protection to the LGRINFO and
DMBINFO blocks below to prevent concurrent teardown races, but completely
omits this protection for the CONNINFO block.

In __smc_diag_dump(), the CONNINFO block locklessly checks
smc->conn.alert_token_local:

	if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
	    smc->conn.alert_token_local) {
		struct smc_connection *conn = &smc->conn;
		struct smc_diag_conninfo cinfo = {
			.token = conn->alert_token_local,
			.sndbuf_size = conn->sndbuf_desc ?
				conn->sndbuf_desc->len : 0,

If smc_conn_free() executes concurrently on another CPU immediately after
the token check, it will free conn->sndbuf_desc and conn->rmb_desc via
smcd_buf_detach().

The dump thread then locklessly dereferences these pointers when
accessing ->len, leading to a use-after-free that can be triggered by
an unprivileged user running a socket dump during a network abort.

Should the CONNINFO block also be wrapped in the same lgr_lnk_lock
protection that was applied to the other diagnostic blocks?

> -	if (smc_conn_lgr_valid(&smc->conn) && !smc->conn.lgr->is_smcd &&
> -	    (req->diag_ext & (1 << (SMC_DIAG_LGRINFO - 1))) &&
> -	    !list_empty(&smc->conn.lgr->list)) {
> -		struct smc_link *link = smc->conn.lnk;
> -
> -		struct smc_diag_lgrinfo linfo = {
> -			.role = smc->conn.lgr->role,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828065439.3582783-1-mjambigi@linux.ibm.com?part=2