[v7] mptcp: Fast Open Mechanism

[RFC PATCH mptcp-next v7 08/11] Fix unxpctd val of subflow->map_seq(dscrd packet)

Posted by Dmytro Shytyi 3 years, 4 months ago

Fix unexpected value of subflow->map_seq (discarded and after
retransmitted 2nd packet)

Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
---
 net/mptcp/subflow.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index c7d49fb6e7bd..075c61d1d37f 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1077,8 +1077,10 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
 		/* will validate the next map after consuming the current one */
 		goto validate_csum;
 	}
-
-	subflow->map_seq = map_seq;
+	if (msk->is_mptfo)
+		subflow->map_seq = READ_ONCE(msk->ack_seq);
+	else
+		subflow->map_seq = map_seq;
 	subflow->map_subflow_seq = mpext->subflow_seq;
 	subflow->map_data_len = data_len;
 	subflow->map_valid = 1;
-- 
2.25.1

Re: [RFC PATCH mptcp-next v7 08/11] Fix unxpctd val of subflow->map_seq(dscrd packet)

Posted by Paolo Abeni 3 years, 4 months ago

On Sun, 2022-09-18 at 00:28 +0200, Dmytro Shytyi wrote:
> Fix unexpected value of subflow->map_seq (discarded and after
> retransmitted 2nd packet)
> 
> Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
> ---
>  net/mptcp/subflow.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index c7d49fb6e7bd..075c61d1d37f 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1077,8 +1077,10 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
>  		/* will validate the next map after consuming the current one */
>  		goto validate_csum;
>  	}
> -
> -	subflow->map_seq = map_seq;
> +	if (msk->is_mptfo)
> +		subflow->map_seq = READ_ONCE(msk->ack_seq);
> +	else
> +		subflow->map_seq = map_seq;
>  	subflow->map_subflow_seq = mpext->subflow_seq;
>  	subflow->map_data_len = data_len;
>  	subflow->map_valid = 1;


This is likely not needed, if you build a special dss mapping at
incoming option processing time.

Cheers,

Paolo

Re: [RFC PATCH mptcp-next v7 08/11] Fix unxpctd val of subflow->map_seq(dscrd packet)

Posted by Matthieu Baerts 3 years, 4 months ago

Hi Paolo, Dmytro,

On 19/09/2022 13:27, Paolo Abeni wrote:
> On Sun, 2022-09-18 at 00:28 +0200, Dmytro Shytyi wrote:
>> Fix unexpected value of subflow->map_seq (discarded and after
>> retransmitted 2nd packet)
>>
>> Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
>> ---
>>  net/mptcp/subflow.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index c7d49fb6e7bd..075c61d1d37f 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -1077,8 +1077,10 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
>>  		/* will validate the next map after consuming the current one */
>>  		goto validate_csum;
>>  	}
>> -
>> -	subflow->map_seq = map_seq;
>> +	if (msk->is_mptfo)
>> +		subflow->map_seq = READ_ONCE(msk->ack_seq);
>> +	else
>> +		subflow->map_seq = map_seq;
>>  	subflow->map_subflow_seq = mpext->subflow_seq;
>>  	subflow->map_data_len = data_len;
>>  	subflow->map_valid = 1;
> 
> 
> This is likely not needed, if you build a special dss mapping at
> incoming option processing time.

Also for this, Benjamin had another approach by delaying MPTCP data_ack
calculation, see from patch 5/10 from [1]. It might be good to discuss
about that at the next weekly meeting.

Cheers,
Matt

[1]
https://lore.kernel.org/mptcp/20220908133829.3410092-1-benjamin.hesmans@tessares.net/T/
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [RFC PATCH mptcp-next v7 08/11] Fix unxpctd val of subflow->map_seq(dscrd packet)

Posted by Paolo Abeni 3 years, 4 months ago

On Mon, 2022-09-19 at 13:31 +0200, Matthieu Baerts wrote:
> Hi Paolo, Dmytro,
> 
> On 19/09/2022 13:27, Paolo Abeni wrote:
> > On Sun, 2022-09-18 at 00:28 +0200, Dmytro Shytyi wrote:
> > > Fix unexpected value of subflow->map_seq (discarded and after
> > > retransmitted 2nd packet)
> > > 
> > > Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
> > > ---
> > >  net/mptcp/subflow.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > > index c7d49fb6e7bd..075c61d1d37f 100644
> > > --- a/net/mptcp/subflow.c
> > > +++ b/net/mptcp/subflow.c
> > > @@ -1077,8 +1077,10 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
> > >  		/* will validate the next map after consuming the current one */
> > >  		goto validate_csum;
> > >  	}
> > > -
> > > -	subflow->map_seq = map_seq;
> > > +	if (msk->is_mptfo)
> > > +		subflow->map_seq = READ_ONCE(msk->ack_seq);
> > > +	else
> > > +		subflow->map_seq = map_seq;
> > >  	subflow->map_subflow_seq = mpext->subflow_seq;
> > >  	subflow->map_data_len = data_len;
> > >  	subflow->map_valid = 1;
> > 
> > 
> > This is likely not needed, if you build a special dss mapping at
> > incoming option processing time.
> 
> Also for this, Benjamin had another approach by delaying MPTCP data_ack
> calculation, see from patch 5/10 from [1]. It might be good to discuss
> about that at the next weekly meeting.

I think that later approach is better. Apropos, regarding the question
raised in patch 6/10 in such series - where to hook for late ack_seq
initialization - I think we could:

- add a 'remote_key_avail' bit in mptcp_subflow_context
- in mptcp_incoming_options(), for MPC_ACK pkts, copy mp_opt->sndr_key
in subflow->remote_key and set  'remote_key_avail'
- in subflow_state_change() when changing to TCP_ESTABLISHED && this is
a TFO passive socket (-> prev state == TCP_SYN_RECV), remote_key_avail
is true, and can_ack == false, do the late ack_seq initialization.

Note that the last quite convoluted condition should catch the
sk->sk_state_change(sk) invocation in tcp_rcv_state_process() for TFO
sockets.

Cheers,

Paolo