Re: [PATCH net v8 2/4] net/smc: bound the receive length to the RMB

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

Critical: 0 · High: 1 (2) · Medium: 0 · 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).

- [High] The patch says that reordering splice_pending in
  smc_rx_pipe_buf_release() means 'no reader counts them twice', which…
- [Low] The new conn->rmb_len field exists so it can be read without the
  socket lock: the patch's comment in smc_rx_data_available() says the…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a
  reachable local DoS.
- [High] This problem wasn't introduced by this patch, but it is a real
  accounting bug.

--

net/smc: bound the receive length to the RMB

This commit clamps smc_rx_data_available() to what fits in the RMB behind
the spliced and peeked bytes, never returning a negative count. poll(),
SIOCINQ and the deferred accept now use this helper too. It also caches the
RMB length in conn->rmb_len for unlocked waiters, and moves the
splice_pending decrement in smc_rx_pipe_buf_release() ahead of the consumer
cursor update.

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but can smc_rx_recvmsg() loop
forever while splice_pending is non-zero?

When data is available and an earlier splice() is still pending, the top of
the loop jumps straight to copy:

		if (smc_rx_recvmsg_data_available(smc, peeked_bytes))
			goto copy;

That skips the !timeo, signal_pending() and sk_err checks. At copy, msg &&
splbytes is true:

		if (!readable || (msg && splbytes)) {
			if (splbytes)
				func = smc_rx_data_available_and_no_splice_pend;
			else
				func = smc_rx_data_available;
			smc_rx_wait(smc, &timeo, peeked_bytes, func);
			continue;
		}

smc_rx_data_available_and_no_splice_pend() stays false until the pipe
buffers are released. The return value of smc_rx_wait() is ignored.

With MSG_DONTWAIT, recv() never returns -EAGAIN; each pass sleeps about a
jiffy and loops again. With a signal pending, including SIGKILL,
wait_woken() returns at once and the task busy-loops. The only ways out are
splice_pending draining or conn->killed. If the same process holds the pipe,
the pipe buffers would never be released.

smc_poll() now reports EPOLLIN from smc_rx_data_available(conn, 0). That
helper uses splice_pending only to cap the length, but recvmsg() refuses to
copy anything while splice_pending is non-zero. While a splice is pending,
does the commit message's claim that these paths "all see the same length"
hold?

 			if (sk->sk_shutdown & RCV_SHUTDOWN)
 				mask |= EPOLLIN | EPOLLRDNORM | EPOLLRDHUP;

[ ... ]

[Severity: Low]
The new comment in smc_rx_data_available() says conn->rmb_len is read
without the socket lock:

	 * INT_MIN, avail - peeked wraps. This also runs unlocked, possibly
	 * after rmb_desc was freed, hence rmb_len.
	 */
	avail = min(avail, conn->rmb_len - atomic_read(&conn->splice_pending));

Should this store and that load use WRITE_ONCE() and READ_ONCE()?

For example, during a non-blocking connect,
smc_connect_work()->__smc_connect()->smc_buf_create()->__smc_buf_create()
writes rmb_len under lock_sock(). At the same time, smc_poll() on the
SMC_INIT socket reads it without the lock. The first unlocked evaluation of
the condition in sk_wait_event() has the same pattern.

KCSAN would report this as a data race. There is no functional effect today,
since bytes_to_rcv is 0 at that point.

 		conn->rmbe_size_comp = bufsize_comp;
 		smc->sk.sk_rcvbuf = bufsize * 2;
 		atomic_set(&conn->bytes_to_rcv, 0);

[Severity: High]
The commit message says this reorder means "no reader counts them twice".
Is there still a path where spliced bytes are counted twice?

In smc_rx_recvmsg(), a copy that wraps past the end of the RMB is done in two
chunks. If the first chunk succeeds and the second fails, the error path
jumps over the bytes_to_rcv and consumer cursor update:

				if (rc < 0) {
					if (!read_done)
						read_done = -EFAULT;
					goto out;
				}

read_done already includes the first chunk. For splice, smc_rx_splice() has
already added that chunk to splice_pending:

	bytes = splice_to_pipe(pipe, &spd);
	if (bytes > 0)
		atomic_add(bytes, &smc->conn.splice_pending);

Those bytes are then counted in both bytes_to_rcv and splice_pending. The
second chunk can fail with -EAGAIN from splice_to_pipe() when the pipe has
only one free slot. It can also fail with -ENOMEM or -EPIPE.

On the next splice(), the start offset is cons + splice_pending, which skips
the first chunk. The readable count still includes it:

	avail = min(avail, conn->rmb_len - atomic_read(&conn->splice_pending));

Would that splice() return stale ring contents from beyond the producer
cursor?

When those pipe buffers are released, cons would move past prod, and
smc_tx_consumer_update() would send that cursor to the peer. bytes_to_rcv
would also stay inflated for good.

In the recvmsg path, a fault in memcpy_to_msg() on the second chunk returns
the first chunk to userspace without consuming it. Would the next recv()
deliver those bytes again?

The new clamp keeps the read inside the RMB, so this corrupts the stream
rather than memory. But it looks like the same double count that the commit
message says is fixed.

[Severity: High]
This is a pre-existing issue, but is a short successful splice counted here
as if the whole chunk went through?

For SMC-R with a virtually contiguous RMB, smc_rx_splice() works out the
number of pipe entries from the page offset:

	nr_pages = !lgr->is_smcd && smc->conn.rmb_desc->is_vm ?
		   PAGE_ALIGN(len + offset) / PAGE_SIZE : 1;

do_splice_read() limits len by the free slot count but ignores the source
page offset:

	p_space = pipe->max_usage - pipe_buf_usage(pipe);
	len = min_t(size_t, len, p_space << PAGE_SHIFT);

Take an empty 16 slot pipe, len = 65536 and an unaligned offset o.
smc_rx_splice() builds 17 entries and splice_to_pipe() returns 65536 - o.
smc_rx_splice() adds that short count to splice_pending.

smc_rx_recvmsg() only checks rc < 0, and then counts the full chunk:

			read_remaining -= chunk_len;
			read_done += chunk_len;

It also subtracts the full copylen from bytes_to_rcv here.

Would splice() then report o more bytes than reached the pipe, and leave
bytes_to_rcv o bytes below the real unread amount?

The pipe release only moves cons forward by what was actually queued, so the
deficit would add up. That could lose trailing data at EOF, or stall the
connection once the deficit gets close to the RMB size.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-b4-disp-33001ace-v8-0-f9bb96e17827%40proton.me