Re: [PATCH net] net/smc: fix handshake abort on a terminating link group

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · Medium: 0 · 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] The patch leaves the stale lgr->conns_all entry in place.
- [High] The new ownership check in smc_lgr_cleanup_early()
  (net/smc/smc_core.c:691) only tests list_empty(&lgr->list).

--

net/smc: fix handshake abort on a terminating link group

This patch makes smc_lgr_cleanup_early() return early when the link group
has already been removed from the lgr list. It also makes smc_conn_kill()
only unregister a connection that is already freed, without the full
connection and socket teardown. The goal is to avoid a self-deadlock and a
wrong abort of a fallback clcsock when a handshake abort races with link
group termination.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 6021322253e93..4e10da5bd8142 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -688,9 +688,15 @@ void smc_lgr_cleanup_early(struct smc_link_group *lgr)
>
>  	smc_lgr_list_head(lgr, &lgr_lock);
>  	spin_lock_bh(lgr_lock);
> +	if (list_empty(&lgr->list)) {
> +		/* another context already removed the link group from the
> +		 * list and will terminate it
> +		 */
> +		spin_unlock_bh(lgr_lock);
> +		return;
> +	}
>  	/* do not use this link group for new connections */
> -	if (!list_empty(&lgr->list))
> -		list_del_init(&lgr->list);
> +	list_del_init(&lgr->list);
>  	spin_unlock_bh(lgr_lock);
>  	__smc_lgr_terminate(lgr, true);
>  }

[Severity: High]

Is list_empty() enough to tell that another context already owns this link
group?

smc_smcd_terminate(), smc_smcd_terminate_all() and
smc_smcr_terminate_all(NULL) move the lgr onto a private list on the stack
while holding lgr_lock, and set lgr->freeing = 1. They then walk that list
with no lock held:

net/smc/smc_core.c:smc_smcd_terminate_all() {
    ...
	spin_lock_bh(&smcd->lgr_lock);
	list_splice_init(&smcd->lgr_list, &lgr_free_list);
	list_for_each_entry(lgr, &lgr_free_list, list)
		lgr->freeing = 1;
	spin_unlock_bh(&smcd->lgr_lock);

	list_for_each_entry_safe(lgr, lg, &lgr_free_list, list) {
		list_del_init(&lgr->list);
		__smc_lgr_terminate(lgr, false);
	}
    ...
}

While the lgr is on that private list, lgr->list is not empty. A
first-contact handshake abort that reaches smc_lgr_cleanup_early() in that
window still passes the new check.

In that case, list_del_init(&lgr->list) unlinks the lgr from the other
context's private list while that list is being walked without a lock. If
this lgr is the cached next entry, it is now linked to itself, and the
walk can loop on it.

__smc_lgr_terminate() would also run at the same time as the other
context's termination. The only thing serialising them is this unlocked
check:

	if (lgr->terminating)
		return;	/* lgr already terminating */
	...
	lgr->terminating = 1;

If smc_lgr_cleanup_early() ends up freeing the lgr,
smc_smcd_terminate_all() or smc_smcr_terminate_all() can later reach the
freed lgr as their cached next element. smc_smcd_terminate() can instead
call schedule_work() on the terminate_work inside the freed lgr.

The peer can trigger smc_smcd_terminate() by sending
ISM_EVENT_CODE_SHUTDOWN, which is one way to reach this window.

The commit message says "Do not terminate a link group that another
context has already removed from the link group list". These contexts
have already removed the lgr from dev->lgr_list or smc_lgr_list.

smc_lgr_terminate_sched() already uses:

	if (list_empty(&lgr->list) || lgr->terminating || lgr->freeing) {

lgr->freeing is written under the same lgr_lock that is held here. Could
this check also test lgr->freeing and lgr->terminating?

> @@ -1519,6 +1525,17 @@ static void smc_conn_kill(struct smc_connection *conn, bool soft)
>  {
>  	struct smc_sock *smc = container_of(conn, struct smc_sock, conn);
>
> +	if (conn->freed) {
> +		/* connection was already freed, e.g. by an aborted
> +		 * handshake, but kept its entry because smc_conn_free() skips
> +		 * the unregister for a link group that is no longer on the lgr
> +		 * list. Remove the stale entry, but do not kill a socket that
> +		 * may be an established fallback connection.
> +		 */
> +		smc_lgr_unregister_conn(conn);
> +		return;
> +	}
> +

[Severity: High]

Can the stale conns_all entry still be reached through the server
handshake retry path?

When smc_conn_free() sees list_empty(&lgr->list), it skips the
unregister:

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

After that, conn->alert_token_local is still non-zero, and conn->alert_node
is still linked in link group A's conns_all, with conn->freed = 1. Because
smc_lgr_cleanup_early() now returns early, the handshake thread carries
on instead of deadlocking.

The server device selection code can then call smc_conn_create() again on
the same new_smc->conn. One example is when smc_buf_create() fails inside
smc_listen_ism_init() with -ENOSPC on DMB exhaustion, or with -ENOMEM:

net/smc/af_smc.c:smc_find_ism_v2_device_serv() {
    ...
		rc = smc_listen_ism_init(new_smc, ini);
		if (rc) {
			smc_init_info_store_rc(rc, ini);
			/* try next active ISM device */
			continue;
		}
    ...
}

The same applies to the fall-through to ISM v1, SMC-R v2 and SMC-R v1,
and to the failure paths in smc_listen_rdma_init() and
smc_listen_rdma_reg().

On the second registration, smc_lgr_register_conn() runs:

	sock_hold(&smc->sk); /* sock_put in smc_lgr_unregister_conn() */
	while (!conn->alert_token_local) {
		...
	}
	smc_lgr_add_alert_token(conn);

The stale token means the token loop is skipped. smc_lgr_add_alert_token()
then inserts an alert_node that is still linked in A's tree into link
group B's tree. A second sock_hold() is taken, but only one sock_put()
will follow. smc_conn_create() then sets conn->freed = 0, so the new
conn->freed check in smc_conn_kill() no longer matches.

A's terminator cannot get in first, because it blocks in lock_sock() and
smc_listen_work() holds that lock for the whole handshake. Once it gets
the lock, smc_conn_kill() runs the full abort on the live connection,
which now belongs to B.

smc_lgr_unregister_conn() then erases the node from conn->lgr (B), not
from A. alert_token_local is now 0, so every later call returns early.
__smc_lgr_terminate(A) keeps getting the same node back from
rb_first(&lgr->conns_all) and never finishes.

If smcr_lgr_conn_assign_link() fails on the retry instead,
smc_lgr_register_conn() sets conn->lgr = NULL and conn->freed stays 1.
The new branch is taken, but smc_lgr_unregister_conn() returns early
because smc_conn_lgr_valid() fails, and the same endless loop follows.

Would it be more robust to do the unregister in
smc_conn_free()/smc_conn_abort() regardless of lgr->list? At minimum that
would be the rb_erase() plus clearing alert_token_local. It would replace
the conn->freed check added to smc_conn_kill().

>  	if (conn->lgr->is_smcd && conn->lgr->peer_shutdown)
>  		conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;
>  	else

[ ... ]

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