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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (2) · Medium: 1 (1) · Low: 0 (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] In smc_rx_recvmsg() (net/smc/smc_rx.c), a wrapped read copies in
  two chunks.
- [Medium] The patch makes smc_rx_data_available() the bounded readiness
  measure: at most rmb_len - splice_pending - peeked, never negative.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  smc_rx_recvmsg() ignores a short positive return from smc_rx_splice().
- [High] This problem wasn't introduced by this patch, but recv() on an
  SMC socket can loop in the kernel indefinitely, ignoring MSG_DONTWAIT,…
- [Medium] This problem wasn't introduced by this patch, but a
  MSG_PEEK|MSG_WAITALL reader keeps peeked_bytes in a local variable…
- [Low] Two existing comments state '/* guarantee 0 <= bytes_to_rcv <=
  rmb_desc->len */': one at net/smc/smc_rx.c:514 in smc_rx_recvmsg(),…

--

net/smc: bound the receive length to the RMB

This patch clamps smc_rx_data_available() so it never reports more than
fits in the RMB behind the spliced and peeked bytes, and never a negative
count. It caches the RMB length in conn->rmb_len for the unlocked wait
predicates, and reorders smc_rx_pipe_buf_release() so released bytes leave
splice_pending before the consumer cursor moves.

> diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
> index 197fddc6271f0..c968ac874d8b9 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,

[ ... ]

>  	lock_sock(sk);
>  	if (conn->freed) {
>  		release_sock(sk);
>  		goto out;
>  	}
> +	/* drop the released bytes from splice_pending before cons moves
> +	 * past them, so no reader counts them twice
> +	 */
> +	drained = atomic_sub_and_test(priv->len, &conn->splice_pending);
>  	smc_rx_update_cons(smc, priv->len);

[Severity: High]
Can spliced bytes still be counted twice through the error path in
smc_rx_recvmsg()? A read that wraps the RMB end is copied in two chunks.
If chunk 0 succeeds and chunk 1 fails, the accounting for chunk 0 is
skipped:

net/smc/smc_rx.c:smc_rx_recvmsg() {
    ...
				if (rc < 0) {
					if (!read_done)
						read_done = -EFAULT;
					goto out;
				}
    ...
			atomic_sub(copylen, &conn->bytes_to_rcv);
    ...
}

By that point smc_rx_splice() has already done atomic_add(bytes,
&smc->conn.splice_pending) for chunk 0.

This looks easy to reach. do_splice_read() limits len to
p_space << PAGE_SHIFT. On SMC-D or physically contiguous SMC-R, each chunk
is a single pipe buffer. With one free pipe slot and data that straddles
the RMB end, chunk 0 takes the slot. splice_to_pipe() then returns -EAGAIN
for chunk 1 because the pipe is full.

When the pipe drains, smc_rx_pipe_buf_release() removes chunk 0 from
splice_pending and advances cons. bytes_to_rcv still includes chunk 0. The
next read sizes readable from a bytes_to_rcv that is too large by chunk 0's
length. It returns stale RMB bytes beyond prod and leaves the consumer
cursor sent to the peer ahead of the producer. Every later read is shifted
by the same amount.

The recvmsg path has a similar problem. If memcpy_to_msg() succeeds for
chunk 0 and faults on chunk 1, read_done is returned but the bytes are
never consumed, so the next recv() returns them again.

The commit message says this ordering ensures "no reader counts them
twice". The notes say the old order made "the next splice() returned stale
bytes in place of unread data". Is that still possible through this goto
out path?

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. Does
smc_rx_recvmsg() handle a short positive return from smc_rx_splice()?

For SMC-R with virtually contiguous RMBs (rmb_desc->is_vm),
smc_rx_splice() builds one buffer per page spanned:

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

do_splice_read() caps len at p_space << PAGE_SHIFT. That cap counts bytes,
not pages spanned. When the RMB offset is not page-aligned, a full-length
chunk spans p_space + 1 pages, so splice_to_pipe() inserts only p_space
buffers. For example, len 4096 at offset 100 with one free slot inserts
3996 bytes.

