Re: [RFC PATCH mptcp-next v7 07/11] mptfo variables for msk, options. Fix loop retrans

[Paolo Abeni <pabeni@redhat.com>]

On Sun, 2022-09-18 at 00:28 +0200, Dmytro Shytyi wrote:
> Introduce mptfo variables for msk and options.
> Also fixe the infinite retransmissions in the end of second session.
> Suggestion @palbeni (SEP 1) during the meting to 'look at ack'
> 
> Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
> ---
>  net/mptcp/fastopen.c | 16 ++++++++++++++++
>  net/mptcp/options.c  |  3 +++
>  net/mptcp/protocol.h |  6 +++++-
>  3 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
> index 436e773d798a..149a4b1d3dac 100644
> --- a/net/mptcp/fastopen.c
> +++ b/net/mptcp/fastopen.c
> @@ -91,3 +91,19 @@ int mptcp_setsockopt_sol_tcp_fastopen(struct mptcp_sock *msk, sockptr_t optval,
>  
>  	return ret;
>  }
> +
> +void mptcp_treat_hshake_ack_fastopen(struct mptcp_sock *msk, struct mptcp_subflow_context *subflow,
> +				     struct mptcp_options_received mp_opt)
> +{
> +	u64 ack_seq;
> +
> +	if (mp_opt.suboptions & OPTIONS_MPTCP_MPC && mp_opt.is_mptfo && msk->is_mptfo) {
> +		msk->can_ack = true;
> +		msk->remote_key = mp_opt.sndr_key;
> +		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);

It's not clear to me why the above is needed. According to the RFC we
should not do additional MPTCP-level key management for TFO' sake ?!?

> +	}
> +}
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 30d289044e71..185b069e57f4 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->is_mptfo = 1;

This looks incorrect. Plain MPTCP MP_CAPABLE handshake will be marked
as TFO regardless of the actual TCP option exchange.

It looks like the only already available hook to catch the 'incoming
fast open' info would be implementing custom mptcp subflow {v4,v6}
send_synack and checking the 'foc' pointer.

Otherwise you can add TCPOPT_FASTOPEN parsing capability to
mptcp_parse_option(), but that would be more duplicate code, I think it
would be better to avoid that if possible.

>  			mp_opt->rcvr_key = get_unaligned_be64(ptr);
>  			ptr += 8;
>  		}
> @@ -1124,6 +1125,8 @@ 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)) {
> +		mptcp_treat_hshake_ack_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 8caaeeedb9da..b90279c734ae 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;
> +		is_mptfo:1,
> +		__unused:1;
>  	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,
> @@ -845,6 +847,8 @@ int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
>  			   size_t *copied);
>  int mptcp_setsockopt_sol_tcp_fastopen(struct mptcp_sock *msk, sockptr_t optval,
>  				      unsigned int optlen);
> +void mptcp_treat_hshake_ack_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)