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

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

[Mahanta Jambigi]

The diag dump walks the socket hash table under a read_lock and dereferences
conn->lgr and conn->lnk.  Two terminal teardown paths drop those references via
smc_conn_free() while the socket is still hashed:

  - smc_conn_kill() -> smc_close_active_abort() -> smc_conn_free()
  - smc_close_passive_work() -> smc_conn_free()

This allows the diag reader to dereference a freed lgr or lnk.

Fix it by unhashing the socket before smc_conn_free() is called at each of these
two sites.  Any socket visible to the diag reader under the hash read_lock then
has valid conn->lgr and conn->lnk pointers.

Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets")
Fixes: 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R connections")
Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
---
Changes in v4:
- dropped smc_conn_unhash() wrapper, conn->unhashed flag, and all
  changes to af_smc.c, smc.h, smc_core.c and smc_core.h; smc_unhash_sk()
  is already idempotent via sk_hashed(), so direct calls at the two
  teardown sites in smc_close.c are sufficient
- dropped the __smc_release() hunk: it needs no change since the
  subsequent unhash there is already a safe no-op
- fixed premature-unhash issue present in v3: smc_conn_free() must not
  unhash because smc_conn_abort() calls it before smc_switch_to_fallback()
  in both smc_listen_decline() and smc_connect_rdma() error paths;
  unhashing there would make live fallback sockets invisible to smcss
- likewise, the ISM/RDMA retry loops (smc_find_ism_v2_device_serv(),
  smc_find_rdma_v2_device_serv()) call smc_conn_abort() on a failed
  attempt and then smc_conn_create() on the next device; unhashing in
  smc_conn_free() would permanently hide the established connection from
  smc_diag since smc_conn_create() does not re-hash the socket

Changes in v3:
- redesigned as a single patch; dropped the lgr_lnk_lock spinlock
  approach and the 2-patch split
- fix is now at the socket hash layer: introduce smc_conn_unhash() with
  a per-connection unhashed flag; smc_conn_free() unhashes before dropping
  lgr/lnk refs, so any socket visible to the diag reader under the hash
  read_lock has valid conn->lgr and conn->lnk pointers
- __smc_release() updated to call smc_conn_unhash() for non-fallback
  sockets so the flag is honoured when smc_conn_free() already ran first
- smc_diag.c needs no changes; the hash read_lock invariant is
  sufficient without any per-connection lock in the dump path
- dropped the clcsock/mutex_trylock fix as that will be addressed separately

Changes in v2:
- this is v2 of the 2-patch series; the earlier submission was mislabelled
  [PATCH v3] but was in fact the first version sent to the list
- split into a 2-patch series; patch 1/2 adds per-connection lgr_lnk_lock
  infrastructure to smc_core, patch 2/2 fixes the diag dump path using it
- dropped lock_sock()/release_sock() from __smc_diag_dump(); v1 held the
  socket lock across all lgr/lnk dereferences, requiring the hash read_lock
  to be dropped and re-acquired around each socket
- dropped the restart-from-head loop in smc_diag_dump_proto(); the new
  design does not drop the hash read_lock mid-walk so the hlist truncation
  concern no longer applies
- dropped refcount_inc_not_zero() socket pinning from the dump loop for the
  same reason: the hash read_lock is now held for the full walk
- added per-connection lgr_lnk_lock spinlock to struct smc_connection;
  conn->lgr and conn->lnk are NULLed under this lock in smc_conn_free()
  before borrowed references are released, establishing the invariant: a
  non-NULL conn->lgr seen under lgr_lnk_lock guarantees the lgr is alive
- added lgr_lnk_lock to smc_switch_link_and_count() to protect the conn->lnk
  pointer swap from concurrent diag readers
- replaced mutex_lock() on clcsock_release_lock in smc_diag_msg_common_fill()
  with mutex_trylock(); mutex_lock() was valid in v1 because the hash
  spinlock had been dropped, but the new design holds the hash read_lock
  throughout so only a non-sleeping trylock is safe; a failed trylock leaves
  address fields zeroed, which is acceptable for a monitoring tool
- all conn->lgr and conn->lnk accesses in __smc_diag_dump() use a
  snapshot-then-use pattern: fields are copied into local stack variables
  under lgr_lnk_lock and nla_put() is called after releasing the lock,
  avoiding any sleeping operation under the spinlock

 net/smc/smc_close.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
index bb0313ef5f7c..9e4b4e9b9d08 100644
--- a/net/smc/smc_close.c
+++ b/net/smc/smc_close.c
@@ -128,6 +128,11 @@ void smc_close_active_abort(struct smc_sock *smc)
 	struct sock *sk = &smc->sk;
 	bool release_clcsock = false;

