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

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

[Mahanta Jambigi]

The handshake abort path can leave a connection that smc_conn_free()
already freed in lgr->conns_all: when a received first-contact CLC
DECLINE has already scheduled the link group termination and emptied
lgr->list, smc_conn_free() skips the unregister. The handshake thread
then calls smc_lgr_cleanup_early() on a link group that is already being
terminated, and __smc_lgr_terminate() hangs on the socket lock that
thread already holds; if the termination work wins the race instead, it
later kills the freed connection and aborts the clcsock of a socket that
has fallen back to TCP.

Do not terminate a link group that another context has already removed
from the link group list, and drop an already freed connection in
smc_conn_kill() without running the connection and socket teardown.

Fixes: 2a0674fffb6b ("net/smc: improve abnormal termination of link groups")
Fixes: f9aab6f2ce57 ("net/smc: immediate freeing in smc_lgr_cleanup_early()")
Cc: stable@vger.kernel.org
Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
---
 net/smc/smc_core.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 6021322253e9..df2ab2da2793 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);
 }
@@ -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;
+	}
+
 	if (conn->lgr->is_smcd && conn->lgr->peer_shutdown)
 		conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;
 	else

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net/smc: rbtree corruption in smc_conn_kill() on device fallback retry
- [High] net/smc: missing lgr->freeing check and set in smc_lgr_cleanup_early()
--

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

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

This commit avoids calling __smc_lgr_terminate() in smc_lgr_cleanup_early()
when lgr->list is already empty, and updates smc_conn_kill() to unregister
and skip connections that already have conn->freed set.

> 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]
Should smc_lgr_cleanup_early() also check lgr->freeing and lgr->terminating,
and set lgr->freeing = 1 before dropping lgr_lock, similar to
smc_lgr_terminate_sched()?

In smc_smcd_terminate(), smc_smcd_terminate_all(), and
smc_smcr_terminate_all(), link groups are moved onto a stack-local
lgr_free_list and marked with lgr->freeing = 1 under lgr_lock before the
lock is dropped:

net/smc/smc_core.c:smc_smcd_terminate() {
	...
			list_move(&lgr->list, &lgr_free_list);
			lgr->freeing = 1;
		}
	}
	spin_unlock_bh(&dev->lgr_lock);

	/* cancel the regular free workers and actually free lgrs */
	list_for_each_entry_safe(lgr, l, &lgr_free_list, list) {
		list_del_init(&lgr->list);
		schedule_work(&lgr->terminate_work);
	}
}

If smc_lgr_cleanup_early() runs after dev->lgr_lock is released but before
the second loop runs list_del_init(&lgr->list), list_empty(&lgr->list) is
still false because lgr->list is linked into the other thread's stack-local
lgr_free_list. Wouldn't smc_lgr_cleanup_early() then corrupt lgr_free_list
with an unsynchronized list_del_init(&lgr->list) and proceed into
__smc_lgr_terminate() while holding the socket lock?

Conversely, when smc_lgr_cleanup_early() wins lgr_lock and unlinks
lgr->list without setting lgr->freeing = 1, __smc_lgr_terminate() only
calls non-synchronous cancel_delayed_work(&lgr->free_work), relying on
lgr->freeing to stop a concurrent smc_lgr_free_work():

net/smc/smc_core.c:smc_lgr_free_work() {
	...
	smc_lgr_list_head(lgr, &lgr_lock);
	spin_lock_bh(lgr_lock);
	if (lgr->freeing) {
		spin_unlock_bh(lgr_lock);
		return;
	}
	...
}

If smc_lgr_free_work() is already running and waiting on lgr_lock, or is
scheduled again before termination finishes, could it see lgr->freeing == 0
and call smc_lgr_free(lgr) concurrently with __smc_lgr_terminate()?

> @@ -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 deferring smc_lgr_unregister_conn(conn) until smc_conn_kill() corrupt
lgr->conns_all or cause an infinite loop in __smc_lgr_terminate() when a
server handshake retries another device or SMC version on the same
new_smc->conn?

