[PATCH net] net/smc: don't spin in smc_rx_recvmsg() while a pipe holds spliced data

[PATCH net] net/smc: don't spin in smc_rx_recvmsg() while a pipe holds spliced data

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

While bytes spliced off the RMB are still in a pipe, recvmsg() waits
for the pipe to release them before it copies the data behind them.
The path to that wait skips the timeout and signal checks, and
smc_rx_wait() returns at once on RCV_SHUTDOWN. With a signal pending,
after SHUT_RD, or once the peer closes or shuts down writing,
recvmsg() spins on a CPU until the pipe is drained, and SIGKILL cannot
stop it. MSG_DONTWAIT and an expired SO_RCVTIMEO wait there too
instead of returning -EAGAIN.

Check the timeout and signals before that wait, and end it when the
pipe releases the data or the connection is killed, not on RCV_SHUTDOWN.
A zero-length read returns 0 without waiting for the pipe.

Fixes: 9014db202cb7 ("smc: add support for splice()")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
Repro on v7.3-rc6 with KASAN (its smc_rx.c is the same as net's),
SMC-D loopback, conforming peer: the reader splices 4096 bytes into a
pipe the test drains only after 3 s, the peer has sent more behind
them, and the reader calls recv(4096).

Unpatched, with SIGUSR1 pending, after SHUT_RD, after the peer's
SHUT_WR, or after the peer closed with data of ours unread (an abort,
sk_err ECONNRESET), recv() spins on a full CPU until the drain, and
SIGKILL doesn't stop it. MSG_DONTWAIT, a 1 s SO_RCVTIMEO and a
zero-length recv() don't return until the drain either.

Patched:
  - MSG_DONTWAIT gets -EAGAIN at once, SO_RCVTIMEO after 1042 ms; a
    signal gives -EINTR, or with SA_RESTART the handler runs and the
    call goes back to sleep; SIGKILL ends the task within 10 ms;
    recv(0) returns 0 at once;
  - after a shutdown or an abort the reader sleeps with no CPU, then
    gets the next 4096 bytes once the pipe is drained;
  - a MSG_WAITALL read that had copied 4096 bytes when another thread
    spliced in front of the rest returns those 4096 at the signal.

In every case the next recv() gets the rest of the stream intact. A
second splice(SPLICE_F_NONBLOCK), with the first undrained, still
returns the next 4096 bytes at once. Over SMC-R on rxe, starting rdma
link del on the reader's only RDMA device kills the connection
(SO_ERROR ECONNABORTED) and the reader returns 14 ms later, patched or
not. The patch gives the same results on top of [2]. No KASAN splat.
Dropping any one check from the patch makes one of the cases above
fail, except the !splice_pending condition, whose race no test can hit
on demand. Logs on request.

I checked that condition in a small state model of the reader, the
pipe release and smc_conn_kill() (atomic steps, sequentially
consistent, a blocking read): over every interleaving, with the
release in today's order or with splice_pending dropped under
lock_sock (2/4 of [2]; Hidayath said in [1] he would do the same),
the reader returns. In today's order the condition is needed: without
it the release's decrement and wake can land between the reader's
check and its add_wait_queue(), and the reader sleeps until something
else wakes the socket. Under LKMM, today's order allows a lost wake
even with the condition (no full barrier between add_wait_queue() and
the condition's read; smc_rx_wait() has the same gap), and the
reorder in [1] or [2] closes it for both. This patch doesn't depend
on that reorder.

For stable: in SMC_CLOSED, SMC_PEERFINCLOSEWAIT and SMC_APPFINCLOSEWAIT
the pipe release drops splice_pending only since c924884743e9
("net/smc: fix use-after-free in smc_rx_pipe_buf_release()"). Every
maintained stable from 6.1.y up has it. 5.10.y and 5.15.y don't (the
backports are posted, not released), and there a reader in those
states would sleep until a signal, its timeout or a kill instead of
spinning. The shutdown and abort cases above reach none of those
states.

[1] https://lore.kernel.org/all/7a174d1f-80d8-4294-ae66-829c9dacd662@linux.ibm.com/
[2] https://lore.kernel.org/all/20261013-b4-disp-f345196c-v7-0-ef3dddfbfa68@proton.me/
---
 net/smc/smc_rx.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
index 197fddc6271f..6f409bda4c37 100644
--- a/net/smc/smc_rx.c
+++ b/net/smc/smc_rx.c
@@ -286,6 +286,22 @@ int smc_rx_wait(struct smc_sock *smc, long *timeo, size_t peeked,
 	return rc;
 }
 
+/* Wait until the pipe releases the spliced data. Unlike smc_rx_wait(),
+ * this does not end on shutdown or a socket error: the data to read is
+ * already in the RMB. smc_conn_kill() wakes the socket, and the caller
+ * checks conn->killed.
+ */
+static void smc_rx_wait_splice(struct smc_sock *smc, long *timeo)
+{
+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
+	struct smc_connection *conn = &smc->conn;
+	struct sock *sk = &smc->sk;
+
+	add_wait_queue(sk_sleep(sk), &wait);
+	sk_wait_event(sk, timeo, !atomic_read(&conn->splice_pending), &wait);
+	remove_wait_queue(sk_sleep(sk), &wait);
+}
+
 static int smc_rx_recv_urg(struct smc_sock *smc, struct msghdr *msg, int len,
 			   int flags)
 {
@@ -443,7 +459,24 @@ int smc_rx_recvmsg(struct smc_sock *smc, struct msghdr *msg,
 		/* could be just 1 byte, even after waiting on data above */
 		readable = smc_rx_data_available(conn, peeked_bytes);
 		splbytes = atomic_read(&conn->splice_pending);
-		if (!readable || (msg && splbytes)) {
+		if (msg && splbytes) {
+			/* a pipe still holds bytes ahead of these, and the
+			 * jumps to copy skip the timeout and signal checks;
+			 * a zero-length read returns 0 at once
+			 */
+			if (!read_remaining)
+				break;
+			if (!timeo || signal_pending(current)) {
+				if (!read_done)
+					read_done = timeo ?
+						    sock_intr_errno(timeo) :
+						    -EAGAIN;
+				break;
+			}
+			smc_rx_wait_splice(smc, &timeo);
+			continue;
+		}
+		if (!readable) {
 			if (splbytes)
 				func = smc_rx_data_available_and_no_splice_pend;
 			else

---
base-commit: 513444cc62e231a11c9b15ea88de0890da9b2b4f
change-id: 20261013-b4-disp-193a2a59-7e8b8ee87a89

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

Re: [PATCH net] net/smc: don't spin in smc_rx_recvmsg() while a pipe holds spliced data

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

Re: [PATCH net] net/smc: don't spin in smc_rx_recvmsg() while a pipe holds spliced data

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] smc: missing SOCKWQ_ASYNC_WAITDATA flag causes spurious SIGIO and -EINTR
--

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

net/smc: don't spin in smc_rx_recvmsg() while a pipe holds spliced data

This commit introduces smc_rx_wait_splice() to allow recvmsg() to sleep
instead of spinning on the CPU while bytes spliced off the RMB are still in
a pipe. It checks for timeouts and signals before waiting, ending the wait
when the pipe releases the data or the connection is killed.

> diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
> index 197fddc6271f0..6f409bda4c37a 100644
> --- a/net/smc/smc_rx.c
> +++ b/net/smc/smc_rx.c
[ ... ]
> @@ -286,6 +286,22 @@ int smc_rx_wait(struct smc_sock *smc, long *timeo, size_t peeked,
>  	return rc;
>  }
>  
> +/* Wait until the pipe releases the spliced data. Unlike smc_rx_wait(),
> + * this does not end on shutdown or a socket error: the data to read is
> + * already in the RMB. smc_conn_kill() wakes the socket, and the caller
> + * checks conn->killed.
> + */
> +static void smc_rx_wait_splice(struct smc_sock *smc, long *timeo)
> +{
> +	DEFINE_WAIT_FUNC(wait, woken_wake_function);
> +	struct smc_connection *conn = &smc->conn;
> +	struct sock *sk = &smc->sk;
> +
> +	add_wait_queue(sk_sleep(sk), &wait);
> +	sk_wait_event(sk, timeo, !atomic_read(&conn->splice_pending), &wait);

[Severity: Medium]
Does smc_rx_wait_splice() need to set and clear the SOCKWQ_ASYNC_WAITDATA bit
around sk_wait_event()?

If FASYNC is enabled on an SMC socket, smc_rx_wake_up() will send a SIGIO
signal to the process unless the SOCKWQ_ASYNC_WAITDATA bit is set.

Since smc_rx_wait_splice() puts the socket to sleep without setting this bit,
a spurious SIGIO could be delivered if the peer sends more data or the pipe
drains while the process is sleeping here.

This could interrupt wait_woken(), causing smc_rx_recvmsg() to prematurely
abort the wait and return -EINTR (or a short read) instead of waiting for
the pipe to drain. While applications generally handle -EINTR by retrying,
this breaks the expected blocking semantics.

For comparison, smc_rx_wait() uses this pattern:

int smc_rx_wait(...) {
    ...
    sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
    add_wait_queue(sk_sleep(sk), &wait);
    rc = sk_wait_event(...);
    remove_wait_queue(sk_sleep(sk), &wait);
    sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);
    ...
}

> +	remove_wait_queue(sk_sleep(sk), &wait);
> +}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-b4-disp-193a2a59-v1-1-ed88ac10a41c@proton.me?part=1