+	/*
+	 * Unhash before any branch calls smc_conn_free() and drops lgr/lnk refs;
+	 * the diag reader must not see a hashed socket with freed lgr/lnk.
+	 */
+	sk->sk_prot->unhash(sk);
 	if (sk->sk_state != SMC_INIT && smc->clcsock && smc->clcsock->sk) {
 		sk->sk_err = ECONNABORTED;
 		if (smc->clcsock && smc->clcsock->sk)
@@ -432,7 +436,12 @@ static void smc_close_passive_work(struct work_struct *work)
 	if (old_state != sk->sk_state) {
 		sk->sk_state_change(sk);
 		if ((sk->sk_state == SMC_CLOSED) &&
 		    (sock_flag(sk, SOCK_DEAD) || !sk->sk_socket)) {
+			/*
+			 * Unhash before smc_conn_free() drops lgr/lnk refs so the diag
+			 * reader cannot see a hashed socket with freed lgr/lnk.
+			 */
+			sk->sk_prot->unhash(sk);
 			smc_conn_free(conn);
 			if (smc->clcsock)
 				release_clcsock = true;
-- 
2.43.7

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

[sashiko-bot]

> The diag dump walks the socket hash table under a read_lock and dereferences
> conn->lgr and conn->lnk.  Two terminal teardown paths drop those references via
> smc_conn_free() while the socket is still hashed:
> 
>   - smc_conn_kill() -> smc_close_active_abort() -> smc_conn_free()
>   - smc_close_passive_work() -> smc_conn_free()
> 
> This allows the diag reader to dereference a freed lgr or lnk.
> 
> Fix it by unhashing the socket before smc_conn_free() is called at each of these
> two sites.  Any socket visible to the diag reader under the hash read_lock then
> has valid conn->lgr and conn->lnk pointers.
> 
> Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets")
> Fixes: 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R connections")
> Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

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

[Dust Li]

On 2026-09-11 11:09:06, Mahanta Jambigi wrote:
>The diag dump walks the socket hash table under a read_lock and dereferences
>conn->lgr and conn->lnk.  Two terminal teardown paths drop those references via
>smc_conn_free() while the socket is still hashed:
>
>  - smc_conn_kill() -> smc_close_active_abort() -> smc_conn_free()
>  - smc_close_passive_work() -> smc_conn_free()
>
>This allows the diag reader to dereference a freed lgr or lnk.
>
>Fix it by unhashing the socket before smc_conn_free() is called at each of these
>two sites.  Any socket visible to the diag reader under the hash read_lock then
>has valid conn->lgr and conn->lnk pointers.
>
>Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets")
>Fixes: 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R connections")
>Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
>---
>Changes in v4:
>- dropped smc_conn_unhash() wrapper, conn->unhashed flag, and all
>  changes to af_smc.c, smc.h, smc_core.c and smc_core.h; smc_unhash_sk()
>  is already idempotent via sk_hashed(), so direct calls at the two
>  teardown sites in smc_close.c are sufficient
>- dropped the __smc_release() hunk: it needs no change since the
>  subsequent unhash there is already a safe no-op
>- fixed premature-unhash issue present in v3: smc_conn_free() must not
>  unhash because smc_conn_abort() calls it before smc_switch_to_fallback()
>  in both smc_listen_decline() and smc_connect_rdma() error paths;
>  unhashing there would make live fallback sockets invisible to smcss
>- likewise, the ISM/RDMA retry loops (smc_find_ism_v2_device_serv(),
>  smc_find_rdma_v2_device_serv()) call smc_conn_abort() on a failed
>  attempt and then smc_conn_create() on the next device; unhashing in
>  smc_conn_free() would permanently hide the established connection from
>  smc_diag since smc_conn_create() does not re-hash the socket

Hi Mahanta,

This version looks clean. And you explained why we can't call unhash in
smc_conn_abort() well. But smc_conn_abort() still calls smc_conn_free(),
when the smc_sk is still hashed, is there still a race window with dump ?

Best regards,
Dust

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

[Mahanta Jambigi]

On 15/09/26 8:43 pm, Dust Li wrote:
> On 2026-09-11 11:09:06, Mahanta Jambigi wrote:
>> The diag dump walks the socket hash table under a read_lock and
>> dereferences conn->lgr and conn->lnk.  Two terminal teardown paths
>> drop those references via smc_conn_free() while the socket is still
>> hashed:
>> 
>> - smc_conn_kill() -> smc_close_active_abort() -> smc_conn_free() -
>> smc_close_passive_work() -> smc_conn_free()
>> 
>> This allows the diag reader to dereference a freed lgr or lnk.
>> 
>> Fix it by unhashing the socket before smc_conn_free() is called at
>> each of these two sites.  Any socket visible to the diag reader
>> under the hash read_lock then has valid conn->lgr and conn->lnk
>> pointers.
>> 
>> Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets") Fixes:
>> 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R
>> connections") Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
>> --- Changes in v4: - dropped smc_conn_unhash() wrapper, conn-
>> >unhashed flag, and all changes to af_smc.c, smc.h, smc_core.c and
>> smc_core.h; smc_unhash_sk() is already idempotent via sk_hashed(),
>> so direct calls at the two teardown sites in smc_close.c are
>> sufficient - dropped the __smc_release() hunk: it needs no change
>> since the subsequent unhash there is already a safe no-op - fixed
>> premature-unhash issue present in v3: smc_conn_free() must not unhash
>> because smc_conn_abort() calls it before smc_switch_to_fallback() in
>> both smc_listen_decline() and smc_connect_rdma() error paths; unhashing
>> there would make live fallback sockets invisible to smcss -
>> likewise, the ISM/RDMA retry loops (smc_find_ism_v2_device_serv(), 
>> smc_find_rdma_v2_device_serv()) call smc_conn_abort() on a failed attempt
>> and then smc_conn_create() on the next device; unhashing in 
>> smc_conn_free() would permanently hide the established connection
>> from smc_diag since smc_conn_create() does not re-hash the socket
> 
> Hi Mahanta,
> 
> This version looks clean. And you explained why we can't call unhash
> in smc_conn_abort() well. But smc_conn_abort() still calls
> smc_conn_free(), when the smc_sk is still hashed, is there still a
> race window with dump ?

Hi Dust,

Thank you for catching this corner case!

During early handshake setup (when sk_state is *SMC_INIT*),
smc_conn_abort() can be called on connection failure/fallback and
invokes smc_conn_free() while the socket remains hashed, leaving a
window where a concurrent diag dump could evaluate smc_conn_lgr_valid()
and dereference conn->lgr / conn->lnk.

Since sockets in *SMC_INIT* are in a transient embryonic handshake phase
and userspace (smcss) *skips displaying link-group, DMB, and connection
details for INIT state sockets anyway*, we could have __smc_diag_dump()
skip inspecting connection/link-group extensions when r->diag_state ==
SMC_INIT:

diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
--- a/net/smc/smc_diag.c
+++ b/net/smc/smc_diag.c
@@ -90,6 +90,9 @@ static int __smc_diag_dump(struct sock *sk, struct
sk_buff *skb,
 	r->diag_state = sk->sk_state;
+	if (r->diag_state == SMC_INIT)
+		return 0;
+
 	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)

Together with unhashing before smc_conn_free() in smc_close.c for
established and closing sockets, this cleanly closes the race window
across all socket states without touching the hash table mechanics
during fallback/retry.

Does this approach look good to you? If you agree, I will prepare and
submit v5 with this change. Please let me know if you have any other
suggestions or alternative approaches, and I'll be happy to look into them.

Best regards,
Mahant

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

[Dust Li]

On 2026-09-16 14:04:42, Mahanta Jambigi wrote:
>
>
>On 15/09/26 8:43 pm, Dust Li wrote:
>> On 2026-09-11 11:09:06, Mahanta Jambigi wrote:
>>> The diag dump walks the socket hash table under a read_lock and
>>> dereferences conn->lgr and conn->lnk.  Two terminal teardown paths
>>> drop those references via smc_conn_free() while the socket is still
>>> hashed:
>>> 
>>> - smc_conn_kill() -> smc_close_active_abort() -> smc_conn_free() -
>>> smc_close_passive_work() -> smc_conn_free()
>>> 
>>> This allows the diag reader to dereference a freed lgr or lnk.
>>> 
>>> Fix it by unhashing the socket before smc_conn_free() is called at
>>> each of these two sites.  Any socket visible to the diag reader
>>> under the hash read_lock then has valid conn->lgr and conn->lnk
>>> pointers.
>>> 
>>> Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets") Fixes:
>>> 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R
>>> connections") Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
>>> --- Changes in v4: - dropped smc_conn_unhash() wrapper, conn-
>>> >unhashed flag, and all changes to af_smc.c, smc.h, smc_core.c and
>>> smc_core.h; smc_unhash_sk() is already idempotent via sk_hashed(),
>>> so direct calls at the two teardown sites in smc_close.c are
>>> sufficient - dropped the __smc_release() hunk: it needs no change
>>> since the subsequent unhash there is already a safe no-op - fixed
>>> premature-unhash issue present in v3: smc_conn_free() must not unhash
>>> because smc_conn_abort() calls it before smc_switch_to_fallback() in
>>> both smc_listen_decline() and smc_connect_rdma() error paths; unhashing
>>> there would make live fallback sockets invisible to smcss -
>>> likewise, the ISM/RDMA retry loops (smc_find_ism_v2_device_serv(), 
>>> smc_find_rdma_v2_device_serv()) call smc_conn_abort() on a failed attempt
>>> and then smc_conn_create() on the next device; unhashing in 
>>> smc_conn_free() would permanently hide the established connection
>>> from smc_diag since smc_conn_create() does not re-hash the socket
>> 
>> Hi Mahanta,
>> 
>> This version looks clean. And you explained why we can't call unhash
>> in smc_conn_abort() well. But smc_conn_abort() still calls
>> smc_conn_free(), when the smc_sk is still hashed, is there still a
>> race window with dump ?
>
>Hi Dust,
>
>Thank you for catching this corner case!
>
>During early handshake setup (when sk_state is *SMC_INIT*),
>smc_conn_abort() can be called on connection failure/fallback and
>invokes smc_conn_free() while the socket remains hashed, leaving a
>window where a concurrent diag dump could evaluate smc_conn_lgr_valid()
>and dereference conn->lgr / conn->lnk.
>
>Since sockets in *SMC_INIT* are in a transient embryonic handshake phase
>and userspace (smcss) *skips displaying link-group, DMB, and connection
>details for INIT state sockets anyway*, we could have __smc_diag_dump()
>skip inspecting connection/link-group extensions when r->diag_state ==
>SMC_INIT:
>
>diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
>--- a/net/smc/smc_diag.c
>+++ b/net/smc/smc_diag.c
>@@ -90,6 +90,9 @@ static int __smc_diag_dump(struct sock *sk, struct
>sk_buff *skb,
> 	r->diag_state = sk->sk_state;
>+	if (r->diag_state == SMC_INIT)
>+		return 0;
>+
> 	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)
>
>Together with unhashing before smc_conn_free() in smc_close.c for
>established and closing sockets, this cleanly closes the race window
>across all socket states without touching the hash table mechanics
>during fallback/retry.
>
>Does this approach look good to you? If you agree, I will prepare and
>submit v5 with this change. Please let me know if you have any other
>suggestions or alternative approaches, and I'll be happy to look into them.

What about this path ?

smc_listen_decline() -> smc_listen_out_err(), where smc_conn_abort() has
already run, and the sock state is then changed from SMC_INIT to SMC_CLOSED.

Best regards,
Dust

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

[Mahanta Jambigi]

On 16/09/26 8:54 pm, Dust Li wrote:
> On 2026-09-16 14:04:42, Mahanta Jambigi wrote:
>>
>>
>> On 15/09/26 8:43 pm, Dust Li wrote:
>>> On 2026-09-11 11:09:06, Mahanta Jambigi wrote:
>>>> The diag dump walks the socket hash table under a read_lock and
>>>> dereferences conn->lgr and conn->lnk.  Two terminal teardown paths
>>>> drop those references via smc_conn_free() while the socket is still
>>>> hashed:
>>>>
>>>> - smc_conn_kill() -> smc_close_active_abort() -> smc_conn_free() -
>>>> smc_close_passive_work() -> smc_conn_free()
>>>>
>>>> This allows the diag reader to dereference a freed lgr or lnk.
>>>>
>>>> Fix it by unhashing the socket before smc_conn_free() is called at
>>>> each of these two sites.  Any socket visible to the diag reader
>>>> under the hash read_lock then has valid conn->lgr and conn->lnk
>>>> pointers.
>>>>
>>>> Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets") Fixes:
>>>> 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R
>>>> connections") Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
>>>> --- Changes in v4: - dropped smc_conn_unhash() wrapper, conn-
>>>>> unhashed flag, and all changes to af_smc.c, smc.h, smc_core.c and
>>>> smc_core.h; smc_unhash_sk() is already idempotent via sk_hashed(),
>>>> so direct calls at the two teardown sites in smc_close.c are
>>>> sufficient - dropped the __smc_release() hunk: it needs no change
>>>> since the subsequent unhash there is already a safe no-op - fixed
>>>> premature-unhash issue present in v3: smc_conn_free() must not unhash
>>>> because smc_conn_abort() calls it before smc_switch_to_fallback() in
>>>> both smc_listen_decline() and smc_connect_rdma() error paths; unhashing
>>>> there would make live fallback sockets invisible to smcss -
>>>> likewise, the ISM/RDMA retry loops (smc_find_ism_v2_device_serv(), 
>>>> smc_find_rdma_v2_device_serv()) call smc_conn_abort() on a failed attempt
>>>> and then smc_conn_create() on the next device; unhashing in 
>>>> smc_conn_free() would permanently hide the established connection
>>>> from smc_diag since smc_conn_create() does not re-hash the socket
>>>
>>> Hi Mahanta,
>>>
>>> This version looks clean. And you explained why we can't call unhash
>>> in smc_conn_abort() well. But smc_conn_abort() still calls
>>> smc_conn_free(), when the smc_sk is still hashed, is there still a
>>> race window with dump ?
>>
>> Hi Dust,
>>
>> Thank you for catching this corner case!
>>
>> During early handshake setup (when sk_state is *SMC_INIT*),
>> smc_conn_abort() can be called on connection failure/fallback and
>> invokes smc_conn_free() while the socket remains hashed, leaving a
>> window where a concurrent diag dump could evaluate smc_conn_lgr_valid()
>> and dereference conn->lgr / conn->lnk.
>>
>> Since sockets in *SMC_INIT* are in a transient embryonic handshake phase
>> and userspace (smcss) *skips displaying link-group, DMB, and connection
>> details for INIT state sockets anyway*, we could have __smc_diag_dump()
>> skip inspecting connection/link-group extensions when r->diag_state ==
>> SMC_INIT:
>>
>> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
>> --- a/net/smc/smc_diag.c
>> +++ b/net/smc/smc_diag.c
>> @@ -90,6 +90,9 @@ static int __smc_diag_dump(struct sock *sk, struct
>> sk_buff *skb,
>> 	r->diag_state = sk->sk_state;
>> +	if (r->diag_state == SMC_INIT)
>> +		return 0;
>> +
>> 	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)
>>
>> Together with unhashing before smc_conn_free() in smc_close.c for
>> established and closing sockets, this cleanly closes the race window
>> across all socket states without touching the hash table mechanics
>> during fallback/retry.
>>
>> Does this approach look good to you? If you agree, I will prepare and
>> submit v5 with this change. Please let me know if you have any other
>> suggestions or alternative approaches, and I'll be happy to look into them.
> 
> What about this path ?
> 
> smc_listen_decline() -> smc_listen_out_err(), where smc_conn_abort() has
> already run, and the sock state is then changed from SMC_INIT to SMC_CLOSED.

Hi Dust,

Good catch on the smc_listen_out_err() path as well!

In smc_listen_decline() -> smc_listen_out_err(), the server socket fails
handshake and is transitioned to SMC_CLOSED while remaining in the hash
table until smc_accept_dequeue() runs.

Since userspace (smcss) skips displaying all connection, link-group, and
DMB details for both SMC_INIT and SMC_CLOSED sockets (smcss.c:186 and
smcss.c:199), we can update __smc_diag_dump() to check for both states:

diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
--- a/net/smc/smc_diag.c
+++ b/net/smc/smc_diag.c
@@ -90,6 +90,9 @@ static int __smc_diag_dump(struct sock *sk, struct
sk_buff *skb,
 	r->diag_state = sk->sk_state;
+	if (r->diag_state == SMC_INIT || r->diag_state == SMC_CLOSED)
+		return 0;
+
 	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)

This guarantees that __smc_diag_dump() never attempts to inspect
connection or link-group fields for sockets that are either still in
embryonic setup (SMC_INIT) or have aborted/closed (SMC_CLOSED).

Combined with unhashing before smc_conn_free() in smc_close.c for
established connections during active/passive close, this cleanly closes
the race window across all tear-down and abort paths.

Does this approach look good to you? If you agree, I will prepare and
submit v5 with this update.

Best regards,
Mahanta

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

[Dust Li]

On 2026-09-17 13:15:23, Mahanta Jambigi wrote:
>
>
>On 16/09/26 8:54 pm, Dust Li wrote:
>> On 2026-09-16 14:04:42, Mahanta Jambigi wrote:
>>>
>>>
>>> On 15/09/26 8:43 pm, Dust Li wrote:
>>>> On 2026-09-11 11:09:06, Mahanta Jambigi wrote:
>>>>> The diag dump walks the socket hash table under a read_lock and
>>>>> dereferences conn->lgr and conn->lnk.  Two terminal teardown paths
>>>>> drop those references via smc_conn_free() while the socket is still
>>>>> hashed:
>>>>>
>>>>> - smc_conn_kill() -> smc_close_active_abort() -> smc_conn_free() -
>>>>> smc_close_passive_work() -> smc_conn_free()
>>>>>
>>>>> This allows the diag reader to dereference a freed lgr or lnk.
>>>>>
>>>>> Fix it by unhashing the socket before smc_conn_free() is called at
>>>>> each of these two sites.  Any socket visible to the diag reader
>>>>> under the hash read_lock then has valid conn->lgr and conn->lnk
>>>>> pointers.
>>>>>
>>>>> Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets") Fixes:
>>>>> 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R
>>>>> connections") Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
>>>>> --- Changes in v4: - dropped smc_conn_unhash() wrapper, conn-
>>>>>> unhashed flag, and all changes to af_smc.c, smc.h, smc_core.c and
>>>>> smc_core.h; smc_unhash_sk() is already idempotent via sk_hashed(),
>>>>> so direct calls at the two teardown sites in smc_close.c are
>>>>> sufficient - dropped the __smc_release() hunk: it needs no change
>>>>> since the subsequent unhash there is already a safe no-op - fixed
>>>>> premature-unhash issue present in v3: smc_conn_free() must not unhash
>>>>> because smc_conn_abort() calls it before smc_switch_to_fallback() in
>>>>> both smc_listen_decline() and smc_connect_rdma() error paths; unhashing
>>>>> there would make live fallback sockets invisible to smcss -
>>>>> likewise, the ISM/RDMA retry loops (smc_find_ism_v2_device_serv(), 
>>>>> smc_find_rdma_v2_device_serv()) call smc_conn_abort() on a failed attempt
>>>>> and then smc_conn_create() on the next device; unhashing in 
>>>>> smc_conn_free() would permanently hide the established connection
>>>>> from smc_diag since smc_conn_create() does not re-hash the socket
>>>>
>>>> Hi Mahanta,
>>>>
>>>> This version looks clean. And you explained why we can't call unhash
>>>> in smc_conn_abort() well. But smc_conn_abort() still calls
>>>> smc_conn_free(), when the smc_sk is still hashed, is there still a
>>>> race window with dump ?
>>>
>>> Hi Dust,
>>>
>>> Thank you for catching this corner case!
>>>
>>> During early handshake setup (when sk_state is *SMC_INIT*),
>>> smc_conn_abort() can be called on connection failure/fallback and
>>> invokes smc_conn_free() while the socket remains hashed, leaving a
>>> window where a concurrent diag dump could evaluate smc_conn_lgr_valid()
>>> and dereference conn->lgr / conn->lnk.
>>>
>>> Since sockets in *SMC_INIT* are in a transient embryonic handshake phase
>>> and userspace (smcss) *skips displaying link-group, DMB, and connection
>>> details for INIT state sockets anyway*, we could have __smc_diag_dump()
>>> skip inspecting connection/link-group extensions when r->diag_state ==
>>> SMC_INIT:
>>>
>>> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
>>> --- a/net/smc/smc_diag.c
>>> +++ b/net/smc/smc_diag.c
>>> @@ -90,6 +90,9 @@ static int __smc_diag_dump(struct sock *sk, struct
>>> sk_buff *skb,
>>> 	r->diag_state = sk->sk_state;
>>> +	if (r->diag_state == SMC_INIT)
>>> +		return 0;
>>> +
>>> 	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)
>>>
>>> Together with unhashing before smc_conn_free() in smc_close.c for
>>> established and closing sockets, this cleanly closes the race window
>>> across all socket states without touching the hash table mechanics
>>> during fallback/retry.
>>>
>>> Does this approach look good to you? If you agree, I will prepare and
>>> submit v5 with this change. Please let me know if you have any other
>>> suggestions or alternative approaches, and I'll be happy to look into them.
>> 
>> What about this path ?
>> 
>> smc_listen_decline() -> smc_listen_out_err(), where smc_conn_abort() has
>> already run, and the sock state is then changed from SMC_INIT to SMC_CLOSED.
>
>Hi Dust,
>
>Good catch on the smc_listen_out_err() path as well!
>
>In smc_listen_decline() -> smc_listen_out_err(), the server socket fails
>handshake and is transitioned to SMC_CLOSED while remaining in the hash
>table until smc_accept_dequeue() runs.
>
>Since userspace (smcss) skips displaying all connection, link-group, and
>DMB details for both SMC_INIT and SMC_CLOSED sockets (smcss.c:186 and
>smcss.c:199), we can update __smc_diag_dump() to check for both states:
>
>diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
>--- a/net/smc/smc_diag.c
>+++ b/net/smc/smc_diag.c
>@@ -90,6 +90,9 @@ static int __smc_diag_dump(struct sock *sk, struct
>sk_buff *skb,
> 	r->diag_state = sk->sk_state;
>+	if (r->diag_state == SMC_INIT || r->diag_state == SMC_CLOSED)
>+		return 0;
>+
> 	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)
>

I checked the code again, and I'm afraid adding SMC_CLOSED is still not right.

smc_listen_decline(): smc_switch_to_fallback(), smc_clc_send_decline() then
fails, and smc_listen_out_err() sets the socket to SMC_CLOSED. The socket
stays on the accept queue, hashed, until accept() reaches smc_accept_dequeue().

But the early return sits after nlmsg_put() but before diag_mode,
smc_diag_msg_attrs_fill() and the SMC_DIAG_FALLBACK attribute are filled.
The record is still emitted, but diag_mode is left at 0 -- which is
SMC_DIAG_MODE_SMCR, not "unknown" -- and the fallback reason attribute is gone.

Why don't you use conn->free instead of sk_state ? It is set unconditionally
at the top of smc_conn_free() before any lgr/lnk reference is dropped.


So maybe something like this ? Please double check.

diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
index bf0beaa23bdb6..a19f16881ae7e 100644
--- a/net/smc/smc_diag.c
+++ b/net/smc/smc_diag.c
@@ -103,6 +103,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 (smc->conn.freed)
+               goto out;
+
        if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
            smc->conn.alert_token_local) {
                struct smc_connection *conn = &smc->conn;
@@ -185,6 +188,7 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
                        goto errout;
        }

+out:
        nlmsg_end(skb, nlh);
        return 0;

BTW, as we talked in the previous threads, the teardown path is messy and lots
of hidden holes, so I think we will finnally refine those.
As for now, if this works, I think we can go with it.

Best regards,
Dust

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

[Mahanta Jambigi]

On 17/09/26 9:23 pm, Dust Li wrote:
> On 2026-09-17 13:15:23, Mahanta Jambigi wrote:
>>
>>
>> On 16/09/26 8:54 pm, Dust Li wrote:
>>> On 2026-09-16 14:04:42, Mahanta Jambigi wrote:
>>>>
>>>>
>>>> On 15/09/26 8:43 pm, Dust Li wrote:
>>>>> On 2026-09-11 11:09:06, Mahanta Jambigi wrote:
>>>>>> The diag dump walks the socket hash table under a read_lock and
>>>>>> dereferences conn->lgr and conn->lnk.  Two terminal teardown paths
>>>>>> drop those references via smc_conn_free() while the socket is still
>>>>>> hashed:
>>>>>>
>>>>>> - smc_conn_kill() -> smc_close_active_abort() -> smc_conn_free() -
>>>>>> smc_close_passive_work() -> smc_conn_free()
>>>>>>
>>>>>> This allows the diag reader to dereference a freed lgr or lnk.
>>>>>>
>>>>>> Fix it by unhashing the socket before smc_conn_free() is called at
>>>>>> each of these two sites.  Any socket visible to the diag reader
>>>>>> under the hash read_lock then has valid conn->lgr and conn->lnk
>>>>>> pointers.
>>>>>>
>>>>>> Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets") Fixes:
>>>>>> 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R
>>>>>> connections") Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
>>>>>> --- Changes in v4: - dropped smc_conn_unhash() wrapper, conn-
>>>>>>> unhashed flag, and all changes to af_smc.c, smc.h, smc_core.c and
>>>>>> smc_core.h; smc_unhash_sk() is already idempotent via sk_hashed(),
>>>>>> so direct calls at the two teardown sites in smc_close.c are
>>>>>> sufficient - dropped the __smc_release() hunk: it needs no change
>>>>>> since the subsequent unhash there is already a safe no-op - fixed
>>>>>> premature-unhash issue present in v3: smc_conn_free() must not unhash
>>>>>> because smc_conn_abort() calls it before smc_switch_to_fallback() in
>>>>>> both smc_listen_decline() and smc_connect_rdma() error paths; unhashing
>>>>>> there would make live fallback sockets invisible to smcss -
>>>>>> likewise, the ISM/RDMA retry loops (smc_find_ism_v2_device_serv(), 
>>>>>> smc_find_rdma_v2_device_serv()) call smc_conn_abort() on a failed attempt
>>>>>> and then smc_conn_create() on the next device; unhashing in 
>>>>>> smc_conn_free() would permanently hide the established connection
>>>>>> from smc_diag since smc_conn_create() does not re-hash the socket
>>>>>
>>>>> Hi Mahanta,
>>>>>
>>>>> This version looks clean. And you explained why we can't call unhash
>>>>> in smc_conn_abort() well. But smc_conn_abort() still calls
>>>>> smc_conn_free(), when the smc_sk is still hashed, is there still a
>>>>> race window with dump ?
>>>>
>>>> Hi Dust,
>>>>
>>>> Thank you for catching this corner case!
>>>>
>>>> During early handshake setup (when sk_state is *SMC_INIT*),
>>>> smc_conn_abort() can be called on connection failure/fallback and
>>>> invokes smc_conn_free() while the socket remains hashed, leaving a
>>>> window where a concurrent diag dump could evaluate smc_conn_lgr_valid()
>>>> and dereference conn->lgr / conn->lnk.
>>>>
>>>> Since sockets in *SMC_INIT* are in a transient embryonic handshake phase
>>>> and userspace (smcss) *skips displaying link-group, DMB, and connection
>>>> details for INIT state sockets anyway*, we could have __smc_diag_dump()
>>>> skip inspecting connection/link-group extensions when r->diag_state ==
>>>> SMC_INIT:
>>>>
>>>> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
>>>> --- a/net/smc/smc_diag.c
>>>> +++ b/net/smc/smc_diag.c
>>>> @@ -90,6 +90,9 @@ static int __smc_diag_dump(struct sock *sk, struct
>>>> sk_buff *skb,
>>>> 	r->diag_state = sk->sk_state;
>>>> +	if (r->diag_state == SMC_INIT)
>>>> +		return 0;
>>>> +
>>>> 	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)
>>>>
>>>> Together with unhashing before smc_conn_free() in smc_close.c for
>>>> established and closing sockets, this cleanly closes the race window
>>>> across all socket states without touching the hash table mechanics
>>>> during fallback/retry.
>>>>
>>>> Does this approach look good to you? If you agree, I will prepare and
>>>> submit v5 with this change. Please let me know if you have any other
>>>> suggestions or alternative approaches, and I'll be happy to look into them.
>>>
>>> What about this path ?
>>>
>>> smc_listen_decline() -> smc_listen_out_err(), where smc_conn_abort() has
>>> already run, and the sock state is then changed from SMC_INIT to SMC_CLOSED.
>>
>> Hi Dust,
>>
>> Good catch on the smc_listen_out_err() path as well!
>>
>> In smc_listen_decline() -> smc_listen_out_err(), the server socket fails
>> handshake and is transitioned to SMC_CLOSED while remaining in the hash
>> table until smc_accept_dequeue() runs.
>>
>> Since userspace (smcss) skips displaying all connection, link-group, and
>> DMB details for both SMC_INIT and SMC_CLOSED sockets (smcss.c:186 and
>> smcss.c:199), we can update __smc_diag_dump() to check for both states:
>>
>> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
>> --- a/net/smc/smc_diag.c
>> +++ b/net/smc/smc_diag.c
>> @@ -90,6 +90,9 @@ static int __smc_diag_dump(struct sock *sk, struct
>> sk_buff *skb,
>> 	r->diag_state = sk->sk_state;
>> +	if (r->diag_state == SMC_INIT || r->diag_state == SMC_CLOSED)
>> +		return 0;
>> +
>> 	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)
>>
> 
> I checked the code again, and I'm afraid adding SMC_CLOSED is still not right.
> 
> smc_listen_decline(): smc_switch_to_fallback(), smc_clc_send_decline() then
> fails, and smc_listen_out_err() sets the socket to SMC_CLOSED. The socket
> stays on the accept queue, hashed, until accept() reaches smc_accept_dequeue().
> 
> But the early return sits after nlmsg_put() but before diag_mode,
> smc_diag_msg_attrs_fill() and the SMC_DIAG_FALLBACK attribute are filled.
> The record is still emitted, but diag_mode is left at 0 -- which is
> SMC_DIAG_MODE_SMCR, not "unknown" -- and the fallback reason attribute is gone.

Regarding your concern about wrong output when the early return fires
before diag_mode and SMC_DIAG_FALLBACK are filled: you are right that
diag_mode must be filled before the guard. *smcss.c* reads diag_mode at
lines 177 and 179 for --smcr/--smcd filtering, which happens before the
SMC_INIT and SMC_CLOSED state checks. If diag_mode is left as 0 for
a failed-fallback SMC_CLOSED socket, --smcr would incorrectly include it
and --smcd would incorrectly exclude it.

*SMC_DIAG_FALLBACK* however is *not* needed before the guard — smcss
only reads it inside the diag_mode == SMC_DIAG_MODE_FALLBACK_TCP branch
which is never reached for *SMC_INIT* or *SMC_CLOSED* sockets, as those
two states hit goto newline before that point.

So the correct placement is after smc_diag_msg_attrs_fill(), which fills
diag_mode, diag_uid, and diag_inode, but before SMC_DIAG_FALLBACK:

    r->diag_state = sk->sk_state;
    if (smc->use_fallback)
        r->diag_mode = SMC_DIAG_MODE_FALLBACK_TCP;
    else if (...)
        ...
    if (smc_diag_msg_attrs_fill(sk, skb, r, user_ns))
        goto errout;

+   if (r->diag_state == SMC_INIT || r->diag_state == SMC_CLOSED)
+       goto out;

    fallback.reason = smc->fallback_rsn;
    ...

+out:
    nlmsg_end(skb, nlh);
    return 0;

At this position all fields smcss reads for these two states are already
filled correctly, including diag_mode for --smcr/--smcd filtering.  The
CONNINFO, LGRINFO, and DMBINFO blocks — which contain the unsafe lgr/lnk
dereferences — are never reached.

What is your opinion on this?
> 
> Why don't you use conn->free instead of sk_state ? It is set unconditionally
> at the top of smc_conn_free() before any lgr/lnk reference is dropped.
> 
> 
> So maybe something like this ? Please double check.
> 
> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
> index bf0beaa23bdb6..a19f16881ae7e 100644
> --- a/net/smc/smc_diag.c
> +++ b/net/smc/smc_diag.c
> @@ -103,6 +103,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 (smc->conn.freed)
> +               goto out;
> +

Regarding conn->freed: it does not fully close the race. The check and
the subsequent lgr/lnk dereferences in the LGRINFO/DMBINFO blocks are
not atomic. The diag reader can pass the conn->freed == 0 check, then
smc_conn_free() runs concurrently and calls smc_lgr_put() which may drop
the lgr refcount to zero and free lgr, and then the reader resumes and
dereferences conn->lgr — a UAF.

>         if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
>             smc->conn.alert_token_local) {
>                 struct smc_connection *conn = &smc->conn;
> @@ -185,6 +188,7 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
>                         goto errout;
>         }
> 
> +out:
>         nlmsg_end(skb, nlh);
>         return 0;
> 
> BTW, as we talked in the previous threads, the teardown path is messy and lots
> of hidden holes, so I think we will finnally refine those.
> As for now, if this works, I think we can go with it.

I agree, as of now I am trying to fix the UAF bug with minimal changes.

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

[Dust Li]

On 2026-09-18 12:57:13, Mahanta Jambigi wrote:
>
>
>On 17/09/26 9:23 pm, Dust Li wrote:
>> On 2026-09-17 13:15:23, Mahanta Jambigi wrote:
>>>
>>>
>>> On 16/09/26 8:54 pm, Dust Li wrote:
>>>> On 2026-09-16 14:04:42, Mahanta Jambigi wrote:
>>>>>
>>>>>
>>>>> On 15/09/26 8:43 pm, Dust Li wrote:
>>>>>> On 2026-09-11 11:09:06, Mahanta Jambigi wrote:
>>>>>>> The diag dump walks the socket hash table under a read_lock and
>>>>>>> dereferences conn->lgr and conn->lnk.  Two terminal teardown paths
>>>>>>> drop those references via smc_conn_free() while the socket is still
>>>>>>> hashed:
>>>>>>>
>>>>>>> - smc_conn_kill() -> smc_close_active_abort() -> smc_conn_free() -
>>>>>>> smc_close_passive_work() -> smc_conn_free()
>>>>>>>
>>>>>>> This allows the diag reader to dereference a freed lgr or lnk.
>>>>>>>
>>>>>>> Fix it by unhashing the socket before smc_conn_free() is called at
>>>>>>> each of these two sites.  Any socket visible to the diag reader
>>>>>>> under the hash read_lock then has valid conn->lgr and conn->lnk
>>>>>>> pointers.
>>>>>>>
>>>>>>> Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets") Fixes:
>>>>>>> 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R
>>>>>>> connections") Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
>>>>>>> --- Changes in v4: - dropped smc_conn_unhash() wrapper, conn-
>>>>>>>> unhashed flag, and all changes to af_smc.c, smc.h, smc_core.c and
>>>>>>> smc_core.h; smc_unhash_sk() is already idempotent via sk_hashed(),
>>>>>>> so direct calls at the two teardown sites in smc_close.c are
>>>>>>> sufficient - dropped the __smc_release() hunk: it needs no change
>>>>>>> since the subsequent unhash there is already a safe no-op - fixed
>>>>>>> premature-unhash issue present in v3: smc_conn_free() must not unhash
>>>>>>> because smc_conn_abort() calls it before smc_switch_to_fallback() in
>>>>>>> both smc_listen_decline() and smc_connect_rdma() error paths; unhashing
>>>>>>> there would make live fallback sockets invisible to smcss -
>>>>>>> likewise, the ISM/RDMA retry loops (smc_find_ism_v2_device_serv(), 
>>>>>>> smc_find_rdma_v2_device_serv()) call smc_conn_abort() on a failed attempt
>>>>>>> and then smc_conn_create() on the next device; unhashing in 
>>>>>>> smc_conn_free() would permanently hide the established connection
>>>>>>> from smc_diag since smc_conn_create() does not re-hash the socket
>>>>>>
>>>>>> Hi Mahanta,
>>>>>>
>>>>>> This version looks clean. And you explained why we can't call unhash
>>>>>> in smc_conn_abort() well. But smc_conn_abort() still calls
>>>>>> smc_conn_free(), when the smc_sk is still hashed, is there still a
>>>>>> race window with dump ?
>>>>>
>>>>> Hi Dust,
>>>>>
>>>>> Thank you for catching this corner case!
>>>>>
>>>>> During early handshake setup (when sk_state is *SMC_INIT*),
>>>>> smc_conn_abort() can be called on connection failure/fallback and
>>>>> invokes smc_conn_free() while the socket remains hashed, leaving a
>>>>> window where a concurrent diag dump could evaluate smc_conn_lgr_valid()
>>>>> and dereference conn->lgr / conn->lnk.
>>>>>
>>>>> Since sockets in *SMC_INIT* are in a transient embryonic handshake phase
>>>>> and userspace (smcss) *skips displaying link-group, DMB, and connection
>>>>> details for INIT state sockets anyway*, we could have __smc_diag_dump()
>>>>> skip inspecting connection/link-group extensions when r->diag_state ==
>>>>> SMC_INIT:
>>>>>
>>>>> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
>>>>> --- a/net/smc/smc_diag.c
>>>>> +++ b/net/smc/smc_diag.c
>>>>> @@ -90,6 +90,9 @@ static int __smc_diag_dump(struct sock *sk, struct
>>>>> sk_buff *skb,
>>>>> 	r->diag_state = sk->sk_state;
>>>>> +	if (r->diag_state == SMC_INIT)
>>>>> +		return 0;
>>>>> +
>>>>> 	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)
>>>>>
>>>>> Together with unhashing before smc_conn_free() in smc_close.c for
>>>>> established and closing sockets, this cleanly closes the race window
>>>>> across all socket states without touching the hash table mechanics
>>>>> during fallback/retry.
>>>>>
>>>>> Does this approach look good to you? If you agree, I will prepare and
>>>>> submit v5 with this change. Please let me know if you have any other
>>>>> suggestions or alternative approaches, and I'll be happy to look into them.
>>>>
>>>> What about this path ?
>>>>
>>>> smc_listen_decline() -> smc_listen_out_err(), where smc_conn_abort() has
>>>> already run, and the sock state is then changed from SMC_INIT to SMC_CLOSED.
>>>
>>> Hi Dust,
>>>
>>> Good catch on the smc_listen_out_err() path as well!
>>>
>>> In smc_listen_decline() -> smc_listen_out_err(), the server socket fails
>>> handshake and is transitioned to SMC_CLOSED while remaining in the hash
>>> table until smc_accept_dequeue() runs.
>>>
>>> Since userspace (smcss) skips displaying all connection, link-group, and
>>> DMB details for both SMC_INIT and SMC_CLOSED sockets (smcss.c:186 and
>>> smcss.c:199), we can update __smc_diag_dump() to check for both states:
>>>
>>> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
>>> --- a/net/smc/smc_diag.c
>>> +++ b/net/smc/smc_diag.c
>>> @@ -90,6 +90,9 @@ static int __smc_diag_dump(struct sock *sk, struct
>>> sk_buff *skb,
>>> 	r->diag_state = sk->sk_state;
>>> +	if (r->diag_state == SMC_INIT || r->diag_state == SMC_CLOSED)
>>> +		return 0;
>>> +
>>> 	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)
>>>
>> 
>> I checked the code again, and I'm afraid adding SMC_CLOSED is still not right.
>> 
>> smc_listen_decline(): smc_switch_to_fallback(), smc_clc_send_decline() then
>> fails, and smc_listen_out_err() sets the socket to SMC_CLOSED. The socket
>> stays on the accept queue, hashed, until accept() reaches smc_accept_dequeue().
>> 
>> But the early return sits after nlmsg_put() but before diag_mode,
>> smc_diag_msg_attrs_fill() and the SMC_DIAG_FALLBACK attribute are filled.
>> The record is still emitted, but diag_mode is left at 0 -- which is
>> SMC_DIAG_MODE_SMCR, not "unknown" -- and the fallback reason attribute is gone.
>
>Regarding your concern about wrong output when the early return fires
>before diag_mode and SMC_DIAG_FALLBACK are filled: you are right that
>diag_mode must be filled before the guard. *smcss.c* reads diag_mode at
>lines 177 and 179 for --smcr/--smcd filtering, which happens before the
>SMC_INIT and SMC_CLOSED state checks. If diag_mode is left as 0 for
>a failed-fallback SMC_CLOSED socket, --smcr would incorrectly include it
>and --smcd would incorrectly exclude it.
>
>*SMC_DIAG_FALLBACK* however is *not* needed before the guard — smcss
>only reads it inside the diag_mode == SMC_DIAG_MODE_FALLBACK_TCP branch
>which is never reached for *SMC_INIT* or *SMC_CLOSED* sockets, as those
>two states hit goto newline before that point.

I don't agree on this.

Here what we are changing is the UAPI behaviour, which should not be
constrained by what smcss did.

And SMC_INIT can coexist with fallback. For example:

smc_sendmsg() calls smc_switch_to_fallback(SMC_CLC_DECL_OPTUNSUPP) under
MSG_FASTOPEN while sk_state is SMC_INIT and never changes it afterwards --
fastopen does not call connect(), so the state never goes to SMC_ACTIVE.

>
>So the correct placement is after smc_diag_msg_attrs_fill(), which fills
>diag_mode, diag_uid, and diag_inode, but before SMC_DIAG_FALLBACK:
>
>    r->diag_state = sk->sk_state;
>    if (smc->use_fallback)
>        r->diag_mode = SMC_DIAG_MODE_FALLBACK_TCP;
>    else if (...)
>        ...
>    if (smc_diag_msg_attrs_fill(sk, skb, r, user_ns))
>        goto errout;
>
>+   if (r->diag_state == SMC_INIT || r->diag_state == SMC_CLOSED)
>+       goto out;
>
>    fallback.reason = smc->fallback_rsn;
>    ...
>
>+out:
>    nlmsg_end(skb, nlh);
>    return 0;
>
>At this position all fields smcss reads for these two states are already
>filled correctly, including diag_mode for --smcr/--smcd filtering.  The
>CONNINFO, LGRINFO, and DMBINFO blocks — which contain the unsafe lgr/lnk
>dereferences — are never reached.
>
>What is your opinion on this?


I didn't have any better ideas to fix this issue with minimal changes.
So I agree that we can go with this the fix.



>> 
>> Why don't you use conn->free instead of sk_state ? It is set unconditionally
>> at the top of smc_conn_free() before any lgr/lnk reference is dropped.
>> 
>> 
>> So maybe something like this ? Please double check.
>> 
>> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
>> index bf0beaa23bdb6..a19f16881ae7e 100644
>> --- a/net/smc/smc_diag.c
>> +++ b/net/smc/smc_diag.c
>> @@ -103,6 +103,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 (smc->conn.freed)
>> +               goto out;
>> +
>
>Regarding conn->freed: it does not fully close the race. The check and
>the subsequent lgr/lnk dereferences in the LGRINFO/DMBINFO blocks are
>not atomic. The diag reader can pass the conn->freed == 0 check, then
>smc_conn_free() runs concurrently and calls smc_lgr_put() which may drop
>the lgr refcount to zero and free lgr, and then the reader resumes and
>dereferences conn->lgr — a UAF.

Your are right.


>
>>         if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
>>             smc->conn.alert_token_local) {
>>                 struct smc_connection *conn = &smc->conn;
>> @@ -185,6 +188,7 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
>>                         goto errout;
>>         }
>> 
>> +out:
>>         nlmsg_end(skb, nlh);
>>         return 0;
>> 
>> BTW, as we talked in the previous threads, the teardown path is messy and lots
>> of hidden holes, so I think we will finnally refine those.
>> As for now, if this works, I think we can go with it.
>
>I agree, as of now I am trying to fix the UAF bug with minimal changes.

