mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH net v5 0/2] net/smc: order the CDC receive path against buffer publication
@ 2026-10-15 11:53 Bryam Vargas via B4 Relay
  2026-10-15 11:53 ` [PATCH net v5 1/2] net/smc: order the CDC receive path against RMB publication Bryam Vargas via B4 Relay
  2026-10-15 11:53 ` [PATCH net v5 2/2] net/smc: skip the DMB nocopy accounting until the send buffer is set Bryam Vargas via B4 Relay
  0 siblings, 2 replies; 5+ messages in thread
From: Bryam Vargas via B4 Relay @ 2026-10-15 11:53 UTC (permalink / raw)
  To: Jakub Kicinski, Dust Li, Sidraya Jayagond, David S. Miller,
	Paolo Abeni, D. Wythe, Mahanta Jambigi, Eric Dumazet
  Cc: Simon Horman, Wenjia Zhang, Ursula Braun, Hans Wippel,
	linux-kernel, netdev, Tony Lu, linux-rdma, Wen Gu, linux-s390

A CDC can reach a connection before its RMB, or its SMC-D nocopy send
buffer, is set, and smc_cdc_msg_recv_action() then derefs a NULL
pointer in softirq. For the SMC-R RMB (1/2) the CDC comes from a peer
in the link group that predicts the next token. For the nocopy sndbuf
(2/2, SMC-D loopback only) it comes from the old peer of a reused DMB.
Each patch sets its pointer last with a release store, and the rx path
drops the CDC (1/2) or skips the nocopy accounting (2/2) while the
pointer is unset.

1/2 goes back to 69cb7dc0218b (v4.18), 2/2 to cc0ab806fc52 (v6.10).
2/2 relies on b395dd319cea ("net/smc: do not dereference an unset send
buffer on the SMC-D teardown path"), already in 6.12.y, 6.18.y and
7.2.y.

The old peer can also close the new connection or, once the sndbuf is
attached, push the new connection's sndbuf_space past the sndbuf size,
with or without this series; 3/4 of my v7 series [1] bounds the send()
copy, not sndbuf_space. FWIW smc_conn_free() releasing the RMB before
the connection leaves the token tree [2] is pre-existing too.

1/2 conflicts in __smc_buf_create() with v7 2/4: keep v7's rmb_len
assignment at the top of the is_rmb block, before the release store,
and the tree is the same in either merge order.

v7 1/4 adds an rmb_desc->len load in smcr_cdc_msg_to_host() that any CDC
in 1/2's window reaches (by inspection), as Sashiko flagged [3]. In C,
net already loads rmb_desc->len later in smc_cdc_msg_recv_action(), as I
told Paolo [4], but in my x86 and s390x builds that load only runs when
the cursor wraps differ. 1/2 drops the CDC before both loads.

Tested with KASAN on x86 (v7.3-rc6, whose net/smc differs from net only
by 64a6b36b0e18) and on s390x under QEMU (net). Unpatched, two cases
with the cursor wraps apart reach their NULL loads: a CDC forged with
the next token (1/2, over rxe), and the old peer's consumer update to
a reused DMB (2/2). In the second, nothing is forged, but the old peer
times its read off a test-only counter and the new connect() is starved
of CPU. KASAN reports a null-ptr-deref on x86; on s390x, with the
default lowcore, each load reads page 0 and nothing faults. With the
series, and test-only delays holding the windows open, test-only probes
see the CDC dropped and the update skip the accounting with the cursor
wraps apart. With either NULL check removed they see the NULL load
again, and with either store moved back they see a CDC handled before
the fields that go with the pointer are set. On top of the v7 series and
two other net fixes [5] [6], the patched runs read the same. The
ordering was also checked against LKMM. Test patch and logs on request.

v5:
 - Split in two, one patch per Fixes: tag, and rebased on net, which
   now has the SMC-D teardown fixes 36cdf5d48ca1 and b395dd319cea.
 - Dropped the sndbuf_desc check on the consumer trigger, unneeded
   after b395dd319cea, and the tasklet's rmb_desc acquire, as
   smcd->lock orders that load (by inspection). The sndbuf_desc
   acquire moved into the nocopy branch.
 - Kept per-buffer checks: one readiness flag would drop a conforming
   client's first CDC on SMC-D, and registering the connection later
   would reorder the connect and listen paths.
 - The Closes: tag now points at Sashiko's report [3], not at a review
   of v2. Dropped the R-b tags from Dust Li and Sidraya Jayagond, since
   the patch changed.
v4: https://lore.kernel.org/all/20260728-b4-disp-52ee4e7d-v4-1-0dda94b0f397@proton.me/
v3: https://lore.kernel.org/all/20260716-b4-disp-aa52955a-v3-1-03a4411a7549@proton.me/
v2: https://lore.kernel.org/all/20260714-b4-disp-835288a6-v2-1-581555ef2145@proton.me/
v1: https://lore.kernel.org/all/20260711-b4-disp-c36a9798-v1-1-340b0c6053fb@proton.me/

[1] https://lore.kernel.org/all/20261013-b4-disp-f345196c-v7-0-ef3dddfbfa68@proton.me/
[2] https://lore.kernel.org/all/20260729165306.BE7C21F000E9@smtp.kernel.org/
[3] https://lore.kernel.org/all/sashiko-outbox-164917@kernel.org/
[4] https://lore.kernel.org/all/20260728172552.69349-1-hexlabsecurity@proton.me/
[5] https://lore.kernel.org/all/20261013-b4-disp-872a3689-v1-1-2a4c63b04761@proton.me/
[6] https://lore.kernel.org/all/20261013-b4-disp-193a2a59-v1-1-ed88ac10a41c@proton.me/

---
Bryam Vargas (2):
      net/smc: order the CDC receive path against RMB publication
      net/smc: skip the DMB nocopy accounting until the send buffer is set

 net/smc/smc_cdc.c  | 16 ++++++++++++++--
 net/smc/smc_core.c | 10 ++++++----
 2 files changed, 20 insertions(+), 6 deletions(-)
---
base-commit: 513444cc62e231a11c9b15ea88de0890da9b2b4f
change-id: 20261015-b4-disp-2a8d85ef-2748bca10e1a

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



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH net v5 1/2] net/smc: order the CDC receive path against RMB publication
  2026-10-15 11:53 [PATCH net v5 0/2] net/smc: order the CDC receive path against buffer publication Bryam Vargas via B4 Relay
