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

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

On 14/10/26 7:46 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 (1) · Medium: 1 · 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 still leaves a use-after-free between the diag reader
>   and the lgr.
> - [Medium] Switching to link->ibname removes the NULL dereference through
>   link->smcibdev->ibdev.
> - [Low] SMC_DIAG_LGRINFO now reports link->ibname, which
>   smcr_copy_dev_info_to_link() (smc_core.c:783-784) caches at link…
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the teardown
>   sites it touches have a deadlock.
> 
> --
> 
> 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. It skips the extension attributes for
> sockets in SMC_INIT or SMC_CLOSED and orders sk_state with acquire/release.
> It also unhashes the socket before several smc_conn_free() and
> smcd_buf_detach() calls, and uses the cached link->ibname in the LGRINFO
> dump.
> 
> [ ... ]
> 
>> diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
>> index bb0313ef5f7c1..aa6363abdd747 100644
>> --- a/net/smc/smc_close.c
>> +++ b/net/smc/smc_close.c
> 
> [ ... ]
> 
>> @@ -433,6 +435,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: High]
> This is a pre-existing issue and was not introduced by this patch. Can this
> smc_conn_free() call deadlock against abort_work?

I agree with your analysis regarding the lock inversion between
lock_sock(&smc->sk) and cancel_work_sync(&conn->abort_work). As this is
a pre-existing issue in the SMC core connection teardown flow, I will
submit a separate, dedicated patch to fix this deadlock so as to keep
this diag race series focused.

> 
> smc_close_passive_work() holds lock_sock(sk) here. For SMC-R,
> smc_conn_free() then does:
> 
> 		if (current_work() != &conn->abort_work)
> 			cancel_work_sync(&conn->abort_work);
> 
> The connection is still registered at this point. A CDC message with
> failover_validation and a seqno ahead can make smc_cdc_msg_validate()
> queue abort_work on smc_close_wq, and it does this without the socket lock.
> smc_close_wq can run that work alongside close_work, and
> smc_conn_abort_work() starts with:
> 
> 	lock_sock(&smc->sk);
> 	smc_conn_kill(conn, true);
> 
> If abort_work is already running and blocked in lock_sock(), would this
> thread wait forever in cancel_work_sync() while it holds that same socket
> lock? The current_work() check only stops abort_work from cancelling
> itself.
> 
> smc_close_active_abort() also reaches smc_conn_free() with the socket lock
> held.
> 
>>  			if (smc->clcsock)
>>  				release_clcsock = true;
> 
> [ ... ]
> 
>> 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
> 
> [ ... ]
> 
>> @@ -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;
> 
> [Severity: High]
> Does this guard miss a fallback socket in SMC_ACTIVE that still has a stale
> alert_token_local after its lgr reference was dropped?

Fixed in v11. I updated smc_conn_free() in smc_core.c to call
smc_lgr_unregister_conn(conn) unconditionally rather than gating it on
!list_empty(&lgr->list). This ensures alert_token_local is always
cleared to 0 when the connection is freed, preventing the diag reader
from entering CONNINFO/LGRINFO/DMBINFO blocks on fallback sockets in
SMC_ACTIVE. Added Fixes: 2a0674fffb6b.

