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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] net/smc: use-after-free of conn->lgr in __smc_diag_dump() for SMC_CLOSED
--

commit 899c06b63c039c1819c9691a7be591a10540701e
Author: Mahanta Jambigi <mjambigi@linux.ibm.com>

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

This commit addresses races between sock_diag dumps and connection, link
group, or link teardown by unhashing sockets before freeing connection
resources, ordering sk_state transitions with acquire/release barriers, and
skipping extended diag attributes for SMC_INIT, SMC_CLOSED, and fallback
sockets.

> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
> index bf0beaa23bdb6..84c45aabc196c 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: Critical]
Can smc->conn.lgr->is_smcd still be dereferenced after smc->conn.lgr has been
freed when r->diag_state is SMC_CLOSED and smc->use_fallback is false?

Further down in __smc_diag_dump(), the extended attribute blocks check for
both SMC_INIT and SMC_CLOSED:

	if (r->diag_state == SMC_INIT || r->diag_state == SMC_CLOSED ||
	    smc->use_fallback)
		goto out;

However, the diag_mode check above only excludes r->diag_state == SMC_INIT.
If a socket reaches SMC_CLOSED without falling back to TCP while still hashed
in smc_hash, __smc_diag_dump() will still evaluate
smc_conn_lgr_valid(&smc->conn) and dereference smc->conn.lgr->is_smcd.

This appears reachable in two ways:

1. When a link group is scheduled for termination (for example via
smc_lgr_terminate_sched() or smc_smcd_terminate()), list_del_init(&lgr->list)
empties lgr->list. If a handshake subsequently aborts via
smc_conn_abort() -> smc_conn_free():

net/smc/smc_core.c:smc_conn_free() {
	...
	if (!list_empty(&lgr->list)) {
		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
		smc_lgr_unregister_conn(conn);
	}
	...
	smc_lgr_put(lgr); /* lgr_hold in smc_conn_create() */
}

smc_conn_free() skips smc_lgr_unregister_conn(conn), leaving
conn->alert_token_local non-zero while dropping the connection's reference to
lgr. If fallback does not happen (for instance in smc_listen_decline() when
reason_code < 0 or smc_switch_to_fallback() fails), new_smc->use_fallback
remains false and smc_listen_out_err() sets sk_state to SMC_CLOSED while the
socket stays hashed:

net/smc/af_smc.c:smc_listen_decline() {
	...
	smc_conn_abort(new_smc, local_first);
	if (reason_code < 0 ||
	    smc_switch_to_fallback(new_smc, reason_code)) {
		/* error, no fallback possible */
		smc_listen_out_err(new_smc);
		return;
	}
}

Once the link group is freed, a concurrent __smc_diag_dump() reader seeing
r->diag_state == SMC_CLOSED and a non-zero alert_token_local can dereference
the freed smc->conn.lgr->is_smcd.

2. On the client side, if smc_connect_rdma() or smc_connect_ism() aborts
after smc_conn_create() with first_contact_local set, smc_conn_abort() clears
alert_token_local and frees lgr via smc_lgr_cleanup_early(). If
smc_connect_decline_fallback() fails without setting use_fallback, sk_state
remains SMC_INIT until the socket is closed via
__smc_release() -> smc_close_active():

net/smc/smc_close.c:smc_close_active() {
	...
	switch (sk->sk_state) {
	case SMC_INIT:
		sk->sk_state = SMC_CLOSED;
		break;
	...
}

Because smc_close_active() writes SMC_CLOSED with a plain store before
__smc_release() calls sk->sk_prot->unhash(sk), on weakly ordered CPUs a
concurrent __smc_diag_dump() reader using smp_load_acquire(&sk->sk_state) can
observe r->diag_state == SMC_CLOSED before the alert_token_local = 0 store is
visible, and dereference the freed smc->conn.lgr->is_smcd.

Should the diag_mode condition also check r->diag_state != SMC_CLOSED?

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