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

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

Pre-existing issues:
- [Critical] SMC receive buffers (`rmb_desc`) are returned to the reusable pool when a connection is freed, even if those buffers are still mapped in a pipe via `splice_to_pipe`. A new connection can reuse the buffer, allowing the old pipe reader to read the new connection's data.
- [High] `smc_rx_pipe_buf_release()` checks `sk->sk_state` outside the socket lock, creating a TOCTOU race condition that leads to a Use-After-Free of connection resources (like `conn->rmb_desc` and `conn->lgr`) if the socket is closed concurrently.
--

--- Patch [1]: [PATCH net v2 1/1] net: smc: fix splice entry lifetime imbalance in smc_rx_splice ---
commit c6407321bb893fb435f1c82222aefbb185ac53e0
Author: Daming Li <[email protected]>

net: smc: fix splice entry lifetime imbalance in smc_rx_splice

smc_rx_splice() passes pages to splice_to_pipe() before taking the
references that cover the lifetime of each splice entry. Take the page and
socket references for every candidate entry before splice_to_pipe(), and
drop the matching private state, page reference, and socket reference from
smc_rx_spd_release() for entries that never get queued.

> diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
> index c1d9b923938da..5c9e4d8b57de4 100644
> --- a/net/smc/smc_rx.c
> +++ b/net/smc/smc_rx.c

[Severity: High]
This isn't a bug introduced by this patch, but could this state check in
smc_rx_pipe_buf_release() lead to a race condition?

net/smc/smc_rx.c:smc_rx_pipe_buf_release() {
    ...
	if (sk->sk_state == SMC_CLOSED ||
	    sk->sk_state == SMC_PEERFINCLOSEWAIT ||
	    sk->sk_state == SMC_APPFINCLOSEWAIT)
		goto out;
	conn = &smc->conn;
	lock_sock(sk);
	smc_rx_update_cons(smc, priv->len);
    ...
}

If the socket state is checked outside the socket lock, can the state change
before lock_sock() is acquired?

For example, if close() runs concurrently, could it transition the state to
SMC_CLOSED and free the connection resources? When smc_rx_pipe_buf_release()
eventually acquires the lock, it doesn't recheck the state before calling
smc_rx_update_cons(), which might access the freed conn->rmb_desc->len.


[Severity: Critical]
This is also a pre-existing issue, but does smc_buf_unuse() return receive
buffers to the reusable pool while they might still be mapped in a splice
pipe?

net/smc/smc_core.c:smc_buf_unuse() {
    ...
	if (conn->rmb_desc) {
		bufsize = conn->rmb_desc->len;
		if (!is_smcd) {
			smcr_buf_unuse(conn->rmb_desc, true, lgr);
		} else {
    ...
}

When a socket is closed, smc_release() calls smc_conn_free(), which calls
smc_buf_unuse() to mark the buffer unused.

If a user had spliced data into a pipe, the buffer pages might still be
mapped. Could a new connection reuse this buffer and write its payload into
memory that is still accessible via the old pipe? This appears it might
allow an old pipe reader to read a new connection's data.

> @@ -150,7 +150,12 @@ static const struct pipe_buf_operations smc_pipe_ops = {
>  static void smc_rx_spd_release(struct splice_pipe_desc *spd,
>  			       unsigned int i)
>  {
> +	struct smc_spd_priv *priv = (struct smc_spd_priv *)spd->partial[i].private;
> +	struct sock *sk = &priv->smc->sk;
> +
> +	kfree(priv);
>  	put_page(spd->pages[i]);
> +	sock_put(sk);
>  }

[ ... ]

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