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

[sashiko-bot@kernel.org]

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