Re: [PATCH mptcp-next v2 2/5] 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: off-by-one undercount in mptcp_inq() when rcv_data_fin is set
--

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

mptcp: implement peek_len for proto_ops

This commit adds mptcp_inq() and wires it into mptcp_peek_len() to implement
the .peek_len callback for both IPv4 and IPv6 MPTCP stream proto_ops. It
computes available bytes using mptcp_data_avail() and subtracts 1 when
msk->rcv_data_fin is set.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 0caa31525b907..6f34e3459e8ac 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -4797,6 +4797,33 @@ 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);
> +	u64 avail;
> +	int answ;
> +
> +	if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV))
> +		return 0;
> +
> +	avail = mptcp_data_avail(msk);
> +	if (avail >= INT_MAX)
> +		return 0;
> +
> +	answ = (unsigned int)avail;
> +
> +	/* Subtract 1, if DATA_FIN was received. Mirror tcp_inq() */
> +	if (answ && READ_ONCE(msk->rcv_data_fin))
> +		answ--;

[Severity: High]
Does subtracting 1 when msk->rcv_data_fin is set cause mptcp_inq() to
under-report the readable payload by 1 byte?

In tcp_inq(), 1 is subtracted from tp->rcv_nxt - tp->copied_seq when a fin is
received because tp->rcv_nxt advances by 1 for the fin sequence number. Here,
however, mptcp_inq() starts from mptcp_data_avail(msk):

net/mptcp/protocol.h:mptcp_data_avail() {
	return READ_ONCE(msk->bytes_received) - READ_ONCE(msk->bytes_consumed);
}

When a subflow receives a mapping with data_fin set, get_mapping_status() in
net/mptcp/subflow.c strips the 1 byte of sequence space before the payload is
queued:

net/mptcp/subflow.c:get_mapping_status() {
	...
		mptcp_update_rcv_data_fin(msk, data_fin_seq, mpext->dsn64);
		pr_debug("DATA_FIN with mapping seq=%llu dsn64=%d\n",
			 data_fin_seq, mpext->dsn64);

		/* Adjust for DATA_FIN using 1 byte of sequence space */
		data_len--;
	...
}

And when mptcp_check_data_fin() in net/mptcp/protocol.c later processes the
pending fin, it only advances msk->ack_seq while clearing msk->rcv_data_fin:

net/mptcp/protocol.c:mptcp_check_data_fin() {
	...
	if (mptcp_pending_data_fin(sk, &rcv_data_fin_seq)) {
		WRITE_ONCE(msk->ack_seq, msk->ack_seq + 1);
		WRITE_ONCE(msk->rcv_data_fin, 0);
	...
}

Since msk->bytes_received only ever counts actual payload bytes queued into
sk->sk_receive_queue, mptcp_data_avail(msk) never includes the extra sequence
byte.

When a peer sends the final bytes of a message together with data_fin (or
when data_fin arrives out of order while unread payload is queued),
mptcp_data_ready() -> move_skbs_to_msk() in net/mptcp/protocol.c moves the
payload to sk->sk_receive_queue, defers mptcp_check_data_fin() to
mptcp_worker:

net/mptcp/protocol.c:move_skbs_to_msk() {
	...
	if (mptcp_pending_data_fin(sk, NULL))
		mptcp_schedule_work(sk);
	return moved;
}

and immediately invokes sk->sk_data_ready(sk) while msk->rcv_data_fin is
still 1.

When strparser (used by ktls rx, kcm, and sockmap) handles that callback via
strp_data_ready() -> strp_recv() in net/strparser/strparser.c:

net/strparser/strparser.c:strp_recv() {
	...
			if (stm->strp.full_len - stm->accum_len >
			    strp_peek_len(strp)) {
				/* Don't have whole message in the socket
				 * buffer. Set timer to wait for the rest
				 * of the message.
				 */
	...
}

strp_peek_len() -> mptcp_peek_len() -> mptcp_inq() will return 1 byte less
than the actual unread payload, causing strp_recv() to stop reading and stall
until the message timeout expires?

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