[PATCH net v8 0/4] net/smc: bound wire-controlled CDC cursors against the local buffers

[PATCH net v8 0/4] net/smc: bound wire-controlled CDC cursors against the local buffers

[Bryam Vargas via B4 Relay]

A peer's CDC cursors come off the wire and are used, with no bound
against the local buffers, as a raw index into the RMB on the urgent
path (1/4), as the rx length (2/4), as the tx write space (3/4), and as
the cursor advance at SMC-R link failover (4/4). Any peer that completes
the SMC handshake controls them. With a forged cursor each use runs past
its buffer: an OOB read of kernel memory returned to the reader, send()
data written past the send buffer, or, at failover, an RDMA write whose
source lies past the send buffer. Two of these also happen with a
conforming peer: a MSG_PEEK|MSG_WAITALL reader racing another recv()
(2/4), and late completions on the old link after a failover (3/4).

With a second link in the group, the peer can force the failover: a
DELETE LINK request, or failing one of our RDMA writes, takes the link
down and switches the connections. Late completions need only ACKs on
the old link that arrive after the switch, from a degrading link or a
peer that delays them.

Bounding each cursor on arrival is not enough. A peer that only bumps
prod.wrap with count 0 hits the differing-wrap branch of
smc_curs_diff(), which returns the full buffer length on every CDC, so
bytes_to_rcv grows past the RMB while every cursor looks in range. IOW
the overflow is in the accumulator, not the cursor. 1/4 can clamp on
arrival because the urgent path indexes with the count itself; 2/4-4/4
bound the value where it is used. Hidayath Khan's "net/smc: abort the
connection when the peer overruns the RMB" [1] (net-next) drops the
connection at CDC receive; this series does not depend on it.

Found by inspection of the cursor handling in net/smc; each case was
then confirmed on a KASAN rig with one end patched to forge its
cursors.

Tested on v7.3-rc6 with KASAN, over SMC-D loopback and over SMC-R on
rxe with two links, one end patched to forge its CDC cursors:
  - unpatched: recv(), MSG_PEEK|MSG_WAITALL, SO_RCVLOWAT and splice()
    read past the RMB, as does recv() once bytes_to_rcv wraps
    negative; the urgent byte comes from past the RMB; send() writes
    past the send buffer; after a failover with a forged consumer
    cursor the next RDMA write takes its source past the send buffer
    (rxe refuses that WR).
  - conforming peers, unpatched: the peek race reads past the RMB; a
    completion held back past the failover (a test hook makes the
    timing deterministic) leaves sndbuf_space at 3x the buffer, and
    send() writes past the send buffer.
  - v6 still reads past the RMB on a peek longer than two ring lengths
    and on repeated undrained splices.
  - v7: each case above stays inside its buffer, with no KASAN report,
    and no KMSAN report on the loopback runs.
  - dropping any one patch, or any one part of 2/4, makes a test fail
    again.
  - conforming traffic is unchanged: streams compared byte for byte,
    peek then recv, SO_RCVLOWAT, splice drained and undrained, urgent
    data inline and out-of-band, deferred accept, SIOCINQ and poll,
    nonblocking reads, close with unread data, failover mid-stream, a
    reader waiting while the link group is torn down.

v8 re-run on v7.3 with KASAN over SMC-D loopback (unpatched, v7, v8):
the SMC-D cases above give the same results (v8 as v7), plus the
poll(), SIOCINQ and deferred accept cases under 2/4 below, each with a
mutant that keeps its one site raw and fails. Not re-run over SMC-R: v8
changes only af_smc.c's code, the rest are comment lines; smc_rx.o,
smc_tx.o and smc_core.o built from v8 are byte-identical to v7's on the
same tree. Logs on request.