@ 2026-10-15 11:53 ` Bryam Vargas via B4 Relay
  2026-10-19 12:14   ` netdev-bot+sashiko
  2026-10-15 11:53 ` [PATCH net v5 2/2] net/smc: skip the DMB nocopy accounting until the send buffer is set Bryam Vargas via B4 Relay
  1 sibling, 1 reply; 5+ messages in thread
From: Bryam Vargas via B4 Relay @ 2026-10-15 11:53 UTC (permalink / raw)
  To: Jakub Kicinski, Dust Li, Sidraya Jayagond, David S. Miller,
	Paolo Abeni, D. Wythe, Mahanta Jambigi, Eric Dumazet
  Cc: Simon Horman, Wenjia Zhang, Ursula Braun, Hans Wippel,
	linux-kernel, netdev, Tony Lu, linux-rdma, Wen Gu, linux-s390

From: Bryam Vargas <hexlabsecurity@proton.me>

smc_conn_create() adds an SMC-R connection to the token tree before
smc_buf_create() sets conn->rmb_desc. Tokens come from one global
counter, so a peer in the link group can predict the next one. A CDC
with that token, arriving in the window, makes
smc_cdc_msg_recv_action() dereference a NULL rmb_desc in softirq.

Set rmb_desc with a release store after the receive fields that go
with it, and drop a CDC in smc_cdc_rx_handler() while rmb_desc is
unset. A conforming peer learns the token only from the CLC accept or
confirm, sent after smc_buf_create(), so nothing it sends is dropped.
The SMC-D receive tasklet needs no check: smc_ism_set_conn() publishes
the connection under smcd->lock, which smcd_handle_irq() also takes.

Fixes: 69cb7dc0218b ("net/smc: add common buffer size in send and receive buffer descriptors")
Closes: https://sashiko.dev/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c@proton.me?part=1
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/smc/smc_cdc.c  | 6 +++++-
 net/smc/smc_core.c | 3 ++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
index 32d6d03df321..7bf902ebd7ab 100644
--- a/net/smc/smc_cdc.c
+++ b/net/smc/smc_cdc.c
@@ -483,7 +483,11 @@ static void smc_cdc_rx_handler(struct ib_wc *wc, void *buf)
 	lgr = smc_get_lgr(link);
 	read_lock_bh(&lgr->conns_lock);
 	conn = smc_lgr_find_conn(ntohl(cdc->token), lgr);
-	if (!conn || conn->out_of_sync) {
+	/* The connection is in the token tree before __smc_buf_create()
+	 * sets its RMB: drop a CDC that comes earlier. rmb_desc is never
+	 * cleared, so the callees can load it plainly.
+	 */
+	if (!conn || conn->out_of_sync || !smp_load_acquire(&conn->rmb_desc)) {
 		read_unlock_bh(&lgr->conns_lock);
 		return;
 	}
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 9974149659c2..26f19a00c159 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -2502,12 +2502,13 @@ static int __smc_buf_create(struct smc_sock *smc, bool is_smcd, bool is_rmb)
 	}
 
 	if (is_rmb) {
-		conn->rmb_desc = buf_desc;
 		conn->rmbe_size_comp = bufsize_comp;
 		smc->sk.sk_rcvbuf = bufsize * 2;
 		atomic_set(&conn->bytes_to_rcv, 0);
 		conn->rmbe_update_limit =
 			smc_rmb_wnd_update_limit(buf_desc->len);
+		/* Set last; pairs with the acquire in smc_cdc_rx_handler() */
+		smp_store_release(&conn->rmb_desc, buf_desc);
 		if (is_smcd)
 			smc_ism_set_conn(conn); /* map RMB/smcd_dev to conn */
 	} else {

-- 
2.56.0



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH net v5 2/2] net/smc: skip the DMB nocopy accounting until the send buffer is set
  2026-10-15 11:53 [PATCH net v5 0/2] net/smc: order the CDC receive path against buffer publication Bryam Vargas via B4 Relay
  2026-10-15 11:53 ` [PATCH net v5 1/2] net/smc: order the CDC receive path against RMB publication Bryam Vargas via B4 Relay
