[PATCH net 0/4] net/smc: fix the failover cursor switch and wrap rollover

[PATCH net 0/4] net/smc: fix the failover cursor switch and wrap rollover

[Bryam Vargas via B4 Relay]

smc_switch_cursor() runs at SMC-R link failover under send_lock only,
while smc_cdc_tx_handler() and the rx path update the same cursors under
the socket spinlock. Its catch-up orders the peer's consumer cursor
against local_tx_ctrl_fin with smc_curs_comp(), which misorders two
cursors across the u16 wrap rollover. The v9 cover of the CDC cursor
bounds series [1] lists the race against smc_cdc_tx_handler() and the
rollover as not fixed there; Sashiko's review of v8 [2] reported both.
The same switch has a third problem: a successful completion on the old
link handled after the switch still credits bytes that are being resent.

1/4 takes the socket spinlock around the switch. 2/4 tags each CDC pend
with a switch generation and credits a completion only from the current
generation and never backwards; both checks rely on 1/4's lock. 3/4
computes the catch-up from the signed cursor distance, bounds it by the
bytes sent and advances the RMB cursors across wraps; it uses
smc_curs_sdiff() from 2/4. 4/4 uses the same helper for the urgent mark
in smc_rx_update_consumer(), which has the same rollover problem.

Tested in an x86 KVM guest on a v7.4-rc2 KASAN kernel, with net/smc of
the base below built as a module, over soft-RoCE on a veth pair between
two network namespaces. Unless noted otherwise the RMB is 16 KiB, the
send buffer 64 KiB, and the failover is triggered by deleting an rdma
device. The module had test counters and test-only knobs, named where
they are used below. Three controls pass with and without the series: a
failover with no knob, one after 1 GiB with both cursors on the same
wrap (there the series is 1/4 to 3/4), and an urgent byte on the
reader's wrap. Leaving out 1/4, either of 2/4's two checks, 3/4 or 4/4
makes its test fail. No KASAN splat or WARN in any run. On a
PROVE_LOCKING build of the same kernel, a failover with no knob and the
first test below give no lockdep report with 1/4 to 3/4 (4/4 takes no
lock); with the two locks of 1/4 taken in the other order, lockdep
reports the inversion.

- Send buffer full. A completion handler held by a knob at its credit
  decision while a test hook takes the link down on another CPU, and a
  second knob holding the resend until the application has written: on
  net the switch runs while the handler is held, sndbuf_space ends 4096
  over the buffer size, and the application's new data replaces bytes
  still to be resent; the reader gets wrong data. With 1/4 the switch
  waits for the socket spinlock and the stream is intact.
- Two completions on the old link held back by a knob and handed back
  after the switch, send buffer full, the resend held as above: on net
  they credit 8192 bytes that are still to be resent, and the reader
  gets wrong data. With 2/4 they credit nothing.
- The peer shut down its write side, so the switch moves the link
  without a rewind; an old-link completion held back by a knob is
  handed back after one on the new link: on net tx_curs_fin moves back
  and sndbuf_space ends 4096 over the buffer size. With 2/4 nothing
  moves.
- 1 GiB through the RMB, so that at the failover local_tx_ctrl_fin is
  on wrap 0 and the peer's consumer cursor 12288 bytes behind it on
  wrap 65535: on net the catch-up runs, sndbuf_space goes 20480 over,
  an RDMA write is refused (remote access error) and the reader gets EOF
  65536 bytes short while every send() succeeded. Also on net, with a
  16 KiB send buffer and one 48 KiB send() after the failover, send()
  takes 36864 bytes and copies 12288 of them past the end of the send
  buffer (a test counter; KASAN does not see it, the buffer is
  page-allocated). With 3/4 there is no catch-up, all data arrives,
  and send() takes 16384.
- Old-link completions held back by a knob while the peer reads 24576
  bytes past local_tx_ctrl_fin, then the failover: on net the catch-up
  is 8192, 16384 short, and the connection stalls with no write space.
  With 3/4 it is 24576 and all data arrives.
