Re: [PATCH mptcp-next v6 3/7] selftests: mptcp: connect: check TFO SIOCINQ overflow

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests: mptcp: commit message mismatch on polling remotesock in main_loop_s()
- [Medium] selftests: mptcp: SIOCINQ check in main_loop_s() races with TFO third ACK
--

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

selftests: mptcp: connect: check TFO SIOCINQ overflow

This commit adds a SIOCINQ check in mptcp_connect's server mode when the
MPTFO socket option is enabled to detect TFO SIOCINQ overflow regressions.

>     selftests: mptcp: connect: check TFO SIOCINQ overflow
>     
>     Add a check in mptcp_connect's server mode to detect TFO SIOCINQ overflow.
>     When a TFO connection is accepted, poll for data to arrive, then check if
>     SIOCINQ returns INT_MAX. This detects the regression where mptcp_inq_hint()
>     incorrectly returns INT_MAX for passive TFO connections.
>     
>     The check runs when the MPTFO sockopt is set. It polls the accepted socket
>     for up to one second, then uses ioctl(SIOCINQ) to verify the available
>     byte count. If SIOCINQ returns INT_MAX, the test fails.
>     
>     This test verifies the fix in the preceding commit "mptcp: fix TFO SIOCINQ
>     overflow in mptcp_inq_hint".
>     
>     Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>

[Severity: Medium]
Does the commit message match the actual code in main_loop_s()? The commit
message states in two places that the test polls the accepted socket for up to
one second before calling ioctl(SIOCINQ), whereas main_loop_s() in
tools/testing/selftests/net/mptcp/mptcp_connect.c invokes ioctl(remotesock,
SIOCINQ, &avail) directly without calling poll().

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> index 178d98d91fea2..83db495704fec 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> @@ -1207,6 +1207,17 @@ int main_loop_s(int listensock)
>  		check_sockaddr(pf, &ss, salen);
>  		check_getpeername(remotesock, &ss, salen);
>  
> +		if (cfg_sockopt_types.mptfo) {
> +			int avail;
> +
> +			if (ioctl(remotesock, SIOCINQ, &avail) == 0 &&
> +			    avail == INT_MAX) {
> +				fprintf(stderr, "TFO SIOCINQ overflow\n");
> +				close(remotesock);
> +				return 1;
> +			}
> +		}

[Severity: Medium]
Can this SIOCINQ check in main_loop_s() race with the TFO third ACK and miss
the overflow regression on unfixed kernels?

When a passive MPTCP Fast Open SYN+data packet with a valid cookie arrives in
tcp_conn_request(), the child msk is created via:

  tcp_try_fastopen()
    -> tcp_fastopen_create_child()
      -> subflow_syn_recv_sock()
        -> mptcp_sk_clone_init()

At that point mp_opt->suboptions only has OPTION_MPTCP_MPC_SYN (not
OPTION_MPTCP_MPC_ACK), leaving msk->ack_seq at 0. Next, af_ops->send_synack()
calls subflow_prep_synack() -> mptcp_fastopen_subflow_synack_set_params() in
net/mptcp/fastopen.c:

	MPTCP_SKB_CB(skb)->map_seq = -skb->len;
	MPTCP_SKB_CB(skb)->end_seq = 0;
	...
	__skb_queue_tail(&sk->sk_receive_queue, skb);

Then tcp_fastopen_create_child() immediately adds the child socket to the
listener accept queue via inet_csk_reqsk_queue_add() and wakes the listener via
sk->sk_data_ready().

Before the client receives the SYN-ACK and replies with the third ACK carrying
OPTION_MPTCP_MPC_ACK, msk->ack_seq is still 0. If main_loop_s() accepts the
socket and calls ioctl(remotesock, SIOCINQ, &avail) before the third ACK is
processed, even an unfixed mptcp_inq_hint() computes:

  READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq = 0 - (-skb->len) = skb->len

rather than overflowing to INT_MAX. The overflow to subflow->iasn + skb->len >=
INT_MAX only occurs after the server processes the third ACK in:

  tcp_rcv_state_process()
    -> tcp_data_queue()
      -> mptcp_incoming_options()
        -> check_fully_established()
          -> __mptcp_subflow_fully_established()
            -> subflow_set_remote_key()

which sets WRITE_ONCE(msk->ack_seq, subflow->iasn) in net/mptcp/subflow.c.
Note also that simply polling POLLIN on remotesock would return immediately
because mptcp_fastopen_subflow_synack_set_params() already queued the SYN data
onto sk_receive_queue prior to accept().

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