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

[Mahanta Jambigi <mjambigi@linux.ibm.com>]

On 20/10/26 4:40 pm, sashiko-bot@kernel.org wrote:
> 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?

The interleaving is theoretically possible but *pre-existing* and not
widened by this patch. It requires a first-contact connection A to fail
and take the smc_lgr_cleanup_early() path, a second connection B to have
joined the same lgr after smc_server_lgr_pending was dropped, and B's
smc_conn_free() to bring conns_num to 0 within a few instructions of A's
__smc_lgr_terminate() cancelling free_work, with A's teardown completing
before B's final smc_lgr_put().

The unlocked "!lgr->freeing" check followed by mod_delayed_work() in
smc_lgr_schedule_free_work() has the same non-atomic window against
smc_lgr_terminate_sched() and __smc_lgr_terminate() in the baseline. The
"!lgr->terminating" check added here only avoids re-arming once
terminating is already visible, so it keeps the window no larger than
before. Closing it fully requires reworking how free_work scheduling is
serialized against lgr termination (check and arm under lgr_lock, or
cancelling free_work in __smc_lgr_free()). That is a separate lgr
lifetime fix, out of scope for this diag reader race patch.

> 
> [ ... ]
> 
>> 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?


On this path alert_token_local is cleared in the connect syscall, under
conns_lock with the socket lock held. The SMC_INIT -> SMC_CLOSED store
only happens later in close() -> smc_close_active() under lock_sock().
The lock_sock()/release_sock() handoff and the conns_lock write_unlock
between the two order the clear before the sk_state store, so a reader
that sees SMC_CLOSED *cannot* see a stale non-zero alert_token_local.