- 1 GiB through the RMB with the reader stopped on wrap 65535 and an
  urgent byte sent so that it lands on wrap 0, no SO_OOBINLINE: on net
  recv(MSG_OOB) fails with EINVAL, the urgent byte is never consumed,
  and the sender, which waits for that, sends nothing more (its send()
  times out with 32768 bytes unsent). With 4/4 the byte is read OOB and
  the stream is intact.

The series is on net and does not depend on [1]. Patches 1/4, 2/4 and
4/4 apply on top of [1] as they are; 3/4 conflicts with [1] 6/6 in
smc_switch_cursor(), where 3/4's version replaces 6/6's: 3/4 bounds the
catch-up by the bytes sent, which is tighter than 6/6's bound,
tx_curs_prep (by inspection). With [1] alone, [1] 3/6 keeps send()
inside the send buffer, but the misordered catch-up still runs: it
advances the cursors by up to tx_curs_prep - tx_curs_fin bytes without
sending them on the new link and tells the peer they are in its RMB (by
inspection).

Not run, by inspection only: a CDC received during the switch (1/4), a
consumer cursor that is ahead across the rollover (3/4) and one past the
bytes sent, where 3/4's bound applies. Not run with 1/4: a completion
handled between the switch's first two cursor copies, which leaves
tx_curs_sent and the producer cursor out of step; [1] measured that case
without this series.

Test patch and logs on request.

[1] https://lore.kernel.org/all/20261112-b4-disp-0287c16e-v9-0-e5e8fa688177@proton.me/
[2] https://lore.kernel.org/all/179364387382.3131.13471253186660322977@kernel.org/

---
Bryam Vargas (4):
      net/smc: take the socket spinlock across the failover cursor switch
      net/smc: do not credit stale CDC completions after a link switch
      net/smc: fix the failover catch-up across the cursor wrap rollover
      net/smc: order the urgent mark across the cursor wrap rollover

 net/smc/smc.h      |  3 +++
 net/smc/smc_cdc.c  | 11 ++++++++++-
 net/smc/smc_cdc.h  | 28 +++++++++++++++++++---------
 net/smc/smc_core.c | 36 +++++++++++++++++++++++++-----------
 net/smc/smc_rx.c   |  4 ++--
 5 files changed, 59 insertions(+), 23 deletions(-)
---
base-commit: ead6fa9a0ab194f02a2f36d634b193e9c5f9a1c6
change-id: 20261113-b4-disp-1a9ab9b0-a4eac61f01b4

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

[PATCH net 1/4] net/smc: take the socket spinlock across the failover cursor switch

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

smc_switch_cursor() rewrites the send cursors, sndbuf_space and
peer_rmbe_space under send_lock only. smc_cdc_tx_handler() updates
tx_curs_fin, local_tx_ctrl_fin and sndbuf_space, and
smc_cdc_msg_recv_action() updates the peer's consumer cursor and
peer_rmbe_space, under bh_lock_sock(). A CDC completion handled during a
link switch can credit its bytes twice and leave tx_curs_fin ahead of
tx_curs_sent, or leave tx_curs_sent and the producer cursor out of step,
so the resend writes at the wrong RMB offset. A CDC received at the same
time can be counted twice in peer_rmbe_space.

Take bh_lock_sock() around the link change and the cursor switch, in
the order the receive path already uses (the socket spinlock, then
send_lock).

