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

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

[Hidayath Khan]

smc_rx_recvmsg() offers a chunk of the RMB to splice_to_pipe() and then
credits the whole chunk regardless of what was taken.

splice_to_pipe() takes only what the pipe has room for. A short count and
a zero count are both non-negative, so both fall through as success: the
call returns more than it delivered, and the consumer cursor and
bytes_to_rcv are advanced by copylen rather than by what the reader
received. The skipped bytes are never handed to anyone.

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.

Fixes: 9014db202cb7 ("smc: add support for splice()")
Cc: stable@vger.kernel.org
Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>
Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
---
 net/smc/smc_rx.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
index 5c9e4d8b57de..d951dbe8eec3 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;
 				}
+				/* 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;
+					}
+					copylen = chunk_len_sum - chunk_len + rc;
+					read_remaining -= rc;
+					read_done += rc;
+					break;
+				}
 			}
 			read_remaining -= chunk_len;
 			read_done += chunk_len;

base-commit: d3083202a78cd0040be6a88953f728b0d3db1990
-- 
2.52.0

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

[sashiko-bot]

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 <hidayath@linux.ibm.com>

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/20260820085543.967490-1-hidayath@linux.ibm.com?part=1

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

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260820085543.967490-1-hidayath@linux.ibm.com?part=1
---
net/smc: do not credit bytes that splice() did not take

The patch adds a short-count path to the chunk loop in smc_rx_recvmsg()
so that only the number of bytes splice_to_pipe() actually accepted is
credited to copylen, read_remaining and read_done.  It also adds a
zero-count path that returns -EAGAIN when nothing was taken.

>     A pipe that is not empty is enough to hit this; SPLICE_F_NONBLOCK on a
>     full pipe produces the zero case.

[Severity: Low]
Is "A pipe that is not empty is enough to hit this" accurate?  A non-zero
short return from splice_to_pipe() requires smc_rx_splice() to submit more
than one page:

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

For SMC-D and for SMC-R with physically contiguous RMBs nr_pages is 1 and
the single pipe_buffer carries partial[0].len == len, so the transfer is
all-or-nothing.  Together with wait_for_space() in splice_file_to_pipe()
guaranteeing at least one free slot on entry, doesn't the short-count case
only occur for SMC-R with is_vm RMBs?

Would it help to name that configuration in the changelog so backporters
know what to test?

> 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 since the patch reworks
exactly this accounting: does this pre-existing exit leak bytes_to_rcv on
a wrapped read?

For a read that wraps the ring, chunk 0 can succeed and chunk 1 fail.
smc_rx_splice() has already committed chunk 0:

net/smc/smc_rx.c:smc_rx_splice() {
	bytes = splice_to_pipe(pipe, &spd);
	if (bytes > 0)
		atomic_add(bytes, &smc->conn.splice_pending);

and the queued buffers advance the consumer cursor when the reader drains
them:

net/smc/smc_rx.c:smc_rx_pipe_buf_release() {
	lock_sock(sk);
	smc_rx_update_cons(smc, priv->len);
	release_sock(sk);

Chunk 1 then finds the pipe full.  fs/splice.c:splice_to_pipe() converts
no progress to an error rather than 0:

	if (!ret)
		ret = -EAGAIN;

so control takes the quoted "if (rc < 0) ... goto out" with read_done ==
chunk 0 length, jumping past the only debit site in smc_rx_recvmsg():

	atomic_sub(copylen, &conn->bytes_to_rcv);

Doesn't that leave cons advanced by the delivered bytes while
bytes_to_rcv still counts them?  smc_rx_data_available() reads
atomic_read(&conn->bytes_to_rcv) - peeked, so would a subsequent
recvmsg()/splice() start at the advanced cursor and copy that many bytes
of stale RMB content from past the producer position?

Also, can the documented invariant "0 <= bytes_to_rcv <= rmb_desc->len"
be violated if this repeats?

> +				/* 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: Medium]
Can rc ever be 0 here?  splice_to_pipe() returns 0 only for
spd->nr_pages == 0, and otherwise ends with:

fs/splice.c:splice_to_pipe() {
	if (!ret)
		ret = -EAGAIN;

smc_rx_splice() always builds nr_pages >= 1 and returns the
splice_to_pipe() value unchanged, so the "took nothing" case arrives as
-EAGAIN and is consumed by the pre-existing block above, which reports
-EFAULT to userspace.  Does that make this "if (!rc)" arm unreachable,
and the changelog claim

     When it took nothing, return -EAGAIN instead of reporting a length
     that was not delivered.

not take effect?

On the same note, does "SPLICE_F_NONBLOCK on a full pipe produces the
zero case" hold?  splice_file_to_pipe() calls wait_for_space() before
do_splice_read():

fs/splice.c:wait_for_space() {
		if (!pipe_is_full(pipe))
			return 0;
		if (flags & SPLICE_F_NONBLOCK)
			return -EAGAIN;

so a completely full pipe fails before smc_splice_read() is entered.
Should the comment block above also be reworded, since it describes a
callee returning zero that splice_to_pipe() does not do?

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

[Severity: High]
This is a pre-existing issue outside the diff, but this patch depends on
the invariant it breaks, so it may be worth mentioning: in
smc_rx_pipe_buf_release() the cursor advance happens under the socket
lock while the splice_pending decrement happens after it is dropped:

net/smc/smc_rx.c:smc_rx_pipe_buf_release() {
	lock_sock(sk);
	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_recvmsg() assumes cons plus splice_pending marks the start of
unread data:

	splbytes = atomic_read(&conn->splice_pending);
	...
	smc_curs_copy(&cons, &conn->local_tx_ctrl.cons, conn);
	...
	if (splbytes)
		smc_curs_add(conn->rmb_desc->len, &cons, splbytes);

In the window between release_sock() and atomic_sub_and_test(), cons
already includes priv->len and splice_pending still counts the same
bytes.  Can a concurrent splice on the same socket then start priv->len
bytes too far, skipping unread data and handing stale ring content to the
pipe?

The recvmsg() path looks protected because it waits while splice_pending
is non-zero:

		if (!readable || (msg && splbytes)) {

but the splice path (msg == NULL) is not gated on splice_pending.  The
pipe mutex doesn't serialize this either, since splice_file_to_pipe()
holds only the destination pipe's lock, so draining pipe A can race with
splicing the same socket into pipe B.

Would moving the atomic_sub_and_test() before release_sock() make cons
and splice_pending change together with respect to the socket lock?

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

[Hidayath Khan]

Thanks for the review, Simon.

All four points are correct. Because the 3rd point removes part of the 
patch,
v2 will be sent as a series.

On 24/08/26 4:17 pm, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260820085543.967490-1-hidayath@linux.ibm.com?part=1
> ---
> net/smc: do not credit bytes that splice() did not take
>
> The patch adds a short-count path to the chunk loop in smc_rx_recvmsg()
> so that only the number of bytes splice_to_pipe() actually accepted is
> credited to copylen, read_remaining and read_done.  It also adds a
> zero-count path that returns -EAGAIN when nothing was taken.
>
>>      A pipe that is not empty is enough to hit this; SPLICE_F_NONBLOCK on a
>>      full pipe produces the zero case.
> [Severity: Low]
> Is "A pipe that is not empty is enough to hit this" accurate?  A non-zero
> short return from splice_to_pipe() requires smc_rx_splice() to submit more
> than one page:
>
> net/smc/smc_rx.c:smc_rx_splice() {
> 	nr_pages = !lgr->is_smcd && smc->conn.rmb_desc->is_vm ?
> 		   PAGE_ALIGN(len + offset) / PAGE_SIZE : 1;
>
> For SMC-D and for SMC-R with physically contiguous RMBs nr_pages is 1 and
> the single pipe_buffer carries partial[0].len == len, so the transfer is
> all-or-nothing.  Together with wait_for_space() in splice_file_to_pipe()
> guaranteeing at least one free slot on entry, doesn't the short-count case
> only occur for SMC-R with is_vm RMBs?

Yes. For SMC-D and SMC-R with physically contiguous RMBs, nr_pages is 1, so
the transfer is all-or-nothing. I will clarify in the commit message that a
short count only happens on SMC-R with virtually contiguous RMBs (is_vm).

>
> Would it help to name that configuration in the changelog so backporters
> know what to test?
>
>> 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 since the patch reworks
> exactly this accounting: does this pre-existing exit leak bytes_to_rcv on
> a wrapped read?
Yes. The "goto out" skips atomic_sub(), so chunk 0 bytes are never
subtracted from bytes_to_rcv. But the pipe reader still updates the cursor,
leaving bytes_to_rcv permanently incorrect.

I will fix this in the series: when chunk 1 fails after chunk 0 was
delivered, set copylen to chunk 0 length and break instead of "goto out".
The memcpy_to_msg() path has the same problem and will be fixed as well.
>
> For a read that wraps the ring, chunk 0 can succeed and chunk 1 fail.
> smc_rx_splice() has already committed chunk 0:
>
> net/smc/smc_rx.c:smc_rx_splice() {
> 	bytes = splice_to_pipe(pipe, &spd);
> 	if (bytes > 0)
> 		atomic_add(bytes, &smc->conn.splice_pending);
>
> and the queued buffers advance the consumer cursor when the reader drains
> them:
>
> net/smc/smc_rx.c:smc_rx_pipe_buf_release() {
> 	lock_sock(sk);
> 	smc_rx_update_cons(smc, priv->len);
> 	release_sock(sk);
>
> Chunk 1 then finds the pipe full.  fs/splice.c:splice_to_pipe() converts
> no progress to an error rather than 0:
>
> 	if (!ret)
> 		ret = -EAGAIN;
>
> so control takes the quoted "if (rc < 0) ... goto out" with read_done ==
> chunk 0 length, jumping past the only debit site in smc_rx_recvmsg():
>
> 	atomic_sub(copylen, &conn->bytes_to_rcv);
>
> Doesn't that leave cons advanced by the delivered bytes while
> bytes_to_rcv still counts them?  smc_rx_data_available() reads
> atomic_read(&conn->bytes_to_rcv) - peeked, so would a subsequent
> recvmsg()/splice() start at the advanced cursor and copy that many bytes
> of stale RMB content from past the producer position?
>
> Also, can the documented invariant "0 <= bytes_to_rcv <= rmb_desc->len"
> be violated if this repeats?
>
>> +				/* 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: Medium]
> Can rc ever be 0 here?  splice_to_pipe() returns 0 only for
> spd->nr_pages == 0, and otherwise ends with:
>
> fs/splice.c:splice_to_pipe() {
> 	if (!ret)
> 		ret = -EAGAIN;
>
> smc_rx_splice() always builds nr_pages >= 1 and returns the
> splice_to_pipe() value unchanged, so the "took nothing" case arrives as
> -EAGAIN and is consumed by the pre-existing block above, which reports
> -EFAULT to userspace.  Does that make this "if (!rc)" arm unreachable,
> and the changelog claim
>
>       When it took nothing, return -EAGAIN instead of reporting a length
>       that was not delivered.
>
> not take effect?
Yes, it is unreachable. smc_rx_splice() always passes nr_pages >= 1, so
splice_to_pipe() returns -EAGAIN instead of 0.

The zero case arrives as -EAGAIN, which the existing code turns into
-EFAULT. In v2, I will drop the dead arm and return rc directly instead
of -EFAULT (which also fixes -ENOMEM reporting). I will also reword the
comment.
>
> On the same note, does "SPLICE_F_NONBLOCK on a full pipe produces the
> zero case" hold?  splice_file_to_pipe() calls wait_for_space() before
> do_splice_read():
>
> fs/splice.c:wait_for_space() {
> 		if (!pipe_is_full(pipe))
> 			return 0;
> 		if (flags & SPLICE_F_NONBLOCK)
> 			return -EAGAIN;
>
> so a completely full pipe fails before smc_splice_read() is entered.
> Should the comment block above also be reworded, since it describes a
> callee returning zero that splice_to_pipe() does not do?
>
>> +					copylen = chunk_len_sum - chunk_len + rc;
>> +					read_remaining -= rc;
>> +					read_done += rc;
>> +					break;
>> +				}
>>   			}
>>   			read_remaining -= chunk_len;
>>   			read_done += chunk_len;
> [Severity: High]
> This is a pre-existing issue outside the diff, but this patch depends on
> the invariant it breaks, so it may be worth mentioning: in
> smc_rx_pipe_buf_release() the cursor advance happens under the socket
> lock while the splice_pending decrement happens after it is dropped:
>
> net/smc/smc_rx.c:smc_rx_pipe_buf_release() {
> 	lock_sock(sk);
> 	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_recvmsg() assumes cons plus splice_pending marks the start of
> unread data:
>
> 	splbytes = atomic_read(&conn->splice_pending);
> 	...
> 	smc_curs_copy(&cons, &conn->local_tx_ctrl.cons, conn);
> 	...
> 	if (splbytes)
> 		smc_curs_add(conn->rmb_desc->len, &cons, splbytes);
>
> In the window between release_sock() and atomic_sub_and_test(), cons
> already includes priv->len and splice_pending still counts the same
> bytes.  Can a concurrent splice on the same socket then start priv->len
> bytes too far, skipping unread data and handing stale ring content to the
> pipe?
>
> The recvmsg() path looks protected because it waits while splice_pending
> is non-zero:
>
> 		if (!readable || (msg && splbytes)) {
>
> but the splice path (msg == NULL) is not gated on splice_pending.  The
> pipe mutex doesn't serialize this either, since splice_file_to_pipe()
> holds only the destination pipe's lock, so draining pipe A can race with
> splicing the same socket into pipe B.
>
> Would moving the atomic_sub_and_test() before release_sock() make cons
> and splice_pending change together with respect to the socket lock?
Yes. I will move the subtraction inside lock_sock() and keep the wake-up
outside. I will send this in a separate patch.

v2 will follow as a series

Thanks,
Hidayath