Re: [PATCH net 1/1] net: smc: fix splice entry lifetime imbalance in smc_rx_splice

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Silent data loss and socket state desynchronization on partial splice due to ignored short return value.
- [High] Legitimate negative error codes from smc_rx_splice() are incorrectly masked as -EFAULT.
--

commit 2f35c4cc130c3fdbc834b04e4b425109926602ac
Author: Daming Li <d4n.for.sec@gmail.com>

net: smc: fix splice entry lifetime imbalance in smc_rx_splice

This commit fixes a splice entry lifetime imbalance in smc_rx_splice() by
taking page and socket references for every candidate splice entry before
calling splice_to_pipe(). Unqueued entries are properly released via
smc_rx_spd_release().

> diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
> index c1d9b923938da..88aee0d935977 100644
> --- a/net/smc/smc_rx.c
> +++ b/net/smc/smc_rx.c
[ ... ]
> @@ -217,16 +226,8 @@ static int smc_rx_splice(struct pipe_inode_info *pipe, char *src, size_t len,
>  	spd.spd_release = smc_rx_spd_release;
>  
>  	bytes = splice_to_pipe(pipe, &spd);

[Severity: High]
This isn't a bug introduced by this patch, but this preexisting issue seems
to cause silent data loss on partial splices.

When smc_rx_splice() queues data into a pipe, it returns the actual number of
bytes successfully spliced. If the pipe nears capacity, this can be a short
count (0 < rc < chunk_len).

However, looking at its caller smc_rx_recvmsg():

net/smc/smc_rx.c:smc_rx_recvmsg() {
...
                } else {
                    rc = smc_rx_splice(pipe, rcvbuf_base +
                            chunk_off, chunk_len,
                            smc);
                }
...
            read_remaining -= chunk_len;
            read_done += chunk_len;
...
            atomic_sub(copylen, &conn->bytes_to_rcv);
...
}

Does this code completely ignore the actual bytes spliced (rc) and
unconditionally assume the full chunk_len was processed?

Since it subtracts the full copylen from conn->bytes_to_rcv, wouldn't this
permanently desynchronize the socket's internal receive buffer accounting
and cause silent data loss?

[Severity: High]
This is a pre-existing issue, but legitimate negative error codes from
smc_rx_splice() seem to be incorrectly masked as -EFAULT.

If smc_rx_splice() returns a negative system error like -EAGAIN or -ENOMEM,
the caller smc_rx_recvmsg() intercepts it:

net/smc/smc_rx.c:smc_rx_recvmsg() {
...
                } else {
                    rc = smc_rx_splice(pipe, rcvbuf_base +
                            chunk_off, chunk_len,
                            smc);
                }
                if (rc < 0) {
                    if (!read_done)
                        read_done = -EFAULT;
                    goto out;
                }
...
}

By overriding retryable conditions with a fatal memory error, wouldn't this
cause userspace applications relying on splice() and non-blocking sockets
to unexpectedly abort connections?

> -	if (bytes > 0) {
> -		sock_hold(&smc->sk);
> -		if (!lgr->is_smcd && smc->conn.rmb_desc->is_vm) {
> -			for (i = 0; i < PAGE_ALIGN(bytes + offset) / PAGE_SIZE; i++)
> -				get_page(pages[i]);
> -		} else {
> -			get_page(smc->conn.rmb_desc->pages);
> -		}
> +	if (bytes > 0)
>  		atomic_add(bytes, &smc->conn.splice_pending);
> -	}
>  	kfree(priv);
>  	kfree(partial);
>  	kfree(pages);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/192d1b44ed358ca143f44ef167d14153bccc51e9.1781097957.git.d4n.for.sec@gmail.com?part=1