[RFC PATCH mptcp-next v8 5/7] Fix unxpctd val of subflow->map_seq(dscrd packet)

Dmytro Shytyi posted 7 patches 1 year, 2 months ago
Maintainers: Eric Dumazet <edumazet@google.com>, "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>, David Ahern <dsahern@kernel.org>, Mat Martineau <mathew.j.martineau@linux.intel.com>, Matthieu Baerts <matthieu.baerts@tessares.net>, Shuah Khan <shuah@kernel.org>
There is a newer version of this series
[RFC PATCH mptcp-next v8 5/7] Fix unxpctd val of subflow->map_seq(dscrd packet)
Posted by Dmytro Shytyi 1 year, 2 months ago
Fix unexpected value of subflow->map_seq (discarded and after
retransmitted 2nd packet(1st after TFO)).
We use mptcp_gen_msk_ackseq_fasopen()
when we know this is the first chunk of data after TFO.

Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
---
 net/mptcp/options.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 8852a13cfe62..8e937a422909 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1212,6 +1212,11 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 			mpext->dsn64 = 1;
 			mpext->mpc_map = 1;
 			mpext->data_fin = 0;
+			
+			if (msk->is_mptfo) {
+				mptcp_gen_msk_ackseq_fastopen(msk, subflow, mp_opt);
+				mpext->data_seq = READ_ONCE(msk->ack_seq);
+			}
 		} else {
 			mpext->data_seq = mp_opt.data_seq;
 			mpext->subflow_seq = mp_opt.subflow_seq;
-- 
2.25.1
Re: [RFC PATCH mptcp-next v8 5/7] Fix unxpctd val of subflow->map_seq(dscrd packet)
Posted by Paolo Abeni 1 year, 2 months ago
On Tue, 2022-09-20 at 14:52 +0200, Dmytro Shytyi wrote:
> Fix unexpected value of subflow->map_seq (discarded and after
> retransmitted 2nd packet(1st after TFO)).

The above is confusing, it looks like the previous patches 
intentionally added a bug that is fixed here. If so it's better re-
order the series, moving this one early-on.

> We use mptcp_gen_msk_ackseq_fasopen()
> when we know this is the first chunk of data after TFO.
> 
> Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
> ---
>  net/mptcp/options.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 8852a13cfe62..8e937a422909 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -1212,6 +1212,11 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>  			mpext->dsn64 = 1;
>  			mpext->mpc_map = 1;
>  			mpext->data_fin = 0;
> +			
> +			if (msk->is_mptfo) {
> +				mptcp_gen_msk_ackseq_fastopen(msk, subflow, mp_opt);
> +				mpext->data_seq = READ_ONCE(msk->ack_seq);
> +			}

With the changes suggested on the next patch the above chunk should not
be needed.

In any case I think it's better to properly keep msk->ack_seq unchanged
after that the TFO syn data landed into the msk receive queue, instead
of resetting it later.

Cheers,

Paolo
Re: [RFC PATCH mptcp-next v8 5/7] Fix unxpctd val of subflow->map_seq(dscrd packet)
Posted by Dmytro Shytyi 1 year, 2 months ago
In v9 I dropped the previous patch and keep only this. I propose to have 
a look at v9 when I will submit it.

Thanks,

Dmytro.

On 9/20/2022 6:04 PM, Paolo Abeni wrote:
> On Tue, 2022-09-20 at 14:52 +0200, Dmytro Shytyi wrote:
>> Fix unexpected value of subflow->map_seq (discarded and after
>> retransmitted 2nd packet(1st after TFO)).
> The above is confusing, it looks like the previous patches
> intentionally added a bug that is fixed here. If so it's better re-
> order the series, moving this one early-on.
>
>> We use mptcp_gen_msk_ackseq_fasopen()
>> when we know this is the first chunk of data after TFO.
>>
>> Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
>> ---
>>   net/mptcp/options.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index 8852a13cfe62..8e937a422909 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -1212,6 +1212,11 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
>>   			mpext->dsn64 = 1;
>>   			mpext->mpc_map = 1;
>>   			mpext->data_fin = 0;
>> +			
>> +			if (msk->is_mptfo) {
>> +				mptcp_gen_msk_ackseq_fastopen(msk, subflow, mp_opt);
>> +				mpext->data_seq = READ_ONCE(msk->ack_seq);
>> +			}
> With the changes suggested on the next patch the above chunk should not
> be needed.
>
> In any case I think it's better to properly keep msk->ack_seq unchanged
> after that the TFO syn data landed into the msk receive queue, instead
> of resetting it later.
>
> Cheers,
>
> Paolo
>