During server handshake in smc_listen_work(), lock_sock(&new_smc->sk) is
held across smc_listen_find_device(), which tries candidate ISM and RDMA
devices and versions in sequence:

  smc_listen_work()
    lock_sock(&new_smc->sk)
    smc_listen_find_device()
      smc_find_ism_v2_device_serv()
      smc_find_ism_v1_device_serv()
      smc_find_rdma_v2_device_serv()
      smc_find_rdma_v1_device_serv()

In each candidate helper, if smc_conn_create() registers new_smc->conn in
a link group (say lgr1) that is concurrently removed from lgr_list for
termination, the subsequent buffer creation or RMB registration (such as
smc_listen_rdma_reg() -> smcr_lgr_reg_rmbs() -> smc_llc_flow_initiate(),
which fails with -ENOLINK when list_empty(&lgr1->list) is true) fails and
calls smc_conn_abort(new_smc, ini->first_contact_local):

net/smc/af_smc.c:smc_find_rdma_v2_device_serv() {
	...
	rc = smc_listen_rdma_init(new_smc, ini);
	if (!rc) {
		rc = smc_listen_rdma_reg(new_smc, ini->first_contact_local);
		if (rc)
			smc_conn_abort(new_smc, ini->first_contact_local);
	}
	...
}

Because list_empty(&lgr1->list) is true, smc_conn_free() skips
smc_lgr_unregister_conn() and leaves new_smc->conn->alert_node in
lgr1->conns_all with a non-zero conn->alert_token_local, while
smc_lgr_cleanup_early() now returns immediately. Meanwhile,
__smc_lgr_terminate(lgr1) cannot run smc_conn_kill() yet because
smc_listen_work() still holds lock_sock(&new_smc->sk).

When smc_listen_find_device() moves on to the next candidate device or
version while still holding lock_sock(&new_smc->sk), it calls
smc_conn_create() -> smc_lgr_register_conn() again on the same
new_smc->conn:

net/smc/smc_core.c:smc_lgr_register_conn() {
	...
	if (!conn->lgr->is_smcd) {
		rc = smcr_lgr_conn_assign_link(conn, first);
		if (rc) {
			conn->lgr = NULL;
			return rc;
		}
	}
	/* find a new alert_token_local value not yet used by some connection
	 * in this link group
	 */
	sock_hold(&smc->sk); /* sock_put in smc_lgr_unregister_conn() */
	while (!conn->alert_token_local) {
		conn->alert_token_local = atomic_inc_return(&nexttoken);
		if (smc_lgr_find_conn(conn->alert_token_local, conn->lgr))
			conn->alert_token_local = 0;
	}
	smc_lgr_add_alert_token(conn);
}

At that point:

1. If the second smc_lgr_register_conn() succeeds on another link group
   lgr2, it sets conn->lgr = lgr2, takes a second sock_hold(&smc->sk),
   reuses the stale conn->alert_token_local, inserts conn->alert_node into
   lgr2->conns_all while it is still linked in lgr1->conns_all, and clears
   conn->freed = 0. When lock_sock(&new_smc->sk) is later released,
   __smc_lgr_terminate(lgr1) wakes up and runs smc_conn_kill(), which sees
   conn->freed == 0, aborts the active connection on lgr2, and unregisters
   conn from conn->lgr (lgr2) instead of lgr1.

2. If the second smc_lgr_register_conn() fails in
   smcr_lgr_conn_assign_link(), it sets conn->lgr = NULL. When
   __smc_lgr_terminate(lgr1) later acquires lock_sock(&new_smc->sk) and
   calls smc_conn_kill() -> smc_lgr_unregister_conn(conn),
   smc_conn_lgr_valid(conn) is false because conn->lgr is NULL, so
   conn->alert_node is never removed from rb_first(&lgr1->conns_all) and
   __smc_lgr_terminate(lgr1) loops forever.

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