[PATCH net v2 0/2] net/smc: do not reuse the DMB of a nocopy device

[PATCH net v2 0/2] net/smc: do not reuse the DMB of a nocopy device

[Bryam Vargas via B4 Relay]

With DMB nocopy (the dibs loopback device) a peer writes into the other
end's RMB through its own attachment. smc_buf_unuse() puts that DMB back
in the link group's pool with its token still registered, IOW the old
peer can still send its CDCs to it. The next connection that takes the
DMB gets those CDCs, and a peer that attaches after the other end gave
up lands in another connection's DMB. 2/2 stops pooling DMBs on nocopy
devices. This is the reused-DMB case that the v6 cover of the CDC
receive ordering series [1] lists as not fixed; Sashiko flagged it on
v5 2/2 [2].

2/2 depends on 1/2, a dibs loopback fix: when the owner unregisters a
DMB that a peer still holds, its dmb_clientid_arr[] entry is never
reset, and rmmod smc then warns in dibs_unregister_client() and leaves
smc registered as a dibs client. On net that needs a link group freed
while a peer still holds one of its DMBs; with 2/2 alone, each of the 20
connections of the rmmod test below left an entry set.

Tested in an x86 KVM guest on a v7.4-rc2 KASAN kernel, with net/smc and
drivers/dibs from the base below built as modules, with and without the
series. The modules had test counters, and some tests set a test-only
knob (a hold, a shorter link group free delay, an injected failure or a
forced terminate), named below. Each test has a control that passes on
both, and each part of the series has a test that fails without it. No
KASAN report or WARN with the series.

- A DMB reused while the old peer is still attached, 8 rounds: 4 with
  the new server end held before its attach by a knob, 4 with no knob,
  the new client's connect() at SCHED_IDLE on a busy CPU instead. On net
  the old peer's CDCs reach the new connection every time: it reads EOF
  while its own client is open, and its send space goes past its buffer
  size. With the series it gets a new DMB and token and sees none of
  them, and with 1/2 the old peer's CDCs raise no interrupt.
- Late attach, after an earlier connection set up the link groups: the
  client held 8 s by a knob after the CLC accept, past the server's 6 s
  wait for the confirm. On net it attaches to a DMB another connection
  already took, and after close() it stays orphaned in PEERCLOSEWAIT1,
  keeping smc in use. With the series the attach fails with -EINVAL and
  the client falls back to TCP.
- rmmod smc after 20 sequential connections, the 600 s link group free
  delay cut to 3 s by a knob: with 2/2 alone it warns, with 118 entries
  still set after the other tests of the same boot; with the series it
  unloads clean, and smc loads and connects again. On net, after the
  same tests, rmmod smc fails with "Module smc is in use" (the orphan
  above). In a separate boot, with the same knob, net warns too when a
  link group is freed while a peer still holds one of its DMBs; 1/2
  alone fixes that.
- 2/2 without the conn->rmb_desc clear: KASAN slab-use-after-free in
  smc_ism_unset_conn() when the client retries the handshake on the same
  socket. It took two injected failures, the CLC confirm send of the
  first attempt and the RMB allocation of the second. Clean with the
  series.
- The same retry with the first attempt's link group terminated by a
  knob before that attempt is aborted: smc_conn_free() then skips
  smc_buf_unuse(), and with v1's 2/2 the pointer stays set. Five KASAN
  reports, the first again in smc_ism_unset_conn(), then a GPF in
  smc_conn_free(), and close() hangs. Clean with v2.
- A connection closed while its link group is being terminated (a knob
  terminates the link group and holds its terminate work before the
  kill loop): v2 unregisters its DMB when the connection is freed, v1
  when the link group is freed. rmmod smc is clean after both.
- 6000 connect/close cycles, more than the device's 5000 DMBs, with 120
  peers kept open and the same 3 s delay: one DMB per connection end, no
  fallback and no -ENOSPC.