@ 2026-10-15 11:53 ` Bryam Vargas via B4 Relay
  2026-10-19 12:14   ` netdev-bot+sashiko
  1 sibling, 1 reply; 5+ messages in thread
From: Bryam Vargas via B4 Relay @ 2026-10-15 11:53 UTC (permalink / raw)
  To: Jakub Kicinski, Dust Li, Sidraya Jayagond, David S. Miller,
	Paolo Abeni, D. Wythe, Mahanta Jambigi, Eric Dumazet
  Cc: Simon Horman, Wenjia Zhang, Ursula Braun, Hans Wippel,
	linux-kernel, netdev, Tony Lu, linux-rdma, Wen Gu, linux-s390

From: Bryam Vargas <hexlabsecurity@proton.me>

With SMC-D DMB nocopy (the loopback device), smcd_buf_attach() sets
conn->sndbuf_desc after the connection can already receive a CDC. When
a DMB is reused, the peer of its previous connection can still hold
the DMB token, and its next consumer update makes
smc_cdc_msg_recv_action() dereference the NULL sndbuf_desc in the
nocopy accounting.

Set sndbuf_desc with a release store after sndbuf_space, and skip the
nocopy accounting while sndbuf_desc is unset. Before the attach the
current peer has consumed nothing of ours, so no update from it is
lost. The rest of the receive path handles an unset sndbuf_desc since
commit b395dd319cea ("net/smc: do not dereference an unset send buffer
on the SMC-D teardown path").

Fixes: cc0ab806fc52 ("net/smc: adapt cursor update when sndbuf and peer DMB are merged")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/smc/smc_cdc.c  | 10 +++++++++-
 net/smc/smc_core.c |  7 ++++---
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
index 7bf902ebd7ab..d6825f48f568 100644
--- a/net/smc/smc_cdc.c
+++ b/net/smc/smc_cdc.c
@@ -333,6 +333,7 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
 	union smc_host_cursor cons_old, prod_old;
 	struct smc_connection *conn = &smc->conn;
 	int diff_cons, diff_prod, diff_tx;
+	struct smc_buf_desc *sndbuf_desc;
 
 	smc_curs_copy(&prod_old, &conn->local_rx_ctrl.prod, conn);
 	smc_curs_copy(&cons_old, &conn->local_rx_ctrl.cons, conn);
@@ -353,12 +354,19 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
 		 * peer RMB, then update tx_curs_fin and sndbuf_space
 		 * here since peer has already consumed the data.
 		 */
+		sndbuf_desc = NULL;
 		if (conn->lgr->is_smcd &&
 		    smc_ism_support_dmb_nocopy(conn->lgr->smcd)) {
+			/* NULL until smcd_buf_attach(), but the old peer of
+			 * a reused DMB can already signal us.
+			 */
+			sndbuf_desc = smp_load_acquire(&conn->sndbuf_desc);
+		}
+		if (sndbuf_desc) {
 			/* Calculate consumed data and
 			 * increment free send buffer space.
 			 */
-			diff_tx = smc_curs_diff(conn->sndbuf_desc->len,
+			diff_tx = smc_curs_diff(sndbuf_desc->len,
 						&conn->tx_curs_fin,
 						&conn->local_rx_ctrl.cons);
 			/* increase local sndbuf space and fin_curs */
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 26f19a00c159..9322c27145df 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -2603,9 +2603,10 @@ int smcd_buf_attach(struct smc_sock *smc)
 	buf_desc->cpu_addr =
 		(u8 *)buf_desc->cpu_addr + sizeof(struct smcd_cdc_msg);
 	buf_desc->len -= sizeof(struct smcd_cdc_msg);
-	conn->sndbuf_desc = buf_desc;
-	conn->sndbuf_desc->used = 1;
-	atomic_set(&conn->sndbuf_space, conn->sndbuf_desc->len);
+	buf_desc->used = 1;
+	atomic_set(&conn->sndbuf_space, buf_desc->len);
+	/* Set last; pairs with the acquire in smc_cdc_msg_recv_action() */
+	smp_store_release(&conn->sndbuf_desc, buf_desc);
 	return 0;
 
 free:

-- 
2.56.0



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net v5 1/2] net/smc: order the CDC receive path against RMB publication
  2026-10-15 11:53 ` [PATCH net v5 1/2] net/smc: order the CDC receive path against RMB publication Bryam Vargas via B4 Relay
@ 2026-10-19 12:14   ` netdev-bot+sashiko
  0 siblings, 0 replies; 5+ messages in thread
From: netdev-bot+sashiko @ 2026-10-19 12:14 UTC (permalink / raw)
  To: hexlabsecurity
  Cc: kuba, dust.li, sidraya, davem, pabeni, alibuda, mjambigi,
	edumazet, horms, wenjia, ubraun, hwippel, linux-kernel, netdev,
	tonylu, linux-rdma, guwen, linux-s390

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (2) · Medium: 0 · Low: 0

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 new `!smp_load_acquire(&conn->rmb_desc)` check in
  smc_cdc_rx_handler() only tells NULL apart from non-NULL.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the teardown
  ordering leaves the RMB reachable after release.
- [High] This problem wasn't introduced by this patch, but the peer's
  producer cursor is never bounds-checked before it indexes the RMB.

--

net/smc: order the CDC receive path against RMB publication

This patch moves the conn->rmb_desc assignment in __smc_buf_create() after
the other receive fields and makes it a release store. smc_cdc_rx_handler()
now uses an acquire load and drops a CDC whose connection has no rmb_desc
yet. The goal is to stop a CDC with a predicted token from reaching a NULL
rmb_desc in softirq.

Closes: https://sashiko.dev/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c@proton.me?part=1

> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index 32d6d03df3214..7bf902ebd7ab6 100644
> --- a/net/smc/smc_cdc.c
> +++ b/net/smc/smc_cdc.c
> @@ -483,7 +483,11 @@ static void smc_cdc_rx_handler(struct ib_wc *wc, void *buf)
>  	lgr = smc_get_lgr(link);
>  	read_lock_bh(&lgr->conns_lock);
>  	conn = smc_lgr_find_conn(ntohl(cdc->token), lgr);
> -	if (!conn || conn->out_of_sync) {
> +	/* The connection is in the token tree before __smc_buf_create()
> +	 * sets its RMB: drop a CDC that comes earlier. rmb_desc is never
> +	 * cleared, so the callees can load it plainly.
> +	 */
> +	if (!conn || conn->out_of_sync || !smp_load_acquire(&conn->rmb_desc)) {

[Severity: High]
Can a NULL check tell us that the current registration's RMB is ready?
Nothing ever resets conn->rmb_desc to NULL. smc_buf_unuse() releases the
RMB and leaves the pointer in place, and smc_conn_create() does not clear
it either.

On the SMC-R listen path, smc_listen_find_device() can try V2 and then
fall back to V1 on the same conn:

smc_find_rdma_v2_device_serv()
  smc_listen_rdma_init()
    smc_conn_create()        /* token registered */
    smc_buf_create()         /* rmb_desc published */
  smc_listen_rdma_reg()      /* fails */
  smc_conn_abort()
    smc_conn_free()
      smc_buf_unuse()
        smcr_buf_unuse()     /* frees or pools the RMB */

smc_find_rdma_v1_device_serv()
  smc_listen_rdma_init()
    smc_conn_create()        /* new token, rmb_desc still stale */
    smc_buf_create()

smcr_buf_unuse() has two outcomes here. If smcr_link_reg_buf() set
is_reg_err, it frees the descriptor through smc_buf_free(). If the peer
failed CONFIRM_RKEY in smcr_lgr_reg_rmbs(), it zeroes the buffer and
returns it to the pool with used=0.

Suppose a CDC with the new token arrives before __smc_buf_create()
publishes the new RMB. That is the same window the commit message
describes. Wouldn't it pass the new check, since the stale pointer is
non-NULL?

smc_cdc_msg_recv_action() would then read conn->rmb_desc->len, and
smc_cdc_handle_urg_data_arrival() would read conn->rmb_desc->cpu_addr. The
descriptor behind them is either freed or owned by another connection.

The stale pointer also matters if the V1 retry fails in
__smc_buf_create(is_rmb). smc_conn_abort() then runs smc_buf_unuse() again:

	if (conn->rmb_desc) {
		bufsize = conn->rmb_desc->len;
		if (!is_smcd) {
			smcr_buf_unuse(conn->rmb_desc, true, lgr);

Could this double free the descriptor or corrupt the rmbs list?

Would it be better to reset conn->rmb_desc and conn->sndbuf_desc to NULL
when the buffers are released, or before the conn is registered again?
The follow-up patch "net/smc: skip the DMB nocopy accounting until the
send buffer is set" doesn't appear to change this.

[Severity: High]
This isn't a bug introduced by this patch, but there is a related problem
at teardown. The comment says rmb_desc is never cleared. That is true of
the pointer value, but what about the lifetime of the object it points to?

smc_conn_free() releases the buffers before the conn leaves the token
tree:

	if (!list_empty(&lgr->list)) {
		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
		smc_lgr_unregister_conn(conn);
	}

For SMC-R, smcr_buf_unuse() frees the descriptor through smc_buf_free()
when buf_desc->is_reg_err is set, for example after smcr_link_reg_buf()
failed. Otherwise it zeroes the buffer and sets used=0, so
smc_buf_get_slot() can hand it to another connection.

smc_cdc_rx_handler() doesn't check conn->freed, and it drops conns_lock
after sock_hold(). bh_lock_sock() doesn't wait for a lock_sock() owner,
and smc_conn_free() only waits for pending TX WRs.

Can a peer CDC that finds the conn before smc_lgr_unregister_conn() still
run smc_cdc_msg_recv_action() and smc_cdc_handle_urg_data_arrival()? If
so, they would read conn->rmb_desc->len and conn->rmb_desc->cpu_addr from
a freed or reassigned descriptor. The new check would pass here too,
since the stale pointer is non-NULL.

[Severity: High]
This is a pre-existing issue, but the new comment says the callees can
load rmb_desc plainly once it is set. Is the peer's producer cursor ever
checked against the RMB length?

smc_cdc_cursor_to_host() only rejects cursors that move backwards:

	temp.count = ntohl(net.count);
	temp.wrap = ntohs(net.wrap);
	if ((old.wrap > temp.wrap) && temp.wrap)
		return;
	if ((old.wrap == temp.wrap) &&
	    (old.count > temp.count))
		return;
	smc_curs_copy(local, &temp, conn);

smc_curs_diff() also doesn't cap its result at size, even though its
comment says the difference cannot exceed size.

So if prod_flags.urg_data_present is set and prod.count moves forward by
any amount, smc_cdc_msg_recv_action() calls
smc_cdc_handle_urg_data_arrival(), which does:

	base = (char *)conn->rmb_desc->cpu_addr + conn->rx_off;
	if (conn->urg_curs.count)
		conn->urg_rx_byte = *(base + conn->urg_curs.count - 1);

Could a peer-controlled urg_curs.count make this read up to about 4 GiB
past the RMB in softirq? An unmapped address would oops. A byte read from
a mapped address would be stored in conn->urg_rx_byte, and
smc_rx_recv_urg() copies it to user space through memcpy_to_msg() on
recv(MSG_OOB).

The follow-up patch in the series doesn't touch this path either.

>  		read_unlock_bh(&lgr->conns_lock);
>  		return;
>  	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015-b4-disp-2a8d85ef-v5-0-6b795e8263c9%40proton.me

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net v5 2/2] net/smc: skip the DMB nocopy accounting until the send buffer is set
  2026-10-15 11:53 ` [PATCH net v5 2/2] net/smc: skip the DMB nocopy accounting until the send buffer is set Bryam Vargas via B4 Relay
@ 2026-10-19 12:14   ` netdev-bot+sashiko
  0 siblings, 0 replies; 5+ messages in thread
From: netdev-bot+sashiko @ 2026-10-19 12:14 UTC (permalink / raw)
  To: hexlabsecurity
  Cc: kuba, dust.li, sidraya, davem, pabeni, alibuda, mjambigi,
	edumazet, horms, wenjia, ubraun, hwippel, linux-kernel, netdev,
	tonylu, linux-rdma, guwen, linux-s390

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 1 · High: 1 · Medium: 0 · Low: 0

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).

- [Critical] The patch only skips the nocopy accounting while
  conn->sndbuf_desc is NULL.
- [High] A stale CDC from the old peer of a reused DMB that arrives
  before attach used to oops whenever diff_cons != 0.

--

net/smc: skip the DMB nocopy accounting until the send buffer is set

The patch changes smcd_buf_attach() so it publishes conn->sndbuf_desc with
a release store after sndbuf_space has been set. smc_cdc_msg_recv_action()
then skips the SMC-D DMB nocopy send buffer accounting while sndbuf_desc is
still NULL. This avoids a NULL dereference when the old peer of a reused
DMB signals the new connection before the attach.

> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index 7bf902ebd7ab6..d6825f48f5688 100644
> --- a/net/smc/smc_cdc.c
> +++ b/net/smc/smc_cdc.c

[ ... ]

> @@ -353,12 +354,19 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
>  		 * peer RMB, then update tx_curs_fin and sndbuf_space
>  		 * here since peer has already consumed the data.
>  		 */
> +		sndbuf_desc = NULL;
>  		if (conn->lgr->is_smcd &&
>  		    smc_ism_support_dmb_nocopy(conn->lgr->smcd)) {
> +			/* NULL until smcd_buf_attach(), but the old peer of
> +			 * a reused DMB can already signal us.
> +			 */
> +			sndbuf_desc = smp_load_acquire(&conn->sndbuf_desc);
> +		}
> +		if (sndbuf_desc) {

[Severity: High]
With this gate, a stale CDC from the old peer that arrives before
smcd_buf_attach() no longer oopses. It is now processed silently, and only
the sndbuf part is skipped. Doesn't the rest of smc_cdc_msg_recv_action()
still apply the old peer's state to the new connection?

smc_cdc_msg_to_host() runs before the gate. It writes the old peer's
prod/cons cursors and flags into conn->local_rx_ctrl:

    smc_cdc_msg_to_host(&conn->local_rx_ctrl, cdc, conn);

    diff_cons = smc_curs_diff(conn->peer_rmbe_size, &cons_old,
                              &conn->local_rx_ctrl.cons);
    if (diff_cons) {
        ...
        atomic_add(diff_cons, &conn->peer_rmbe_space);

After that, diff_prod is added to bytes_to_rcv and sk_data_ready() is
called. Can recvmsg() on the new socket then return whatever the old peer
wrote into the reused DMB? Stale conn_state_flags could also abort or close
the new connection.

smcd_buf_attach() resets only sndbuf_space and sndbuf_desc. It never
resyncs local_rx_ctrl.cons/prod or peer_rmbe_space, so the current peer's
first consumer update is diffed against the stale cons_old.

For example, say the stale cursor is {wrap=1, count=x>0}. The real peer
then drains a full buffer and sends {wrap=1, count=0}. smc_curs_diff()
returns 0, so the whole if (diff_cons) block is skipped. sndbuf_space is not
credited, tx_curs_fin does not move, and smc_tx_sndbuf_nonfull() is not
called.

smcd_cdc_msg_send() also returns early for nocopy:

    if (smc_ism_support_dmb_nocopy(conn->lgr->smcd))
        ...
        return 0;

Is there any other path that recovers this credit? A sender that has
filled the buffer could end up blocked forever.

If the wraps differ instead, peer_rmbe_space is over-credited past
peer_rmbe_size. Then this check further down in smc_cdc_msg_recv_action()
can never be true:

    if (diff_cons && conn->urg_tx_pend &&
        atomic_read(&conn->peer_rmbe_space) == conn->peer_rmbe_size) {

Would sendmsg() after MSG_OOB then block?

This also seems to contradict the commit message:

    "Before the attach the current peer has consumed nothing of ours, so
    no update from it is lost."

>  			/* Calculate consumed data and
>  			 * increment free send buffer space.
>  			 */
> -			diff_tx = smc_curs_diff(conn->sndbuf_desc->len,
> +			diff_tx = smc_curs_diff(sndbuf_desc->len,
>  						&conn->tx_curs_fin,
>  						&conn->local_rx_ctrl.cons);

[Severity: Critical]
This only covers the window before smcd_buf_attach(). After
smp_store_release() publishes sndbuf_desc, can the old peer of the reused
DMB still signal this connection and have its consumer cursor counted here
as if the current peer had sent it?

Nothing in smcd_buf_attach() filters CDCs from the old peer, which still
holds the DMB token. smcd_cdc_rx_tsklet() reads whichever peer last wrote
the DMB header. smcd_cdc_msg_to_host() then copies the cursors without the
monotonicity check that smc_cdc_cursor_to_host() does for SMC-R.

The new connection's tx_curs_fin starts at {0,0}. If the stale cons is
{wrap!=0, count=c}, then diff_tx = len + c, and sndbuf_space becomes about
2 * len + c. That breaks the invariant sndbuf_space <= sndbuf_desc->len.

smc_tx_sendmsg() then uses that value:

    copylen = min_t(size_t, send_remaining, writespace);
    ...
    chunk_len = min_t(size_t, copylen, conn->sndbuf_desc->len -
                      tx_cnt_prep);
    ...
            chunk_len = copylen - chunk_len; /* remainder */
            chunk_len_sum += chunk_len;
            chunk_off = 0; /* modulo offset in send ring buffer */

The second chunk is copied to offset 0 by memcpy_from_msg(), and nothing
bounds it against sndbuf_desc->len. With prep at 0 that chunk is len + c
bytes. Does that write c bytes of user data past the end of the peer's DMB
folio? The folio is exactly the DMB size, so the extra bytes would land in
the pages next to it.

Smaller over-credits stay inside the buffer. They can still overwrite data
the real peer has not consumed yet, and tx_curs_fin is overwritten with the
foreign cursor.

One way to get there: smc_release() calls smc_close_active_abort() when a
non-blocking connect finishes during close. That sends no CDC to the peer.
smc_conn_free()->smc_buf_unuse() then only zeroes the DMB and clears used,
and the token stays registered. The peer is still active and still
attached.

__smc_buf_create() can then reuse the slot, and smc_ism_set_conn() maps it
to the new connection. The old peer's next CDC follows this path:

    smcd_cdc_msg_send()->dibs_lo_move_data()->handle_irq()
        ->smcd_cdc_rx_tsklet()

and is processed on the new connection. AF_SMC needs no privilege, so a
local user who controls both ends and the timing could trigger this
wherever loopback-ism is enabled.

This post-attach path already existed before this patch. Since the commit
message names "the old peer of a reused DMB can still signal us" as the
root cause, would it be better to not reuse a DMB that a peer may still
have attached, or to filter CDCs from the stale peer? Gating on
sndbuf_desc only hides the NULL dereference.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015-b4-disp-2a8d85ef-v5-0-6b795e8263c9%40proton.me

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-10-19 12:14 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-15 11:53 [PATCH net v5 0/2] net/smc: order the CDC receive path against buffer publication Bryam Vargas via B4 Relay
2026-10-15 11:53 ` [PATCH net v5 1/2] net/smc: order the CDC receive path against RMB publication Bryam Vargas via B4 Relay
2026-10-19 12:14   ` netdev-bot+sashiko
2026-10-15 11:53 ` [PATCH net v5 2/2] net/smc: skip the DMB nocopy accounting until the send buffer is set Bryam Vargas via B4 Relay
2026-10-19 12:14   ` netdev-bot+sashiko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®