Re: [PATCH net v2 2/2] net/smc: serialize early link group cleanup with termination

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 1

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 new temporary hold in smc_lgr_create()/smc_conn_create()
  keeps only the struct smc_link_group memory alive across the window…
- [Low] The fix only works if the preceding series patch "net/smc:
  serialize link group free work scheduling" is also applied, and this…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the exclusive-
  teardown protocol this patch builds on is unsafe against an already-…

--

net/smc: serialize early link group cleanup with termination

This patch makes smc_lgr_cleanup_early() check lgr->freeing under the link
group list lock, so it does not tear down a group that another path
already owns. It also takes a temporary lgr reference in smc_conn_abort().
For newly created groups, it takes another temporary reference from
publication in smc_lgr_create() until registration in smc_conn_create()
finishes.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 478c8bc2759a0..1918a79ff60e4 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -693,6 +693,10 @@ void smc_lgr_cleanup_early(struct smc_link_group *lgr)
>  
>  	smc_lgr_list_head(lgr, &lgr_lock);
>  	spin_lock_bh(lgr_lock);
> +	if (lgr->freeing) {
> +		spin_unlock_bh(lgr_lock);
> +		return;
> +	}

[Severity: Low]

This early return only gives exclusive teardown ownership in combination
with two other pieces: the lgr->freeing = 1 write later in
smc_lgr_cleanup_early(), and freeing being a standalone bool instead of a
bitfield that shares storage with sync_err and terminating.

Both of those come from the earlier series patch "net/smc: serialize link
group free work scheduling". The baseline smc_lgr_cleanup_early() never
sets freeing.

The commit body only says:

    The existing locked freeing transition then claims exclusive teardown
    ownership

The explicit dependency is stated only in the v2 changelog below the ---
line, and git am drops that part.

The context lines of this hunk don't include the lgr->freeing = 1 line,
so the hunk applies cleanly to a tree without the prerequisite. The two
patches also carry different Fixes: tags (8e316b9e7260 and f9aab6f2ce57).

Suppose a stable tree takes only this patch. smc_lgr_cleanup_early()
would then check freeing but never claim it. Couldn't
smc_lgr_terminate_sched(), smc_smcd_terminate() or
smc_smcr_terminate_all() still claim the group after the check, so that
both paths enter __smc_lgr_terminate()?

Could the body mention the dependency? Another option is a "# needs ..."
annotation on the Cc: stable line.

[Severity: High]

This is a pre-existing issue and wasn't introduced by this patch. Still,
the exclusive teardown protocol that this check builds on doesn't seem
safe against a free_work instance that is already running.

smc_lgr_free_work() holds no lgr reference. It dereferences the group
before it can see that freeing was claimed:

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

__smc_lgr_terminate() only uses the non-synchronous cancel:

	/* cancel pending free_work; a running instance rechecks freeing */
	cancel_delayed_work(&lgr->free_work);

Neither smc_lgr_free() nor __smc_lgr_free() waits for free_work before
kfree(lgr).

Can this sequence happen on an idle group (conns_num == 0)?

CPU1 (free_work timer expired)
smc_lgr_free_work()
  <preempted before spin_lock_bh(lgr_lock)>

CPU2 (peer DELETE_LINK with SMC_LLC_FLAG_DEL_LINK_ALL)
smc_llc_process_cli_delete_link()
  smc_lgr_terminate_sched()
    lgr->freeing = 1;
    schedule_work(&lgr->terminate_work);
smc_lgr_terminate_work()
  __smc_lgr_terminate()
    cancel_delayed_work(&lgr->free_work);   <- does not wait
    smc_lgr_free()
      smc_lgr_put()
        __smc_lgr_free()
          kfree(lgr);

CPU1 resumes
  smc_lgr_list_head(lgr, &lgr_lock);   <- reads freed lgr
  spin_lock_bh(lgr_lock);

The resumed worker would take a spinlock through a pointer loaded from
freed memory. If the slab has been reused and reads freeing == 0 with an
empty tree, the worker could also run list_del_init() and smc_lgr_free()
on foreign memory.

Other terminators would reach the same window, for example an LLC protocol
violation, or smc_smcd_terminate() from an ISM event.

The smc_conn_abort()->smc_lgr_cleanup_early() path doesn't look like a
realistic trigger here. smc_lgr_schedule_free_work() is only called once
conns_num reaches zero, and the creating connection stays registered
until its own abort.

Could free_work hold its own lgr reference, taken when it is armed and
dropped when the work exits or is cancelled successfully? Alternatively,
the final put could use cancel_delayed_work_sync() when it is not running
from free_work itself.

