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

[PATCH net v7 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 and destination lie past the send buffer and the peer's RMB. 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.

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.
Logs on request.

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/

---
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/smc.h      |  3 +++
 net/smc/smc_cdc.h  | 17 ++++++++++++++---
 net/smc/smc_core.c | 15 +++++++++++----
 net/smc/smc_rx.c   |  7 ++++++-
 net/smc/smc_rx.h   | 11 ++++++++++-
 net/smc/smc_tx.c   |  8 ++++++++
 6 files changed, 52 insertions(+), 9 deletions(-)
---
base-commit: af32da41b0327b9c6a37856ba82b6760d6c8d10e
change-id: 20261013-b4-disp-f345196c-2add58b30202

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

[PATCH net v7 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 v7 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.

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 and the wait predicates. 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/smc.h      |  3 +++
 net/smc/smc_core.c |  1 +
 net/smc/smc_rx.c   |  7 ++++++-
 net/smc/smc_rx.h   | 11 ++++++++++-
 4 files changed, 20 insertions(+), 2 deletions(-)

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_core.c b/net/smc/smc_core.c
index 9974149659c2..8829517299de 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -2503,6 +2503,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..c968ac874d8b 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);
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

Re: [PATCH net v7 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: 1 (1) · Low: 0 (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] In smc_rx_recvmsg() (net/smc/smc_rx.c), a wrapped read copies in
  two chunks.
- [Medium] The patch makes smc_rx_data_available() the bounded readiness
  measure: at most rmb_len - splice_pending - peeked, never negative.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  smc_rx_recvmsg() ignores a short positive return from smc_rx_splice().
- [High] This problem wasn't introduced by this patch, but recv() on an
  SMC socket can loop in the kernel indefinitely, ignoring MSG_DONTWAIT,…
- [Medium] This problem wasn't introduced by this patch, but a
  MSG_PEEK|MSG_WAITALL reader keeps peeked_bytes in a local variable…
- [Low] Two existing comments state '/* guarantee 0 <= bytes_to_rcv <=
  rmb_desc->len */': one at net/smc/smc_rx.c:514 in smc_rx_recvmsg(),…

--

net/smc: bound the receive length to the RMB

This patch clamps smc_rx_data_available() so it never reports more than
fits in the RMB behind the spliced and peeked bytes, and never a negative
count. It caches the RMB length in conn->rmb_len for the unlocked wait
predicates, and reorders smc_rx_pipe_buf_release() so released bytes leave
splice_pending before the consumer cursor moves.

> diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
> index 197fddc6271f0..c968ac874d8b9 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,

[ ... ]

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

[Severity: High]
Can spliced bytes still be counted twice through the error path in
smc_rx_recvmsg()? A read that wraps the RMB end is copied in two chunks.
If chunk 0 succeeds and chunk 1 fails, the accounting for chunk 0 is
skipped:

net/smc/smc_rx.c:smc_rx_recvmsg() {
    ...
				if (rc < 0) {
					if (!read_done)
						read_done = -EFAULT;
					goto out;
				}
    ...
			atomic_sub(copylen, &conn->bytes_to_rcv);
    ...
}

By that point smc_rx_splice() has already done atomic_add(bytes,
&smc->conn.splice_pending) for chunk 0.

This looks easy to reach. do_splice_read() limits len to
p_space << PAGE_SHIFT. On SMC-D or physically contiguous SMC-R, each chunk
is a single pipe buffer. With one free pipe slot and data that straddles
the RMB end, chunk 0 takes the slot. splice_to_pipe() then returns -EAGAIN
for chunk 1 because the pipe is full.

When the pipe drains, smc_rx_pipe_buf_release() removes chunk 0 from
splice_pending and advances cons. bytes_to_rcv still includes chunk 0. The
next read sizes readable from a bytes_to_rcv that is too large by chunk 0's
length. It returns stale RMB bytes beyond prod and leaves the consumer
cursor sent to the peer ahead of the producer. Every later read is shifted
by the same amount.

The recvmsg path has a similar problem. If memcpy_to_msg() succeeds for
chunk 0 and faults on chunk 1, read_done is returned but the bytes are
never consumed, so the next recv() returns them again.

The commit message says this ordering ensures "no reader counts them
twice". The notes say the old order made "the next splice() returned stale
bytes in place of unread data". Is that still possible through this goto
out path?

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. Does
smc_rx_recvmsg() handle a short positive return from smc_rx_splice()?

For SMC-R with virtually contiguous RMBs (rmb_desc->is_vm),
smc_rx_splice() builds one buffer per page spanned:

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

do_splice_read() caps len at p_space << PAGE_SHIFT. That cap counts bytes,
not pages spanned. When the RMB offset is not page-aligned, a full-length
chunk spans p_space + 1 pages, so splice_to_pipe() inserts only p_space
buffers. For example, len 4096 at offset 100 with one free slot inserts
3996 bytes.

smc_rx_splice() adds only that short count to splice_pending. The caller
only checks rc < 0 and then accounts the full chunk:

			read_remaining -= chunk_len;
			read_done += chunk_len;
    ...
			atomic_sub(copylen, &conn->bytes_to_rcv);

Wouldn't splice() then report more bytes than actually reached the pipe?
And wouldn't bytes_to_rcv stay low by (copylen - rc) for good?

Repeated short splices make the deficit grow. The stranded bytes are never
delivered: at EOF, recvmsg sees bytes_to_rcv == 0 with RCV_SHUTDOWN and
returns 0 while data is still in the RMB.

[ ... ]

> diff --git a/net/smc/smc_rx.h b/net/smc/smc_rx.h
> index 994f5e42d1ba2..6bf44e3cc33a0 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

[Severity: Low]
This is a pre-existing issue, not one introduced by this patch. The new
comment contradicts two existing comments on the same field:

net/smc/smc_rx.c:smc_rx_recvmsg() {
    ...
			atomic_sub(copylen, &conn->bytes_to_rcv);
			/* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */
    ...
}

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

Neither site enforces that bound. Could those two comments be corrected
along with this one?

> +	 * 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;

[Severity: Medium]
Now that this helper is bounded, should the other readiness checks use it
too? Several sites still read the raw counter:

net/smc/af_smc.c:smc_poll() {
    ...
			if (atomic_read(&smc->conn.bytes_to_rcv))
				mask |= EPOLLIN | EPOLLRDNORM;
    ...
}

net/smc/af_smc.c:smc_ioctl() {
    ...
			answ = atomic_read(&smc->conn.bytes_to_rcv);
    ...
}

The defer-accept precheck in smc_accept() does the same:

		} else if (!atomic_read(&smc_sk(nsk)->conn.bytes_to_rcv)) {

The notes say wrap bumps from the peer can still grow bytes_to_rcv. If it
goes negative, smc_rx_data_available() returns 0 and a MSG_DONTWAIT recv()
gets -EAGAIN from smc_rx_recvmsg(). smc_poll() keeps reporting EPOLLIN
for the same socket. Before this patch a negative result was truthy, so
poll and recvmsg agreed.

Can a level-triggered epoll loop now spin on such a socket? SIOCINQ and
FIONREAD would also return a negative or larger-than-RMB value to
userspace.

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. The clamp
keeps the MSG_PEEK|MSG_WAITALL case inside the RMB, but the peeked data can
still have a gap in it.

peeked_bytes is a local that survives smc_rx_wait(), which drops the
socket lock. Suppose another recv() consumes M bytes in that window. The
peeker then resumes from the moved cursor plus the stale count:

		smc_curs_copy(&cons, &conn->local_tx_ctrl.cons, conn);
		if ((flags & MSG_PEEK) && peeked_bytes)
			smc_curs_add(conn->rmb_desc->len, &cons, peeked_bytes);

Doesn't the peeking reader then skip M bytes it never saw? TCP catches
this by comparing peek_seq with copied_seq and resyncing, and there is no
matching check here.

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. Can recv()
loop in the kernel indefinitely when unread data and unreleased spliced
bytes coexist?

Say part of the data is spliced into a pipe that nobody drains. Then
splice_pending > 0 and bytes_to_rcv > 0, and smc_rx_data_available() stays
positive as long as splice_pending < rmb_len. The loop in
smc_rx_recvmsg() then runs:

		if (smc_rx_recvmsg_data_available(smc, peeked_bytes))
			goto copy;
    ...
copy:
    ...
		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;
		}

The goto skips the !timeo and signal_pending() checks. The return value of
smc_rx_wait() is ignored, and read_remaining does not change. So this
repeats until the pipe is drained or conn->killed is set.

With MSG_DONTWAIT, each pass sleeps about one jiffy instead of returning
-EAGAIN. With a fatal signal pending, or once sk_err or RCV_SHUTDOWN makes
the wait condition true, smc_rx_wait() returns at once. The task then
spins at full CPU and never reaches a signal check.

This patch narrows the window to 0 < splice_pending < rmb_len but does not
close it.

>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013-b4-disp-f345196c-v7-0-ef3dddfbfa68%40proton.me

[PATCH net v7 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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c
index 3144b4b1fe29..58d9dfdcdf33 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 */

-- 
2.56.0

Re: [PATCH net v7 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 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] smc: flow control bypass and data corruption on negative
  sndbuf_space
- [Low] The new comment in smc_tx_sendmsg() (net/smc/smc_tx.c:236-240)
  says sndbuf_space 'can be out of range' because of late SMC-R…

Pre-existing issues:
- [High] The clamp bounds the size of each copy, not the flow control.
- [High] smc_switch_cursor() (net/smc/smc_core.c:1033) reads tx_curs_fin
  and local_tx_ctrl_fin, rewrites tx_curs_sent, tx_curs_fin and…

--

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

The patch clamps writespace in smc_tx_sendmsg() to sndbuf_desc->len. It
also maps negative values to len. This keeps the copy inside the send
buffer when conn->sndbuf_space has been over-credited, either after an
SMC-R link failover or through the SMC-D DMB merge path.

> diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c
> index 3144b4b1fe290..58d9dfdcdf33e 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.

[Severity: Low]
This isn't a bug, but the new comment says sndbuf_space can be out of
range. A few lines further down in the same loop, the existing comment
still says the opposite:

net/smc/smc_tx.c:smc_tx_sendmsg() {
    ...
	atomic_sub(copylen, &conn->sndbuf_space);
	/* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */
	smp_mb__after_atomic();
    ...
}

The same "guarantee" comment also appears in the credit paths that the
new comment names: smc_cdc_tx_handler(), smcd_cdc_msg_send() and the DMB
merge branch of smc_cdc_msg_recv_action().

The clamped subtraction doesn't bring the value back into that range. If
sndbuf_space starts at 3 * len, subtracting copylen <= len leaves at
least 2 * len. A negative value is mapped to len and then pushed further
below zero.

Should those comments be updated or dropped so they don't contradict the
new one?

> +		 * 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]
Mapping a negative writespace to sndbuf_desc->len is new behaviour from
this patch. It turns a negative sndbuf_space into a full-ring copy on
every pass of the loop. Is that intended?

The gate at the top of the loop treats only zero as no space:

	if (!atomic_read(&conn->sndbuf_space) || conn->urg_tx_pend) {

A negative sndbuf_space is non-zero, so it skips smc_tx_wait()
entirely. The new check then sets writespace to sndbuf_desc->len, so
copylen can be a whole ring.

memcpy_from_msg() writes that at tx_curs_prep. It overwrites whatever
is still unsent in [tx_curs_sent, tx_curs_prep), and whatever is still
unconfirmed in [tx_curs_fin, tx_curs_sent) that an in-flight RDMA write
may still be reading.

The atomic_sub(copylen, &conn->sndbuf_space) afterwards pushes the
value further below zero. This path can therefore never bring it back
to zero. The loop keeps copying until the whole message is consumed,
with no back-pressure. That lasts until CDC completions happen to
credit sndbuf_space back above zero.

smc_tx_wait(), smc_tx_should_cork() and smc_poll() use the same
non-zero test. While the space is negative, a blocked writer would be
woken, corking would still apply, and a poller would see EPOLLOUT.

The note below the --- line says 0 was avoided because the send loop
would spin. That is true with the current !atomic_read() gate. Would it
be better to make the gate treat sndbuf_space <= 0 as no space, along
with the matching checks in smc_tx_wait(), smc_tx_should_cork() and
smc_poll()? A negative value would then block the sender instead of
letting it overwrite data the peer has not received.

Before this patch a negative value caused an out-of-bounds write. With
it, the result is silent stream corruption that sendmsg() reports as
success.

[Severity: High]
This is a pre-existing issue, and this patch didn't introduce it. The
clamp limits the size of each copy but doesn't fix the flow control.
While sndbuf_space is over-credited, can smc_tx_sendmsg() still overwrite
data in the send buffer that hasn't been sent or confirmed yet?

The wait check at the top of the loop counts any non-zero value as free
space:

	if (!atomic_read(&conn->sndbuf_space) || conn->urg_tx_pend) {

smc_tx_wait(), smc_tx_should_cork() and smc_poll() make the same check.
Each pass then copies up to sndbuf_desc->len bytes at tx_curs_prep and
subtracts only copylen. A value of 3 * len stays positive for two more
passes. A negative value never reaches zero.

That lets tx_curs_prep run more than a full ring past tx_curs_sent and
tx_curs_fin. The copy then overwrites unsent bytes in
[tx_curs_sent, tx_curs_prep), plus bytes in [tx_curs_fin, tx_curs_sent)
that in-flight RDMA writes may still be reading.

smc_tx_rdma_writes() then computes:

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

That value is now larger than len, so the overwritten ring contents get
sent. sendmsg() reports success, but the peer receives a corrupted
stream.

One trigger is a peer DELETE LINK while the old QP still works:

smc_llc_process_cli_delete_link()
  smc_switch_conns(lgr, lnk_del, false)

A CDC completion still pending on the old link is later handled by
smc_wr_tx_process_cqe()->smc_cdc_tx_handler() with a stale
cdcpend->cursor. If tx_curs_fin is one wrap ahead, smc_curs_diff()
returns nearly 2 * len and tx_curs_fin moves back. The next completion
then credits the same bytes again.

If corking with MSG_MORE or a full peer RMBE keeps tx_curs_sent from
moving between passes, the overwrite is guaranteed.

The note below the --- line already says that "send() can still
overwrite data in the buffer that the peer has not received yet". Before
this patch, the same over-credit caused an out-of-bounds write instead.
Is a fix for the credit in smc_cdc_tx_handler() planned as a follow-up?

[Severity: High]
This is also a pre-existing issue. What serializes smc_switch_cursor()
against smc_cdc_tx_handler()?

smc_switch_conns() holds only conn->send_lock around the switch:

	spin_lock_bh(&conn->send_lock);
	smc_switch_link_and_count(conn, to_lnk);
	rc = smc_switch_cursor(smc, pend, wr_buf);
	spin_unlock_bh(&conn->send_lock);

smc_switch_cursor() rewrites tx_curs_sent, tx_curs_fin,
local_tx_ctrl.prod and local_tx_ctrl_fin, and it adds to sndbuf_space.

smc_cdc_tx_handler() runs from the send CQ tasklet. It does a non-atomic
read, diff, add and copy while holding only bh_lock_sock():

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

Neither path takes the other's lock, and the tasklet can run on another
CPU in the middle of the switch. One possible interleaving:

CPU1 (smc_switch_cursor)
  smc_curs_copy(&conn->tx_curs_sent, &conn->tx_curs_fin, conn);
  smc_curs_copy(&fin, &conn->local_tx_ctrl_fin, conn);

CPU2 (smc_cdc_tx_handler)
  atomic_add(diff, &cdcpend->conn->sndbuf_space);
  smc_curs_copy(&conn->tx_curs_fin, &cdcpend->cursor, conn);

CPU1 (smc_switch_cursor, diff computed from the older fin)
  smc_curs_add(conn->sndbuf_desc->len, &conn->tx_curs_fin, diff);
  atomic_add(diff, &conn->sndbuf_space);

Does this credit the same bytes twice and leave tx_curs_fin ahead of
tx_curs_sent?

The patch bounds only the local writespace snapshot. The shared
sndbuf_space would stay inflated, so smc_tx_sendmsg() could still
overwrite unsent data. On SMC-R an RDMA write may still be using that
data. With SMC-D nocopy, the peer may still be reading it from its DMB.

>  		/* not more than what user space asked for */
>  		copylen = min_t(size_t, send_remaining, writespace);
>  		/* determine start of sndbuf */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013-b4-disp-f345196c-v7-0-ef3dddfbfa68%40proton.me

[PATCH net v7 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 however far 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 can take its source past the send buffer and its
destination past the peer's RMB.

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.

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.
---
 net/smc/smc_core.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 8829517299de..36aee89e5f5f 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);
+	smc_curs_copy(&prep, &conn->tx_curs_prep, conn);
 
-	if (smc_curs_comp(conn->peer_rmbe_size, &cons, &fin) < 0) {
+	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. Fix that.
+		 * 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 data in the send buffer.
 		 */
-		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 v7 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 2 potential
issue(s) to consider.

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

- [High] The fix stops a forged cursor from moving the RDMA source past
  the send buffer, but the destination can still run past the peer's RMB…
- [Low] After the patch, smc_switch_cursor() can leave local_tx_ctrl.prod
  behind local_rx_ctrl.cons.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  smc_switch_cursor() reads and changes tx_curs_fin, local_tx_ctrl_fin…
- [High] This problem wasn't introduced by this patch, but
  smcr_tx_rdma_writes() (net/smc/smc_tx.c:421-422) sizes the first…
- [Medium] This problem wasn't introduced by this patch, but the two
  consumers of the peer's wire consumer cursor now treat it differently.
- [Medium] This problem wasn't introduced by this patch, but
  smc_curs_comp() (net/smc/smc_cdc.h) compares the u16 wrap fields as…

--

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

This patch hardens smc_switch_cursor() against a forged peer consumer
cursor during SMC-R link failover. The catch-up advance now runs only when
cons.count is inside the peer RMB. It is also capped at the amount of data
between tx_curs_sent and tx_curs_prep.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 8829517299def..36aee89e5f5f7 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);
> +	smc_curs_copy(&prep, &conn->tx_curs_prep, conn);
>  
> -	if (smc_curs_comp(conn->peer_rmbe_size, &cons, &fin) < 0) {
> +	if (cons.count < conn->peer_rmbe_size &&

[Severity: High]
Does this fully close the destination side of the overrun that the commit
message describes? The new cons.count check only decides whether the
catch-up branch runs. The forged cursor is not rejected and stays in
conn->local_rx_ctrl.cons.

Right after the if-block, smc_switch_cursor() still does:

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

smc_write_space() computes the window from that same cursor:

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

Take peer_rmbe_size = 16384, fin/prod = {wrap 2, count 1000} and a forged
cons = {wrap 3, count 17000}. The branch is skipped because 17000 >= 16384.
In smc_curs_diff() the wraps differ, so (size - old->count) is computed as
unsigned and wraps around. The diff comes out as 384, which makes
peer_rmbe_space 16000.

smc_tx_rdma_writes() then sees prod.wrap != cons.wrap and takes the
single-chunk path:

		dst_len = len;

This write starts at dst_off = prod.count = 1000, and nothing limits it to
peer_rmbe_size - prod.count. Wouldn't that RDMA write cover bytes
1000..17000 of a 16384 byte peer RMB?

The receive path in smc_cdc_msg_recv_action() also credits
peer_rmbe_space from the same cursor. smcr_cdc_msg_to_host() imports cons
with max_count == 0:

	smc_cdc_cursor_to_host(&local->cons, &peer->cons, conn, 0);

So this looks reachable with or without failover.

Would it be better to validate the consumer cursor once at import (count
against peer_rmbe_size, plus a sane wrap delta)? Alternatively,
peer_rmbe_space and dst_len could be clamped to the peer RMB. A check
inside the catch-up branch alone doesn't seem to cover it.

[Severity: Medium]
This isn't a bug introduced by this patch, but the two users of the wire
consumer cursor now handle it differently. smc_switch_cursor() bounds its
use of local_rx_ctrl.cons. smc_cdc_msg_recv_action() in net/smc/smc_cdc.c
still credits the window without a limit:

	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);
		/* guarantee 0 <= peer_rmbe_space <= peer_rmbe_size */

A forged cons with the same wrap as prod and a large count pushes
peer_rmbe_space above peer_rmbe_size. smc_tx_rdma_writes() then uses
len = min(to_send, rmbespace). That can be larger than peer_rmbe_size when
the sndbuf is bigger than the peer RMB.

The second destination chunk in smcr_tx_rdma_writes() is then:

		dst_off = 0; /* modulo offset in RMBE ring buffer */
		dst_len = len - dst_len; /* remainder */

Can that second chunk run past the end of the peer RMB? Should the
receive path get the same bound as the failover path?

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

[Severity: Medium]
This is a pre-existing issue, but smc_curs_comp() in net/smc/smc_cdc.h
compares the u16 wrap fields as plain numbers:

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

smc_cdc_cursor_to_host() and smc_curs_diff_large() handle the 65535 to 0
wrap rollover explicitly, but smc_curs_comp() does not.

Take peer_rmbe_size = 16384, fin = {wrap 65535, count 12288} and a
legitimately advanced cons = {wrap 0, count 4096}. Here
smc_curs_comp(P, &cons, &fin) returns +24576, so the catch-up is skipped
even though cons is 8192 bytes ahead. smc_write_space() then returns
16384 - 24576 = -8192.

smc_tx_sndbuf_nonempty() then returns early:

	if (unlikely(atomic_read(&conn->peer_rmbe_space) <= 0)) {

If the peer has nothing left to consume, no new cons update arrives to
reopen the window. Can the connection stall permanently if failover hits
this window? Reaching it takes about 65536 wraps of the peer RMB before
the failover.

>  		/* cons cursor advanced more than fin, and prod was set
> -		 * fin above, so now prod is smaller than cons. Fix that.
> +		 * 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 data in the send buffer.
>  		 */
> -		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));

[Severity: Low]
Two cases leave local_tx_ctrl.prod behind local_rx_ctrl.cons: the prep
term of this min() wins, or the branch is skipped because
cons.count >= peer_rmbe_size. The comment above still says the catch-up
fixes prod < cons.

smc_write_space() assumes prod >= cons. Suppose cons is one wrap ahead,
for example cons = {w + 1, c} and prod = {w, p} with p > c. Then
smc_curs_diff(buffer_len, &cons, &prod) is larger than buffer_len, and the
returned space is c - p, which is negative. In the equal-wrap case
smc_curs_diff() clamps to 0, so space is just peer_rmbe_size.

A negative space breaks the "guarantee 0 <= peer_rmbe_space <=
peer_rmbe_size" comments in smc_cdc.c and smc_tx.c. Sending on the
connection also stops while peer_rmbe_space <= 0.

For a conforming peer neither the skip nor the cap should fire. So the
effect seems limited to a stall of the misbehaving peer's own connection.
Is this state intended, and should the comment be updated to match?

>  		smc_curs_add(conn->sndbuf_desc->len,
>  			     &conn->tx_curs_sent, diff);

[Severity: High]
This isn't a bug introduced by this patch, but the RDMA write takes its
offsets from these cursors. Is the source side of smcr_tx_rdma_writes() in
net/smc/smc_tx.c bounded for the second destination chunk?

		dst_off = 0; /* modulo offset in RMBE ring buffer */
		dst_len = len - dst_len; /* remainder */
		dst_len_sum += dst_len;
		src_len = min_t(int, dst_len, conn->sndbuf_desc->len -
				sent_count);

This uses the original sent_count rather than the current src_off.
smcd_tx_rdma_writes() uses src_off here:

		src_len = min_t(int, dst_len, conn->sndbuf_desc->len - src_off);

A conforming peer doesn't seem able to reach this. The SMC-R sndbuf and
RMB sizes are both 1 << (comp + 14), and tx_curs_sent and
local_tx_ctrl.prod advance together. It does become reachable once the
unbounded credit in smc_cdc_msg_recv_action() pushes peer_rmbe_space above
peer_rmbe_size.

Example: peer_rmbe_size = 16384, sndbuf_desc->len = 65536,
prod.count = sent.count = 4096, a forged same-wrap cons with a large
count, and to_send = 65536.

The first destination chunk has dst_len = 12288, and src_off becomes
16384. The second has dst_len = 53248 and
src_len = min(53248, 65536 - 4096) = 53248.

Wouldn't sge[0] then cover [16384, 69632)? That is 4096 bytes past the
end of the 65536 byte sndbuf, and sndbuf [0, 4096) is never sent.

For physically contiguous sndbufs without an IOMMU, could this send
adjacent kernel memory to the peer?

>  		smc_curs_add(conn->sndbuf_desc->len,

[Severity: High]
This is a pre-existing issue, but smc_switch_cursor() reads and updates
tx_curs_fin, local_tx_ctrl_fin and sndbuf_space while holding only
conn->send_lock. smc_switch_conns() takes that lock:

			spin_lock_bh(&conn->send_lock);
			smc_switch_link_and_count(conn, to_lnk);
			rc = smc_switch_cursor(smc, pend, wr_buf);

smc_cdc_tx_handler() updates the same fields from the send completion
tasklet. It holds only bh_lock_sock() and never takes send_lock:

	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_llc_delete_asym_link() calls smc_switch_conns() before it waits for
pending sends on the old link, and the DELETE_LINK path does the same:

	lnk_new = smc_switch_conns(lgr, lnk_asym, false);
	smc_wr_tx_wait_no_pending_sends(lnk_asym);

So CDC sends already posted on the old link can complete on another CPU
during the switch. One possible interleaving:

CPU0 smc_switch_cursor()
  tx_curs_sent = tx_curs_fin = F0
  fin = L0
                          CPU1 smc_cdc_tx_handler()
                            sndbuf_space += F1 - F0
                            tx_curs_fin = F1
CPU0
  tx_curs_fin += diff    /* now F1 + diff */
  sndbuf_space += diff

Doesn't that leave sndbuf_space overcounted by F1 - F0? Also, an old-link
completion that lands after the switch can move tx_curs_fin backwards, and
later completions would then credit the same bytes again.

The earlier patch in this series, "net/smc: bound the send length to the
send buffer in smc_tx_sendmsg()", clamps writespace to
sndbuf_desc->len. That keeps the copy inside the buffer.

Even so, can smc_tx_sendmsg() still overwrite unsent or unconfirmed data
between the real tx_curs_fin and tx_curs_prep?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013-b4-disp-f345196c-v7-0-ef3dddfbfa68%40proton.me

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.