[MPTCP] Re: [PATCH mptcp-net] mptcp: fix DATA_FIN generation on early shutdown.

[MPTCP] Re: [PATCH mptcp-net] mptcp: fix DATA_FIN generation on early shutdown.

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1469 bytes --]

Hi Paolo, Mat,

On 11/02/2021 23:13, Paolo Abeni wrote:
> If the msk is closed before sending or receiving any data,
> no DATA_FIN is generated, instead an MPC ack packet is
> crafted out.
> 
> In the above scenario, the MPTCP protocol create and send a
> pure ack and such packets matches also the criteria for an
> MPC ack and the protocol tries first to insert MPC options,
> leading the described error.
> 
> This change address the issue skipping MPC option for DATA_FIN
> packets or if the subflow is not established.
> 
> To avoid doing multiple times the same test, fetch the data_fin
> flag in a bool variable and pass it to both the interested
> helpers.
> 
> Fixes: 6d0060f600ad ("mptcp: Write MPTCP DSS headers to outgoing data packets")
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>

Thank you for the patch and the review!

> ---
> The scenario is tested by a specific pktdrill tested,
> already pushed

Thank you for that! (and for having use "mptcp-net" :) even if we can 
change that if it is not OK).
I just applied your patch at the bottom of the tree, after a patch from 
Florian (mptcp: reset last_snd on subflow close). With Mat's RvB tag and 
without a dot at the end of the title:

- b7ee76d557c4: mptcp: fix DATA_FIN generation on early shutdown
- Results: 655ef2e493c8..82aaf8587e26

Tests + export are in progress!

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

[MPTCP] Re: [PATCH mptcp-net] mptcp: fix DATA_FIN generation on early shutdown.

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1021 bytes --]

On 15/02/2021 16:29, Matthieu Baerts wrote:
>> The scenario is tested by a specific pktdrill tested,
>> already pushed
> 
> Thank you for that! (and for having use "mptcp-net" :) even if we can 
> change that if it is not OK).
> I just applied your patch at the bottom of the tree, after a patch from 
> Florian (mptcp: reset last_snd on subflow close). With Mat's RvB tag and 
> without a dot at the end of the title:
> 
> - b7ee76d557c4: mptcp: fix DATA_FIN generation on early shutdown
> - Results: 655ef2e493c8..82aaf8587e26

I had to move it in our tree after "mptcp: better msk receive window 
updates" which is not in net-next yet.

- 50ae59ab415b: mptcp: fix DATA_FIN generation on early shutdown
- Results: 82aaf8587e26..1de5e73fbb6c

Tests + export have been restarted. My CI is frequently hitting bug 160 
-- it did here so I don't have all results -- but it is not as frequent 
on my local environment.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

[MPTCP] Re: [PATCH mptcp-net] mptcp: fix DATA_FIN generation on early shutdown.

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 1601 bytes --]

On Fri, 2021-02-12 at 17:06 -0800, Mat Martineau wrote:
> On Thu, 11 Feb 2021, Paolo Abeni wrote:
> 
> > If the msk is closed before sending or receiving any data,
> > no DATA_FIN is generated, instead an MPC ack packet is
> > crafted out.
> > 
> > In the above scenario, the MPTCP protocol create and send a
> > pure ack and such packets matches also the criteria for an
> > MPC ack and the protocol tries first to insert MPC options,
> > leading the described error.
> > 
> > This change address the issue skipping MPC option for DATA_FIN
> > packets or if the subflow is not established.
> > 
> > To avoid doing multiple times the same test, fetch the data_fin
> > flag in a bool variable and pass it to both the interested
> > helpers.
> > 
> > Fixes: 6d0060f600ad ("mptcp: Write MPTCP DSS headers to outgoing data packets")
> > Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> > ---
> > The scenario is tested by a specific pktdrill tested,
> > already pushed
> > ---
> > net/mptcp/options.c | 23 ++++++++++++++---------
> > 1 file changed, 14 insertions(+), 9 deletions(-)
> 
> Hi Paolo -
> 
> Change looks good to me:
> 
> Reviewed-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>

Thanks! 

> Were you intending this for the export branch or the -net tree? (Unsure 
> due to the [PATCH mptcp-net] prefix)

I'm trying to follow the rules listed here:

https://annuel2.framapad.org/p/mptcp_upstreaming_20201126

(starting from line 188). 'mptcp-net' is targeting the -net tree after
being staged in the export branch.

Cheers,

Paolo

[MPTCP] Re: [PATCH mptcp-net] mptcp: fix DATA_FIN generation on early shutdown.

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 4648 bytes --]


On Thu, 11 Feb 2021, Paolo Abeni wrote:

