Re: [PATCH mptcp-next v10 6/9] 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] mptcp_inq() erroneously subtracts 1 from the available data count when the socket is locally shut down (SHUT_RD) or closed (TCP_CLOSE) without receiving a remote DATA_FIN.
--

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

mptcp: implement peek_len for proto_ops

This commit adds mptcp_inq() to compute readable bytes at the MPTCP level
by calculating the difference between ack_seq and copied_seq. It also
wires this calculation into the .peek_len stream operations so upper layers
can accurately query the in-queue byte count.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index fce85967b90ee..639122b7845a6 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -4797,6 +4797,39 @@ 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 &&
> +		    (sk->sk_state == TCP_CLOSE ||
> +		     (sk->sk_shutdown & RCV_SHUTDOWN)))
> +			answ--;

[Severity: High]
Does this logic correctly handle local shutdowns or connections reset via RST?

If an MPTCP socket has 1 byte of unread data and a local application calls
shutdown(fd, SHUT_RD), it sets RCV_SHUTDOWN without incrementing ack_seq
because no remote DATA_FIN was received.

Since this subtracts 1 from the answer, it would return 0. Could this cause
kernel consumers of .peek_len() (like KTLS/BPF strparser) to falsely assume
no data is available and hang indefinitely waiting for data that will never
arrive on the shut down socket?

> +	}
> +
> +	return answ;
> +}
[ ... ]

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