Re: [RFC PATCH mptcp-next v8 4/7] mptfo variables for msk, options. Fix loop retrans

[Paolo Abeni <pabeni@redhat.com>]

On Tue, 2022-09-20 at 14:52 +0200, Dmytro Shytyi wrote:
> Introduce mptfo variables for msk and options.
> Also fix the infinite retransmissions in the end of second session.

I think the above sentence belongs more the changelog than to the
commit message itself.

> The variable 2nd ack received in struct mptcp_options_received identifies the
> received ack on the listener side during 3way handshake in mptfo context
> and miningless alone if used alone. It is further used(checked)
> in conjunction in the same "if" statement with variable is_mptfo
> from struct mptcp_sock.
> 
> Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
> ---
>  net/mptcp/fastopen.c | 14 ++++++++++++++
>  net/mptcp/options.c  |  4 ++++
>  net/mptcp/protocol.h |  6 +++++-
>  3 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
> index 9ef49a2d2ea2..92885e459f93 100644
> --- a/net/mptcp/fastopen.c
> +++ b/net/mptcp/fastopen.c
> @@ -30,3 +30,17 @@ int mptcp_setsockopt_sol_tcp_fastopen(struct mptcp_sock *msk, sockptr_t optval,
>  
>  	return ret;
>  }
> +
> +void mptcp_gen_msk_ackseq_fastopen(struct mptcp_sock *msk, struct mptcp_subflow_context *subflow,
> +				   struct mptcp_options_received mp_opt)
> +{
> +	u64 ack_seq;
> +
> +	msk->can_ack = true;
> +	msk->remote_key = mp_opt.sndr_key;

likely the above two statements need WRITE_ONCE() annotation.

Additionally it's better to add some safeguards here:
- ssk should be in TCP_SYN_RECV state - to be verified, it's guess and
I haven't check it yet.
- msk should be in the same state of the ssk subflow.

> +	mptcp_crypto_key_sha(msk->remote_key, NULL, &ack_seq);
> +	ack_seq++;
> +	WRITE_ONCE(msk->ack_seq, ack_seq);
> +	pr_debug("ack_seq=%llu sndr_key=%llu", msk->ack_seq, mp_opt.sndr_key);
> +	atomic64_set(&msk->rcv_wnd_sent, ack_seq);
> +}
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 30d289044e71..8852a13cfe62 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -91,6 +91,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>  			ptr += 8;
>  		}
>  		if (opsize >= TCPOLEN_MPTCP_MPC_ACK) {
> +			mp_opt->hns_2nd_ack_rcvd = 1;

Not needed, see below...

>  			mp_opt->rcvr_key = get_unaligned_be64(ptr);
>  			ptr += 8;
>  		}
> @@ -1124,6 +1125,9 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>  		return sk->sk_state != TCP_CLOSE;
>  
>  	if (unlikely(mp_opt.suboptions != OPTION_MPTCP_DSS)) {
> +		if (mp_opt.suboptions & OPTIONS_MPTCP_MPC && mp_opt.hns_2nd_ack_rcvd && msk->is_mptfo)

You can simply check for:

		if (mp_opt.suboptions & OPTIONS_MPTCP_MPC_ACK) && msk->is_mptfo)

> +			mptcp_gen_msk_ackseq_fastopen(msk, subflow, mp_opt);
> +
>  		if ((mp_opt.suboptions & OPTION_MPTCP_FASTCLOSE) &&
>  		    msk->local_key == mp_opt.rcvr_key) {
>  			WRITE_ONCE(msk->rcv_fastclose, true);
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 57596cdfb1f9..3b9a349a7080 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -155,7 +155,8 @@ struct mptcp_options_received {
>  		echo:1,
>  		backup:1,
>  		deny_join_id0:1,
> -		__unused:2;
> +		__unused:1;
> +	u8	hns_2nd_ack_rcvd:1;

... again not needed, see above ;)

>  	u8	join_id;
>  	u64	thmac;
>  	u8	hmac[MPTCPOPT_HMAC_LEN];
> @@ -282,6 +283,7 @@ struct mptcp_sock {
>  	bool		use_64bit_ack; /* Set when we received a 64-bit DSN */
>  	bool		csum_enabled;
>  	bool		allow_infinite_fallback;
> +	bool		is_mptfo;
>  	u8		mpc_endpoint_id;
>  	u8		recvmsg_inq:1,
>  			cork:1,
> @@ -842,6 +844,8 @@ int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr, int addr_l
>  // Fast Open Mechanism functions begin
>  int mptcp_setsockopt_sol_tcp_fastopen(struct mptcp_sock *msk, sockptr_t optval,
>  				      unsigned int optlen);
> +void mptcp_gen_msk_ackseq_fastopen(struct mptcp_sock *msk, struct mptcp_subflow_context *subflow,
> +				   struct mptcp_options_received mp_opt);
>  // Fast Open Mechanism functions end
>  
>  static inline bool mptcp_pm_should_add_signal(struct mptcp_sock *msk)