Great!

Best regards,
Dust

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

[Mahanta Jambigi]

On 21/09/26 3:26 pm, Dust Li wrote:
> On 2026-09-18 12:57:13, Mahanta Jambigi wrote:
>>
>>
>> On 17/09/26 9:23 pm, Dust Li wrote:
>>> On 2026-09-17 13:15:23, Mahanta Jambigi wrote:
>>>>
>>>>
>>>> On 16/09/26 8:54 pm, Dust Li wrote:
>>>>> On 2026-09-16 14:04:42, Mahanta Jambigi wrote:
>>>>>>
>>>>>>
>>>>>> On 15/09/26 8:43 pm, Dust Li wrote:
>>>>>>> On 2026-09-11 11:09:06, Mahanta Jambigi wrote:
>>>>>>>> The diag dump walks the socket hash table under a read_lock and
>>>>>>>> dereferences conn->lgr and conn->lnk.  Two terminal teardown paths
>>>>>>>> drop those references via smc_conn_free() while the socket is still
>>>>>>>> hashed:
>>>>>>>>
>>>>>>>> - smc_conn_kill() -> smc_close_active_abort() -> smc_conn_free() -
>>>>>>>> smc_close_passive_work() -> smc_conn_free()
>>>>>>>>
>>>>>>>> This allows the diag reader to dereference a freed lgr or lnk.
>>>>>>>>
>>>>>>>> Fix it by unhashing the socket before smc_conn_free() is called at
>>>>>>>> each of these two sites.  Any socket visible to the diag reader
>>>>>>>> under the hash read_lock then has valid conn->lgr and conn->lnk
>>>>>>>> pointers.
>>>>>>>>
>>>>>>>> Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets") Fixes:
>>>>>>>> 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R
>>>>>>>> connections") Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
>>>>>>>> --- Changes in v4: - dropped smc_conn_unhash() wrapper, conn-
>>>>>>>>> unhashed flag, and all changes to af_smc.c, smc.h, smc_core.c and
>>>>>>>> smc_core.h; smc_unhash_sk() is already idempotent via sk_hashed(),
>>>>>>>> so direct calls at the two teardown sites in smc_close.c are
>>>>>>>> sufficient - dropped the __smc_release() hunk: it needs no change
>>>>>>>> since the subsequent unhash there is already a safe no-op - fixed
>>>>>>>> premature-unhash issue present in v3: smc_conn_free() must not unhash
>>>>>>>> because smc_conn_abort() calls it before smc_switch_to_fallback() in
>>>>>>>> both smc_listen_decline() and smc_connect_rdma() error paths; unhashing
>>>>>>>> there would make live fallback sockets invisible to smcss -
>>>>>>>> likewise, the ISM/RDMA retry loops (smc_find_ism_v2_device_serv(), 
>>>>>>>> smc_find_rdma_v2_device_serv()) call smc_conn_abort() on a failed attempt
>>>>>>>> and then smc_conn_create() on the next device; unhashing in 
>>>>>>>> smc_conn_free() would permanently hide the established connection
>>>>>>>> from smc_diag since smc_conn_create() does not re-hash the socket
>>>>>>>
>>>>>>> Hi Mahanta,
>>>>>>>
>>>>>>> This version looks clean. And you explained why we can't call unhash
>>>>>>> in smc_conn_abort() well. But smc_conn_abort() still calls
>>>>>>> smc_conn_free(), when the smc_sk is still hashed, is there still a
>>>>>>> race window with dump ?
>>>>>>
>>>>>> Hi Dust,
>>>>>>
>>>>>> Thank you for catching this corner case!
>>>>>>
>>>>>> During early handshake setup (when sk_state is *SMC_INIT*),
>>>>>> smc_conn_abort() can be called on connection failure/fallback and
>>>>>> invokes smc_conn_free() while the socket remains hashed, leaving a
>>>>>> window where a concurrent diag dump could evaluate smc_conn_lgr_valid()
>>>>>> and dereference conn->lgr / conn->lnk.
>>>>>>
>>>>>> Since sockets in *SMC_INIT* are in a transient embryonic handshake phase
>>>>>> and userspace (smcss) *skips displaying link-group, DMB, and connection
>>>>>> details for INIT state sockets anyway*, we could have __smc_diag_dump()
>>>>>> skip inspecting connection/link-group extensions when r->diag_state ==
>>>>>> SMC_INIT:
>>>>>>
>>>>>> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
>>>>>> --- a/net/smc/smc_diag.c
>>>>>> +++ b/net/smc/smc_diag.c
>>>>>> @@ -90,6 +90,9 @@ static int __smc_diag_dump(struct sock *sk, struct
>>>>>> sk_buff *skb,
>>>>>> 	r->diag_state = sk->sk_state;
>>>>>> +	if (r->diag_state == SMC_INIT)
>>>>>> +		return 0;
>>>>>> +
>>>>>> 	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)
>>>>>>
>>>>>> Together with unhashing before smc_conn_free() in smc_close.c for
>>>>>> established and closing sockets, this cleanly closes the race window
>>>>>> across all socket states without touching the hash table mechanics
>>>>>> during fallback/retry.
>>>>>>
>>>>>> Does this approach look good to you? If you agree, I will prepare and
>>>>>> submit v5 with this change. Please let me know if you have any other
>>>>>> suggestions or alternative approaches, and I'll be happy to look into them.
>>>>>
>>>>> What about this path ?
>>>>>
>>>>> smc_listen_decline() -> smc_listen_out_err(), where smc_conn_abort() has
>>>>> already run, and the sock state is then changed from SMC_INIT to SMC_CLOSED.
>>>>
>>>> Hi Dust,
>>>>
>>>> Good catch on the smc_listen_out_err() path as well!
>>>>
>>>> In smc_listen_decline() -> smc_listen_out_err(), the server socket fails
>>>> handshake and is transitioned to SMC_CLOSED while remaining in the hash
>>>> table until smc_accept_dequeue() runs.
>>>>
>>>> Since userspace (smcss) skips displaying all connection, link-group, and
>>>> DMB details for both SMC_INIT and SMC_CLOSED sockets (smcss.c:186 and
>>>> smcss.c:199), we can update __smc_diag_dump() to check for both states:
>>>>
>>>> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
>>>> --- a/net/smc/smc_diag.c
>>>> +++ b/net/smc/smc_diag.c
>>>> @@ -90,6 +90,9 @@ static int __smc_diag_dump(struct sock *sk, struct
>>>> sk_buff *skb,
>>>> 	r->diag_state = sk->sk_state;
>>>> +	if (r->diag_state == SMC_INIT || r->diag_state == SMC_CLOSED)
>>>> +		return 0;
>>>> +
>>>> 	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)
>>>>
>>>
>>> I checked the code again, and I'm afraid adding SMC_CLOSED is still not right.
>>>
>>> smc_listen_decline(): smc_switch_to_fallback(), smc_clc_send_decline() then
>>> fails, and smc_listen_out_err() sets the socket to SMC_CLOSED. The socket
>>> stays on the accept queue, hashed, until accept() reaches smc_accept_dequeue().
>>>
>>> But the early return sits after nlmsg_put() but before diag_mode,
>>> smc_diag_msg_attrs_fill() and the SMC_DIAG_FALLBACK attribute are filled.
>>> The record is still emitted, but diag_mode is left at 0 -- which is
>>> SMC_DIAG_MODE_SMCR, not "unknown" -- and the fallback reason attribute is gone.
>>
>> Regarding your concern about wrong output when the early return fires
>> before diag_mode and SMC_DIAG_FALLBACK are filled: you are right that
>> diag_mode must be filled before the guard. *smcss.c* reads diag_mode at
>> lines 177 and 179 for --smcr/--smcd filtering, which happens before the
>> SMC_INIT and SMC_CLOSED state checks. If diag_mode is left as 0 for
>> a failed-fallback SMC_CLOSED socket, --smcr would incorrectly include it
>> and --smcd would incorrectly exclude it.
>>
>> *SMC_DIAG_FALLBACK* however is *not* needed before the guard — smcss
>> only reads it inside the diag_mode == SMC_DIAG_MODE_FALLBACK_TCP branch
>> which is never reached for *SMC_INIT* or *SMC_CLOSED* sockets, as those
>> two states hit goto newline before that point.
> 
> I don't agree on this.
> 
> Here what we are changing is the UAPI behaviour, which should not be
> constrained by what smcss did.
> 
> And SMC_INIT can coexist with fallback. For example:
> 
> smc_sendmsg() calls smc_switch_to_fallback(SMC_CLC_DECL_OPTUNSUPP) under
> MSG_FASTOPEN while sk_state is SMC_INIT and never changes it afterwards --
> fastopen does not call connect(), so the state never goes to SMC_ACTIVE.