Cost, one run of 2000 connect/echo/close cycles per RMB size on the same
KASAN kernel, so relative only: 3.6% fewer cycles/s with 64 KiB RMBs
(4251 to 4098), 2.9% fewer with 1 MiB (3328 to 3232); handshake median
107 to 113 us and 112 to 167 us; a 1 GiB stream with 64 KiB RMBs 4759 to
4750 MB/s. Each connection end now registers its own DMB instead of
taking a pooled one. Not measured: under memory pressure that allocation
can fail, and the connection then gets a smaller DMB or falls back to
TCP, as a first connection does today.

ISM pools its DMBs the same way and an old ISM peer also writes its CDCs
by token, so AFAICT the reuse applies there too. This series does not
change ISM, and I have not tested it.

With the series a loopback peer that is still attached is not told the
DMB is gone, as on net; its writes land in the old DMB, which nobody
reads and which is freed when that peer closes.

A client that retries the handshake on the same socket keeps
conn->rmb_desc and conn->sndbuf_desc from its first attempt; v6 2/3 of
[1] clears them. 2/2 clears the pointer to the DMB it frees, now also
when smc_conn_free() finds the link group off its list, so its own free
does not depend on that series. A connection that the termination
already killed keeps its DMB until the link group is freed, as on net;
by inspection that socket cannot start a new handshake.

Test patch and logs on request.

v2:
 - 2/2: also unregister the DMB and clear conn->rmb_desc when
   smc_conn_free() finds the link group off its list and skips
   smc_buf_unuse(); v1 left the pointer set there, which Sashiko
   found [3].
 - 1/2: unchanged.
v1: https://lore.kernel.org/all/20261113-b4-disp-19244662-v1-0-39b256b70ca7@proton.me/

[1] https://lore.kernel.org/all/20261112-b4-disp-1c8d3a53-v6-0-36a38ae35021@proton.me/
[2] https://lore.kernel.org/all/179241208501.3135.14193534440423847161@kernel.org/
[3] https://lore.kernel.org/all/sashiko-outbox-214598@kernel.org/

---
Bryam Vargas (2):
      dibs: loopback: reset the client id when the owner unregisters a DMB
      net/smc: do not reuse the DMB of a nocopy device

 drivers/dibs/dibs_loopback.c | 14 ++++++++------
 net/smc/smc_core.c           | 28 ++++++++++++++++++++++++++--
 2 files changed, 34 insertions(+), 8 deletions(-)
---
base-commit: ead6fa9a0ab194f02a2f36d634b193e9c5f9a1c6
change-id: 20261114-b4-disp-4e6374c2-3d032eecd15f

Best regards,
--  
Bryam Vargas <hexlabsecurity@proton.me>

[PATCH net v2 1/2] dibs: loopback: reset the client id when the owner unregisters a DMB

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

dibs_lo_unregister_dmb() resets the DMB's dmb_clientid_arr[] entry only
on the last put. If an attacher still holds the DMB, as when an SMC link
group is freed while a peer is still attached, the entry stays set after
the attacher's dibs_lo_detach_dmb() frees the DMB.
dibs_unregister_client() then warns at rmmod smc and returns -EBUSY,
which smc_ism_exit() ignores: smc stays in clients[] and dibs->subs[],
and a later dibs_dev_add() or dibs_dev_del() (rmmod dibs, ISM hotplug)
calls into the unloaded module.

Reset the entry when the owner unregisters, before its put, as
ism_unregister_dmb() does. The index stays allocated until the last put,
and the attacher's writes no longer raise an interrupt for a client that
gave the DMB up.

Fixes: cc21191b584c ("dibs: Move data path to dibs layer")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 drivers/dibs/dibs_loopback.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/dibs/dibs_loopback.c b/drivers/dibs/dibs_loopback.c
index 44a2e74c2efc..566e61a013d0 100644
--- a/drivers/dibs/dibs_loopback.c
+++ b/drivers/dibs/dibs_loopback.c
@@ -138,6 +138,13 @@ static int dibs_lo_unregister_dmb(struct dibs_dev *dibs, struct dibs_dmb *dmb)
 
 	ldev = dibs->drv_priv;
 