smc_rx_splice() adds only that short count to splice_pending. The caller
only checks rc < 0 and then accounts the full chunk:

			read_remaining -= chunk_len;
			read_done += chunk_len;
    ...
			atomic_sub(copylen, &conn->bytes_to_rcv);

Wouldn't splice() then report more bytes than actually reached the pipe?
And wouldn't bytes_to_rcv stay low by (copylen - rc) for good?

Repeated short splices make the deficit grow. The stranded bytes are never
delivered: at EOF, recvmsg sees bytes_to_rcv == 0 with RCV_SHUTDOWN and
returns 0 while data is still in the RMB.

[ ... ]

> diff --git a/net/smc/smc_rx.h b/net/smc/smc_rx.h
> index 994f5e42d1ba2..6bf44e3cc33a0 100644
> --- a/net/smc/smc_rx.h
> +++ b/net/smc/smc_rx.h
> @@ -25,7 +25,16 @@ 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

[Severity: Low]
This is a pre-existing issue, not one introduced by this patch. The new
comment contradicts two existing comments on the same field:

net/smc/smc_rx.c:smc_rx_recvmsg() {
    ...
			atomic_sub(copylen, &conn->bytes_to_rcv);
			/* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */
    ...
}

net/smc/smc_cdc.c:smc_cdc_msg_recv_action() {
    ...
		atomic_add(diff_prod, &conn->bytes_to_rcv);
		/* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */
    ...
}

Neither site enforces that bound. Could those two comments be corrected
along with this one?

> +	 * 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, conn->rmb_len - atomic_read(&conn->splice_pending));
> +	return avail > (int)peeked ? avail - (int)peeked : 0;

[Severity: Medium]
Now that this helper is bounded, should the other readiness checks use it
too? Several sites still read the raw counter:

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

net/smc/af_smc.c:smc_ioctl() {
    ...
			answ = atomic_read(&smc->conn.bytes_to_rcv);
    ...
}

The defer-accept precheck in smc_accept() does the same:

		} else if (!atomic_read(&smc_sk(nsk)->conn.bytes_to_rcv)) {

The notes say wrap bumps from the peer can still grow bytes_to_rcv. If it
goes negative, smc_rx_data_available() returns 0 and a MSG_DONTWAIT recv()
gets -EAGAIN from smc_rx_recvmsg(). smc_poll() keeps reporting EPOLLIN
for the same socket. Before this patch a negative result was truthy, so
poll and recvmsg agreed.

Can a level-triggered epoll loop now spin on such a socket? SIOCINQ and
FIONREAD would also return a negative or larger-than-RMB value to
userspace.

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. The clamp
keeps the MSG_PEEK|MSG_WAITALL case inside the RMB, but the peeked data can
still have a gap in it.

peeked_bytes is a local that survives smc_rx_wait(), which drops the
socket lock. Suppose another recv() consumes M bytes in that window. The
peeker then resumes from the moved cursor plus the stale count:

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

Doesn't the peeking reader then skip M bytes it never saw? TCP catches
this by comparing peek_seq with copied_seq and resyncing, and there is no
matching check here.

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. Can recv()
loop in the kernel indefinitely when unread data and unreleased spliced
bytes coexist?

Say part of the data is spliced into a pipe that nobody drains. Then
splice_pending > 0 and bytes_to_rcv > 0, and smc_rx_data_available() stays
positive as long as splice_pending < rmb_len. The loop in
smc_rx_recvmsg() then runs:

		if (smc_rx_recvmsg_data_available(smc, peeked_bytes))
			goto copy;
    ...
copy:
    ...
		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;
		}

The goto skips the !timeo and signal_pending() checks. The return value of
smc_rx_wait() is ignored, and read_remaining does not change. So this
repeats until the pipe is drained or conn->killed is set.

With MSG_DONTWAIT, each pass sleeps about one jiffy instead of returning
-EAGAIN. With a fatal signal pending, or once sk_err or RCV_SHUTDOWN makes
the wait condition true, smc_rx_wait() returns at once. The task then
spins at full CPU and never reaches a signal check.

This patch narrows the window to 0 < splice_pending < rmb_len but does not
close it.

>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013-b4-disp-f345196c-v7-0-ef3dddfbfa68%40proton.me