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);
+	}
+}
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;
 			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)
-- 
2.25.1

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)

This is refactored in v8.


Best regards,

Dmytro

On 9/19/2022 1:11 PM, Paolo Abeni wrote:
> 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)

On Mon, 2022-09-19 at 13:11 +0200, Paolo Abeni wrote:
> 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 ?!?

Addendum: instead here you likely want to setup-up a special DSS
mapping for the TFO data. Such mapping will additionally require some
special handling in  __mptcp_move_skb(), as 'msk->ack_seq' must not be
incremented by the in-sequence TFO data.

@Christoph: AFAICS, even with TFO, the initiator nor the listener can't
send any other data after the syn until the MPC handshake is completed.
Is that correct?

Thanks!

Paolo

Hello Paolo,

> On Sep 19, 2022, at 4:26 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> 
> On Mon, 2022-09-19 at 13:11 +0200, Paolo Abeni wrote:
>> 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 ?!?
> 
> Addendum: instead here you likely want to setup-up a special DSS
> mapping for the TFO data. Such mapping will additionally require some
> special handling in  __mptcp_move_skb(), as 'msk->ack_seq' must not be
> incremented by the in-sequence TFO data.
> 
> @Christoph: AFAICS, even with TFO, the initiator nor the listener can't
> send any other data after the syn until the MPC handshake is completed.
> Is that correct?

The initiator can only send data in the SYN. No subsequent packets after that unless the SYN/ACK has been received.

The listener can send data after the SYN/ACK. It does not yet at this stage know about the DATA_ACK it should use, but that is ok.


Hope I could help.

Cheers,
Christoph