+	/* Reset before the owner's put: after it, an attacher's detach can
+	 * free the index for a new DMB.
+	 */
+	spin_lock_irqsave(&dibs->lock, flags);
+	dibs->dmb_clientid_arr[dmb->idx] = NO_DIBS_CLIENT;
+	spin_unlock_irqrestore(&dibs->lock, flags);
+
 	/* find dmb from hash table */
 	write_lock_bh(&ldev->dmb_ht_lock);
 	hash_for_each_possible(ldev->dmb_ht, tmp_node, list, dmb->dmb_tok) {
@@ -155,13 +162,8 @@ static int dibs_lo_unregister_dmb(struct dibs_dev *dibs, struct dibs_dmb *dmb)
 		hash_del(&dmb_node->list);
 	write_unlock_bh(&ldev->dmb_ht_lock);
 
-	if (last) {
-		spin_lock_irqsave(&dibs->lock, flags);
-		dibs->dmb_clientid_arr[dmb_node->sba_idx] = NO_DIBS_CLIENT;
-		spin_unlock_irqrestore(&dibs->lock, flags);
-
+	if (last)
 		dibs_lo_free_dmb(ldev, dmb_node);
-	}
 	return 0;
 }
 

-- 
2.56.0

Re: [PATCH net v2 1/2] dibs: loopback: reset the client id when the owner unregisters a DMB

[sashiko-bot]

> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> dibs_lo_unregister_dmb() resets the DMB's dmb_clientid_arr[] entry only
> on the last put. If an attacher still holds the DMB, as when an SMC link
> group is freed while a peer is still attached, the entry stays set after
> the attacher's dibs_lo_detach_dmb() frees the DMB.
> dibs_unregister_client() then warns at rmmod smc and returns -EBUSY,
> which smc_ism_exit() ignores: smc stays in clients[] and dibs->subs[],
> and a later dibs_dev_add() or dibs_dev_del() (rmmod dibs, ISM hotplug)
> calls into the unloaded module.
> 
> Reset the entry when the owner unregisters, before its put, as
> ism_unregister_dmb() does. The index stays allocated until the last put,
> and the attacher's writes no longer raise an interrupt for a client that
> gave the DMB up.
> 
> Fixes: cc21191b584c ("dibs: Move data path to dibs layer")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-b4-disp-4e6374c2-v2-0-3b7050bddef7@proton.me?part=1

[PATCH net v2 2/2] net/smc: do not reuse the DMB of a nocopy device

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

With DMB nocopy (the dibs loopback device) the peer writes into the RMB
through its own attachment, and it can attach late. smc_buf_unuse() puts
the DMB back in the link group's pool with its token still registered
while the old peer can still write to it: the next connection that takes
it gets the old peer's CDCs, and a peer that attaches after the other
end gave up lands in another connection's DMB.

Unregister a nocopy DMB when the connection ends instead of pooling it,
including when smc_conn_free() finds the link group off its list and
skips smc_buf_unuse(). The next connection gets a new DMB and token, a
peer still attached keeps the old DMB until it detaches, and a later
attach fails. Clear conn->rmb_desc, which a retried connect() would
otherwise use after the free. Devices without nocopy keep their pool.

Fixes: ae2be35cbed2 ("net/smc: {at|de}tach sndbuf to peer DMB if supported")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/smc/smc_core.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index a3164201c142..6a673ce74937 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1243,6 +1243,22 @@ static void smcd_buf_detach(struct smc_connection *conn)
 	kfree(buf_desc);
 }
 
