[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

[netdev-bot+sashiko]

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

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

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

- [Medium] smc_rx_wait_splice() sleeps until
  !atomic_read(&conn->splice_pending).
- [Low] The new smc_rx_wait_splice() (net/smc/smc_rx.c:294-303) sleeps on
  sk_sleep(sk) via add_wait_queue()/sk_wait_event() but never sets or…

Pre-existing issues:
- [Medium] After this patch, a non-blocking recv() whose readable data
  sits behind bytes still held in a pipe returns -EAGAIN.
- [Medium] smc_rx_pipe_buf_release() advances the consumer cursor
  (smc_rx_update_cons) under lock_sock, but decrements…

--

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

This patch changes smc_rx_recvmsg() so that, while spliced bytes are still
held in a pipe, recvmsg() checks the timeout and pending signals before it
waits. The wait moves to a new helper, smc_rx_wait_splice(), which ends when
splice_pending drops to zero or the connection is killed, and no longer ends
on RCV_SHUTDOWN. A zero-length read now returns 0 right away.

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

[Severity: Low]
smc_rx_wait() sets SOCKWQ_ASYNC_WAITDATA around the same kind of sleep:

	sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
	add_wait_queue(sk_sleep(sk), &wait);
	...
	sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);

Should smc_rx_wait_splice() do the same?

The wake callback smc_rx_wake_up() calls
sk_wake_async_rcu(sk, SOCK_WAKE_WAITD, POLL_IN). For SOCK_WAKE_WAITD,
sock_wake_async() sends SIGIO only when SOCKWQ_ASYNC_WAITDATA is clear.

Before this patch, a recvmsg() blocked behind spliced data waited in
smc_rx_wait(..., smc_rx_data_available_and_no_splice_pend), which set the
bit.

Now take an O_ASYNC socket whose F_SETOWN owner is the reader. Each new
peer data arrival during this wait sends SIGIO. The next loop pass sees
splbytes != 0 and signal_pending(current), and returns -EINTR or
-ERESTARTSYS. A reader that used to keep sleeping is now interrupted.

> +	sk_wait_event(sk, timeo, !atomic_read(&conn->splice_pending), &wait);

[Severity: Medium]
Can this wait miss the final wakeup on weakly ordered architectures?

The only waker for this condition is smc_rx_pipe_buf_release(). It
decrements splice_pending and wakes only after release_sock():

net/smc/smc_rx.c:smc_rx_pipe_buf_release() {
    ...
	smc_rx_update_cons(smc, priv->len);
	release_sock(sk);
	if (atomic_sub_and_test(priv->len, &conn->splice_pending))
		smc_rx_wake_up(sk);
    ...
}

smc_rx_wake_up() goes through skwq_has_sleeper(), which does smp_mb() and
then waitqueue_active(). The wq_has_sleeper() comment says this barrier has
to pair with one on the waiting side.

On the reader side, two steps sit between the list store in add_wait_queue()
and the relaxed atomic_read(&conn->splice_pending) in sk_wait_event():

- spin_unlock_irqrestore() on the wait queue lock
- spin_lock_bh()/spin_unlock_bh() on sk_lock.slock inside release_sock()

Neither is a full barrier. Under LKMM, an unlock and a lock on different
locks don't order an earlier store before a later load. That leaves a
store-buffering pattern:

reader                                releaser
add_wait_queue()
  (store not yet visible)
                                      atomic_sub_and_test() -> 0
                                      smp_mb()
                                      waitqueue_active() -> false
atomic_read(&conn->splice_pending)
  -> stale N
wait_woken() sleeps

Before this patch, the RCV_SHUTDOWN, peer close and abort states spun here,
so they could not lose a wakeup. They now sleep in smc_rx_wait_splice() and
depend on this single wakeup.

Once the peer has shut down or closed, no more CDC traffic wakes
sk_sleep(). A blocking recvmsg() without SO_RCVTIMEO could then sleep until
a signal arrives, even though the data is already in the RMB. On x86 and
s390 the lock acquire is a full barrier, so this would only show up on
architectures like powerpc.

The notes below the --- line already say "Under LKMM, today's order allows a
lost wake even with the condition". The old smc_rx_wait() has the same gap,
but this patch newly relies on it for the shutdown and abort states.

Would an smp_mb() between add_wait_queue() and the condition read close
this? Moving the splice_pending decrement and the wakeup under lock_sock()
in smc_rx_pipe_buf_release() looks like another option.

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

[Severity: Medium]
This is a pre-existing issue that this patch didn't introduce: can the
splice path read a stale splbytes here?

smc_rx_pipe_buf_release() moves the consumer cursor under lock_sock(), but
it decrements splice_pending only after release_sock() (see the snippet
above).

The releasing pipe's mutex doesn't serialize a concurrent smc_splice_read()
into a different pipe. In that window, the second splice can take
lock_sock(), reach copy:, and read splbytes == N here. It then adds N to a
cons that has already moved by N:

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

That skips N bytes of valid stream data. It also splices N bytes from past
the producer cursor into the second pipe.

smcr_buf_unuse() and smc_buf_unuse() zero the RMBs. So this corrupts only
the connection's own stream and doesn't expose another connection's data.
The accesses stay inside the RMB.

The notes below the --- line say a separate series moves the splice_pending
decrement under lock_sock(). That looks like it would also close this
window for the msg == NULL path.

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

[Severity: Medium]
This is a pre-existing issue rather than one added by this patch, but
smc_poll() still disagrees with this -EAGAIN:

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

bytes_to_rcv doesn't count spliced bytes, because smc_rx_recvmsg()
subtracts copylen for them too. Suppose a pipe holds N bytes and the peer
has sent M more behind them. Then bytes_to_rcv == M.

poll(), select() and level-triggered epoll keep reporting EPOLLIN. Meanwhile
every recv(MSG_DONTWAIT), and every recv() on an O_NONBLOCK socket, returns
-EAGAIN from this branch. An event-loop reader would busy-loop in userspace
until the pipe is drained.

Before this patch the same non-blocking recv() spun inside the kernel, so
CPU use is no worse than before. The commit message doesn't mention that
this case remains, though. Should smc_poll() also look at
conn->splice_pending?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013-b4-disp-193a2a59-v1-1-ed88ac10a41c%40proton.me