Re: [PATCH net] net/smc: do not credit bytes that splice() did not take

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

Pre-existing issues:
- [High] Error paths and early exits during the second chunk of a wrapped read skip trailing socket state updates, causing data duplication and leaking `bytes_to_rcv`.
- [High] The patch fails to handle a full pipe correctly, introducing dead code because `splice_to_pipe()` returns `-EAGAIN`, which is intercepted by the preexisting `rc < 0` handler and mapped to `-EFAULT`.
--

--- Patch [1]: [PATCH net] net/smc: do not credit bytes that splice() did not take ---
commit 13a906bee6c672b72e5c22c2fd7111bc713c0c62
Author: Hidayath Khan <[email protected]>

net/smc: do not credit bytes that splice() did not take

This patch modifies smc_rx_recvmsg() to account for the actual number of
bytes consumed by splice_to_pipe(). It prevents advancing the consumer cursor
when splice_to_pipe() takes less data than offered, and stops processing
if the pipe is full.

>     A pipe that is not empty is enough to hit this; SPLICE_F_NONBLOCK on a
>     full pipe produces the zero case.
>     
>     Account what splice_to_pipe() actually took, and stop there rather than
>     continuing to the second chunk of a wrapped read. When it took nothing,
>     return -EAGAIN instead of reporting a length that was not delivered.

Is this description accurate?

splice_to_pipe() natively returns -EAGAIN when the pipe is full and the call
is non-blocking. Since -EAGAIN is negative, it gets intercepted by the
pre-existing error handler which maps it to -EFAULT when read_done is 0.

Does this mean the zero-case handling added in this patch is unreachable?

> diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
> index 5c9e4d8b57de4..d951dbe8eec3c 100644
> --- a/net/smc/smc_rx.c
> +++ b/net/smc/smc_rx.c
> @@ -488,6 +488,24 @@ int smc_rx_recvmsg(struct smc_sock *smc, struct msghdr *msg,
>  						read_done = -EFAULT;
>  					goto out;
>  				}

[Severity: High]
This isn't a bug introduced by this patch, but does this error handler
unintentionally map -EAGAIN to -EFAULT?

If smc_rx_splice() returns -EAGAIN on a full pipe, this block catches the
negative return value and changes it to -EFAULT when read_done is 0,
completely bypassing the new logic below.

> +				/* splice_to_pipe() takes only what the pipe
> +				 * has room for, which may be less than was
> +				 * offered, or nothing. Account what it took
> +				 * and stop: crediting the whole chunk would
> +				 * advance the consumer past data the reader
> +				 * never received.
> +				 */
> +				if (!msg && rc < chunk_len) {
> +					if (!rc) {
> +						if (!read_done)
> +							read_done = -EAGAIN;
> +						goto out;
> +					}

[Severity: High]
This is a pre-existing issue, but does using 'goto out' here skip trailing
socket state updates?

If a ring buffer wrap occurs requiring two chunks, and the first chunk is
successfully processed (read_done > 0), but the second chunk encounters an
error or returns 0, jumping to 'out' bypasses the batched cursor update:

net/smc/smc_rx.c:smc_rx_recvmsg() {
    ...
    /* update cursors */
    if (!(flags & MSG_PEEK)) {
        ...
        atomic_sub(copylen, &conn->bytes_to_rcv);
        ...
        if (msg && smc_rx_update_consumer(smc, cons, copylen))
            goto out;
    }
    ...
}

Does this leave the socket state permanently corrupted by leaking
bytes_to_rcv and duplicating data on the next read?

> +					copylen = chunk_len_sum - chunk_len + rc;
> +					read_remaining -= rc;
> +					read_done += rc;
> +					break;
> +				}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1