+/* With DMB nocopy the peer writes into the RMB through its own
+ * attachment, which can outlive this connection: unregister the DMB
+ * instead of pooling it, so the next connection gets a new one.
+ */
+static void smcd_rmb_drop(struct smc_connection *conn,
+			  struct smc_link_group *lgr)
+{
+	struct smc_buf_desc *rmb_desc = conn->rmb_desc;
+
+	down_write(&lgr->rmbs_lock);
+	smc_lgr_buf_list_del(lgr, true, rmb_desc);
+	up_write(&lgr->rmbs_lock);
+	WRITE_ONCE(conn->rmb_desc, NULL);
+	smc_buf_free(lgr, true, rmb_desc);
+}
+
 static void smc_buf_unuse(struct smc_connection *conn,
 			  struct smc_link_group *lgr)
 {
@@ -1266,8 +1282,13 @@ static void smc_buf_unuse(struct smc_connection *conn,
 			smcr_buf_unuse(conn->rmb_desc, true, lgr);
 		} else {
 			bufsize += sizeof(struct smcd_cdc_msg);
-			memzero_explicit(conn->rmb_desc->cpu_addr, bufsize);
-			WRITE_ONCE(conn->rmb_desc->used, 0);
+			if (smc_ism_support_dmb_nocopy(lgr->smcd)) {
+				smcd_rmb_drop(conn, lgr);
+			} else {
+				memzero_explicit(conn->rmb_desc->cpu_addr,
+						 bufsize);
+				WRITE_ONCE(conn->rmb_desc->used, 0);
+			}
 		}
 		SMC_STAT_RMB_SIZE(smc, is_smcd, true, false, bufsize);
 	}
@@ -1306,6 +1327,9 @@ void smc_conn_free(struct smc_connection *conn)
 	 */
 	if (!list_empty(&lgr->list))
 		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
+	else if (lgr->is_smcd && conn->rmb_desc &&
+		 smc_ism_support_dmb_nocopy(lgr->smcd))
+		smcd_rmb_drop(conn, lgr);
 	smc_lgr_unregister_conn(conn);
 
 	if (!lgr->conns_num)

-- 
2.56.0

Re: [PATCH net v2 2/2] net/smc: do not reuse the DMB of a nocopy device

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] net/smc: smc_conn_free() skips smcd_rmb_drop() after
  smc_conn_kill()
--

commit 0276e117c19ed3d088959046a595c0d87ce90a1b
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/smc: do not reuse the DMB of a nocopy device

This commit unregisters and frees the DMB of a nocopy SMC-D device when a
connection ends instead of returning it to the link group's buffer pool.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index a3164201c1420..6a673ce74937e 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c

[ ... ]

> @@ -1306,6 +1327,9 @@ void smc_conn_free(struct smc_connection *conn)
>  	 */
>  	if (!list_empty(&lgr->list))
>  		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
> +	else if (lgr->is_smcd && conn->rmb_desc &&
> +		 smc_ism_support_dmb_nocopy(lgr->smcd))
> +		smcd_rmb_drop(conn, lgr);
>  	smc_lgr_unregister_conn(conn);
>  
>  	if (!lgr->conns_num)

[Severity: Medium]
Does smc_conn_free() skip smcd_rmb_drop() when the connection was already
killed by smc_conn_kill()?

When __smc_lgr_terminate() calls smc_conn_kill(), it detaches sndbuf_desc
and calls smc_lgr_unregister_conn(conn) (clearing conn->alert_token_local)
before smc_conn_free() runs:

net/smc/smc_core.c:smc_conn_kill() {
	...
	if (conn->lgr->is_smcd) {
		...
		if (smc_ism_support_dmb_nocopy(conn->lgr->smcd)) {
			smc->sk.sk_prot->unhash(&smc->sk);
			smcd_buf_detach(conn);
		}
	} else {
		smc_cdc_wait_pend_tx_wr(conn);
	}
	smc_lgr_unregister_conn(conn);
	smc_close_active_abort(smc);
}

When smc_conn_free() then runs (either from smc_close_active_abort() or
later from __smc_release()), smc_conn_lgr_valid(conn) is false and it
jumps to lgr_put before reaching smcd_rmb_drop().  While any other killed
socket in the link group remains open and holds a reference on lgr, the
closed connection's nocopy DMB stays registered in the device, allowing a
late peer attach to succeed on the dead connection's DMB, and
conn->rmb_desc is left non-NULL.

Running the smcd_rmb_drop() check at the lgr_put label before
smc_lgr_put(lgr) would cover both the off-list case and connections
unregistered by smc_conn_kill().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-b4-disp-4e6374c2-v2-0-3b7050bddef7@proton.me?part=2