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

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

On 01/11/26 12:45 pm, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 1 (2) · 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 fix is incomplete.
> - [Medium] After this patch, __smc_diag_dump() (net/smc/smc_diag.c)
>   reports SMC_DIAG_MODE_SMCR for every non-fallback socket in SMC_INIT…
> - [Low] The commit message's 'Fourth' item says the
>   smp_store_release(&newsmcsk->sk_state, SMC_CLOSED) in…
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but
>   smc_conn_free() (net/smc/smc_core.c:1281-1282) calls…
> - [Medium] This problem wasn't introduced by this patch, but
>   smc_diag_handler_dump() (net/smc/smc_diag.c) never checks that the…
> 
> --
> 
> net/smc: fix lgr/lnk lifetime vs diag reader race
> 
> This patch tries to close races between the SMC sock_diag dump and the
> teardown of conn/lgr/lnk state. __smc_diag_dump() now skips the extension
> attributes and the lgr based diag_mode test for sockets in SMC_INIT,
> SMC_CLOSED or fallback, and several teardown paths unhash the socket
> before smc_conn_free() or smcd_buf_detach(). It also adds acquire/release
> ordering on sk_state and copies the cached link->ibname.
> 
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index d10244af5bd29..ec5980add7a9d 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
> 
> [ ... ]
> 
>> @@ -1971,7 +1971,7 @@ static void smc_listen_out_err(struct smc_sock *new_smc)
>>  	this_cpu_inc(net->smc.smc_stats->srv_hshake_err_cnt);
>>  	if (newsmcsk->sk_state == SMC_INIT)
>>  		sock_put(&new_smc->sk); /* passive closing */
>> -	newsmcsk->sk_state = SMC_CLOSED;
>> +	smp_store_release(&newsmcsk->sk_state, SMC_CLOSED);
> 
> [Severity: Low]
> 
> The commit message says:
> 
>   Fourth, smc_listen_out_err() sets sk_state = SMC_CLOSED with a plain
>   store after smc_conn_abort() -> smc_conn_free() has already cleared
>   alert_token_local under write_lock_bh(). On weakly ordered CPUs a
>   concurrent diag reader using smp_load_acquire() on sk_state can observe
>   SMC_CLOSED while the preceding store to alert_token_local has not yet
>   become visible, causing it to dereference an lgr whose reference was
>   already dropped.
> 
> Is this still accurate with the current __smc_diag_dump()?
> 
> A reader that sees SMC_CLOSED now skips the conn->lgr dereference in the
> diag_mode test, and the new goto out skips the extension blocks.
> 
> Before this store the state is SMC_INIT. smc_listen_work() holds the
> socket lock until smc_listen_out(), so close_work can't change it in
> between, and SMC_INIT is skipped the same way.
> 
> So whichever value the reader sees, it never reads alert_token_local or
> conn->lgr, and the release ordering doesn't seem to protect anything the
> reader relies on. The commit message already says this for the
> stale-token case ("the SMC_CLOSED check in __smc_diag_dump() keeps the
> reader from dereferencing conn->lgr").
> 
> This reasoning seems to date from v9, before v14 added SMC_CLOSED to the
> diag_mode test. Could the fourth item be described as hardening, or
> dropped, so stable backporters don't treat the barrier as a required
> standalone fix?

Reworded as hardening: a reader that observes SMC_CLOSED skips the
connection fields in any case, so the smp_store_release() in
smc_listen_out_err() is uniformity with the SMC_ACTIVE transitions,
not a required ordering. Stable backporters should not treat it as a
standalone fix.

> 
>>  
>>  	smc_listen_out(new_smc);
>>  }
>> diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
>> index a8d4b4b65f244..e26586114f802 100644
>> --- a/net/smc/smc_close.c
>> +++ b/net/smc/smc_close.c
> 
> [ ... ]
> 
>> @@ -437,6 +439,7 @@ static void smc_close_passive_work(struct work_struct *work)
>>  		sk->sk_state_change(sk);
>>  		if ((sk->sk_state == SMC_CLOSED) &&
>>  		    (sock_flag(sk, SOCK_DEAD) || !sk->sk_socket)) {
>> +			sk->sk_prot->unhash(sk);
>>  			smc_conn_free(conn);
> 
> [Severity: Medium]
> 
> This is a pre-existing issue and wasn't introduced by this patch. Does
> smc_conn_free() leak the socket reference held by abort_work when it
> cancels a pending abort_work?

Yes its pre-existing & I will address this seperately.

> 
> smc_conn_free() in net/smc/smc_core.c does:
> 
> 	} else {
> 		smc_cdc_wait_pend_tx_wr(conn);
> 		if (current_work() != &conn->abort_work)
> 			cancel_work_sync(&conn->abort_work);
> 	}
> 
> and ignores the return value. The only code that schedules abort_work is
> smc_cdc_msg_validate(), and it hands a reference to the work:
> 
> 		sock_hold(&smc->sk); /* sock_put in abort_work */
> 		if (!queue_work(smc_close_wq, &conn->abort_work))
> 			sock_put(&smc->sk);
> 
> smc_conn_abort_work() drops that reference when it runs. Suppose
> abort_work is still pending when an SMC-R connection reaches
> smc_conn_free() from outside abort_work, for example via __smc_release().
> Then cancel_work_sync() returns true, nobody drops the reference, and the
> smc_sock is never freed.
> 
> smc_close_cancel_work() handles close_work the same way with:
> 
> 	if (cancel_work_sync(&smc->conn.close_work))
> 		sock_put(sk);
> 
> Should smc_conn_free() do the same for abort_work?
> 
> [ ... ]
> 
>> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
>> index bf0beaa23bdb6..7b4656d6560a6 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 && r->diag_state != SMC_CLOSED &&
>> +		 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: Medium]
> 
> With this change every non-fallback socket in SMC_INIT or SMC_CLOSED is
> reported as SMC_DIAG_MODE_SMCR. That includes SMC-D connections, which
> used to report SMC_DIAG_MODE_SMCD.
> 
> include/uapi/linux/smc_diag.h defines diag_mode as:
> 
> /* Mode of a connection */
> enum {
> 	SMC_DIAG_MODE_SMCR,
> 	SMC_DIAG_MODE_FALLBACK_TCP,
> 	SMC_DIAG_MODE_SMCD,
> };
> 
> It has no "unknown" value. Is it intended that an SMC-D connection is
> reported as SMC-R here?
> 
> Live SMC-D sockets can stay hashed in SMC_CLOSED with a valid lgr and
> alert_token_local. smc_close_passive_work() or
> smc_close_passive_abort_received() can move a socket to SMC_CLOSED while
> the fd is still open. smc_close_passive_work() only unhashes and frees
> when:
> 
> 		if ((sk->sk_state == SMC_CLOSED) &&
> 		    (sock_flag(sk, SOCK_DEAD) || !sk->sk_socket)) {
> 
> SMC-D sockets in SMC_INIT after smc_conn_create() are affected the same
> way. A tool that filters by mode (smcss -D / -R style) would put these
> sockets in the wrong group.
> 
> Could the race be avoided without misreporting? For example, is_smcd
> could be cached in the smc_sock, or SMC-D could be inferred from
> conn.lgr != NULL && conn.lnk == NULL, which only reads pointers and
> doesn't dereference the lgr.

Fixed as you suggested: the type is cached in conn->is_smcd at
smc_conn_create() time (struct smc_connection is embedded in the
smc_sock, so the lifetime is identical) and diag_mode is taken from
the cached flag. The conn->lgr dereference and the state condition
are gone from the diag_mode test, and SMC-D sockets in SMC_INIT and
SMC_CLOSED report SMC_DIAG_MODE_SMCD again.


> 
>>  	user_ns = sk_user_ns(NETLINK_CB(cb->skb).sk);
>>  	if (smc_diag_msg_attrs_fill(sk, skb, r, user_ns))
>>  		goto errout;
>> @@ -103,6 +104,10 @@ 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 ||
>> +	    smc->use_fallback)
>> +		goto out;
>> +
> 
> [Severity: High]
> 
> Can a socket whose connection was already released by smc_conn_abort()
> still move into a state this guard doesn't cover?
> 
> The guard only checks the sk_state snapshot. Take a client handshake
> that fails with a negative rc after smc_conn_create():
> 
> smc_connect_rdma()
>   connect_abort:
>     smc_conn_abort()
>       smc_conn_free()   /* sets conn->freed, drops lgr/lnk refs */
> 
> smc_connect_decline_fallback() returns early without changing the state,
> and smc_connect_work() only does:
> 
> 	rc = __smc_connect(smc);
> 	if (rc < 0)
> 		smc->sk.sk_err = -rc;
> 
> So the socket stays hashed in SMC_INIT with use_fallback == false.
> 
> Suppose smc_lgr_terminate_sched() had already emptied lgr->list, which
> is the stale-token case the changelog mentions. Then smc_conn_free()
> skips the unregister:
> 
> 	if (!list_empty(&lgr->list)) {
> 		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
> 		smc_lgr_unregister_conn(conn);
> 	}
> 
> alert_token_local stays non-zero and the conn stays in lgr->conns_all.
> 
> smc_cdc_rx_handler() doesn't check conn->freed:
> 
> 	conn = smc_lgr_find_conn(ntohl(cdc->token), lgr);
> 	if (!conn || conn->out_of_sync) {
> 
> So a close or peer-abort CDC can still queue close_work. Once the
> handshake thread drops the socket lock, smc_close_passive_work() does:
> 
> 	case SMC_INIT:
> 		sk->sk_state = SMC_APPCLOSEWAIT1;
> 		break;
> 
> Or smc_close_passive_abort_received() moves SMC_INIT to
> SMC_PROCESSABORT. Both are plain stores, and neither path reaches the
> new unhash plus smc_conn_free() branch.
> 
> A diag reader under read_lock(&h->lock) would then see APPCLOSEWAIT1 or
> PROCESSABORT and pass both new checks. It would find
> smc_conn_lgr_valid() true, because conn->lgr is never cleared. It then
> dereferences smc->conn.lgr->is_smcd, conn->sndbuf_desc, conn->rmb_desc
> and lgr->list.
> 
> Meanwhile terminate_work runs
> __smc_lgr_terminate()->smc_conn_kill()->smc_close_active_abort(). Its
> ACTIVE/APPCLOSEWAIT branch goes to SMC_CLOSED without unhashing:
> 
> 	case SMC_ACTIVE:
> 	case SMC_APPCLOSEWAIT1:
> 	case SMC_APPCLOSEWAIT2:
> 		sk->sk_state = SMC_PEERABORTWAIT;
> 		smc_close_cancel_work(smc);
> 		if (sk->sk_state != SMC_PEERABORTWAIT)
> 			break;
> 		sk->sk_state = SMC_CLOSED;
> 		sock_put(sk); /* (postponed) passive closing */
> 
> So the hash write_lock is never taken before smc_lgr_free() drops the
> last lgr reference. Wouldn't the reader then be dereferencing a freed
> smc_link_group?
> 
> There is also a weaker variant, where the token is cleared normally and
> the lgr is freed synchronously via smc_lgr_cleanup_early(). Safety then
> depends on the token clear being visible to a reader that acquire-loads
> an sk_state written with a plain store. This is probably ordered in
> practice through lock_sock()/release_sock(), but the patch doesn't
> establish it.
> 
> Separately, after a failed handshake the same SMC_INIT to
> SMC_APPCLOSEWAIT1 transition later runs the "postponed passive closing"
> sock_put() in smc_close_active() or smc_close_active_abort(). But
> smc_connect_decline_fallback() has already dropped that reference.

Fixed by returning early from smc_close_passive_work() when
conn->freed is set. I kept your conn->freed check, but at execution
time instead of in smc_cdc_rx_handler(): a queue-time check cannot
cover a close_work that was queued before the abort, and the CDC
receive path runs under bh_lock_sock(), which does not exclude the
handshake thread's lock_sock(), so check-then-queue would be racy
anyway. At execution time the check holds the socket lock that every
smc_conn_free() caller holds as well, so it is serialized against all
free sites. A hashed socket with a freed connection now stays in
SMC_INIT or SMC_CLOSED until release, which is exactly what the filter
covers; the only states reachable after a normal token clear are
therefore guarded ones, so the reader relies on no ordering of the
clear. This also fixes the double passive-closing sock_put() you
noted (Fixes: 46c28dbd4c23, 2a0674fffb6b).

> 
> Would one of these work better?
> 
> - unhash a socket whose conn was freed by smc_conn_abort(), or
>   otherwise hide it from diag
> - cancel close_work, or check conn->freed in smc_cdc_rx_handler() and
>   in the diag reader
> - pin the lgr for as long as stale conn state is visible
> 
>>  	if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
>>  	    smc->conn.alert_token_local) {
> 
> [Severity: Medium]
> 
> This is a pre-existing issue and wasn't introduced by this patch. Is
> req->diag_ext guaranteed to be inside the request payload here?

Yes its pre-existing & I will address this seperately.

pw-bot: cr