>  	/* do not use this link group for new connections */
>  	if (!list_empty(&lgr->list))
>  		list_del_init(&lgr->list);
> @@ -999,6 +1003,7 @@ static int smc_lgr_create(struct smc_sock *smc, struct smc_init_info *ini)
>  		lgr->buf_type = lgr->net->smc.sysctl_smcr_buf_type;
>  		atomic_inc(&lgr_cnt);
>  	}
> +	smc_lgr_hold(lgr); /* lgr_put in smc_conn_create() */
>  	smc->conn.lgr = lgr;
>  	spin_lock_bh(lgr_lock);
>  	list_add_tail(&lgr->list, lgr_list);
> @@ -2054,10 +2059,13 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini)
>  		write_unlock_bh(&lgr->conns_lock);
>  		if (rc) {
>  			smc_lgr_cleanup_early(lgr);
> +			smc_lgr_put(lgr); /* lgr_hold in smc_lgr_create() */
>  			goto out;
>  		}
>  	}
>  	smc_lgr_hold(conn->lgr); /* lgr_put in smc_conn_free() */
> +	if (ini->first_contact_local)
> +		smc_lgr_put(conn->lgr); /* lgr_hold in smc_lgr_create() */

[Severity: High]

The temporary hold taken in smc_lgr_create() keeps the lgr allocation
alive between list_add_tail() and smc_lgr_register_conn(). Does it also
stop a terminator from tearing the group down in that window?

smc_conn_create() only takes lgr->conns_lock around
smc_lgr_register_conn(conn, true). Nothing rechecks lgr->freeing under
lgr_lock. For SMC-D, this sequence seems possible:

CPU1 (first-contact connect or listen)
smc_conn_create()
  smc_lgr_create()
    smc_lgr_hold(lgr);
    list_add_tail(&lgr->list, lgr_list);

CPU2 (peer ISM GID or shutdown event)
smc_smcd_terminate()
  lgr->peer_shutdown = 1;
  list_move(&lgr->list, &lgr_free_list);
  lgr->freeing = 1;
  schedule_work(&lgr->terminate_work);
smc_lgr_terminate_work()
  __smc_lgr_terminate()
    rb_first(&lgr->conns_all) is NULL, kill loop skipped
    smc_lgr_free()
      destroy_workqueue(lgr->tx_wq);
      smc_ism_put_vlan(); put_device();
      smc_lgr_put(lgr);   <- only the temporary hold remains

CPU1
  write_lock_bh(&lgr->conns_lock);
  smc_lgr_register_conn(conn, true);  <- cannot fail for SMC-D,
                                         sock_hold(), conns_num++
  smc_lgr_hold(conn->lgr);
  smc_lgr_put(conn->lgr);             <- temporary hold dropped
  return 0;

The kill loop never visited this connection, so conn->killed stays clear.
Two problems seem to follow.

First, smc_tx_consumer_update() can get -EPIPE from
smc_cdc_get_slot_and_msg_send() because peer_shutdown is set. It would
then queue work on the destroyed workqueue:

		if ((smc_cdc_get_slot_and_msg_send(conn) < 0) &&
		    !conn->killed) {
			queue_delayed_work(conn->lgr->tx_wq, &conn->tx_work,
					   SMC_TX_WORK_DELAY);

Second, smc_conn_free() sees list_empty(&lgr->list) and skips
smc_lgr_unregister_conn(). That leaks the sock_hold() taken at
registration. The final smc_lgr_put() would then free the lgr while
smc_conn_lgr_valid() still returns true for the connection.

The publication-to-registration window itself predates this patch. Before
it, the later write_lock_bh(&lgr->conns_lock) was a direct
use-after-free.

The commit message says "On success, acquire the ordinary connection
reference before releasing the temporary hold". This handoff doesn't seem
to serialize registration with the freeing claim.

Could first-contact registration take lgr_lock, fail if lgr->freeing is
set, and then take conns_lock and register? That would match the lock
order of the reuse path. Alternatively, could the group be published only
after its first connection is registered?

The SMC-R variant looks mostly blocked. smc_lgr_free() clears the links,
so registration then fails with SMC_CLC_DECL_NOACTLINK and takes the
fixed failure path. It is unclear whether a concurrent smcr_link_clear()
could still hand out a link while it is clearing.

>  	if (!conn->lgr->is_smcd)
>  		smcr_link_hold(conn->lnk); /* link_put in smc_conn_free() */
>  	conn->freed = 0;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261003183237.2284245-1-nicoyip.dev%40gmail.com