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

[netdev-bot+sashiko@kernel.org]

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