> 
> On the server side, the peer can answer with a CLC DECLINE that has the
> first contact bit set instead of a CONFIRM. smc_clc_wait_msg() then does:
> 
> net/smc/smc_clc.c:smc_clc_wait_msg() {
>     ...
> 		if ((dclc->hdr.typev2 & SMC_FIRST_CONTACT_MASK) &&
> 		    smc->conn.lgr) {
> 			smc->conn.lgr->sync_err = 1;
> 			smc_lgr_terminate_sched(smc->conn.lgr);
> 		}
>     ...
> }
> 
> smc_lgr_terminate_sched() calls list_del_init(&lgr->list).
> 
> The handshake then aborts through
> smc_listen_decline()->smc_conn_abort()->smc_conn_free(). At that point
> lgr->list is empty:
> 
> 	if (!list_empty(&lgr->list)) {
> 		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
> 		smc_lgr_unregister_conn(conn);
> 	}
> 
> So the connection is not unregistered and alert_token_local stays
> non-zero, but smc_lgr_put() still drops the lgr reference. The socket then
> falls back, smc_listen_out_connected() stores SMC_ACTIVE, and the socket
> stays hashed.
> 
> Later, terminate_work runs __smc_lgr_terminate():
> 
> 		lock_sock(&smc->sk);
> 		smc_conn_kill(conn, soft);
> 		release_sock(&smc->sk);
> 	...
> 	smc_lgr_cleanup(lgr);
> 	smc_lgr_free(lgr);
> 
> For SMC-R and non-nocopy SMC-D, smc_conn_kill() does not unhash the
> socket. Nothing on this path takes the smc hash write_lock either.
> 
> A diag reader that already saw a non-zero alert_token_local holds only
> read_lock(&smc_hash->lock). Can it then read conn->sndbuf_desc->len and
> conn->rmb_desc->len after smc_lgr_free() has freed them? The same question
> applies to conn->lgr->is_smcd and lgr->list in the LGRINFO/DMBINFO
> conditions.
> 
> On non-RT kernels the window is small, but on PREEMPT_RT the read_lock is
> preemptible.
> 
> Would it work to unregister the connection, or clear alert_token_local, in
> smc_conn_free() even when lgr->list is empty? Another option is to skip
> the CONNINFO/LGRINFO/DMBINFO blocks when conn->freed is set.
> 
> [ ... ]
> 
>> @@ -154,8 +158,7 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
>>  			.lnk[0].link_id = link->link_id,
>>  		};
>>  
>> -		memcpy(linfo.lnk[0].ibname, link->smcibdev->ibdev->name,
>> -		       sizeof(link->smcibdev->ibdev->name));
>> +		memcpy(linfo.lnk[0].ibname, link->ibname, sizeof(link->ibname));
> 
> [Severity: Medium]
> The commit message says this change is meant "to avoid racing with link
> clearing during SMC-R link failover". It removes the NULL dereference
> through link->smcibdev->ibdev, but is the race itself still there?

Updated the commit message description to clarify that copying cached
link->ibname specifically eliminates the NULL pointer dereference / UAF
of link->smcibdev->ibdev during concurrent link clearing.

> 
> This block loads link = smc->conn.lnk with a plain load. It then copies
> ibport, link_id, ibname, gid and peer_gid while holding only
> read_lock(&smc_hash->lock).
> 
> At the same time, smcr_link_down()->smc_switch_conns()->
> smc_switch_link_and_count() can run:
> 
> 	atomic_dec(&conn->lnk->conn_cnt);
> 	smcr_link_put(conn->lnk);
> 	conn->lnk = to_lnk;
> 
> This runs under lgr->conns_lock and conn->send_lock, and the diag reader
> takes neither. smcr_link_clear() then does the final
> smcr_link_put()->__smcr_link_clear():
> 
> 	memset(lnk, 0, sizeof(struct smc_link));
> 	lnk->state = SMC_LNK_UNUSED;
> 
> smcr_link_init() can later reuse the slot. smc_cdc_msg_validate() also
> rewrites conn->lnk under send_lock.
> 
> The link is embedded in lgr->lnk[], so this is not a use-after-free.
> However, could user space get an SMC_DIAG_LGRINFO record with an empty
> ibname, ibport 0, link_id 0 and all-zero GIDs, or a mix of two links?
> 
> Would taking conn->send_lock around the conn->lnk load and the copy, or
> holding a link reference, give a consistent snapshot? If not, could the
> commit message say that only the NULL dereference is avoided?
> 
> [Severity: Low]
> This isn't a bug, but link->ibname is only written by
> smcr_copy_dev_info_to_link() when the link is created. SMC_NLA_DEV_IB_NAME
> in smc_nl_handle_smcr_dev() and the pnet table still use the live
> smcibdev->ibdev->name.
> 
> After an ib_device_rename(), the name in SMC_DIAG_LGRINFO will not match
> the SMC device dump or the pnet configuration. The commit message
> documents this, and it matches the existing SMC_NLA_LINK_IB_DEV link dump.

Acknowledged. As noted, this matches the existing SMC_NLA_LINK_IB_DEV
netlink dump behavior.

pw-bot: cr