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

[Bryam Vargas <hexlabsecurity@proton.me>]

On Thu, 15 Oct 2026 20:15:20 +0000, netdev-bot+sashiko@kernel.org wrote:
> [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.

Yes, and this patch doesn't change it. When chunk 1 fails after chunk 0
went into the pipe, the goto out skips the bytes_to_rcv update for chunk
0, so the count stays high by that much while the pipe release moves
cons past it; the next read is off by chunk 0's length. Same before and
after 2/4. The reorder in 2/4 is for the other double count, a reader
that takes the lock between the cursor update and the splice_pending
drop. Hidayath Khan's v2, announced in [1], will account the first chunk
instead of taking that goto, on both the splice and the memcpy paths,
and would close this one.

> [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()?

It doesn't. The bytes that didn't reach the pipe stay in the RMB and
the next read returns them first; the count is what goes wrong, low by
the difference for the rest of the connection, so the tail is lost at
EOF. Measured with a vmalloc RMB over SMC-R, the cursor 100 bytes into
a page and a full 65536-byte RMB spliced into a 16-slot pipe: splice()
returns 65536, FIONREAD on the pipe shows 65436. Hidayath's patch [2]
covers it; not this series.

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

They should; v8 does it. (The raw reads left -- the stats in
smc_rx_recvmsg(), the write_blocked tests in smc_tx.c, the unread-data
test in smc_close.c -- size nothing and gate no reader.) With the count
driven negative (forged wrap bumps, 2^31/len of them) v7's poll()
reports POLLIN while recv() gives -EAGAIN, so a level-triggered reader
spins: 1,887,464 wakeups and 1000 ms of CPU in 1 s; SIOCINQ returns the
negative count; a deferred accept returns at once with nothing to read.
With smc_poll(), SIOCINQ and the accept precheck reading through
smc_rx_data_available(), the same run: 0 wakeups, SIOCINQ 0, accept()
waits out its timeout; with the count at 6x the RMB, SIOCINQ returns the
RMB (v7: 6x). Conforming traffic is unchanged: bytes_to_rcv +
splice_pending does not exceed the RMB there (short of the goto-out
double count above), so the min() is a no-op. The two "guarantee 0 <=
bytes_to_rcv <= rmb_desc->len" comments from your Low item say "can be
out of range" now.

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

It can: with a peek asleep at 8192 bytes while a second recv() takes
4096, the peek resumes 8192 bytes past the new cursor, so the peek never
shows the 4096 between what it had and that point (by inspection; the
measured run ends with the peek returning the 8192 it had and nothing
more arriving). 2/4 keeps that read inside the RMB, nothing more; a
peek_seq/copied_seq resync like TCP's is outside this series.

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

Yes; that is the spin [3] fixes, which 2/4 doesn't touch.

[1] https://lore.kernel.org/all/7a174d1f-80d8-4294-ae66-829c9dacd662@linux.ibm.com/
[2] https://lore.kernel.org/all/20260820085543.967490-1-hidayath@linux.ibm.com/
[3] https://lore.kernel.org/all/20261029-b4-disp-da7f8e6a-v2-1-9052904adf72@proton.me/

pw-bot: cr