Re: [PATCH net] net/smc: don't spin in smc_rx_recvmsg() while a pipe holds spliced data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] smc: missing SOCKWQ_ASYNC_WAITDATA flag causes spurious SIGIO and -EINTR
--

commit ffdc9d46059a93d975c6fd059f43bc43b71fe12f
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/smc: don't spin in smc_rx_recvmsg() while a pipe holds spliced data

This commit introduces smc_rx_wait_splice() to allow recvmsg() to sleep
instead of spinning on the CPU while bytes spliced off the RMB are still in
a pipe. It checks for timeouts and signals before waiting, ending the wait
when the pipe releases the data or the connection is killed.

> diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
> index 197fddc6271f0..6f409bda4c37a 100644
> --- a/net/smc/smc_rx.c
> +++ b/net/smc/smc_rx.c
[ ... ]
> @@ -286,6 +286,22 @@ int smc_rx_wait(struct smc_sock *smc, long *timeo, size_t peeked,
>  	return rc;
>  }
>  
> +/* Wait until the pipe releases the spliced data. Unlike smc_rx_wait(),
> + * this does not end on shutdown or a socket error: the data to read is
> + * already in the RMB. smc_conn_kill() wakes the socket, and the caller
> + * checks conn->killed.
> + */
> +static void smc_rx_wait_splice(struct smc_sock *smc, long *timeo)
> +{
> +	DEFINE_WAIT_FUNC(wait, woken_wake_function);
> +	struct smc_connection *conn = &smc->conn;
> +	struct sock *sk = &smc->sk;
> +
> +	add_wait_queue(sk_sleep(sk), &wait);
> +	sk_wait_event(sk, timeo, !atomic_read(&conn->splice_pending), &wait);

[Severity: Medium]
Does smc_rx_wait_splice() need to set and clear the SOCKWQ_ASYNC_WAITDATA bit
around sk_wait_event()?

If FASYNC is enabled on an SMC socket, smc_rx_wake_up() will send a SIGIO
signal to the process unless the SOCKWQ_ASYNC_WAITDATA bit is set.

Since smc_rx_wait_splice() puts the socket to sleep without setting this bit,
a spurious SIGIO could be delivered if the peer sends more data or the pipe
drains while the process is sleeping here.

This could interrupt wait_woken(), causing smc_rx_recvmsg() to prematurely
abort the wait and return -EINTR (or a short read) instead of waiting for
the pipe to drain. While applications generally handle -EINTR by retrying,
this breaks the expected blocking semantics.

For comparison, smc_rx_wait() uses this pattern:

int smc_rx_wait(...) {
    ...
    sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
    add_wait_queue(sk_sleep(sk), &wait);
    rc = sk_wait_event(...);
    remove_wait_queue(sk_sleep(sk), &wait);
    sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);
    ...
}

> +	remove_wait_queue(sk_sleep(sk), &wait);
> +}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-b4-disp-193a2a59-v1-1-ed88ac10a41c@proton.me?part=1