Changes in v8:
  - 2/4: smc_poll(), SIOCINQ and the deferred accept read bytes_to_rcv
    through the bounded helper too (Sashiko's review of v7 [8]). With
    the count driven negative (wrap bumps past INT_MAX), v7's poll()
    said POLLIN while recv() gave -EAGAIN, so a level-triggered reader
    spun (1,887,464 wakeups and 1000 ms of CPU in 1 s; v8: 0 wakeups),
    SIOCINQ returned the negative count (v8: 0), and a deferred accept
    returned at once with nothing to read (v8 waits out its timeout).
    With the count at 6x the RMB, SIOCINQ said 6x (v8: the RMB).
    Conforming traffic sees no change. The two comments that claimed
    0 <= bytes_to_rcv <= len say "can be out of range" now.
  - 3/4: code unchanged; the comment that claimed 0 <= sndbuf_space <=
    len points at the clamp instead [9]. R-b kept.
  - 4/4: code unchanged; the catch-up comment now says why it is bounded
    (a peer past the bound stalls). The message no longer claims the
    destination side: smc_write_space() still sizes the window from the
    peer's cursor, so a forged one can take the next write past the
    peer's own RMB, where its HCA refuses it [10].
  - Rebased on net after the net-next merge (fcc00b15cb21).
  v7: https://lore.kernel.org/all/20261013-b4-disp-f345196c-v7-0-ef3dddfbfa68@proton.me/

Changes in v7:
  - 2/4 (was v6 2/3): per Sashiko's reviews of v6 [2][3][5], MSG_PEEK
    and splice() offsets could start the copy outside the RMB, a local
    peek race [3] could leave the available length negative, and wrap
    bumps grow bytes_to_rcv (the accumulator case above). The bound
    moves into smc_rx_data_available(), counts peeked and spliced
    bytes, maps a negative length to 0, not a full ring, and covers
    the wait predicates too. The RMB length is cached in conn->rmb_len
    for the unlocked wait, and splice_pending now drops before the
    consumer cursor moves. New Fixes:, Reported-by: and Closes: tags;
    R-b dropped.
  - 4/4 is new: the failover cursor advance, which 3/4 does not cover.
  - 3/4: code unchanged. [4] noted that DMB merge runs only on the
    loopback device, so its peer is the local kernel; the message now
    leads with the over-credit after a failover, which needs no forged
    cursor, and says it bounds the copy. New Fixes:, R-b kept.
  - 1/4: code unchanged, comment and message reworded. R-b kept.
  - [6] (rmb_desc NULL before smc_buf_create()) is pre-existing; my
    fix for it, [7], is marked changes requested in patchwork and
    touches the same lines of __smc_buf_create() as 2/4.
  v6: https://lore.kernel.org/all/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c@proton.me/

Changes in v6: repost on net; 2/3 message corrected (the over-read goes
to the reader, not the peer); Sidraya Jayagond's R-b.
  v5: https://lore.kernel.org/all/20260723-b4-disp-0d07164f-v5-0-6a9e235dbc4e@proton.me/

Changes in v5: repost of v4.
  v4: https://lore.kernel.org/all/20260705-b4-disp-28a1bbca-v4-0-be089b98acc6@proton.me/

Changes in v4: split off the validate/abort change, per Dust Li.
  v3: https://lore.kernel.org/all/20260614-b4-disp-edd64be9-v3-0-551fa514257e@proton.me/

[1] https://lore.kernel.org/all/20260804141109.542202-1-hidayath@linux.ibm.com/
[2] https://lore.kernel.org/all/sashiko-outbox-164918@kernel.org/
[3] https://lore.kernel.org/all/179152807838.434549.2207466107170796457@kernel.org/
[4] https://lore.kernel.org/all/179152807992.434549.3295281690534839972@kernel.org/
[5] https://lore.kernel.org/all/179152807674.434549.14168488606722848951@kernel.org/
[6] https://lore.kernel.org/all/sashiko-outbox-164917@kernel.org/
[7] https://lore.kernel.org/all/20260728-b4-disp-52ee4e7d-v4-1-0dda94b0f397@proton.me/
[8] https://lore.kernel.org/all/179209532096.434549.4189685148052558990@kernel.org/
[9] https://lore.kernel.org/all/179209532279.434549.3831566859438447501@kernel.org/
[10] https://lore.kernel.org/all/179209532452.434549.6588379141939967966@kernel.org/

---
Bryam Vargas (4):
      net/smc: bound the wire-controlled producer cursor to the RMB
      net/smc: bound the receive length to the RMB
      net/smc: bound the send length to the send buffer in smc_tx_sendmsg()
      net/smc: bound the link failover cursor advance to the send buffer

 net/smc/af_smc.c   |  6 +++---
 net/smc/smc.h      |  3 +++
 net/smc/smc_cdc.c  |  2 +-
 net/smc/smc_cdc.h  | 17 ++++++++++++++---
 net/smc/smc_core.c | 19 +++++++++++++------
 net/smc/smc_rx.c   |  9 +++++++--
 net/smc/smc_rx.h   | 11 ++++++++++-
 net/smc/smc_tx.c   | 10 +++++++++-
 8 files changed, 60 insertions(+), 17 deletions(-)
---
base-commit: fcc00b15cb21a6de76826e5012b2c758505f5e65
change-id: 20261029-b4-disp-33001ace-db9b3e0669a6

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

[PATCH net v8 1/4] net/smc: bound the wire-controlled producer cursor to the RMB

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

smcr_cdc_msg_to_host() and smcd_cdc_msg_to_host() import the peer's
producer cursor from the wire into conn->local_rx_ctrl.prod without
bounding it to the RMB. smc_cdc_handle_urg_data_arrival() then uses its
count as a raw index into the RMB, so a producer cursor past
rmb_desc->len makes the receive tasklet read past the RMB, and
recv(MSG_OOB) hands that byte to the application.

Bound the producer cursor count to rmb_desc->len when it comes off the
wire, for SMC-R and SMC-D. Leave the consumer cursor alone: it indexes
the peer's RMB, which can be larger than ours, so clamping it to our
RMB would under-credit peer_rmbe_space and could stall sending to that
peer. Conforming peers are unaffected.

Fixes: de8474eb9d50 ("net/smc: urgent data support")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
Reviewed-by: Dust Li <dust.li@linux.alibaba.com>
Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>
---
 net/smc/smc_cdc.h | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/net/smc/smc_cdc.h b/net/smc/smc_cdc.h
index 696cc11f2303..4d630a305118 100644
--- a/net/smc/smc_cdc.h
+++ b/net/smc/smc_cdc.h
@@ -221,7 +221,8 @@ static inline void smc_host_msg_to_cdc(struct smc_cdc_msg *peer,
 
 static inline void smc_cdc_cursor_to_host(union smc_host_cursor *local,
 					  union smc_cdc_cursor *peer,
-					  struct smc_connection *conn)
+					  struct smc_connection *conn,
+					  int max_count)
 {
 	union smc_host_cursor temp, old;
 	union smc_cdc_cursor net;
@@ -235,6 +236,12 @@ static inline void smc_cdc_cursor_to_host(union smc_host_cursor *local,
 	if ((old.wrap == temp.wrap) &&
 	    (old.count > temp.count))
 		return;
+	/* The urgent path uses the producer count as a raw index into our
+	 * RMB, so bound it. max_count == 0 leaves the consumer cursor alone:
+	 * it indexes the peer's RMB, which can be larger than ours.
+	 */
+	if (max_count && temp.count > max_count)
+		temp.count = max_count;
 	smc_curs_copy(local, &temp, conn);
 }
 
@@ -246,8 +253,9 @@ static inline void smcr_cdc_msg_to_host(struct smc_host_cdc_msg *local,
 	local->len = peer->len;
 	local->seqno = ntohs(peer->seqno);
 	local->token = ntohl(peer->token);
-	smc_cdc_cursor_to_host(&local->prod, &peer->prod, conn);
-	smc_cdc_cursor_to_host(&local->cons, &peer->cons, conn);
+	smc_cdc_cursor_to_host(&local->prod, &peer->prod, conn,
+			       conn->rmb_desc->len);
+	smc_cdc_cursor_to_host(&local->cons, &peer->cons, conn, 0);
 	local->prod_flags = peer->prod_flags;
 	local->conn_state_flags = peer->conn_state_flags;
 }
@@ -260,6 +268,9 @@ static inline void smcd_cdc_msg_to_host(struct smc_host_cdc_msg *local,
 
 	temp.wrap = peer->prod.wrap;
 	temp.count = peer->prod.count;
+	/* as in smc_cdc_cursor_to_host(): bound prod, not cons */
+	if (temp.count > conn->rmb_desc->len)
+		temp.count = conn->rmb_desc->len;
 	smc_curs_copy(&local->prod, &temp, conn);
 
 	temp.wrap = peer->cons.wrap;

-- 
2.56.0

[PATCH net v8 2/4] net/smc: bound the receive length to the RMB

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

smc_rx_recvmsg() sizes the copy from bytes_to_rcv and starts it past the
bytes MSG_PEEK or splice() already took. A forged producer cursor drives
bytes_to_rcv past the RMB or negative, and with a conforming peer it
drops below the peeked count when a MSG_PEEK|MSG_WAITALL reader sleeps
while another recv() consumes what it peeked. Either way the copy reads
past the RMB into the reader's buffer: the second chunk runs long, or
the start offset leaves the RMB, as smc_curs_add() wraps only once.
poll(), SIOCINQ and the deferred accept read the same raw count, so
SIOCINQ hands it to userspace as is, and bounding recv() alone would
leave a level-triggered reader spinning on a socket recv() finds empty.

Make smc_rx_data_available() report at most what fits in the RMB behind
the peeked and spliced bytes, never a negative count, and use it for the
copy, the wait predicates, poll(), SIOCINQ and the deferred accept, so
they all see the same length. Cache the RMB length for the unlocked
wait, and drop released bytes from splice_pending before the consumer
cursor moves past them, so no reader counts them twice.

Fixes: 952310ccf2d8 ("smc: receive data from RMBE")
Fixes: 9014db202cb7 ("smc: add support for splice()")
Fixes: a4b6539038c1 ("net/smc: fix data error when recvmsg with MSG_PEEK flag")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c@proton.me?part=2
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c%40proton.me?part=2
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c%40proton.me?part=1
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
The wait predicates run unlocked, where a link group termination may
already have freed rmb_desc, hence the cached length. With the old
splice_pending order, a reader that took the lock between the cursor
update and the drop counted the bytes twice, and the next splice()
returned stale bytes in place of unread data. 1/4 bounds the producer
count, but wrap bumps still grow bytes_to_rcv past the RMB (see the
cover).
---
 net/smc/af_smc.c   |  6 +++---
 net/smc/smc.h      |  3 +++
 net/smc/smc_cdc.c  |  2 +-
 net/smc/smc_core.c |  1 +
 net/smc/smc_rx.c   |  9 +++++++--
 net/smc/smc_rx.h   | 11 ++++++++++-
 6 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index d10244af5bd2..d492c7716128 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -2774,7 +2774,7 @@ int smc_accept(struct socket *sock, struct socket *new_sock,
 			if (skb_queue_empty(&clcsk->sk_receive_queue))
 				sk_wait_data(clcsk, &timeo, NULL);
 			release_sock(clcsk);
-		} else if (!atomic_read(&smc_sk(nsk)->conn.bytes_to_rcv)) {
+		} else if (!smc_rx_data_available(&smc_sk(nsk)->conn, 0)) {
 			lock_sock(nsk);
 			smc_rx_wait(smc_sk(nsk), &timeo, 0, smc_rx_data_available);
 			release_sock(nsk);
@@ -2939,7 +2939,7 @@ __poll_t smc_poll(struct file *file, struct socket *sock,
 						mask |= EPOLLOUT | EPOLLWRNORM;
 				}
 			}
-			if (atomic_read(&smc->conn.bytes_to_rcv))
+			if (smc_rx_data_available(&smc->conn, 0))
 				mask |= EPOLLIN | EPOLLRDNORM;
 			if (sk->sk_shutdown & RCV_SHUTDOWN)
 				mask |= EPOLLIN | EPOLLRDNORM | EPOLLRDHUP;
@@ -3237,7 +3237,7 @@ int smc_ioctl(struct socket *sock, unsigned int cmd,
 		    smc->sk.sk_state == SMC_CLOSED)
 			answ = 0;
 		else
-			answ = atomic_read(&smc->conn.bytes_to_rcv);
+			answ = smc_rx_data_available(&smc->conn, 0);
 		break;
 	case SIOCOUTQ:
 		/* output queue size (not send + not acked) */
diff --git a/net/smc/smc.h b/net/smc/smc.h
index 427b6d63b993..d4325a0a8fd8 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -203,6 +203,9 @@ struct smc_connection {
 						 * rmbe
 						 */
 	int			rtoken_idx;	/* idx to peer RMB rkey/addr */
+	int			rmb_len;	/* rmb_desc->len, still valid
+						 * after rmb_desc is freed
+						 */
 
 	struct smc_buf_desc	*sndbuf_desc;	/* send buffer descriptor */
 	struct smc_buf_desc	*rmb_desc;	/* RMBE descriptor */
diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
index 32d6d03df321..e2c765ff2ba3 100644
--- a/net/smc/smc_cdc.c
+++ b/net/smc/smc_cdc.c
@@ -381,7 +381,7 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
 		/* bytes_to_rcv is decreased in smc_recvmsg */
 		smp_mb__before_atomic();
 		atomic_add(diff_prod, &conn->bytes_to_rcv);
-		/* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */
+		/* can be out of range, see smc_rx_data_available() */
 		smp_mb__after_atomic();
 		smc->sk.sk_data_ready(&smc->sk);
 	} else {
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 6021322253e9..fe6e4a2f7935 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -2505,6 +2505,7 @@ static int __smc_buf_create(struct smc_sock *smc, bool is_smcd, bool is_rmb)
 
 	if (is_rmb) {
 		conn->rmb_desc = buf_desc;
+		conn->rmb_len = buf_desc->len;
 		conn->rmbe_size_comp = bufsize_comp;
 		smc->sk.sk_rcvbuf = bufsize * 2;
 		atomic_set(&conn->bytes_to_rcv, 0);
diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
index 197fddc6271f..85654d9f321b 100644
--- a/net/smc/smc_rx.c
+++ b/net/smc/smc_rx.c
@@ -118,15 +118,20 @@ static void smc_rx_pipe_buf_release(struct pipe_inode_info *pipe,
 	struct smc_connection *conn = &priv->smc->conn;
 	struct smc_sock *smc = priv->smc;
 	struct sock *sk = &smc->sk;
+	bool drained;
 
 	lock_sock(sk);
 	if (conn->freed) {
 		release_sock(sk);
 		goto out;
 	}
+	/* drop the released bytes from splice_pending before cons moves
+	 * past them, so no reader counts them twice
+	 */
+	drained = atomic_sub_and_test(priv->len, &conn->splice_pending);
 	smc_rx_update_cons(smc, priv->len);
 	release_sock(sk);
-	if (atomic_sub_and_test(priv->len, &conn->splice_pending))
+	if (drained)
 		smc_rx_wake_up(sk);
 out:
 	kfree(priv);
@@ -506,7 +511,7 @@ int smc_rx_recvmsg(struct smc_sock *smc, struct msghdr *msg,
 			/* increased in recv tasklet smc_cdc_msg_rcv() */
 			smp_mb__before_atomic();
 			atomic_sub(copylen, &conn->bytes_to_rcv);
-			/* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */
+			/* can be out of range, see smc_rx_data_available() */
 			smp_mb__after_atomic();
 			if (msg && smc_rx_update_consumer(smc, cons, copylen))
 				goto out;
diff --git a/net/smc/smc_rx.h b/net/smc/smc_rx.h
index 994f5e42d1ba..6bf44e3cc33a 100644
--- a/net/smc/smc_rx.h
+++ b/net/smc/smc_rx.h
@@ -25,7 +25,16 @@ int smc_rx_wait(struct smc_sock *smc, long *timeo, size_t peeked,
 		int (*fcrit)(struct smc_connection *conn, size_t baseline));
 static inline int smc_rx_data_available(struct smc_connection *conn, size_t peeked)
 {
-	return atomic_read(&conn->bytes_to_rcv) - peeked;
+	int avail = atomic_read(&conn->bytes_to_rcv);
+
+	/* bytes_to_rcv follows the peer's producer cursor and can be past
+	 * the RMB or negative. Report no more than fits in the RMB behind the
+	 * bytes already spliced or peeked. Compare before subtracting: near
+	 * INT_MIN, avail - peeked wraps. This also runs unlocked, possibly
+	 * after rmb_desc was freed, hence rmb_len.
+	 */
+	avail = min(avail, conn->rmb_len - atomic_read(&conn->splice_pending));
+	return avail > (int)peeked ? avail - (int)peeked : 0;
 }
 
 #endif /* SMC_RX_H */

-- 
2.56.0

[PATCH net v8 3/4] net/smc: bound the send length to the send buffer in smc_tx_sendmsg()

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

conn->sndbuf_space can run past the send buffer. After an SMC-R link
failover, a late completion from the old link can move tx_curs_fin back,
and the next completion credits the same bytes again; across the ring
end, smc_curs_diff() credits the step back itself as more than a whole
ring, so even with a conforming peer sndbuf_space can reach three times
the buffer. The failover catch-up and SMC-D DMB merge (only the loopback
device today) also credit it from the peer's consumer cursor without a
bound. smc_tx_sendmsg() takes it as the write space, and the second
chunk of its copy writes past the send buffer.

Clamp writespace, which sizes the copy, to sndbuf_desc->len, negative
values included, so the copy stays in the send buffer.

Fixes: c6f02ebeea3a ("net/smc: switch connections to alternate link")
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>
Reviewed-by: Dust Li <dust.li@linux.alibaba.com>
Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>
---
This keeps the copy inside the send buffer but does not undo the
over-credit, so after such a failover send() can still overwrite data in
the buffer that the peer has not received yet (by inspection; not
tested). Fixing the credit in smc_cdc_tx_handler() needs a cursor
compare that survives the u16 wrap rollover, which smc_curs_comp() does
not do, so it is left out here. A negative value maps to len, not 0,
since 0 would make the send loop spin.
---
 net/smc/smc_tx.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c
index 0f0c6df3bf2c..3913ef103b35 100644
--- a/net/smc/smc_tx.c
+++ b/net/smc/smc_tx.c
@@ -233,6 +233,14 @@ int smc_tx_sendmsg(struct smc_sock *smc, struct msghdr *msg, size_t len)
 		/* initialize variables for 1st iteration of subsequent loop */
 		/* could be just 1 byte, even after smc_tx_wait above */
 		writespace = atomic_read(&conn->sndbuf_space);
+		/* sndbuf_space can be out of range: late completions of an
+		 * old SMC-R link credit it again after a failover, and with
+		 * SMC-D DMB merge it follows the peer's consumer cursor.
+		 * Bound writespace, which sizes the copy; a negative value
+		 * would become huge in the size_t min_t() below.
+		 */
+		if (writespace < 0 || writespace > conn->sndbuf_desc->len)
+			writespace = conn->sndbuf_desc->len;
 		/* not more than what user space asked for */
 		copylen = min_t(size_t, send_remaining, writespace);
 		/* determine start of sndbuf */
@@ -278,7 +286,7 @@ int smc_tx_sendmsg(struct smc_sock *smc, struct msghdr *msg, size_t len)
 		/* increased in send tasklet smc_cdc_tx_handler() */
 		smp_mb__before_atomic();
 		atomic_sub(copylen, &conn->sndbuf_space);
-		/* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */
+		/* can be out of range, see writespace above */
 		smp_mb__after_atomic();
 		/* since we just produced more new data into sndbuf,
 		 * trigger sndbuf consumer: RDMA write into peer RMBE and CDC

-- 
2.56.0

[PATCH net v8 4/4] net/smc: bound the link failover cursor advance to the send buffer

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

On SMC-R link failover, smc_switch_cursor() rewinds the send and
producer cursors to their values at the last completed CDC send, then
advances them by the distance the peer's consumer cursor is ahead. That
cursor comes from the wire, so a forged one adds an arbitrary amount to
sndbuf_space and to the cursors. smc_curs_add() wraps them only once, so
the next RDMA write takes its source past the send buffer.

A conforming peer keeps its cursor count inside its RMB and cannot
consume past the data in the send buffer. Advance only for such a count,
and no further than tx_curs_prep. The pre-failover tx_curs_sent cannot
be the bound: an earlier failover may have rewound it below the peer's
cursor. The destination side is not bounded here: smc_write_space()
still sizes the window from the peer's cursor, so a forged one can take
the next write past the peer's own RMB, where its HCA refuses it and
the link goes down.

Fixes: c6f02ebeea3a ("net/smc: switch connections to alternate link")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
Bounding the copy in smc_tx_sendmsg() (3/4) does not cover this: the
RDMA write takes its offsets from the cursors. Measured on rxe, send
buffer 16K, peer RMB 32K, 4096 bytes sent and unconfirmed, the consumer
cursor forged one wrap ahead at count 64: unpatched, tx_curs_sent lands
64 bytes past the send buffer and the next write takes its source from
there (rxe refuses that WR); with the patch the advance stops at
tx_curs_prep, nothing is pending, and nothing is written after the
switch. The destination side, with the consumer cursor forged two wraps
ahead at the peer's RMB + 4096 (send buffer 64K, peer RMB 16K): with the
patch the catch-up is skipped, the window is the whole RMB and the first
write after the switch, from prod.count 1, ends one byte past the peer's
RMB; its HCA refuses the write and the link goes down. Without the patch
the catch-up lands prod on the forged count, smc_write_space() is 0 and
nothing more is written.
---
 net/smc/smc_core.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index fe6e4a2f7935..42d83a002ffb 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1034,7 +1034,7 @@ static int smc_switch_cursor(struct smc_sock *smc, struct smc_cdc_tx_pend *pend,
 			     struct smc_wr_buf *wr_buf)
 {
 	struct smc_connection *conn = &smc->conn;
-	union smc_host_cursor cons, fin;
+	union smc_host_cursor cons, fin, prep;
 	int rc = 0;
 	int diff;
 
@@ -1043,12 +1043,18 @@ static int smc_switch_cursor(struct smc_sock *smc, struct smc_cdc_tx_pend *pend,
 	/* set prod cursor to old state, enforce tx_rdma_writes() */
 	smc_curs_copy(&conn->local_tx_ctrl.prod, &fin, conn);
 	smc_curs_copy(&cons, &conn->local_rx_ctrl.cons, conn);
-
-	if (smc_curs_comp(conn->peer_rmbe_size, &cons, &fin) < 0) {
-		/* cons cursor advanced more than fin, and prod was set
-		 * fin above, so now prod is smaller than cons. Fix that.
+	smc_curs_copy(&prep, &conn->tx_curs_prep, conn);
+
+	if (cons.count < conn->peer_rmbe_size &&
+	    smc_curs_comp(conn->peer_rmbe_size, &cons, &fin) < 0) {
+		/* cons cursor advanced more than fin, and prod was set fin
+		 * above, so now prod is smaller than cons. cons comes off the
+		 * wire: catch up only for a count inside the peer's RMB and no
+		 * further than the send buffer holds, else that peer stalls.
 		 */
-		diff = smc_curs_diff(conn->peer_rmbe_size, &fin, &cons);
+		diff = min(smc_curs_diff(conn->peer_rmbe_size, &fin, &cons),
+			   smc_curs_diff(conn->sndbuf_desc->len,
+					 &conn->tx_curs_sent, &prep));
 		smc_curs_add(conn->sndbuf_desc->len,
 			     &conn->tx_curs_sent, diff);
 		smc_curs_add(conn->sndbuf_desc->len,

-- 
2.56.0

Re: [PATCH net v8 1/4] net/smc: bound the wire-controlled producer cursor to the RMB

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (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).

- [High] This patch (commit "net/smc: bound the wire-controlled producer
  cursor to the RMB") only clamps the count field of the peer's producer…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but: on SMC-R, the
  CDC receive path can read a freed RMB descriptor.

--

net/smc: bound the wire-controlled producer cursor to the RMB

This patch clamps the count of the producer cursor the peer sends to
rmb_desc->len. It does this in smcr_cdc_msg_to_host() and
smcd_cdc_msg_to_host(), which stops smc_cdc_handle_urg_data_arrival()
from indexing past the RMB. The consumer cursor is not clamped, because
it indexes the peer's RMB.

> diff --git a/net/smc/smc_cdc.h b/net/smc/smc_cdc.h
> index 696cc11f2303b..4d630a3051184 100644
> --- a/net/smc/smc_cdc.h
> +++ b/net/smc/smc_cdc.h

[ ... ]

> @@ -235,6 +236,12 @@ static inline void smc_cdc_cursor_to_host(union smc_host_cursor *local,
>  	if ((old.wrap == temp.wrap) &&
>  	    (old.count > temp.count))
>  		return;
> +	/* The urgent path uses the producer count as a raw index into our
> +	 * RMB, so bound it. max_count == 0 leaves the consumer cursor alone:
> +	 * it indexes the peer's RMB, which can be larger than ours.
> +	 */
> +	if (max_count && temp.count > max_count)
> +		temp.count = max_count;

[Severity: High]
Only temp.count is bounded here. The code still accepts a forward jump
in temp.wrap from the wire. Can a peer use that to push
conn->bytes_to_rcv past rmb_desc->len?

When the wraps differ, smc_curs_diff() returns:

	if (old->wrap != new->wrap)
		return max_t(int, 0, ((size - old->count) + new->count));

That can approach 2 * rmb_desc->len for a single CDC message.
smc_cdc_msg_recv_action() then adds it with no check against free RMB
space:

	atomic_add(diff_prod, &conn->bytes_to_rcv);

The SMC-D path in smcd_cdc_msg_to_host() also clamps only the count. It
has no ordering or wrap check on prod.

With only this patch applied, smc_rx_recvmsg() takes readable straight
from bytes_to_rcv. The second chunk of the copy starts at offset 0:

	chunk_len = copylen - chunk_len; /* remainder */
	chunk_len_sum += chunk_len;
	chunk_off = 0;

Wouldn't memcpy_to_msg() or smc_rx_splice() then copy memory past the
end of the RMB to user space?

The later commit in this series, "net/smc: bound the receive length to
the RMB", seems to close this path. It bounds smc_rx_data_available():

	avail = min(avail, conn->rmb_len - atomic_read(&conn->splice_pending));
	return avail > (int)peeked ? avail - (int)peeked : 0;

It also uses that value for readable in smc_rx_recvmsg().

This patch is marked Cc: stable. Should both patches be backported
together, or should this function also reject a wrap that advances by
more than one?

>  	smc_curs_copy(local, &temp, conn);
>  }
>  
> @@ -246,8 +253,9 @@ static inline void smcr_cdc_msg_to_host(struct smc_host_cdc_msg *local,
>  	local->len = peer->len;
>  	local->seqno = ntohs(peer->seqno);
>  	local->token = ntohl(peer->token);
> -	smc_cdc_cursor_to_host(&local->prod, &peer->prod, conn);
> -	smc_cdc_cursor_to_host(&local->cons, &peer->cons, conn);
> +	smc_cdc_cursor_to_host(&local->prod, &peer->prod, conn,
> +			       conn->rmb_desc->len);

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. Still,
this adds another read of conn->rmb_desc->len, and on SMC-R this path
looks like it can run after the descriptor has been freed.

If adding a link fails to register an in-use RMB, smcr_link_reg_buf()
marks the buffer:

	if (smc_wr_reg_send(link, buf_desc->mr[link->link_idx])) {
		buf_desc->is_reg_err = true;
		return -EFAULT;
	}

At teardown, smc_conn_free() releases the buffers before it unregisters
the connection:

	smc_buf_unuse(conn, lgr); /* allow buffer reuse */
	smc_lgr_unregister_conn(conn);

Because is_reg_err is set, the call chain
smc_buf_unuse()->smcr_buf_unuse()->smc_buf_free()->smcr_buf_free()
calls kfree() on the descriptor. conn->rmb_desc is left pointing at the
freed memory. For SMC-R, smc_conn_free() only waits for pending TX WRs.
Nothing quiesces the RX side.

On the receive side, smc_cdc_rx_handler() takes only a socket reference
before it drops conns_lock, and it never checks conn->freed:

	conn = smc_lgr_find_conn(ntohl(cdc->token), lgr);
	if (!conn || conn->out_of_sync) {...}
	smc = container_of(conn, struct smc_sock, conn);
	sock_hold(&smc->sk);
	read_unlock_bh(&lgr->conns_lock);

The bh_lock_sock() in smc_cdc_msg_recv() does not serialize against a
lock_sock() owner in process context either.

Could a CDC message that arrives during teardown reach
smcr_cdc_msg_to_host() and read the freed conn->rmb_desc->len here? It
might be a close or abort from a conforming peer, or a message from a
hostile peer at any time.

smc_cdc_msg_recv_action() reads the same descriptor again:

	diff_prod = smc_curs_diff(conn->rmb_desc->len, &prod_old,
				  &conn->local_rx_ctrl.prod);

If urg_data_present is set, smc_cdc_handle_urg_data_arrival() also reads
a byte from the freed cpu_addr.

The later smc_rx_data_available() change in this series uses
conn->rmb_len because rmb_desc may already be freed at that point. Would
conn->rmb_len also be the safer value to pass here?

> +	smc_cdc_cursor_to_host(&local->cons, &peer->cons, conn, 0);
>  	local->prod_flags = peer->prod_flags;
>  	local->conn_state_flags = peer->conn_state_flags;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-b4-disp-33001ace-v8-0-f9bb96e17827%40proton.me

Re: [PATCH net v8 2/4] net/smc: bound the receive length to the RMB

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch says that reordering splice_pending in
  smc_rx_pipe_buf_release() means 'no reader counts them twice', which…
- [Low] The new conn->rmb_len field exists so it can be read without the
  socket lock: the patch's comment in smc_rx_data_available() says the…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a
  reachable local DoS.
- [High] This problem wasn't introduced by this patch, but it is a real
  accounting bug.

--

net/smc: bound the receive length to the RMB

This commit clamps smc_rx_data_available() to what fits in the RMB behind
the spliced and peeked bytes, never returning a negative count. poll(),
SIOCINQ and the deferred accept now use this helper too. It also caches the
RMB length in conn->rmb_len for unlocked waiters, and moves the
splice_pending decrement in smc_rx_pipe_buf_release() ahead of the consumer
cursor update.

> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index d10244af5bd29..d492c77161283 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c

[ ... ]

> @@ -2939,7 +2939,7 @@ __poll_t smc_poll(struct file *file, struct socket *sock,
>  						mask |= EPOLLOUT | EPOLLWRNORM;
>  				}
>  			}
> -			if (atomic_read(&smc->conn.bytes_to_rcv))
> +			if (smc_rx_data_available(&smc->conn, 0))
>  				mask |= EPOLLIN | EPOLLRDNORM;

[Severity: High]
This isn't a bug introduced by this patch, but can smc_rx_recvmsg() loop
forever while splice_pending is non-zero?

When data is available and an earlier splice() is still pending, the top of
the loop jumps straight to copy:

		if (smc_rx_recvmsg_data_available(smc, peeked_bytes))
			goto copy;

That skips the !timeo, signal_pending() and sk_err checks. At copy, msg &&
splbytes is true:

		if (!readable || (msg && splbytes)) {
			if (splbytes)
				func = smc_rx_data_available_and_no_splice_pend;
			else
				func = smc_rx_data_available;
			smc_rx_wait(smc, &timeo, peeked_bytes, func);
			continue;
		}

smc_rx_data_available_and_no_splice_pend() stays false until the pipe
buffers are released. The return value of smc_rx_wait() is ignored.

With MSG_DONTWAIT, recv() never returns -EAGAIN; each pass sleeps about a
jiffy and loops again. With a signal pending, including SIGKILL,
wait_woken() returns at once and the task busy-loops. The only ways out are
splice_pending draining or conn->killed. If the same process holds the pipe,
the pipe buffers would never be released.

smc_poll() now reports EPOLLIN from smc_rx_data_available(conn, 0). That
helper uses splice_pending only to cap the length, but recvmsg() refuses to
copy anything while splice_pending is non-zero. While a splice is pending,
does the commit message's claim that these paths "all see the same length"
hold?

>  			if (sk->sk_shutdown & RCV_SHUTDOWN)
>  				mask |= EPOLLIN | EPOLLRDNORM | EPOLLRDHUP;

[ ... ]

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 6021322253e93..fe6e4a2f7935a 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -2505,6 +2505,7 @@ static int __smc_buf_create(struct smc_sock *smc, bool is_smcd, bool is_rmb)
>  
>  	if (is_rmb) {
>  		conn->rmb_desc = buf_desc;
> +		conn->rmb_len = buf_desc->len;

[Severity: Low]
The new comment in smc_rx_data_available() says conn->rmb_len is read
without the socket lock:

	 * INT_MIN, avail - peeked wraps. This also runs unlocked, possibly
	 * after rmb_desc was freed, hence rmb_len.
	 */
	avail = min(avail, conn->rmb_len - atomic_read(&conn->splice_pending));

Should this store and that load use WRITE_ONCE() and READ_ONCE()?

For example, during a non-blocking connect,
smc_connect_work()->__smc_connect()->smc_buf_create()->__smc_buf_create()
writes rmb_len under lock_sock(). At the same time, smc_poll() on the
SMC_INIT socket reads it without the lock. The first unlocked evaluation of
the condition in sk_wait_event() has the same pattern.

KCSAN would report this as a data race. There is no functional effect today,
since bytes_to_rcv is 0 at that point.

>  		conn->rmbe_size_comp = bufsize_comp;
>  		smc->sk.sk_rcvbuf = bufsize * 2;
>  		atomic_set(&conn->bytes_to_rcv, 0);

> diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
> index 197fddc6271f0..85654d9f321b0 100644
> --- a/net/smc/smc_rx.c
> +++ b/net/smc/smc_rx.c
> @@ -118,15 +118,20 @@ static void smc_rx_pipe_buf_release(struct pipe_inode_info *pipe,
>  	struct smc_connection *conn = &priv->smc->conn;
>  	struct smc_sock *smc = priv->smc;
>  	struct sock *sk = &smc->sk;
> +	bool drained;
>  
>  	lock_sock(sk);
>  	if (conn->freed) {
>  		release_sock(sk);
>  		goto out;
>  	}
> +	/* drop the released bytes from splice_pending before cons moves
> +	 * past them, so no reader counts them twice
> +	 */
> +	drained = atomic_sub_and_test(priv->len, &conn->splice_pending);

[Severity: High]
The commit message says this reorder means "no reader counts them twice".
Is there still a path where spliced bytes are counted twice?

In smc_rx_recvmsg(), a copy that wraps past the end of the RMB is done in two
chunks. If the first chunk succeeds and the second fails, the error path
jumps over the bytes_to_rcv and consumer cursor update:

				if (rc < 0) {
					if (!read_done)
						read_done = -EFAULT;
					goto out;
				}

read_done already includes the first chunk. For splice, smc_rx_splice() has
already added that chunk to splice_pending:

	bytes = splice_to_pipe(pipe, &spd);
	if (bytes > 0)
		atomic_add(bytes, &smc->conn.splice_pending);

Those bytes are then counted in both bytes_to_rcv and splice_pending. The
second chunk can fail with -EAGAIN from splice_to_pipe() when the pipe has
only one free slot. It can also fail with -ENOMEM or -EPIPE.

On the next splice(), the start offset is cons + splice_pending, which skips
the first chunk. The readable count still includes it:

	avail = min(avail, conn->rmb_len - atomic_read(&conn->splice_pending));

Would that splice() return stale ring contents from beyond the producer
cursor?

When those pipe buffers are released, cons would move past prod, and
smc_tx_consumer_update() would send that cursor to the peer. bytes_to_rcv
would also stay inflated for good.

In the recvmsg path, a fault in memcpy_to_msg() on the second chunk returns
the first chunk to userspace without consuming it. Would the next recv()
deliver those bytes again?

The new clamp keeps the read inside the RMB, so this corrupts the stream
rather than memory. But it looks like the same double count that the commit
message says is fixed.

>  	smc_rx_update_cons(smc, priv->len);
>  	release_sock(sk);
> -	if (atomic_sub_and_test(priv->len, &conn->splice_pending))
> +	if (drained)
>  		smc_rx_wake_up(sk);
>  out:
>  	kfree(priv);
> @@ -506,7 +511,7 @@ int smc_rx_recvmsg(struct smc_sock *smc, struct msghdr *msg,
>  			/* increased in recv tasklet smc_cdc_msg_rcv() */
>  			smp_mb__before_atomic();
>  			atomic_sub(copylen, &conn->bytes_to_rcv);

[Severity: High]
This is a pre-existing issue, but is a short successful splice counted here
as if the whole chunk went through?

For SMC-R with a virtually contiguous RMB, smc_rx_splice() works out the
number of pipe entries from the page offset:

	nr_pages = !lgr->is_smcd && smc->conn.rmb_desc->is_vm ?
		   PAGE_ALIGN(len + offset) / PAGE_SIZE : 1;

do_splice_read() limits len by the free slot count but ignores the source
page offset:

	p_space = pipe->max_usage - pipe_buf_usage(pipe);
	len = min_t(size_t, len, p_space << PAGE_SHIFT);

Take an empty 16 slot pipe, len = 65536 and an unaligned offset o.
smc_rx_splice() builds 17 entries and splice_to_pipe() returns 65536 - o.
smc_rx_splice() adds that short count to splice_pending.

smc_rx_recvmsg() only checks rc < 0, and then counts the full chunk:

			read_remaining -= chunk_len;
			read_done += chunk_len;

It also subtracts the full copylen from bytes_to_rcv here.

Would splice() then report o more bytes than reached the pipe, and leave
bytes_to_rcv o bytes below the real unread amount?

The pipe release only moves cons forward by what was actually queued, so the
deficit would add up. That could lose trailing data at EOF, or stall the
connection once the deficit gets close to the RMB size.

> -			/* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */
> +			/* can be out of range, see smc_rx_data_available() */
>  			smp_mb__after_atomic();
>  			if (msg && smc_rx_update_consumer(smc, cons, copylen))
>  				goto out;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-b4-disp-33001ace-v8-0-f9bb96e17827%40proton.me

Re: [PATCH net v8 3/4] net/smc: bound the send length to the send buffer in smc_tx_sendmsg()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message of "net/smc: bound the send length to the send
  buffer in smc_tx_sendmsg()" only says the clamp keeps the copy inside…
- [Low] The patch changes the comment after atomic_sub() in
  smc_tx_sendmsg() from 'guarantee 0 <= sndbuf_space <=…

Pre-existing issues:
- [High] smc_tx_sendmsg() now clamps writespace to sndbuf_desc->len, not
  to the free ring space actually left (len minus the bytes between…

--

net/smc: bound the send length to the send buffer in smc_tx_sendmsg()

This patch clamps writespace in smc_tx_sendmsg() to sndbuf_desc->len, and
negative values get the same clamp. The copy into the send buffer can then
no longer run past the end of the ring when conn->sndbuf_space has been
over-credited, either after an SMC-R link failover or by SMC-D DMB merge.
It also changes the comment after atomic_sub() so it no longer claims that
the counter stays in range.

> Clamp writespace, which sizes the copy, to sndbuf_desc->len, negative
> values included, so the copy stays in the send buffer.

[Severity: Low]
Should the changelog also say what this patch leaves unfixed?

The notes below the --- line say two things. The over-credit is not undone,
and send() can still overwrite data in the buffer that the peer has not
received yet. Those notes are dropped when the patch is applied.

The patch has Cc: stable and the two Fixes: tags. Without the notes, the
permanent log suggests the failover and DMB merge accounting problem is
fixed. Only its out-of-bounds symptom in smc_tx_sendmsg() is.

Later in the series, "net/smc: bound the link failover cursor advance to the
send buffer" bounds the smc_switch_cursor() catch-up credit. Nothing in the
series changes the late-completion credit in smc_cdc_tx_handler():

net/smc/smc_cdc.c:smc_cdc_tx_handler() {
    ...
		atomic_add(diff, &cdcpend->conn->sndbuf_space);
		/* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */
		smp_mb__after_atomic();
		smc_curs_copy(&conn->tx_curs_fin, &cdcpend->cursor, conn);
    ...
}

The diff_tx credit in smc_cdc_msg_recv_action() is also left alone.

Could the --- note be moved into the commit message, so stable backporters
can see the remaining risk?

> diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c
> index 0f0c6df3bf2ce..3913ef103b356 100644
> --- a/net/smc/smc_tx.c
> +++ b/net/smc/smc_tx.c
> @@ -233,6 +233,14 @@ int smc_tx_sendmsg(struct smc_sock *smc, struct msghdr *msg, size_t len)
>  		/* initialize variables for 1st iteration of subsequent loop */
>  		/* could be just 1 byte, even after smc_tx_wait above */
>  		writespace = atomic_read(&conn->sndbuf_space);
> +		/* sndbuf_space can be out of range: late completions of an
> +		 * old SMC-R link credit it again after a failover, and with
> +		 * SMC-D DMB merge it follows the peer's consumer cursor.
> +		 * Bound writespace, which sizes the copy; a negative value
> +		 * would become huge in the size_t min_t() below.
> +		 */
> +		if (writespace < 0 || writespace > conn->sndbuf_desc->len)
> +			writespace = conn->sndbuf_desc->len;

[Severity: High]
This is a pre-existing issue, but this clamp limits the copy to the size of
the ring, not to the free space left in it. The free space is len minus the
bytes between tx_curs_fin and tx_curs_prep.

Can stream data still be overwritten in smc_tx_sendmsg() once sndbuf_space
has been over-credited?

The only decrement is the atomic_sub(copylen, &conn->sndbuf_space) further
down. It removes exactly what was copied, so any excess X carries over to
the next loop iteration and to the next sendmsg().

For example, with prep == fin and sndbuf_space == len + X:

  iteration 1: copylen = len, sndbuf_space becomes X
               nothing is posted if the peer RMB is full or the
               socket is corked
  iteration 2: !atomic_read(&conn->sndbuf_space) is false, so no wait
               memcpy_from_msg() writes X bytes at
               tx_cnt_prep == fin.count

The second copy lands on data that has not been sent or confirmed yet.
sendmsg() still reports every byte as sent.

For SMC-R, the old link stays alive during failover:

  smcr_link_down()
    smc_switch_conns()
      smc_switch_cursor()  <- under conn->send_lock
    ...
    smcr_link_clear()      <- old QP destroyed only here

Until smcr_link_clear() runs, successful CQEs from the old link still reach
smc_cdc_tx_handler(). That handler runs under bh_lock_sock only. It copies
cdcpend->cursor into tx_curs_fin without checking that the cursor is newer.

The range [tx_curs_fin, tx_curs_sent) can still be the source of RDMA WRITE
WRs posted on the old link. Does that mean the CPU write races with the HCA
read?

With SMC-D DMB merge the sndbuf is the peer DMB. The same copy would then
overwrite peer data that has not been consumed yet.

A negative sndbuf_space is mapped to len here. smc_tx_wait() also treats
any non-zero value as writable:

net/smc/smc_tx.c:smc_tx_wait() {
    ...
		if (atomic_read(&conn->sndbuf_space) && !conn->urg_tx_pend)
			break; /* at least 1 byte of free & no urgent data */
    ...
}

So a negative counter never blocks the sender either.

Once prep has lapped sent, smc_curs_diff() in smc_tx_rdma_writes() also
gets the wrong to_send:

  to_send = smc_curs_diff(conn->sndbuf_desc->len, &sent, &prep);

Bytes could then be skipped or sent twice.

The --- note says this is known. "net/smc: bound the link failover cursor
advance to the send buffer" only bounds the smc_switch_cursor() catch-up.
Nothing in the series changes the credit in smc_cdc_tx_handler().

Would it make sense to also bound the copy by the real free ring space? The
other option is a tx_curs_fin update that only moves forward and survives
the u16 wrap. Either one would extend this clamp, not replace it.

[ ... ]

> @@ -278,7 +286,7 @@ int smc_tx_sendmsg(struct smc_sock *smc, struct msghdr *msg, size_t len)
>  		/* increased in send tasklet smc_cdc_tx_handler() */
>  		smp_mb__before_atomic();
>  		atomic_sub(copylen, &conn->sndbuf_space);
> -		/* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */
> +		/* can be out of range, see writespace above */

[Severity: Low]
This comment now says sndbuf_space can be out of range. Three credit sites
in net/smc/smc_cdc.c still carry the old text: smc_cdc_tx_handler(),
smcd_cdc_msg_send() and smc_cdc_msg_recv_action(). For example:

net/smc/smc_cdc.c:smc_cdc_msg_recv_action() {
    ...
			atomic_add(diff_tx, &conn->sndbuf_space);
			/* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */
    ...
}

Should these be updated too? smc_cdc_tx_handler() and
smc_cdc_msg_recv_action() are the two sites that the new comment in
smc_tx_sendmsg() blames for the over-credit.

Earlier in this series, "net/smc: bound the receive length to the RMB"
changed the matching bytes_to_rcv comment in smc_cdc_msg_recv_action() to
"can be out of range, see smc_rx_data_available()".

With both versions in the tree, someone reading only smc_cdc.c could take
the clamp for dead code and remove it.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-b4-disp-33001ace-v8-0-f9bb96e17827%40proton.me

Re: [PATCH net v8 4/4] net/smc: bound the link failover cursor advance to the send buffer

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch adds a cons.count < peer_rmbe_size gate to
  smc_switch_cursor().
- [Medium] The new bound in smc_switch_cursor(), diff <=
  smc_curs_diff(len, &tx_curs_sent, &prep), is computed from a snapshot…
- [Low] The new comment in smc_switch_cursor()
  (net/smc/smc_core.c:1050-1054) says the catch-up runs 'only for a…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the catch-up
  decision in smc_switch_cursor() uses smc_curs_comp(), which compares…
- [High] This problem wasn't introduced by this patch, but
  smc_switch_cursor() reads and updates conn->tx_curs_fin,…

--

net/smc: bound the link failover cursor advance to the send buffer

On SMC-R link failover, smc_switch_cursor() advances the send and producer
cursors by the distance the peer's consumer cursor is ahead of the last
confirmed producer cursor. This patch performs that catch-up only when the
peer's consumer cursor count lies inside the peer RMB. It also caps the
advance at tx_curs_prep, so the RDMA source stays inside the send buffer.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index fe6e4a2f7935a..42d83a002ffb3 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c

[ ... ]

> @@ -1043,12 +1043,18 @@ static int smc_switch_cursor(struct smc_sock *smc, struct smc_cdc_tx_pend *pend,
>  	/* set prod cursor to old state, enforce tx_rdma_writes() */
>  	smc_curs_copy(&conn->local_tx_ctrl.prod, &fin, conn);
>  	smc_curs_copy(&cons, &conn->local_rx_ctrl.cons, conn);
> -
> -	if (smc_curs_comp(conn->peer_rmbe_size, &cons, &fin) < 0) {
> -		/* cons cursor advanced more than fin, and prod was set
> -		 * fin above, so now prod is smaller than cons. Fix that.
> +	smc_curs_copy(&prep, &conn->tx_curs_prep, conn);
> +
> +	if (cons.count < conn->peer_rmbe_size &&

[Severity: Medium]

What happens here when the peer sends a consumer cursor with count >=
peer_rmbe_size and a wrap ahead of fin?

The catch-up is skipped and local_tx_ctrl.prod stays at fin. The forged
cons is still used right after the block:

	/* recalculate, value is used by tx_rdma_writes() */
	atomic_set(&smc->conn.peer_rmbe_space, smc_write_space(conn));

In smc_write_space(), cons.wrap != prod.wrap, so:

	space = buffer_len - smc_curs_diff(buffer_len, &cons, &prod);

Here smc_curs_diff() computes (size - cons.count) + prod.count in unsigned
arithmetic. With cons.count >= size, max_t() clamps that to 0, or it is
small. That leaves peer_rmbe_space at roughly peer_rmbe_size.

smc_tx_rdma_writes() then takes the wrapped branch:

	dst_off = prod.count;
	if (prod.wrap == cons.wrap) {
		...
	} else {
		...
		dst_len = len;
	}

len can be up to peer_rmbe_size. The WRITE that smc_tx_rdma_write() posts
would then end prod.count bytes past the peer RMBE.

When the peer HCA refuses it, the QP goes into the error state and the link
goes down. The next smc_switch_conns()->smc_switch_cursor() sees the same
forged local_rx_ctrl.cons and an unchanged local_tx_ctrl_fin. It posts the
same oversized WRITE on the alternate link, and this repeats until the link
group is terminated. The peer can also force the failovers itself, for
example with LLC DELETE LINK.

Before this patch, the same input moved prod onto the forged count,
smc_write_space() returned 0, and only that connection stalled. That
matches the second measurement in the notes below the --- line.

Should the skip path also avoid sizing peer_rmbe_space from this cons?

> +	    smc_curs_comp(conn->peer_rmbe_size, &cons, &fin) < 0) {

[Severity: High]

This is a pre-existing issue that was not introduced by this patch.
smc_curs_comp() compares the u16 wrap values as plain numbers:

	if (old->wrap > new->wrap ||
	    (old->wrap == new->wrap && old->count > new->count))
		return -smc_curs_diff(size, new, old);

A wrap rollover from 65535 to 0 is a normal state. smc_curs_add() does
wrap++ on a u16, and smc_cdc_cursor_to_host() accepts temp.wrap == 0.

Can this misorder cons and fin around the rollover?

Case 1: cons is ahead across the rollover. For example, peer_rmbe_size is
16384, fin=(65535,12288) and cons=(0,4096). smc_curs_comp() returns +24576,
so the catch-up is skipped. smc_write_space() then returns 16384 - 24576 =
-8192, and smc_tx_rdma_writes() returns early on rmbespace <= 0. The
connection stalls, even with a conforming peer.

Case 2: cons is correctly behind fin, with fin=(0,y), cons=(65535,x) and
x >= y. smc_curs_comp(R, &cons, &fin) returns -(R - y + x), which is
negative, and cons.count < R passes the new check. With data pending,
diff = min(R - y + x, prep - sent) > 0.

The four cursors tx_curs_sent, tx_curs_fin, local_tx_ctrl.prod and
local_tx_ctrl_fin all move forward. sndbuf_space is credited for bytes the
peer never consumed.

The data in [fin, fin + diff) is never re-sent, but prod tells the peer
that it is present. If diff is larger than the peer's free space (x - y),
the peer's bytes_to_rcv exceeds its RMB. Would the peer application then
read stale RMB contents as stream data?

The min() added here keeps the source inside the send buffer. It does not
fix this ordering problem.

> +		/* cons cursor advanced more than fin, and prod was set fin
> +		 * above, so now prod is smaller than cons. cons comes off the
> +		 * wire: catch up only for a count inside the peer's RMB and no
> +		 * further than the send buffer holds, else that peer stalls.
>  		 */

[Severity: Low]

Is "else that peer stalls" accurate for the cons.count >= peer_rmbe_size
case? As described above, that path leaves peer_rmbe_space at about
peer_rmbe_size. The next write overruns the peer RMBE and the link goes
down. That is also the outcome the notes below the --- line report for the
second scenario.

When the send buffer limit in the min() is the smaller value, prod can
also still be behind cons after this block. That doesn't fit the "now prod
is smaller than cons" framing either.

The commit message has the same issue:

  The destination side is not bounded here: smc_write_space()
  still sizes the window from the peer's cursor, so a forged one can take
  the next write past the peer's own RMB, where its HCA refuses it and
  the link goes down.

For the forged count >= peer_rmbe_size input, the write past the RMB is
new with this patch. Before it, the connection stalled. Could the wording
say so instead of "still"?

> -		diff = smc_curs_diff(conn->peer_rmbe_size, &fin, &cons);
> +		diff = min(smc_curs_diff(conn->peer_rmbe_size, &fin, &cons),
> +			   smc_curs_diff(conn->sndbuf_desc->len,
> +					 &conn->tx_curs_sent, &prep));
>  		smc_curs_add(conn->sndbuf_desc->len,
>  			     &conn->tx_curs_sent, diff);
>  		smc_curs_add(conn->sndbuf_desc->len,

[Severity: High]

This is a pre-existing issue that was not introduced by this patch. The
rest of the block updates shared connection state:

net/smc/smc_core.c:smc_switch_cursor() {
    ...
		smc_curs_add(conn->sndbuf_desc->len,
			     &conn->tx_curs_fin, diff);

		smp_mb__before_atomic();
		atomic_add(diff, &conn->sndbuf_space);
		smp_mb__after_atomic();
    ...
}

smc_switch_conns() holds only conn->send_lock here. smc_cdc_tx_handler()
updates the same fields under bh_lock_sock() only:

	bh_lock_sock(&smc->sk);
	if (!wc_status && sndbuf_desc) {
		diff = smc_curs_diff(sndbuf_desc->len,
				     &cdcpend->conn->tx_curs_fin,
				     &cdcpend->cursor);
		...
		atomic_add(diff, &cdcpend->conn->sndbuf_space);
		...
		smc_curs_copy(&conn->tx_curs_fin, &cdcpend->cursor, conn);
		smc_curs_copy(&conn->local_tx_ctrl_fin, &cdcpend->p_cursor,
			      conn);

Late successful completions on the old link can run during the switch.
smc_llc_delete_asym_link() calls smc_switch_conns() before
smc_wr_tx_wait_no_pending_sends(). smc_wr_tx_process_cqe() calls the
handler without checking the link state.

Can this credit sndbuf_space twice? Take a sndbuf len of 64K, tx_curs_fin
0, tx_curs_sent 32K, tx_curs_prep 48K and cons 32K:

  smc_switch_cursor():   tx_curs_sent = 0, diff = 32K
  smc_cdc_tx_handler():  sndbuf_space += 32K, tx_curs_fin = 32K
  smc_switch_cursor():   tx_curs_fin += 32K -> (1,0), sndbuf_space += 32K

sndbuf_space ends at 80K, but only 48K is actually free. smc_tx_sendmsg()
clamps to 64K and copies 64K starting at prep 48K. That overwrites the
unsent bytes at [32K,48K) before they are RDMA-written.

smc_curs_add() also does a non-atomic read-modify-write of count and wrap,
so it can tear against the handler's smc_curs_copy().

[Severity: Medium]

The new limit is computed from the tx_curs_sent copy taken from
tx_curs_fin at the top of smc_switch_cursor(). diff is then added to the
live conn->tx_curs_fin and conn->local_tx_ctrl_fin, which
smc_cdc_tx_handler() can rewrite at the same time, as described above.

Does the limit still hold under that race? With a single racing
completion, the source does stay in bounds. A chain of events can still
get past it:

  failover 1 + race: tx_curs_fin ends one wrap ahead of prep, with
    count < prep.count
  failover 2: smc_curs_diff(len, &tx_curs_sent, &prep) sees different
    wraps and returns more than len; a forged cons with count <
    peer_rmbe_size and a wrap ahead passes the check, so diff2 > len
  race 2: the handler sets tx_curs_fin = prep before smc_curs_add(), so
    tx_curs_fin.count = prep.count + diff2 - len >= len
  failover 3: tx_curs_fin is copied into tx_curs_sent and the catch-up
    is skipped

smc_tx_rdma_writes() then uses sent.count >= sndbuf_desc->len as the
source offset:

		src_len = conn->sndbuf_desc->len - sent.count;

This needs two precisely timed late completions in successive failovers
plus a forged cursor, so it is unlikely. It does break the source-side
limit the commit message describes.

Would it help to derive tx_curs_fin and local_tx_ctrl_fin from the advanced
local copies, or to serialize against smc_cdc_tx_handler()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-b4-disp-33001ace-v8-0-f9bb96e17827%40proton.me