In that case how about moving the check further down, like below? This
way I am not changing the UAPI behaviour and SMC_INIT can co-exist with
fallback.

smc_sendmsg() calls smc_switch_to_fallback(SMC_CLC_DECL_OPTUNSUPP) under
MSG_FASTOPEN while sk_state is SMC_INIT and it never transitions further
— connect() is not called in that path, so the fallback reason is real
diagnostic information that should be reported. The guard after the
SMC_DIAG_FALLBACK nla_put preserves that.

--- a/net/smc/smc_diag.c
+++ b/net/smc/smc_diag.c
@@ -103,6 +103,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 (sk->sk_state == SMC_INIT || sk->sk_state == SMC_CLOSED)
+		goto out;
+
 	if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
 	    smc->conn.alert_token_local) {
@@ -185,5 +188,6 @@ static int __smc_diag_dump(struct sock *sk, struct
sk_buff *skb,
 		goto errout;
 	}

+out:
 	nlmsg_end(skb, nlh);
 	return 0;

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

[Dust Li]

On 2026-09-22 18:57:24, Mahanta Jambigi wrote:
>
>
>On 21/09/26 3:26 pm, Dust Li wrote:
>> On 2026-09-18 12:57:13, Mahanta Jambigi wrote:
>>>
>>>
>>> On 17/09/26 9:23 pm, Dust Li wrote:
>>>> On 2026-09-17 13:15:23, Mahanta Jambigi wrote:
>>>>>
>>>>>
>>>>> On 16/09/26 8:54 pm, Dust Li wrote:
>>>>>> On 2026-09-16 14:04:42, Mahanta Jambigi wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 15/09/26 8:43 pm, Dust Li wrote:
>>>>>>>> On 2026-09-11 11:09:06, Mahanta Jambigi wrote:
>>>>>>>>> The diag dump walks the socket hash table under a read_lock and
>>>>>>>>> dereferences conn->lgr and conn->lnk.  Two terminal teardown paths
>>>>>>>>> drop those references via smc_conn_free() while the socket is still
>>>>>>>>> hashed:
>>>>>>>>>
>>>>>>>>> - smc_conn_kill() -> smc_close_active_abort() -> smc_conn_free() -
>>>>>>>>> smc_close_passive_work() -> smc_conn_free()
>>>>>>>>>
>>>>>>>>> This allows the diag reader to dereference a freed lgr or lnk.
>>>>>>>>>
>>>>>>>>> Fix it by unhashing the socket before smc_conn_free() is called at
>>>>>>>>> each of these two sites.  Any socket visible to the diag reader
>>>>>>>>> under the hash read_lock then has valid conn->lgr and conn->lnk
>>>>>>>>> pointers.
>>>>>>>>>
>>>>>>>>> Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets") Fixes:
>>>>>>>>> 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R
>>>>>>>>> connections") Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
>>>>>>>>> --- Changes in v4: - dropped smc_conn_unhash() wrapper, conn-
>>>>>>>>>> unhashed flag, and all changes to af_smc.c, smc.h, smc_core.c and
>>>>>>>>> smc_core.h; smc_unhash_sk() is already idempotent via sk_hashed(),
>>>>>>>>> so direct calls at the two teardown sites in smc_close.c are
>>>>>>>>> sufficient - dropped the __smc_release() hunk: it needs no change
>>>>>>>>> since the subsequent unhash there is already a safe no-op - fixed
>>>>>>>>> premature-unhash issue present in v3: smc_conn_free() must not unhash
>>>>>>>>> because smc_conn_abort() calls it before smc_switch_to_fallback() in
>>>>>>>>> both smc_listen_decline() and smc_connect_rdma() error paths; unhashing
>>>>>>>>> there would make live fallback sockets invisible to smcss -
>>>>>>>>> likewise, the ISM/RDMA retry loops (smc_find_ism_v2_device_serv(), 
>>>>>>>>> smc_find_rdma_v2_device_serv()) call smc_conn_abort() on a failed attempt
>>>>>>>>> and then smc_conn_create() on the next device; unhashing in 
>>>>>>>>> smc_conn_free() would permanently hide the established connection
>>>>>>>>> from smc_diag since smc_conn_create() does not re-hash the socket
>>>>>>>>
>>>>>>>> Hi Mahanta,
>>>>>>>>
>>>>>>>> This version looks clean. And you explained why we can't call unhash
>>>>>>>> in smc_conn_abort() well. But smc_conn_abort() still calls
>>>>>>>> smc_conn_free(), when the smc_sk is still hashed, is there still a
>>>>>>>> race window with dump ?
>>>>>>>
>>>>>>> Hi Dust,
>>>>>>>
>>>>>>> Thank you for catching this corner case!
>>>>>>>
>>>>>>> During early handshake setup (when sk_state is *SMC_INIT*),
>>>>>>> smc_conn_abort() can be called on connection failure/fallback and
>>>>>>> invokes smc_conn_free() while the socket remains hashed, leaving a
>>>>>>> window where a concurrent diag dump could evaluate smc_conn_lgr_valid()
>>>>>>> and dereference conn->lgr / conn->lnk.
>>>>>>>
>>>>>>> Since sockets in *SMC_INIT* are in a transient embryonic handshake phase
>>>>>>> and userspace (smcss) *skips displaying link-group, DMB, and connection
>>>>>>> details for INIT state sockets anyway*, we could have __smc_diag_dump()
>>>>>>> skip inspecting connection/link-group extensions when r->diag_state ==
>>>>>>> SMC_INIT:
>>>>>>>
>>>>>>> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
>>>>>>> --- a/net/smc/smc_diag.c
>>>>>>> +++ b/net/smc/smc_diag.c
>>>>>>> @@ -90,6 +90,9 @@ static int __smc_diag_dump(struct sock *sk, struct
>>>>>>> sk_buff *skb,
>>>>>>> 	r->diag_state = sk->sk_state;
>>>>>>> +	if (r->diag_state == SMC_INIT)
>>>>>>> +		return 0;
>>>>>>> +
>>>>>>> 	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)
>>>>>>>
>>>>>>> Together with unhashing before smc_conn_free() in smc_close.c for
>>>>>>> established and closing sockets, this cleanly closes the race window
>>>>>>> across all socket states without touching the hash table mechanics
>>>>>>> during fallback/retry.
>>>>>>>
>>>>>>> Does this approach look good to you? If you agree, I will prepare and
>>>>>>> submit v5 with this change. Please let me know if you have any other
>>>>>>> suggestions or alternative approaches, and I'll be happy to look into them.
>>>>>>
>>>>>> What about this path ?
>>>>>>
>>>>>> smc_listen_decline() -> smc_listen_out_err(), where smc_conn_abort() has
>>>>>> already run, and the sock state is then changed from SMC_INIT to SMC_CLOSED.
>>>>>
>>>>> Hi Dust,
>>>>>
>>>>> Good catch on the smc_listen_out_err() path as well!
>>>>>
>>>>> In smc_listen_decline() -> smc_listen_out_err(), the server socket fails
>>>>> handshake and is transitioned to SMC_CLOSED while remaining in the hash
>>>>> table until smc_accept_dequeue() runs.
>>>>>
>>>>> Since userspace (smcss) skips displaying all connection, link-group, and
>>>>> DMB details for both SMC_INIT and SMC_CLOSED sockets (smcss.c:186 and
>>>>> smcss.c:199), we can update __smc_diag_dump() to check for both states:
>>>>>
>>>>> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
>>>>> --- a/net/smc/smc_diag.c
>>>>> +++ b/net/smc/smc_diag.c
>>>>> @@ -90,6 +90,9 @@ static int __smc_diag_dump(struct sock *sk, struct
>>>>> sk_buff *skb,
>>>>> 	r->diag_state = sk->sk_state;
>>>>> +	if (r->diag_state == SMC_INIT || r->diag_state == SMC_CLOSED)
>>>>> +		return 0;
>>>>> +
>>>>> 	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)
>>>>>
>>>>
>>>> I checked the code again, and I'm afraid adding SMC_CLOSED is still not right.
>>>>
>>>> smc_listen_decline(): smc_switch_to_fallback(), smc_clc_send_decline() then
>>>> fails, and smc_listen_out_err() sets the socket to SMC_CLOSED. The socket
>>>> stays on the accept queue, hashed, until accept() reaches smc_accept_dequeue().
>>>>
>>>> But the early return sits after nlmsg_put() but before diag_mode,
>>>> smc_diag_msg_attrs_fill() and the SMC_DIAG_FALLBACK attribute are filled.
>>>> The record is still emitted, but diag_mode is left at 0 -- which is
>>>> SMC_DIAG_MODE_SMCR, not "unknown" -- and the fallback reason attribute is gone.
>>>
>>> Regarding your concern about wrong output when the early return fires
>>> before diag_mode and SMC_DIAG_FALLBACK are filled: you are right that
>>> diag_mode must be filled before the guard. *smcss.c* reads diag_mode at
>>> lines 177 and 179 for --smcr/--smcd filtering, which happens before the
>>> SMC_INIT and SMC_CLOSED state checks. If diag_mode is left as 0 for
>>> a failed-fallback SMC_CLOSED socket, --smcr would incorrectly include it
>>> and --smcd would incorrectly exclude it.
>>>
>>> *SMC_DIAG_FALLBACK* however is *not* needed before the guard — smcss
>>> only reads it inside the diag_mode == SMC_DIAG_MODE_FALLBACK_TCP branch
>>> which is never reached for *SMC_INIT* or *SMC_CLOSED* sockets, as those
>>> two states hit goto newline before that point.
>> 
>> I don't agree on this.
>> 
>> Here what we are changing is the UAPI behaviour, which should not be
>> constrained by what smcss did.
>> 
>> And SMC_INIT can coexist with fallback. For example:
>> 
>> smc_sendmsg() calls smc_switch_to_fallback(SMC_CLC_DECL_OPTUNSUPP) under
>> MSG_FASTOPEN while sk_state is SMC_INIT and never changes it afterwards --
>> fastopen does not call connect(), so the state never goes to SMC_ACTIVE.
>
>In that case how about moving the check further down, like below? This
>way I am not changing the UAPI behaviour and SMC_INIT can co-exist with
>fallback.

Yes, I think we should do that.

Best regards,
Dust

>
>smc_sendmsg() calls smc_switch_to_fallback(SMC_CLC_DECL_OPTUNSUPP) under
>MSG_FASTOPEN while sk_state is SMC_INIT and it never transitions further
>— connect() is not called in that path, so the fallback reason is real
>diagnostic information that should be reported. The guard after the
>SMC_DIAG_FALLBACK nla_put preserves that.
>
>--- a/net/smc/smc_diag.c
>+++ b/net/smc/smc_diag.c
>@@ -103,6 +103,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 (sk->sk_state == SMC_INIT || sk->sk_state == SMC_CLOSED)
>+		goto out;
>+
> 	if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
> 	    smc->conn.alert_token_local) {
>@@ -185,5 +188,6 @@ static int __smc_diag_dump(struct sock *sk, struct
>sk_buff *skb,
> 		goto errout;
> 	}
>
>+out:
> 	nlmsg_end(skb, nlh);
> 	return 0;