Re: [PATCH mptcp-next v11 07/10] mptcp: implement peek_len for proto_ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing memory barriers between `ack_seq` and `SOCK_DONE` updates in `mptcp_check_data_fin()` and their lockless reads in `mptcp_inq()` allow `mptcp_inq()` to observe `SOCK_DONE` without observing the `ack_seq` increment, leading to an under-reporting of available data.
--

commit 3b3f825ecdc0054e45c6ee37b3404a7c1aeaead3
Author: Geliang Tang <geliang@kernel.org>

mptcp: implement peek_len for proto_ops

This commit introduces mptcp_inq() to compute readable bytes at the MPTCP
level using ack_seq and copied_seq. It returns 0 during handshakes and
subtracts 1 if a FIN is received (SOCK_DONE flag is set), mirroring
tcp_inq(). It is wired up as the .peek_len proto_op.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 2c0822ef4ae3b..81452a2b6fd0c 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -4809,6 +4809,37 @@ static ssize_t mptcp_splice_read(struct socket *sock, loff_t *ppos,
>  	return ret;
>  }
>  
> +static int mptcp_inq(struct sock *sk)
> +{
> +	const struct mptcp_sock *msk = mptcp_sk(sk);
> +	int answ;
> +
> +	if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV)) {
> +		answ = 0;
> +	} else if (test_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
> +		answ = 0;
> +	} else {
> +		u64 hint_val;
> +
> +		hint_val = READ_ONCE(msk->ack_seq) - READ_ONCE(msk->copied_seq);
> +		if (hint_val >= INT_MAX)
> +			hint_val = INT_MAX;
> +
> +		answ = (unsigned int)hint_val;
> +
> +		/* Subtract 1, if FIN was received. Mirror tcp_inq() */
> +		if (answ && sock_flag(sk, SOCK_DONE))
> +			answ--;

[Severity: High]
Could this lockless read observe SOCK_DONE without observing the updated
ack_seq?

In net/mptcp/protocol.c:mptcp_check_data_fin(), the write side updates
ack_seq and SOCK_DONE without a separating memory barrier:

    WRITE_ONCE(msk->ack_seq, msk->ack_seq + 1);
    WRITE_ONCE(msk->rcv_data_fin, 0);

    WRITE_ONCE(sk->sk_shutdown, sk->sk_shutdown | RCV_SHUTDOWN);
    sock_set_flag(sk, SOCK_DONE);
    smp_mb__before_atomic(); /* SHUTDOWN must be visible first */

If mptcp_inq() is called locklessly (e.g., from strp_data_ready) concurrently
with mptcp_check_data_fin(), could the lack of an smp_wmb() on the write side
and an smp_rmb() on this read side allow the CPU to reorder operations?

If mptcp_inq() reads SOCK_DONE as true but reads the old, un-incremented
ack_seq, it would calculate the available bytes and erroneously subtract 1.

If only 1 byte of payload remains, returning 1 byte less causes it to return
0. This could cause the upper layer (like TLS or BPF sockmap/strparser) to
skip waking up the process, resulting in an infinite wait for data that will
never arrive. This could be triggered remotely by network traffic (a data FIN
packet) on an MPTCP socket, leading to a denial of service.

Should there be an smp_rmb() added here before checking SOCK_DONE, and an
smp_wmb() added to mptcp_check_data_fin() after incrementing ack_seq?

> +	}
> +
> +	return answ;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788099847.git.tanggeliang@kylinos.cn?part=7