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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] net/smc: race in smc_lgr_schedule_free_work() can arm free_work before kfree
- [High] net/smc: missing release barrier on client SMC_INIT to SMC_CLOSED transition
--

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

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

This commit addresses several races between SMC socket/link group teardown
and concurrent sock_diag readers by adjusting sk_state ordering, unhashing
sockets earlier in teardown paths, skipping extended diag attributes in
SMC_INIT/SMC_CLOSED, and unconditionally unregistering connections in
smc_conn_free().

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 9974149659c2f..9d8165d1b40c5 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -84,7 +84,7 @@ static void smc_lgr_schedule_free_work(struct smc_link_group *lgr)
>  	 * creation. For client use a somewhat higher removal delay time,
>  	 * otherwise there is a risk of out-of-sync link groups.
>  	 */
> -	if (!lgr->freeing) {
> +	if (!lgr->freeing && !lgr->terminating) {
>  		mod_delayed_work(system_percpu_wq, &lgr->free_work,
>  				 (!lgr->is_smcd && lgr->role == SMC_CLNT) ?
>  						SMC_LGR_FREE_DELAY_CLNT :
> @@ -1279,10 +1279,9 @@ void smc_conn_free(struct smc_connection *conn)
>  		if (current_work() != &conn->abort_work)
>  			cancel_work_sync(&conn->abort_work);
>  	}
> -	if (!list_empty(&lgr->list)) {
> +	if (!list_empty(&lgr->list))
>  		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
> -		smc_lgr_unregister_conn(conn);
> -	}
> +	smc_lgr_unregister_conn(conn);
>  
>  	if (!lgr->conns_num)
>  		smc_lgr_schedule_free_work(lgr);

[Severity: Critical]
Can smc_lgr_schedule_free_work() still race with __smc_lgr_terminate() and
re-arm lgr->free_work right before lgr is freed?

In smc_lgr_cleanup_early(), the link group is unlinked from lgr->list while
both lgr->freeing and lgr->terminating are still 0, and then
__smc_lgr_terminate() cancels free_work before setting lgr->terminating:

net/smc/smc_core.c:__smc_lgr_terminate() {
	if (lgr->terminating)
		return;	/* lgr already terminating */
	/* cancel free_work sync, will terminate when lgr->freeing is set */
	cancel_delayed_work(&lgr->free_work);
	lgr->terminating = 1;
	...
}

Because smc_conn_free() now calls smc_lgr_unregister_conn(conn)
unconditionally and invokes smc_lgr_schedule_free_work(lgr) outside
lgr->conns_lock, can the following interleaving occur when another
connection on the same link group is concurrently freed?

CPU0 (smc_conn_free)                   CPU1 (smc_lgr_cleanup_early)
--------------------                   ----------------------------
smc_lgr_unregister_conn(conn)
  lgr->conns_num becomes 0
smc_lgr_schedule_free_work(lgr)
  sees !lgr->freeing &&
       !lgr->terminating
                                       __smc_lgr_terminate(lgr, true)
                                         cancel_delayed_work(&lgr->free_work)
                                         lgr->terminating = 1
                                         ...
                                         smc_lgr_free(lgr)
                                           smc_lgr_put(lgr)
  mod_delayed_work(..., &lgr->free_work, ...)
smc_lgr_put(lgr)
  __smc_lgr_free(lgr)
    kfree(lgr)

If that happens, wouldn't lgr be freed via kfree(lgr) while lgr->free_work
remains queued in system_percpu_wq?

[ ... ]

> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
> index bf0beaa23bdb6..36a3b8f31f28d 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: High]
Should this condition also exclude r->diag_state == SMC_CLOSED, or should
smc_close_active() use smp_store_release() when transitioning from SMC_INIT
to SMC_CLOSED?

On the client side, if smc_connect_rdma() or smc_connect_ism() fails after
smc_conn_create() and jumps to connect_abort, smc_conn_abort() calls
smc_conn_free() and smc_lgr_cleanup_early(), which clears
conn->alert_token_local and frees conn->lgr while leaving conn->lgr non-NULL.

If fallback is not possible in smc_connect_decline_fallback() (or fails in
smc_connect_fallback()), smc->use_fallback remains false and sk->sk_state
remains SMC_INIT.

When the socket is then closed via smc_release() -> __smc_release() ->
smc_close_active(), sk->sk_state is set to SMC_CLOSED with a plain store
while the socket is still hashed:

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

On weakly ordered CPUs, if a concurrent reader in __smc_diag_dump() observes
r->diag_state == SMC_CLOSED from that plain store, there is no release
barrier ordering the earlier conn->alert_token_local = 0 store before the
sk_state transition, and the diag_mode check here only filters out SMC_INIT
(unlike the SMC_INIT || SMC_CLOSED check further down). Could
__smc_diag_dump() then see a stale non-zero alert_token_local and
dereference the freed smc->conn.lgr->is_smcd?

[ ... ]

> @@ -103,6 +104,9 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
>  	if (nla_put(skb, SMC_DIAG_FALLBACK, sizeof(fallback), &fallback) < 0)
>  		goto errout;
>  
> +	if (r->diag_state == SMC_INIT || r->diag_state == SMC_CLOSED)
> +		goto out;
> +
>  	if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
>  	    smc->conn.alert_token_local) {
>  		struct smc_connection *conn = &smc->conn;

[ ... ]

>
> base-commit: 07bd391eb9572e74f073c49acef327611234f9bb

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