> If the msk is closed before sending or receiving any data,
> no DATA_FIN is generated, instead an MPC ack packet is
> crafted out.
>
> In the above scenario, the MPTCP protocol create and send a
> pure ack and such packets matches also the criteria for an
> MPC ack and the protocol tries first to insert MPC options,
> leading the described error.
>
> This change address the issue skipping MPC option for DATA_FIN
> packets or if the subflow is not established.
>
> To avoid doing multiple times the same test, fetch the data_fin
> flag in a bool variable and pass it to both the interested
> helpers.
>
> Fixes: 6d0060f600ad ("mptcp: Write MPTCP DSS headers to outgoing data packets")
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
> The scenario is tested by a specific pktdrill tested,
> already pushed
> ---
> net/mptcp/options.c | 23 ++++++++++++++---------
> 1 file changed, 14 insertions(+), 9 deletions(-)

Hi Paolo -

Change looks good to me:

Reviewed-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com>

Were you intending this for the export branch or the -net tree? (Unsure 
due to the [PATCH mptcp-net] prefix)


Thanks,

Mat



>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index b63574d6b812e..444a38681e93e 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -411,6 +411,7 @@ static void clear_3rdack_retransmission(struct sock *sk)
> }
>
> static bool mptcp_established_options_mp(struct sock *sk, struct sk_buff *skb,
> +					 bool snd_data_fin_enable,
> 					 unsigned int *size,
> 					 unsigned int remaining,
> 					 struct mptcp_out_options *opts)
> @@ -428,9 +429,10 @@ static bool mptcp_established_options_mp(struct sock *sk, struct sk_buff *skb,
> 	if (!skb)
> 		return false;
>
> -	/* MPC/MPJ needed only on 3rd ack packet */
> -	if (subflow->fully_established ||
> -	    subflow->snd_isn != TCP_SKB_CB(skb)->seq)
> +	/* MPC/MPJ needed only on 3rd ack packet, DATA_FIN and TCP shutdown take precedence */
> +	if (subflow->fully_established || snd_data_fin_enable ||
> +	    subflow->snd_isn != TCP_SKB_CB(skb)->seq ||
> +	    sk->sk_state != TCP_ESTABLISHED)
> 		return false;
>
> 	if (subflow->mp_capable) {
> @@ -502,20 +504,20 @@ static void mptcp_write_data_fin(struct mptcp_subflow_context *subflow,
> }
>
> static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
> +					  bool snd_data_fin_enable,
> 					  unsigned int *size,
> 					  unsigned int remaining,
> 					  struct mptcp_out_options *opts)
> {
> 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> 	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> -	u64 snd_data_fin_enable, ack_seq;
> 	unsigned int dss_size = 0;
> 	struct mptcp_ext *mpext;
> 	unsigned int ack_size;
> 	bool ret = false;
> +	u64 ack_seq;
>
> 	mpext = skb ? mptcp_get_ext(skb) : NULL;
> -	snd_data_fin_enable = mptcp_data_fin_enabled(msk);
>
> 	if (!skb || (mpext && mpext->use_map) || snd_data_fin_enable) {
> 		unsigned int map_size;
> @@ -717,12 +719,15 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
> 			       unsigned int *size, unsigned int remaining,
> 			       struct mptcp_out_options *opts)
> {
> +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> +	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> 	unsigned int opt_size = 0;
> +	bool snd_data_fin;
> 	bool ret = false;
>
> 	opts->suboptions = 0;
>
> -	if (unlikely(mptcp_check_fallback(sk)))
> +	if (unlikely(__mptcp_check_fallback(msk)))
> 		return false;
>
> 	/* prevent adding of any MPTCP related options on reset packet
> @@ -731,10 +736,10 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
> 	if (unlikely(skb && TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST))
> 		return false;
>
> -	if (mptcp_established_options_mp(sk, skb, &opt_size, remaining, opts))
> +	snd_data_fin = mptcp_data_fin_enabled(msk);
> +	if (mptcp_established_options_mp(sk, skb, snd_data_fin, &opt_size, remaining, opts))
> 		ret = true;
> -	else if (mptcp_established_options_dss(sk, skb, &opt_size, remaining,
> -					       opts))
> +	else if (mptcp_established_options_dss(sk, skb, snd_data_fin, &opt_size, remaining, opts))
> 		ret = true;
>
> 	/* we reserved enough space for the above options, and exceeding the
> -- 
> 2.26.2
> _______________________________________________
> mptcp mailing list -- mptcp(a)lists.01.org
> To unsubscribe send an email to mptcp-leave(a)lists.01.org
>

--
Mat Martineau
Intel