From: Bryam Vargas 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 Reviewed-by: Dust Li Reviewed-by: Sidraya Jayagond --- 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 From: Bryam Vargas 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. poll(), SIOCINQ and the deferred accept read the same raw count. 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 in all of these. Cache the RMB length for the unlocked wait, where rmb_desc may already be freed, and drop released bytes from splice_pending under the socket lock, with the consumer cursor update, so a reader cannot run between the two and count 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 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 --- The wait predicates run unlocked, where a link group termination may already have freed rmb_desc, hence the cached length; v9 reads and writes it with READ_ONCE()/WRITE_ONCE(). 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/6 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 | 12 +++++++++++- 6 files changed, 26 insertions(+), 7 deletions(-) diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c index ec5980add7a9..90e7d0d2e398 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 36b243b34a9a..8601304b3c06 100644 --- a/net/smc/smc.h +++ b/net/smc/smc.h @@ -204,6 +204,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 0a9f5418329a..0edbe6106c1a 100644 --- a/net/smc/smc_core.c +++ b/net/smc/smc_core.c @@ -2511,6 +2511,7 @@ static int __smc_buf_create(struct smc_sock *smc, bool is_smcd, bool is_rmb) if (is_rmb) { conn->rmb_desc = buf_desc; + WRITE_ONCE(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 63c178c10bf1..78ab3df1689c 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; } + /* lower splice_pending under the socket lock, with the cons update, or + * a reader running between the two counts the released bytes 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); @@ -543,7 +548,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..1833eca15bfb 100644 --- a/net/smc/smc_rx.h +++ b/net/smc/smc_rx.h @@ -25,7 +25,17 @@ 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, READ_ONCE(conn->rmb_len) - + atomic_read(&conn->splice_pending)); + return avail > (int)peeked ? avail - (int)peeked : 0; } #endif /* SMC_RX_H */ -- 2.56.0 From: Bryam Vargas 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. The over-credit itself is not undone: with sndbuf_space too large, send() can still overwrite data in the send buffer that the peer has not received. 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 Reviewed-by: Dust Li Reviewed-by: Sidraya Jayagond --- The overwrite after an over-credit is 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_cdc.c | 2 +- net/smc/smc_tx.c | 10 +++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c index e2c765ff2ba3..90a79a983b48 100644 --- a/net/smc/smc_cdc.c +++ b/net/smc/smc_cdc.c @@ -43,7 +43,7 @@ static void smc_cdc_tx_handler(struct smc_wr_tx_pend_priv *pnd_snd, /* sndbuf_space is decreased in smc_sendmsg */ smp_mb__before_atomic(); atomic_add(diff, &cdcpend->conn->sndbuf_space); - /* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */ + /* can be out of range, see smc_tx_sendmsg() */ smp_mb__after_atomic(); smc_curs_copy(&conn->tx_curs_fin, &cdcpend->cursor, conn); smc_curs_copy(&conn->local_tx_ctrl_fin, &cdcpend->p_cursor, 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 From: Bryam Vargas smcr_tx_rdma_writes() sizes the first source SGE of the second destination chunk from the send cursor as it was before the first chunk, so that SGE runs past the send buffer when the buffer ends inside the second chunk. A physically contiguous send buffer goes out under the local DMA lkey, which does not bound the SGE; on rxe the peer is sent the memory that follows the buffer. A conforming peer gets there when a completion handled inside smc_switch_cursor() leaves tx_curs_sent behind local_tx_ctrl.prod. Otherwise it takes a peer_rmbe_space forged larger than the RMBE and a send buffer larger than the RMBE. Size the SGE from the running offset, as smcd_tx_rdma_writes() does. A conforming write sends the same bytes. Fixes: e6727f39004b ("smc: send data (through RDMA)") Reported-by: Sashiko Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013-b4-disp-f345196c-v7-0-ef3dddfbfa68%40proton.me?part=4 Cc: stable@vger.kernel.org Signed-off-by: Bryam Vargas --- New in v9. Sashiko's review of v7's 4/4 [1] pointed this bug out ("could this send adjacent kernel memory to the peer?"); the review, and my reply to it, took it for unreachable by a conforming peer. The first case below reaches it. rxe, two links (v7.4-rc2 plus d7900fa51642, KASAN), send buffer and peer RMBE 64K, physically contiguous, conforming peer. Every connection carries one byte first; the counts include it. "With the patch" is the whole series. - 60000 bytes sent and read, then 600 and 400 more, read too; failover, with the completion of the 600 handed to smc_cdc_tx_handler() between the first two cursor copies of smc_switch_cursor() (test hook) and the resend held; then send(9400). The switch leaves tx_curs_sent at 60400 and prod at 61000, so the next write is the 600 bytes between them again plus the 9400, 4536 + 5464. Unpatched and v8 size the second chunk's first source SGE as 5136 bytes from offset 64936, 4536 past the send buffer. The peer reads the first 9400 bytes of that write; from byte 5136 on (4536 + 600) they come from outside the send buffer. KASAN splat in rxe_mr_copy() on the unpatched run, where a freed slab object follows the buffer; none on v8's. With the patch no SGE ends past the buffer and the peer reads what is in it. It gets 600 bytes twice either way (see 6/6). - no failover, 60000 bytes sent and read, one send(20000): the write is 5536 + 14464 bytes and the send buffer ends with the first chunk. Unpatched builds 3 source SGEs, the patch 2; the peer reads the same 20000 bytes. With 5/6 and 6/6 applied and this change reverted, the first case and the fourth case under 5/6 fail. Nothing else does: with the two cursors in step, one RMBE per call already keeps the source inside (the second case under 5/6). A model of smcr_tx_rdma_writes() with the old and the new sizing, over send buffers and RMBEs of 2 to 16 bytes: the new sizing gives 1344 of 12928 conforming writes a different SGE list, never a longer one, and every write the same bytes. Not tested on a real HCA, nor with a vmalloc'ed send buffer, where the SGE goes out under the buffer's own MR. [1] https://lore.kernel.org/all/179209532452.434549.6588379141939967966@kernel.org/ --- net/smc/smc_tx.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c index 3913ef103b35..d37c74754461 100644 --- a/net/smc/smc_tx.c +++ b/net/smc/smc_tx.c @@ -379,7 +379,6 @@ static int smcr_tx_rdma_writes(struct smc_connection *conn, size_t len, sg_dma_address(conn->sndbuf_desc->sgt[link->link_idx].sgl); u64 virt_addr = (uintptr_t)conn->sndbuf_desc->cpu_addr; int src_len_sum = src_len, dst_len_sum = dst_len; - int sent_count = src_off; int srcchunk, dstchunk; int num_sges; int rc; @@ -425,8 +424,7 @@ static int smcr_tx_rdma_writes(struct smc_connection *conn, size_t len, 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); + src_len = min_t(int, dst_len, conn->sndbuf_desc->len - src_off); src_len_sum = src_len; } return 0; -- 2.56.0 From: Bryam Vargas smc_tx_rdma_writes() takes the length of a write from peer_rmbe_space and decides from the cursor wraps whether the destination is one chunk or two. Both follow the peer's consumer cursor, which nothing bounds: a forged one takes a one-chunk write past the peer's RMBE, and so does a conforming peer that keeps reading across the end of its RMBE before the resend after an SMC-R link failover. With one RMBE per RMB, as Linux allocates them, the peer's HCA refuses the write and the link goes down. Write at most one RMBE per call and end the first chunk no later than the RMBE, whatever the wraps say. A conforming peer outside failover gets the chunks it got before. A write that was one chunk can now be two, which needs "net/smc: keep the source of an RDMA write inside the send buffer"; the first chunk needs prod.count inside the RMBE, which "net/smc: bound the link failover cursor advance to the send buffer" keeps at failover. Fixes: e6727f39004b ("smc: send data (through RDMA)") Fixes: c6f02ebeea3a ("net/smc: switch connections to alternate link") Reported-by: Sashiko Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013-b4-disp-f345196c-v7-0-ef3dddfbfa68%40proton.me?part=4 Cc: stable@vger.kernel.org # see patch description, needs two other patches Signed-off-by: Bryam Vargas --- New in v9, for the first point of Sashiko's review of v8's 4/4 [1]: v8 skipped the failover advance for a consumer count past the peer's RMBE, but the window for this write was still sized from the same cursor. A check in smc_write_space() is not enough: smc_cdc_msg_recv_action() credits peer_rmbe_space from the next cursor, and the differing-wrap branch here then writes the whole length from prod.count as before. The review of v7's 4/4 [2] had already pointed out the one-chunk write and the window larger than the RMBE, both in the unpatched code, hence the Reported-by and Closes tags. rxe, two links (v7.4-rc2 plus d7900fa51642, KASAN), send buffer 64K, peer RMBE 16K, test hooks forging the peer's cursor and making the timing deterministic. Every connection carries one byte first; the counts include it. "With the patch" is the whole series. - consumer cursor forged one wrap ahead at 3 * RMBE, then send(40000): unpatched and v8 post 40000 bytes in one chunk from offset 1, rxe refuses the write and the link goes down. With the patch the first write is 16383 + 1 bytes, neither chunk ends past the RMBE and both links stay up. - 40000 bytes sent and read, the consumer count then forged a million bytes ahead with the wrap unchanged, then send(30000): unpatched and v8 post 9152 + 20848 bytes. The second chunk is longer than the RMBE and its source SGE is 20848 bytes from offset 49152 of the 65536-byte send buffer, 4464 past its end; rxe refuses the write. With the patch: 9152 + 7232, every source SGE inside. Same result with 4/6 reverted; with this patch's length bound reverted the second chunk is still longer than the RMBE and its source stays inside (4/6). - conforming peer: 12001 bytes sent and confirmed, then 9000 whose completions are lost with the failing link, the failover, the resend held until the peer has read them, then 2000 more. Unpatched and v8 post 11000 bytes in one chunk from offset 12001 of the 16384-byte RMBE; the surviving link goes down and the peer's read fails. With the patch the write is 4383 + 6617 bytes and the peer reads the stream intact. - conforming peer, send buffer and RMBE 64K: 60000 bytes sent and read, then 600 and 6000 that the peer reads only after the switch; the completion of the 600 handled between the first two cursor copies of the switch, as in the first case under 4/6, which leaves tx_curs_sent 600 bytes behind prod; the resend held until the peer has read; then send(400). Unpatched and v8 post 7000 bytes in one chunk that ends 2064 past the RMBE, rxe refuses the write and the connection is lost. With this patch and not 4/6 the write is 4936 + 2064, inside the RMBE, and the second chunk's source ends 1464 bytes past the send buffer: the peer reads bytes from outside it. With 4/6 the source is inside too, the link stays up and the peer gets 600 bytes twice (see 6/6; the run reads 400 of them). - the second case of the note under v8's 4/4 (consumer cursor forged two wraps ahead at count RMBE + 4096; 40001 bytes sent and read, the completions of all but the first held): unpatched moves prod onto the forged count and writes nothing more, 3137 bytes stay queued (SIOCOUTQ); v8 posts 16384 bytes from offset 1, rxe refuses the write and the link goes down; with the patch the write is 16383 + 1 and the link stays up. - 600001 bytes of a conforming stream across a failover, with the patch: 39 writes, 18 in two chunks, each chunked as the two removed branches would chunk it (compared in the kernel, write by write). Without the length bound the first two cases fail; with the chunk decided from the wraps again, the first, third, fourth and fifth do. The comment on the credit in smc_cdc_msg_recv_action() still claims 0 <= peer_rmbe_space <= peer_rmbe_size: the lines after it exist only since v6.10, and a hunk there would not apply to 6.6.y and older. The first chunk needs prod.count inside the RMBE. smc_tx_advance_cursors() keeps it there and, with 6/6, so does smc_switch_cursor(); without 6/6 a forged cursor can still leave it outside at failover, and the write then starts where it started before this patch (by inspection). On SMC-D the same lines size the ISM write. On loopback smcd_tx_rdma_writes() returns before it writes; not tested on ISM hardware. [1] https://lore.kernel.org/all/179364387382.3131.13471253186660322977@kernel.org/ [2] https://lore.kernel.org/all/179209532452.434549.6588379141939967966@kernel.org/ --- net/smc/smc_tx.c | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c index d37c74754461..c90bdc77f82d 100644 --- a/net/smc/smc_tx.c +++ b/net/smc/smc_tx.c @@ -362,7 +362,7 @@ static inline void smc_tx_advance_cursors(struct smc_connection *conn, smp_mb__before_atomic(); /* data in flight reduces usable snd_wnd */ atomic_sub(len, &conn->peer_rmbe_space); - /* guarantee 0 <= peer_rmbe_space <= peer_rmbe_size */ + /* can be out of range, see smc_tx_rdma_writes() */ smp_mb__after_atomic(); smc_curs_add(conn->sndbuf_desc->len, sent, len); } @@ -480,7 +480,7 @@ static int smc_tx_rdma_writes(struct smc_connection *conn, struct smc_rdma_wr *wr_rdma_buf) { size_t len, src_len, dst_off, dst_len; /* current chunk values */ - union smc_host_cursor sent, prep, prod, cons; + union smc_host_cursor sent, prep, prod; struct smc_cdc_producer_flags *pflags; int to_send, rmbespace; int rc; @@ -503,31 +503,23 @@ static int smc_tx_rdma_writes(struct smc_connection *conn, return 0; } smc_curs_copy(&prod, &conn->local_tx_ctrl.prod, conn); - smc_curs_copy(&cons, &conn->local_rx_ctrl.cons, conn); /* if usable snd_wnd closes ask peer to advertise once it opens again */ pflags = &conn->local_tx_ctrl.prod_flags; pflags->write_blocked = (to_send >= rmbespace); - /* cf. usable snd_wnd */ - len = min(to_send, rmbespace); + /* cf. usable snd_wnd. The window follows the peer's consumer cursor, + * which comes off the wire: write at most one RMBE per call. + */ + len = min3(to_send, rmbespace, conn->peer_rmbe_size); /* initialize variables for first iteration of subsequent nested loop */ dst_off = prod.count; - if (prod.wrap == cons.wrap) { - /* the filled destination area is unwrapped, - * hence the available free destination space is wrapped - * and we need 2 destination chunks of sum len; start with 1st - * which is limited by what's available in sndbuf - */ - dst_len = min_t(size_t, - conn->peer_rmbe_size - prod.count, len); - } else { - /* the filled destination area is wrapped, - * hence the available free destination space is unwrapped - * and we need a single destination chunk of entire len - */ - dst_len = len; - } + /* at most 2 destination chunks of sum len; the 1st ends no later than + * the peer's RMBE. The cursor wraps do not decide that: the peer's + * cursor comes off the wire, and after a link failover prod can be + * behind it. + */ + dst_len = min_t(size_t, conn->peer_rmbe_size - prod.count, len); /* dst_len determines the maximum src_len */ if (sent.count + dst_len <= conn->sndbuf_desc->len) { /* unwrapped src case: single chunk of entire dst_len */ -- 2.56.0 From: Bryam Vargas On SMC-R link failover, smc_switch_cursor() rewinds the send and producer cursors to the last completed CDC send, then advances them by the distance the peer's consumer cursor is ahead. That cursor comes from the wire: a forged one adds an arbitrary amount to sndbuf_space and to the cursors, and the next RDMA write takes its source past the send buffer. Advance only for a count inside the peer's RMBE, and no further than tx_curs_prep; a conforming peer's cursor is inside both. For any other count the write that follows can end past the peer's RMBE and needs "net/smc: keep an RDMA write inside the peer's RMBE" to bound it. Set tx_curs_fin and local_tx_ctrl_fin from the advanced cursors instead of adding to them: smc_cdc_tx_handler() can rewrite both while the switch runs. The switch is still not serialized against the handler: a completion handled inside it is still credited more than once and can make the next write hand the peer bytes it has already read. Fixes: c6f02ebeea3a ("net/smc: switch connections to alternate link") Cc: stable@vger.kernel.org # see patch description, needs another patch Signed-off-by: Bryam Vargas --- v9: tx_curs_fin and local_tx_ctrl_fin are copied from the advanced tx_curs_sent and local_tx_ctrl.prod, which change only under send_lock, instead of being added to (second point of [1]). The write past the peer's RMBE that v8 introduced for a count past the RMBE is bounded in 5/6. The cap is tx_curs_prep and not the pre-failover tx_curs_sent: an earlier failover may have rewound that one below the peer's cursor. rxe, two links (v7.4-rc2 plus d7900fa51642, KASAN), send buffer 64K, peer RMBE 16K: one byte and then 4096 confirmed; 12000 and 5000 sent and not confirmed, the 5000 in two writes, 287 up to the end of the RMBE and 4713; all read by a conforming peer. A hook holds the pending completions and hands the first one to smc_cdc_tx_handler() inside smc_switch_cursor(), after it has read tx_curs_fin and local_tx_ctrl_fin: unpatched and v8 leave local_tx_ctrl_fin at count 16713 (the RMBE is 16384 bytes) and tx_curs_fin 12000 bytes past tx_curs_prep; v9 leaves tx_curs_fin on tx_curs_prep and local_tx_ctrl_fin on prod. Without the hook, v8 ends the same run with the cursors v9 leaves. The first case of the note under v8's 4/4 (cursor forged one wrap ahead at count 64, with 4096 bytes unconfirmed in one run and corked in another) gives on v9 what that note reports for v8: the advance stops at tx_curs_prep and nothing is written after the switch. v8 and unpatched were not rerun for it. Not changed: the switch is not serialized against the handler. The handler takes send_lock inside the socket spinlock (smc_tx_pending()) and the switch holds send_lock only, so keeping it out means taking the socket spinlock around send_lock in smc_switch_conns(). What remains, on unpatched, v8 and v9 alike unless noted: - a completion handled inside the switch is credited to sndbuf_space by the handler and again by the advance: in the run above sndbuf_space is 77536 after the switch, for a 65536-byte send buffer. The old-link completions that follow set tx_curs_fin back and credit 4713 bytes once more: 82249. 3/6 keeps send() inside the buffer, but send() can then overwrite data the peer has not received (by inspection). - a completion between the first two cursor copies of the switch leaves tx_curs_sent behind local_tx_ctrl.prod. The next write starts at tx_curs_sent and lands at prod, that far past its place in the RMBE, so the peer is handed that many bytes a second time. In the first case under 4/6 the peer reads 600 bytes twice on all three. In the fourth case under 5/6, where the peer has read across the end of its RMBE before the resend, unpatched and v8 have the write refused and the connection lost; v9's goes out and the peer gets 600 bytes twice there too. - v9 only: the first case under 4/6 ends with sndbuf_space 1200 bytes over the buffer, against 600 on unpatched and v8. v9 sets tx_curs_fin back to tx_curs_sent, and the next old-link completion, when it is handled before the validation message's, credits those 600 once more. Handled after it, all three end 600 over (by inspection). - the completion of the switch's validation message carries tx_curs_sent and steps tx_curs_fin back to it whenever a completion handled before it left tx_curs_fin ahead; across the end of the ring that step is credited as more than a whole ring (the mechanism is in 3/6's message). In the fourth case under 5/6 it is credited 124472 bytes for a 65536-byte send buffer on every variant, and on v9, where the last write goes out, sndbuf_space ends at 3x the buffer. v9's smc_curs_copy() removes one route to that state, the switch itself leaving tx_curs_fin ahead (by inspection); late old-link completions are still one. [1] https://lore.kernel.org/all/179364387382.3131.13471253186660322977@kernel.org/ --- net/smc/smc_core.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c index 0edbe6106c1a..b5625f1a8792 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,16 +1043,22 @@ 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: catch up. cons comes off the wire, so only for a + * count inside the peer's RMBE and no further than + * tx_curs_prep. smc_cdc_tx_handler() is not excluded and can + * rewrite the fin cursors meanwhile: set them, do not add. */ - 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, - &conn->tx_curs_fin, diff); + smc_curs_copy(&conn->tx_curs_fin, &conn->tx_curs_sent, conn); smp_mb__before_atomic(); atomic_add(diff, &conn->sndbuf_space); @@ -1060,8 +1066,8 @@ static int smc_switch_cursor(struct smc_sock *smc, struct smc_cdc_tx_pend *pend, 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_copy(&conn->local_tx_ctrl_fin, + &conn->local_tx_ctrl.prod, conn); } /* recalculate, value is used by tx_rdma_writes() */ atomic_set(&smc->conn.peer_rmbe_space, smc_write_space(conn)); -- 2.56.0