Fixes: c6f02ebeea3a ("net/smc: switch connections to alternate link")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-b4-disp-33001ace-v8-0-f9bb96e17827%40proton.me?part=4
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/smc/smc_core.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index a3164201c142..73e86100e6db 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1171,11 +1171,18 @@ struct smc_link *smc_switch_conns(struct smc_link_group *lgr,
 		/* pre-fetch buffer outside of send_lock, might sleep */
 		rc = smc_cdc_get_free_slot(conn, to_lnk, &wr_buf, NULL, &pend);
 		if (!rc) {
-			/* avoid race with smcr_tx_sndbuf_nonempty() */
-			spin_lock_bh(&conn->send_lock);
+			/* the CDC completion handler and the receive path
+			 * update the same cursors under the socket's spinlock,
+			 * smcr_tx_sndbuf_nonempty() under send_lock
+			 */
+			local_bh_disable();
+			bh_lock_sock(&smc->sk);
+			spin_lock(&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);
+			spin_unlock(&conn->send_lock);
+			bh_unlock_sock(&smc->sk);
+			local_bh_enable();
 		}
 		sock_put(&smc->sk);
 		if (rc)

-- 
2.56.0

[PATCH net 2/4] net/smc: do not credit stale CDC completions after a link switch

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

A successful completion on the old link handled after
smc_switch_cursor() credits bytes that were rewound for resend, so the
application can overwrite send buffer data before it is resent. A
connection in a closing state is moved to the new link without a rewind;
when completions on the two links are then handled out of order, one
whose cursor is behind tx_curs_fin moves tx_curs_fin back and the same
bytes are credited again, which can take sndbuf_space past the send
buffer size.

Tag each CDC pend with a switch generation that smc_switch_cursor()
bumps, and credit a completion only if it is of the current generation
and its cursor is not behind tx_curs_fin.

Fixes: c6f02ebeea3a ("net/smc: switch connections to alternate link")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/smc/smc.h      |  3 +++
 net/smc/smc_cdc.c  | 11 ++++++++++-
 net/smc/smc_cdc.h  | 13 +++++++++++++
 net/smc/smc_core.c |  1 +
 4 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/net/smc/smc.h b/net/smc/smc.h
index 36b243b34a9a..96f017938b9c 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -233,6 +233,9 @@ struct smc_connection {
 	atomic_t		sndbuf_space;	/* remaining space in sndbuf */
 	u16			tx_cdc_seq;	/* sequence # for CDC send */
 	u16			tx_cdc_seq_fin;	/* sequence # - tx completed */
+	u16			tx_switch_gen;	/* bumped when a link switch
+						 * rewinds the tx cursors
+						 */
 	spinlock_t		send_lock;	/* protect wr_sends */
 	atomic_t		cdc_pend_tx_wr; /* number of pending tx CDC wqe
 						 * - inc when post wqe,
diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
index 32d6d03df321..d1f346376c08 100644
--- a/net/smc/smc_cdc.c
+++ b/net/smc/smc_cdc.c
@@ -36,7 +36,15 @@ static void smc_cdc_tx_handler(struct smc_wr_tx_pend_priv *pnd_snd,
 	sndbuf_desc = conn->sndbuf_desc;
 	smc = container_of(conn, struct smc_sock, conn);
 	bh_lock_sock(&smc->sk);
-	if (!wc_status && sndbuf_desc) {
+	/* A CDC posted before a link switch rewound the cursors: its bytes
+	 * are resent on the new link or credited by the switch's catch-up.
+	 * A cursor behind tx_curs_fin (completions on two links in any
+	 * order) was credited already.
+	 */
+	if (!wc_status && sndbuf_desc &&
+	    cdcpend->switch_gen == conn->tx_switch_gen &&
+	    smc_curs_sdiff(sndbuf_desc->len, &conn->tx_curs_fin,
+			   &cdcpend->cursor) >= 0) {
 		diff = smc_curs_diff(sndbuf_desc->len,
 				     &cdcpend->conn->tx_curs_fin,
 				     &cdcpend->cursor);
@@ -107,6 +115,7 @@ static inline void smc_cdc_add_pending_send(struct smc_connection *conn,
 	pend->cursor = conn->tx_curs_sent;
 	pend->p_cursor = conn->local_tx_ctrl.prod;
 	pend->ctrl_seq = conn->tx_cdc_seq;
+	pend->switch_gen = conn->tx_switch_gen;
 }
 
 int smc_cdc_msg_send(struct smc_connection *conn,
diff --git a/net/smc/smc_cdc.h b/net/smc/smc_cdc.h
index 696cc11f2303..597acb2e123f 100644
--- a/net/smc/smc_cdc.h
+++ b/net/smc/smc_cdc.h
@@ -170,6 +170,18 @@ static inline int smc_curs_comp(unsigned int size,
 	return smc_curs_diff(size, old, new);
 }
 
+/* signed distance from old to new; the wrap difference is taken as s16,
+ * so it holds across the u16 wrap rollover, for cursors less than 32768
+ * rings apart
+ */
+static inline s64 smc_curs_sdiff(unsigned int size,
+				 union smc_host_cursor *old,
+				 union smc_host_cursor *new)
+{
+	return (s64)(s16)(new->wrap - old->wrap) * size +
+	       (s64)new->count - old->count;
+}
+
 /* calculate cursor difference between old and new, where old <= new and
  * difference may exceed size
  */
@@ -284,6 +296,7 @@ struct smc_cdc_tx_pend {
 	union smc_host_cursor	cursor;		/* tx sndbuf cursor sent */
 	union smc_host_cursor	p_cursor;	/* rx RMBE cursor produced */
 	u16			ctrl_seq;	/* conn. tx sequence # */
+	u16			switch_gen;	/* tx_switch_gen at send */
 };
 
 int smc_cdc_get_free_slot(struct smc_connection *conn,
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 73e86100e6db..fb9f6a4eb683 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1058,6 +1058,7 @@ static int smc_switch_cursor(struct smc_sock *smc, struct smc_cdc_tx_pend *pend,
 	int rc = 0;
 	int diff;
 
+	conn->tx_switch_gen++;	/* completions of earlier sends: no credit */
 	smc_curs_copy(&conn->tx_curs_sent, &conn->tx_curs_fin, conn);
 	smc_curs_copy(&fin, &conn->local_tx_ctrl_fin, conn);
 	/* set prod cursor to old state, enforce tx_rdma_writes() */

-- 
2.56.0

[PATCH net 3/4] net/smc: fix the failover catch-up across the cursor wrap rollover

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

smc_switch_cursor() orders the peer's consumer cursor against
local_tx_ctrl_fin with smc_curs_comp(), which misorders them across the
65535 -> 0 rollover of the u16 wrap. A consumer cursor behind reads as
ahead: the catch-up can credit sndbuf_space for bytes never sent and
tell the peer they are in its RMB, so the next send() can copy past the
end of the send buffer. One ahead reads as behind and the catch-up is
skipped. With a send buffer larger than the peer's RMB, the catch-up can
also be short once the peer is more than one RMB past local_tx_ctrl_fin,
and the connection then stalls.

Use the signed cursor distance, advance the RMB cursors across any
number of wraps, and bound the catch-up by the bytes sent, which a
peer's consumer cursor cannot legitimately pass.

Fixes: c6f02ebeea3a ("net/smc: switch connections to alternate link")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-b4-disp-33001ace-v8-0-f9bb96e17827%40proton.me?part=4
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/smc/smc_cdc.h  | 10 ++++++++++
 net/smc/smc_core.c | 22 ++++++++++++++--------
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/net/smc/smc_cdc.h b/net/smc/smc_cdc.h
index 597acb2e123f..4b586a20dfc2 100644
--- a/net/smc/smc_cdc.h
+++ b/net/smc/smc_cdc.h
@@ -97,6 +97,16 @@ static inline void smc_curs_add(int size, union smc_host_cursor *curs,
 	}
 }
 
+/* smc_curs_add() for a value that may span more than one ring */
+static inline void smc_curs_advance(unsigned int size,
+				    union smc_host_cursor *curs, u32 value)
+{
+	u32 count = curs->count + value;
+
+	curs->wrap += count / size;
+	curs->count = count % size;
+}
+
 /* Copy cursor src into tgt */
 static inline void smc_curs_copy(union smc_host_cursor *tgt,
 				 union smc_host_cursor *src,
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index fb9f6a4eb683..76a577a20e4a 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1054,22 +1054,28 @@ 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, sent;
 	int rc = 0;
-	int diff;
+	s64 diff;
 
 	conn->tx_switch_gen++;	/* completions of earlier sends: no credit */
+	smc_curs_copy(&sent, &conn->tx_curs_sent, conn);
 	smc_curs_copy(&conn->tx_curs_sent, &conn->tx_curs_fin, conn);
 	smc_curs_copy(&fin, &conn->local_tx_ctrl_fin, conn);
 	/* 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) {
+	/* not past what was sent, whatever cons says; that also keeps
+	 * diff within one send buffer for smc_curs_add()
+	 */
+	diff = min(smc_curs_sdiff(conn->peer_rmbe_size, &fin, &cons),
+		   smc_curs_sdiff(conn->sndbuf_desc->len,
+				  &conn->tx_curs_fin, &sent));
+	if (diff > 0) {
 		/* cons cursor advanced more than fin, and prod was set
 		 * fin above, so now prod is smaller than cons. Fix that.
 		 */
-		diff = smc_curs_diff(conn->peer_rmbe_size, &fin, &cons);
 		smc_curs_add(conn->sndbuf_desc->len,
 			     &conn->tx_curs_sent, diff);
 		smc_curs_add(conn->sndbuf_desc->len,
@@ -1079,10 +1085,10 @@ static int smc_switch_cursor(struct smc_sock *smc, struct smc_cdc_tx_pend *pend,
 		atomic_add(diff, &conn->sndbuf_space);
 		smp_mb__after_atomic();
 
-		smc_curs_add(conn->peer_rmbe_size,
-			     &conn->local_tx_ctrl.prod, diff);
-		smc_curs_add(conn->peer_rmbe_size,
-			     &conn->local_tx_ctrl_fin, diff);
+		smc_curs_advance(conn->peer_rmbe_size,
+				 &conn->local_tx_ctrl.prod, diff);
+		smc_curs_advance(conn->peer_rmbe_size,
+				 &conn->local_tx_ctrl_fin, diff);
 	}
 	/* recalculate, value is used by tx_rdma_writes() */
 	atomic_set(&smc->conn.peer_rmbe_space, smc_write_space(conn));

-- 
2.56.0

[PATCH net 4/4] net/smc: order the urgent mark across the cursor wrap rollover

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

smc_rx_update_consumer() orders the consumer cursor against urg_curs
with smc_curs_comp(), which misorders them across the 65535 -> 0
rollover of the RMB wrap. Without SO_OOBINLINE, with the reader on wrap
65535 and the urgent byte on wrap 0, the mark reads as already passed:
urg_state becomes SMC_URG_READ before the byte is reached and
recv(MSG_OOB) fails with -EINVAL. The urgent byte, which bytes_to_rcv
leaves out, is then never skipped, and the sender, which waits for its
urgent data to be consumed before it sends more, sends nothing more:
the connection stalls.

Use smc_curs_sdiff(), which orders the two cursors across the
rollover. smc_curs_comp() has no other user; remove it.

Fixes: de8474eb9d50 ("net/smc: urgent data support")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/smc/smc_cdc.h | 13 -------------
 net/smc/smc_rx.c  |  4 ++--
 2 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/net/smc/smc_cdc.h b/net/smc/smc_cdc.h
index 4b586a20dfc2..eecd0cc5bb79 100644
--- a/net/smc/smc_cdc.h
+++ b/net/smc/smc_cdc.h
@@ -167,19 +167,6 @@ static inline int smc_curs_diff(unsigned int size,
 	return max_t(int, 0, (new->count - old->count));
 }
 
-/* calculate cursor difference between old and new - returns negative
- * value in case old > new
- */
-static inline int smc_curs_comp(unsigned int size,
-				union smc_host_cursor *old,
-				union smc_host_cursor *new)
-{
-	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);
-}
-
 /* signed distance from old to new; the wrap difference is taken as s16,
  * so it holds across the u16 wrap rollover, for cursors less than 32768
  * rings apart
diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
index 63c178c10bf1..0345167cdc0d 100644
--- a/net/smc/smc_rx.c
+++ b/net/smc/smc_rx.c
@@ -68,8 +68,8 @@ static int smc_rx_update_consumer(struct smc_sock *smc,
 
 	/* did we process urgent data? */
 	if (conn->urg_state == SMC_URG_VALID || conn->urg_rx_skip_pend) {
-		diff = smc_curs_comp(conn->rmb_desc->len, &cons,
-				     &conn->urg_curs);
+		diff = smc_curs_sdiff(conn->rmb_desc->len, &cons,
+				      &conn->urg_curs);
 		if (sock_flag(sk, SOCK_URGINLINE)) {
 			if (diff == 0) {
 				force = true;

-- 
2.56.0

Re: [PATCH net 1/4] net/smc: take the socket spinlock across the failover cursor switch

[sashiko-bot]

> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> smc_switch_cursor() rewrites the send cursors, sndbuf_space and
> peer_rmbe_space under send_lock only. smc_cdc_tx_handler() updates
> tx_curs_fin, local_tx_ctrl_fin and sndbuf_space, and
> smc_cdc_msg_recv_action() updates the peer's consumer cursor and
> peer_rmbe_space, under bh_lock_sock(). A CDC completion handled during a
> link switch can credit its bytes twice and leave tx_curs_fin ahead of
> tx_curs_sent, or leave tx_curs_sent and the producer cursor out of step,
> so the resend writes at the wrong RMB offset. A CDC received at the same
> time can be counted twice in peer_rmbe_space.
> 
> Take bh_lock_sock() around the link change and the cursor switch, in
> the order the receive path already uses (the socket spinlock, then
> send_lock).
> 
> Fixes: c6f02ebeea3a ("net/smc: switch connections to alternate link")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-b4-disp-33001ace-v8-0-f9bb96e17827%40proton.me?part=4
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-b4-disp-1a9ab9b0-v1-0-4a7c8d85d849@proton.me?part=1

Re: [PATCH net 2/4] net/smc: do not credit stale CDC completions after a link switch

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] net/smc: non-atomic in-place cursor updates in
  smc_switch_cursor() [1]
--

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

net/smc: do not credit stale CDC completions after a link switch

This commit tags pending CDC transmissions with a link-switch generation
counter and checks the completion cursor against tx_curs_fin so stale or
out-of-order completions after a link switch do not credit send buffer space.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 73e86100e6db2..fb9f6a4eb6835 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -1058,6 +1058,7 @@ static int smc_switch_cursor(struct smc_sock *smc, struct smc_cdc_tx_pend *pend,
>  	int rc = 0;
>  	int diff;
>  
> +	conn->tx_switch_gen++;	/* completions of earlier sends: no credit */
>  	smc_curs_copy(&conn->tx_curs_sent, &conn->tx_curs_fin, conn);
>  	smc_curs_copy(&fin, &conn->local_tx_ctrl_fin, conn);
>  	/* set prod cursor to old state, enforce tx_rdma_writes() */

[Severity: Medium]
This problem wasn't introduced by this patch, but can a concurrent reader
observe a torn conn->tx_curs_sent when smc_switch_cursor() advances it in
place with smc_curs_add() [1]?

net/smc/smc_core.c:smc_switch_cursor() {
	...
	if (smc_curs_comp(conn->peer_rmbe_size, &cons, &fin) < 0) {
		diff = smc_curs_diff(conn->peer_rmbe_size, &fin, &cons);
		smc_curs_add(conn->sndbuf_desc->len,
			     &conn->tx_curs_sent, diff);
		smc_curs_add(conn->sndbuf_desc->len,
			     &conn->tx_curs_fin, diff);
	...
}

smc_switch_cursor() holds conn->send_lock, not the socket lock.  Unlike
smc_curs_copy(), smc_curs_add() mutates curs->count and curs->wrap in place
Across three steps:

CPU0                                  CPU1
smc_switch_cursor()                   smc_close_stream_wait()
  smc_curs_add(..., &conn->tx_curs_sent, diff)
    curs->count += value;
    curs->wrap++;
                                        smc_tx_prepared_sends()
                                          smc_curs_copy(&sent,
                                            &conn->tx_curs_sent, conn)
                                          smc_curs_diff(..., &sent, &prep)
                                            <-- torn cursor, returns 0
    curs->count -= size;

When conn->tx_curs_sent wraps to match conn->tx_curs_prep.wrap, a reader that
copies conn->tx_curs_sent before curs->count is reduced sees
sent.wrap == prep.wrap with sent.count > prep.count.  smc_curs_diff() then
clamps prep.count - sent.count to 0, which lets smc_close_stream_wait() stop
waiting for prepared sends and close the connection before queued data is
transmitted.

Advancing local cursor copies with smc_curs_add() and publishing them via
smc_curs_copy() would avoid exposing the intermediate state.

[1] https://sashiko.dev/#/bug/linux-9856806b-d08f-4d48-bb6c-9f1dcc135c1e

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-b4-disp-1a9ab9b0-v1-0-4a7c8d85d849@proton.me?part=2

Re: [PATCH net 3/4] net/smc: fix the failover catch-up across the cursor wrap rollover

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] net/smc: clamping diff in smc_switch_cursor() leaves prod
  behind cons
--

commit 232f7aaa1516220f36350d189111dfcee52e69da
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/smc: fix the failover catch-up across the cursor wrap rollover

This commit updates smc_switch_cursor() to compute the failover catch-up
distance using smc_curs_sdiff(), bound it by the number of sent bytes,
and advance the RMB cursors across multiple ring wraps.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index fb9f6a4eb6835..76a577a20e4ae 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -1054,22 +1054,28 @@ 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, sent;
>  	int rc = 0;
> -	int diff;
> +	s64 diff;
>  
>  	conn->tx_switch_gen++;	/* completions of earlier sends: no credit */
> +	smc_curs_copy(&sent, &conn->tx_curs_sent, conn);
>  	smc_curs_copy(&conn->tx_curs_sent, &conn->tx_curs_fin, conn);
>  	smc_curs_copy(&fin, &conn->local_tx_ctrl_fin, conn);
>  	/* 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) {
> +	/* not past what was sent, whatever cons says; that also keeps
> +	 * diff within one send buffer for smc_curs_add()
> +	 */
> +	diff = min(smc_curs_sdiff(conn->peer_rmbe_size, &fin, &cons),
> +		   smc_curs_sdiff(conn->sndbuf_desc->len,
> +				  &conn->tx_curs_fin, &sent));
> +	if (diff > 0) {
>  		/* cons cursor advanced more than fin, and prod was set
>  		 * fin above, so now prod is smaller than cons. Fix that.
>  		 */
> -		diff = smc_curs_diff(conn->peer_rmbe_size, &fin, &cons);
>  		smc_curs_add(conn->sndbuf_desc->len,
>  			     &conn->tx_curs_sent, diff);
>  		smc_curs_add(conn->sndbuf_desc->len,
> @@ -1079,10 +1085,10 @@ static int smc_switch_cursor(struct smc_sock *smc, struct smc_cdc_tx_pend *pend,
>  		atomic_add(diff, &conn->sndbuf_space);
>  		smp_mb__after_atomic();
>  
> -		smc_curs_add(conn->peer_rmbe_size,
> -			     &conn->local_tx_ctrl.prod, diff);
> -		smc_curs_add(conn->peer_rmbe_size,
> -			     &conn->local_tx_ctrl_fin, diff);
> +		smc_curs_advance(conn->peer_rmbe_size,
> +				 &conn->local_tx_ctrl.prod, diff);
> +		smc_curs_advance(conn->peer_rmbe_size,
> +				 &conn->local_tx_ctrl_fin, diff);
>  	}

[Severity: High]
Can clamping diff to tx_curs_fin..sent leave conn->local_tx_ctrl.prod
behind conn->local_rx_ctrl.cons?

Whenever fin..cons exceeds tx_curs_fin..sent, local_tx_ctrl.prod is only
advanced by diff (to fin + diff), which remains strictly behind cons.
That happens if a peer advertises cons ahead of tx_curs_sent, or across
two link switches when data sent on the first link reaches the peer and is
consumed after the first smc_switch_cursor() already rewound tx_curs_sent
to tx_curs_fin (making tx_curs_fin..sent zero on the second switch before
tx_work resends).

Both smc_write_space() and smc_tx_rdma_writes() assume cons <= prod:

net/smc/smc_tx.c:smc_tx_rdma_writes() {
	...
	if (prod.wrap == cons.wrap) {
		dst_len = min_t(size_t,
				conn->peer_rmbe_size - prod.count, len);
	} else {
		dst_len = len;
	}
	...
}

When prod < cons, smc_write_space() can compute a negative
peer_rmbe_space (stalling the connection), or when prod.wrap != cons.wrap
with prod.count <= cons.count, smc_tx_rdma_writes() skips clamping
dst_len to conn->peer_rmbe_size - prod.count and can issue an RDMA write
past the end of the peer RMBE.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-b4-disp-1a9ab9b0-v1-0-4a7c8d85d849@proton.me?part=3

Re: [PATCH net 4/4] net/smc: order the urgent mark across the cursor wrap rollover

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] net/smc: TOCTOU race in smc_rx_recv_urg() [1]
--

commit 474a1c81553cc3992c5253fbaae536390326e390
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/smc: order the urgent mark across the cursor wrap rollover

This commit replaces smc_curs_comp() with smc_curs_sdiff() in
smc_rx_update_consumer() so that consumer and urgent cursors are ordered
correctly across the 16-bit wrap rollover, and removes the unused
smc_curs_comp() helper.

> diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
> index 63c178c10bf1a..0345167cdc0dc 100644
> --- a/net/smc/smc_rx.c
> +++ b/net/smc/smc_rx.c
> @@ -68,8 +68,8 @@ static int smc_rx_update_consumer(struct smc_sock *smc,
>  
>  	/* did we process urgent data? */
>  	if (conn->urg_state == SMC_URG_VALID || conn->urg_rx_skip_pend) {
> -		diff = smc_curs_comp(conn->rmb_desc->len, &cons,
> -				     &conn->urg_curs);
> +		diff = smc_curs_sdiff(conn->rmb_desc->len, &cons,
> +				      &conn->urg_curs);
>  		if (sock_flag(sk, SOCK_URGINLINE)) {
>  			if (diff == 0) {
>  				force = true;

[Severity: Medium]
This problem wasn't introduced by this patch, but can smc_rx_recv_urg()
race [1] with incoming CDC messages and duplicate an urgent byte?

net/smc/smc_rx.c:smc_rx_recv_urg() {
	...
	if (conn->urg_state == SMC_URG_VALID) {
		if (!(flags & MSG_PEEK))
			smc->conn.urg_state = SMC_URG_READ;
		msg->msg_flags |= MSG_OOB;
		if (len > 0) {
			if (!(flags & MSG_TRUNC))
				rc = memcpy_to_msg(msg, &conn->urg_rx_byte, 1);
	...
}

Although smc_rx_recv_urg() runs under lock_sock(), smc_cdc_msg_recv() in
softirq context only takes bh_lock_sock() without checking
sock_owned_by_user():

CPU0                                  CPU1 (softirq)
smc_rx_recv_urg()
  smc->conn.urg_state = SMC_URG_READ;
                                      smc_cdc_msg_recv()
                                        bh_lock_sock(&smc->sk)
                                        smc_cdc_handle_urg_data_arrival()
                                          conn->urg_rx_byte = new_byte;
                                          conn->urg_state = SMC_URG_VALID;
                                        bh_unlock_sock(&smc->sk)
  memcpy_to_msg(..., &conn->urg_rx_byte, 1)  <-- copies new_byte

The first urgent byte is lost, the current recv(MSG_OOB) returns the new
urgent byte, and because conn->urg_state was reset to SMC_URG_VALID, the
next recv(MSG_OOB) reads the same new byte a second time.

[1] https://sashiko.dev/#/bug/linux-fba17bf0-1137-4667-8969-554d43e1d736

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-b4-disp-1a9ab9b0-v1-0-4a7c8d85d849@proton.me?part=4