[PATCH mptcp-next v7 1/4] Squash to "mptcp: implement fastclose xmit path"

Matthieu Baerts posted 4 patches 4 months, 3 weeks ago
Maintainers: Matthieu Baerts <matthieu.baerts@tessares.net>, "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, Mat Martineau <mathew.j.martineau@linux.intel.com>
[PATCH mptcp-next v7 1/4] Squash to "mptcp: implement fastclose xmit path"
Posted by Matthieu Baerts 4 months, 3 weeks ago
From: Geliang Tang <geliang.tang@suse.com>

MP_FAIL could be sent with RST or DSS, and FASTCLOSE can be sent with
RST too. So we should use a similar xmit logic for FASTCLOSE and
MP_FAIL in both mptcp_write_options() and mptcp_established_options*().

Cc: Paolo Abeni <pabeni@redhat.com>
Co-developed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/options.c | 66 ++++++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 25 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index c6726e8389ec..9d2c1c9edbe6 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -829,8 +829,12 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
 
 	if (unlikely(skb && TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST)) {
 		if (mptcp_established_options_fastclose(sk, &opt_size, remaining, opts) ||
-		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts) ||
-		    mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
+		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts)) {
+			*size += opt_size;
+			remaining -= opt_size;
+		}
+		/* MP_RST can be used with MP_FASTCLOSE and MP_FAIL if there is room */
+		if (mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
 			*size += opt_size;
 			remaining -= opt_size;
 		}
@@ -1257,21 +1261,7 @@ static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
 void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 			 struct mptcp_out_options *opts)
 {
-	if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
-		const struct sock *ssk = (const struct sock *)tp;
-		struct mptcp_subflow_context *subflow;
-
-		subflow = mptcp_subflow_ctx(ssk);
-		subflow->send_mp_fail = 0;
-
-		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
-				      TCPOLEN_MPTCP_FAIL,
-				      0, 0);
-		put_unaligned_be64(opts->fail_seq, ptr);
-		ptr += 2;
-	}
-
-	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and RST are mutually exclusive,
+	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and FAIL are mutually exclusive,
 	 * see mptcp_established_options*()
 	 */
 	if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
@@ -1328,6 +1318,10 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 						   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
 			}
 		}
+
+		/* We might need to add MP_FAIL options in rare cases */
+		if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions))
+			goto mp_fail;
 	} else if (OPTIONS_MPTCP_MPC & opts->suboptions) {
 		u8 len, flag = MPTCP_CAP_HMAC_SHA256;
 
@@ -1460,19 +1454,41 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 				ptr += 1;
 			}
 		}
-	} else if (unlikely(OPTION_MPTCP_RST & opts->suboptions)) {
-		/* RST is mutually exclusive with everything else */
-		*ptr++ = mptcp_option(MPTCPOPT_RST,
-				      TCPOLEN_MPTCP_RST,
-				      opts->reset_transient,
-				      opts->reset_reason);
-		return;
 	} else if (unlikely(OPTION_MPTCP_FASTCLOSE & opts->suboptions)) {
-		/* FASTCLOSE is mutually exclusive with everything else */
+		/* FASTCLOSE is mutually exclusive with others except RST */
 		*ptr++ = mptcp_option(MPTCPOPT_MP_FASTCLOSE,
 				      TCPOLEN_MPTCP_FASTCLOSE,
 				      0, 0);
 		put_unaligned_be64(opts->rcvr_key, ptr);
+
+		if (OPTION_MPTCP_RST & opts->suboptions)
+			goto mp_rst;
+		return;
+	} else if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions))
+mp_fail:
+	{
+		/* MP_FAIL is mutually exclusive with others except RST */
+		const struct sock *ssk = (const struct sock *)tp;
+		struct mptcp_subflow_context *subflow;
+
+		subflow = mptcp_subflow_ctx(ssk);
+		subflow->send_mp_fail = 0;
+
+		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
+				      TCPOLEN_MPTCP_FAIL,
+				      0, 0);
+		put_unaligned_be64(opts->fail_seq, ptr);
+		ptr += 2;
+
+		if (OPTION_MPTCP_RST & opts->suboptions)
+			goto mp_rst;
+		return;
+	} else if (unlikely(OPTION_MPTCP_RST & opts->suboptions)) {
+mp_rst:
+		*ptr++ = mptcp_option(MPTCPOPT_RST,
+				      TCPOLEN_MPTCP_RST,
+				      opts->reset_transient,
+				      opts->reset_reason);
 		return;
 	}
 
-- 
2.33.1


Re: [PATCH mptcp-next v7 1/4] Squash to "mptcp: implement fastclose xmit path"
Posted by Geliang Tang 4 months, 2 weeks ago
On Thu, Dec 30, 2021 at 08:16:48PM +0100, Matthieu Baerts wrote:
> From: Geliang Tang <geliang.tang@suse.com>
> 
> MP_FAIL could be sent with RST or DSS, and FASTCLOSE can be sent with
> RST too. So we should use a similar xmit logic for FASTCLOSE and
> MP_FAIL in both mptcp_write_options() and mptcp_established_options*().
> 
> Cc: Paolo Abeni <pabeni@redhat.com>
> Co-developed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/options.c | 66 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 41 insertions(+), 25 deletions(-)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index c6726e8389ec..9d2c1c9edbe6 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -829,8 +829,12 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
>  
>  	if (unlikely(skb && TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST)) {
>  		if (mptcp_established_options_fastclose(sk, &opt_size, remaining, opts) ||
> -		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts) ||
> -		    mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
> +		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts)) {
> +			*size += opt_size;
> +			remaining -= opt_size;
> +		}
> +		/* MP_RST can be used with MP_FASTCLOSE and MP_FAIL if there is room */
> +		if (mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
>  			*size += opt_size;
>  			remaining -= opt_size;
>  		}
> @@ -1257,21 +1261,7 @@ static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
>  void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>  			 struct mptcp_out_options *opts)
>  {
> -	if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
> -		const struct sock *ssk = (const struct sock *)tp;
> -		struct mptcp_subflow_context *subflow;
> -
> -		subflow = mptcp_subflow_ctx(ssk);
> -		subflow->send_mp_fail = 0;
> -
> -		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
> -				      TCPOLEN_MPTCP_FAIL,
> -				      0, 0);
> -		put_unaligned_be64(opts->fail_seq, ptr);
> -		ptr += 2;
> -	}

Hi Matt,

Sorry, this patch doesn't work on my test. We write both DSS + MP_FAIL,
but MP_FAIL is lost on the received side. Only DSS is received.

If we write MP_FAIL + DSS like the orignal code, MP_FAIL and DSS will be
received correctly.

I haven't figured out why yet.

Could we just keep this MP_FAIL writting code still at the beginning of
the function just like the orignal code did?

> -
> -	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and RST are mutually exclusive,
> +	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and FAIL are mutually exclusive,
>  	 * see mptcp_established_options*()
>  	 */
>  	if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
> @@ -1328,6 +1318,10 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>  						   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
>  			}
>  		}
> +
> +		/* We might need to add MP_FAIL options in rare cases */
> +		if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions))
> +			goto mp_fail;
>  	} else if (OPTIONS_MPTCP_MPC & opts->suboptions) {
>  		u8 len, flag = MPTCP_CAP_HMAC_SHA256;
>  
> @@ -1460,19 +1454,41 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>  				ptr += 1;
>  			}
>  		}
> -	} else if (unlikely(OPTION_MPTCP_RST & opts->suboptions)) {
> -		/* RST is mutually exclusive with everything else */
> -		*ptr++ = mptcp_option(MPTCPOPT_RST,
> -				      TCPOLEN_MPTCP_RST,
> -				      opts->reset_transient,
> -				      opts->reset_reason);
> -		return;
>  	} else if (unlikely(OPTION_MPTCP_FASTCLOSE & opts->suboptions)) {
> -		/* FASTCLOSE is mutually exclusive with everything else */
> +		/* FASTCLOSE is mutually exclusive with others except RST */
>  		*ptr++ = mptcp_option(MPTCPOPT_MP_FASTCLOSE,
>  				      TCPOLEN_MPTCP_FASTCLOSE,
>  				      0, 0);
>  		put_unaligned_be64(opts->rcvr_key, ptr);

Here, 'ptr += 2;' is needed.

Thanks,
-Geliang

> +
> +		if (OPTION_MPTCP_RST & opts->suboptions)
> +			goto mp_rst;
> +		return;
> +	} else if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions))
> +mp_fail:
> +	{
> +		/* MP_FAIL is mutually exclusive with others except RST */
> +		const struct sock *ssk = (const struct sock *)tp;
> +		struct mptcp_subflow_context *subflow;
> +
> +		subflow = mptcp_subflow_ctx(ssk);
> +		subflow->send_mp_fail = 0;
> +
> +		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
> +				      TCPOLEN_MPTCP_FAIL,
> +				      0, 0);
> +		put_unaligned_be64(opts->fail_seq, ptr);
> +		ptr += 2;
> +
> +		if (OPTION_MPTCP_RST & opts->suboptions)
> +			goto mp_rst;
> +		return;
> +	} else if (unlikely(OPTION_MPTCP_RST & opts->suboptions)) {
> +mp_rst:
> +		*ptr++ = mptcp_option(MPTCPOPT_RST,
> +				      TCPOLEN_MPTCP_RST,
> +				      opts->reset_transient,
> +				      opts->reset_reason);
>  		return;
>  	}
>  
> -- 
> 2.33.1
> 


Re: [PATCH mptcp-next v7 1/4] Squash to "mptcp: implement fastclose xmit path"
Posted by Matthieu Baerts 4 months, 2 weeks ago
Hi Geliang,

On 05/01/2022 09:50, Geliang Tang wrote:
> On Thu, Dec 30, 2021 at 08:16:48PM +0100, Matthieu Baerts wrote:
>> From: Geliang Tang <geliang.tang@suse.com>
>>
>> MP_FAIL could be sent with RST or DSS, and FASTCLOSE can be sent with
>> RST too. So we should use a similar xmit logic for FASTCLOSE and
>> MP_FAIL in both mptcp_write_options() and mptcp_established_options*().
>>
>> Cc: Paolo Abeni <pabeni@redhat.com>
>> Co-developed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>> ---
>>  net/mptcp/options.c | 66 ++++++++++++++++++++++++++++-----------------
>>  1 file changed, 41 insertions(+), 25 deletions(-)
>>
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index c6726e8389ec..9d2c1c9edbe6 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -829,8 +829,12 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
>>  
>>  	if (unlikely(skb && TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST)) {
>>  		if (mptcp_established_options_fastclose(sk, &opt_size, remaining, opts) ||
>> -		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts) ||
>> -		    mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
>> +		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts)) {
>> +			*size += opt_size;
>> +			remaining -= opt_size;
>> +		}
>> +		/* MP_RST can be used with MP_FASTCLOSE and MP_FAIL if there is room */
>> +		if (mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
>>  			*size += opt_size;
>>  			remaining -= opt_size;
>>  		}
>> @@ -1257,21 +1261,7 @@ static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
>>  void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>  			 struct mptcp_out_options *opts)
>>  {
>> -	if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
>> -		const struct sock *ssk = (const struct sock *)tp;
>> -		struct mptcp_subflow_context *subflow;
>> -
>> -		subflow = mptcp_subflow_ctx(ssk);
>> -		subflow->send_mp_fail = 0;
>> -
>> -		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
>> -				      TCPOLEN_MPTCP_FAIL,
>> -				      0, 0);
>> -		put_unaligned_be64(opts->fail_seq, ptr);
>> -		ptr += 2;
>> -	}
> 
> Hi Matt,
> 
> Sorry, this patch doesn't work on my test. We write both DSS + MP_FAIL,
> but MP_FAIL is lost on the received side. Only DSS is received.
> 
> If we write MP_FAIL + DSS like the orignal code, MP_FAIL and DSS will be
> received correctly.
> 
> I haven't figured out why yet.
> 
> Could we just keep this MP_FAIL writting code still at the beginning of
> the function just like the orignal code did?

Thank you for having tested and reviewed the series!

Is this issue here fixed by your patch you sent a couple of minutes ago
(mptcp: fix a DSS option writting error)

That would make sense as the only thing I did was to send the MP_FAIL
after the DSS option.

Do you mind if I send a v8 series with your new fix the missing 'ptr +=
2;' below and have the code moving the writing of the MP_FAIL option
later in a dedicated commit?

> 
>> -
>> -	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and RST are mutually exclusive,
>> +	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and FAIL are mutually exclusive,
>>  	 * see mptcp_established_options*()
>>  	 */
>>  	if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
>> @@ -1328,6 +1318,10 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>  						   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
>>  			}
>>  		}
>> +
>> +		/* We might need to add MP_FAIL options in rare cases */
>> +		if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions))
>> +			goto mp_fail;
>>  	} else if (OPTIONS_MPTCP_MPC & opts->suboptions) {
>>  		u8 len, flag = MPTCP_CAP_HMAC_SHA256;
>>  
>> @@ -1460,19 +1454,41 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>  				ptr += 1;
>>  			}
>>  		}
>> -	} else if (unlikely(OPTION_MPTCP_RST & opts->suboptions)) {
>> -		/* RST is mutually exclusive with everything else */
>> -		*ptr++ = mptcp_option(MPTCPOPT_RST,
>> -				      TCPOLEN_MPTCP_RST,
>> -				      opts->reset_transient,
>> -				      opts->reset_reason);
>> -		return;
>>  	} else if (unlikely(OPTION_MPTCP_FASTCLOSE & opts->suboptions)) {
>> -		/* FASTCLOSE is mutually exclusive with everything else */
>> +		/* FASTCLOSE is mutually exclusive with others except RST */
>>  		*ptr++ = mptcp_option(MPTCPOPT_MP_FASTCLOSE,
>>  				      TCPOLEN_MPTCP_FASTCLOSE,
>>  				      0, 0);
>>  		put_unaligned_be64(opts->rcvr_key, ptr);
> 
> Here, 'ptr += 2;' is needed.

Good catch!

Note related to my modification but to "mptcp: implement fastclose xmit
path" so it should be in this squash-to patch!

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

Re: [PATCH mptcp-next v7 1/4] Squash to "mptcp: implement fastclose xmit path"
Posted by Geliang Tang 4 months, 2 weeks ago
On Wed, Jan 05, 2022 at 11:01:59AM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 05/01/2022 09:50, Geliang Tang wrote:
> > On Thu, Dec 30, 2021 at 08:16:48PM +0100, Matthieu Baerts wrote:
> >> From: Geliang Tang <geliang.tang@suse.com>
> >>
> >> MP_FAIL could be sent with RST or DSS, and FASTCLOSE can be sent with
> >> RST too. So we should use a similar xmit logic for FASTCLOSE and
> >> MP_FAIL in both mptcp_write_options() and mptcp_established_options*().
> >>
> >> Cc: Paolo Abeni <pabeni@redhat.com>
> >> Co-developed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> >> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> >> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> >> ---
> >>  net/mptcp/options.c | 66 ++++++++++++++++++++++++++++-----------------
> >>  1 file changed, 41 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >> index c6726e8389ec..9d2c1c9edbe6 100644
> >> --- a/net/mptcp/options.c
> >> +++ b/net/mptcp/options.c
> >> @@ -829,8 +829,12 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
> >>  
> >>  	if (unlikely(skb && TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST)) {
> >>  		if (mptcp_established_options_fastclose(sk, &opt_size, remaining, opts) ||
> >> -		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts) ||
> >> -		    mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
> >> +		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts)) {
> >> +			*size += opt_size;
> >> +			remaining -= opt_size;
> >> +		}
> >> +		/* MP_RST can be used with MP_FASTCLOSE and MP_FAIL if there is room */
> >> +		if (mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
> >>  			*size += opt_size;
> >>  			remaining -= opt_size;
> >>  		}
> >> @@ -1257,21 +1261,7 @@ static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
> >>  void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >>  			 struct mptcp_out_options *opts)
> >>  {
> >> -	if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
> >> -		const struct sock *ssk = (const struct sock *)tp;
> >> -		struct mptcp_subflow_context *subflow;
> >> -
> >> -		subflow = mptcp_subflow_ctx(ssk);
> >> -		subflow->send_mp_fail = 0;
> >> -
> >> -		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
> >> -				      TCPOLEN_MPTCP_FAIL,
> >> -				      0, 0);
> >> -		put_unaligned_be64(opts->fail_seq, ptr);
> >> -		ptr += 2;
> >> -	}
> > 
> > Hi Matt,
> > 
> > Sorry, this patch doesn't work on my test. We write both DSS + MP_FAIL,
> > but MP_FAIL is lost on the received side. Only DSS is received.
> > 
> > If we write MP_FAIL + DSS like the orignal code, MP_FAIL and DSS will be
> > received correctly.
> > 
> > I haven't figured out why yet.
> > 
> > Could we just keep this MP_FAIL writting code still at the beginning of
> > the function just like the orignal code did?
> 
> Thank you for having tested and reviewed the series!
> 
> Is this issue here fixed by your patch you sent a couple of minutes ago
> (mptcp: fix a DSS option writting error)
> 
> That would make sense as the only thing I did was to send the MP_FAIL
> after the DSS option.
> 
> Do you mind if I send a v8 series with your new fix the missing 'ptr +=
> 2;' below and have the code moving the writing of the MP_FAIL option
> later in a dedicated commit?

Hi Matt,

I think about it again. How about just using the v2 of this patch
(https://patchwork.kernel.org/project/mptcp/patch/5a8c6024481d6106c662c3e892ae5e499b4a7f76.1638156809.git.geliang.tang@suse.com/)
as a squash-to patch (We have reached an agreement on this part), and move
all the other code in this v7 as a new patch (It still needs to be
improved)?

Thanks,
-Geliang

> 
> > 
> >> -
> >> -	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and RST are mutually exclusive,
> >> +	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and FAIL are mutually exclusive,
> >>  	 * see mptcp_established_options*()
> >>  	 */
> >>  	if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
> >> @@ -1328,6 +1318,10 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >>  						   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
> >>  			}
> >>  		}
> >> +
> >> +		/* We might need to add MP_FAIL options in rare cases */
> >> +		if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions))
> >> +			goto mp_fail;
> >>  	} else if (OPTIONS_MPTCP_MPC & opts->suboptions) {
> >>  		u8 len, flag = MPTCP_CAP_HMAC_SHA256;
> >>  
> >> @@ -1460,19 +1454,41 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >>  				ptr += 1;
> >>  			}
> >>  		}
> >> -	} else if (unlikely(OPTION_MPTCP_RST & opts->suboptions)) {
> >> -		/* RST is mutually exclusive with everything else */
> >> -		*ptr++ = mptcp_option(MPTCPOPT_RST,
> >> -				      TCPOLEN_MPTCP_RST,
> >> -				      opts->reset_transient,
> >> -				      opts->reset_reason);
> >> -		return;
> >>  	} else if (unlikely(OPTION_MPTCP_FASTCLOSE & opts->suboptions)) {
> >> -		/* FASTCLOSE is mutually exclusive with everything else */
> >> +		/* FASTCLOSE is mutually exclusive with others except RST */
> >>  		*ptr++ = mptcp_option(MPTCPOPT_MP_FASTCLOSE,
> >>  				      TCPOLEN_MPTCP_FASTCLOSE,
> >>  				      0, 0);
> >>  		put_unaligned_be64(opts->rcvr_key, ptr);
> > 
> > Here, 'ptr += 2;' is needed.
> 
> Good catch!
> 
> Note related to my modification but to "mptcp: implement fastclose xmit
> path" so it should be in this squash-to patch!
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
> 


Re: [PATCH mptcp-next v7 1/4] Squash to "mptcp: implement fastclose xmit path"
Posted by Matthieu Baerts 4 months, 2 weeks ago
Hi Geliang,

On 05/01/2022 11:55, Geliang Tang wrote:
> On Wed, Jan 05, 2022 at 11:01:59AM +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 05/01/2022 09:50, Geliang Tang wrote:
>>> On Thu, Dec 30, 2021 at 08:16:48PM +0100, Matthieu Baerts wrote:
>>>> From: Geliang Tang <geliang.tang@suse.com>
>>>>
>>>> MP_FAIL could be sent with RST or DSS, and FASTCLOSE can be sent with
>>>> RST too. So we should use a similar xmit logic for FASTCLOSE and
>>>> MP_FAIL in both mptcp_write_options() and mptcp_established_options*().
>>>>
>>>> Cc: Paolo Abeni <pabeni@redhat.com>
>>>> Co-developed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>>>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>>>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>>>> ---
>>>>  net/mptcp/options.c | 66 ++++++++++++++++++++++++++++-----------------
>>>>  1 file changed, 41 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>> index c6726e8389ec..9d2c1c9edbe6 100644
>>>> --- a/net/mptcp/options.c
>>>> +++ b/net/mptcp/options.c
>>>> @@ -829,8 +829,12 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
>>>>  
>>>>  	if (unlikely(skb && TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST)) {
>>>>  		if (mptcp_established_options_fastclose(sk, &opt_size, remaining, opts) ||
>>>> -		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts) ||
>>>> -		    mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
>>>> +		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts)) {
>>>> +			*size += opt_size;
>>>> +			remaining -= opt_size;
>>>> +		}
>>>> +		/* MP_RST can be used with MP_FASTCLOSE and MP_FAIL if there is room */
>>>> +		if (mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
>>>>  			*size += opt_size;
>>>>  			remaining -= opt_size;
>>>>  		}
>>>> @@ -1257,21 +1261,7 @@ static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
>>>>  void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>>>  			 struct mptcp_out_options *opts)
>>>>  {
>>>> -	if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
>>>> -		const struct sock *ssk = (const struct sock *)tp;
>>>> -		struct mptcp_subflow_context *subflow;
>>>> -
>>>> -		subflow = mptcp_subflow_ctx(ssk);
>>>> -		subflow->send_mp_fail = 0;
>>>> -
>>>> -		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
>>>> -				      TCPOLEN_MPTCP_FAIL,
>>>> -				      0, 0);
>>>> -		put_unaligned_be64(opts->fail_seq, ptr);
>>>> -		ptr += 2;
>>>> -	}
>>>
>>> Hi Matt,
>>>
>>> Sorry, this patch doesn't work on my test. We write both DSS + MP_FAIL,
>>> but MP_FAIL is lost on the received side. Only DSS is received.
>>>
>>> If we write MP_FAIL + DSS like the orignal code, MP_FAIL and DSS will be
>>> received correctly.
>>>
>>> I haven't figured out why yet.
>>>
>>> Could we just keep this MP_FAIL writting code still at the beginning of
>>> the function just like the orignal code did?
>>
>> Thank you for having tested and reviewed the series!
>>
>> Is this issue here fixed by your patch you sent a couple of minutes ago
>> (mptcp: fix a DSS option writting error)
>>
>> That would make sense as the only thing I did was to send the MP_FAIL
>> after the DSS option.
>>
>> Do you mind if I send a v8 series with your new fix the missing 'ptr +=
>> 2;' below and have the code moving the writing of the MP_FAIL option
>> later in a dedicated commit?
> 
> Hi Matt,
> 
> I think about it again. How about just using the v2 of this patch
> (https://patchwork.kernel.org/project/mptcp/patch/5a8c6024481d6106c662c3e892ae5e499b4a7f76.1638156809.git.geliang.tang@suse.com/)
> as a squash-to patch (We have reached an agreement on this part), and move
> all the other code in this v7 as a new patch (It still needs to be
> improved)?

Yes, I was thinking about something similar except that we also need to
modify mptcp_write_options() to allow sending a RST and a FC together.

I just made the split and for this afternoon, I'm hoping to launch the
new tests you added and try to understand the issue. But if you prefer I
already sent the v8 I have, I can.

Thank you for the instructions from your previous email!

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

Re: [PATCH mptcp-next v7 1/4] Squash to "mptcp: implement fastclose xmit path"
Posted by Geliang Tang 4 months, 2 weeks ago
On Wed, Jan 05, 2022 at 11:01:59AM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 05/01/2022 09:50, Geliang Tang wrote:
> > On Thu, Dec 30, 2021 at 08:16:48PM +0100, Matthieu Baerts wrote:
> >> From: Geliang Tang <geliang.tang@suse.com>
> >>
> >> MP_FAIL could be sent with RST or DSS, and FASTCLOSE can be sent with
> >> RST too. So we should use a similar xmit logic for FASTCLOSE and
> >> MP_FAIL in both mptcp_write_options() and mptcp_established_options*().
> >>
> >> Cc: Paolo Abeni <pabeni@redhat.com>
> >> Co-developed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> >> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> >> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> >> ---
> >>  net/mptcp/options.c | 66 ++++++++++++++++++++++++++++-----------------
> >>  1 file changed, 41 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >> index c6726e8389ec..9d2c1c9edbe6 100644
> >> --- a/net/mptcp/options.c
> >> +++ b/net/mptcp/options.c
> >> @@ -829,8 +829,12 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
> >>  
> >>  	if (unlikely(skb && TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST)) {
> >>  		if (mptcp_established_options_fastclose(sk, &opt_size, remaining, opts) ||
> >> -		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts) ||
> >> -		    mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
> >> +		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts)) {
> >> +			*size += opt_size;
> >> +			remaining -= opt_size;
> >> +		}
> >> +		/* MP_RST can be used with MP_FASTCLOSE and MP_FAIL if there is room */
> >> +		if (mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
> >>  			*size += opt_size;
> >>  			remaining -= opt_size;
> >>  		}
> >> @@ -1257,21 +1261,7 @@ static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
> >>  void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >>  			 struct mptcp_out_options *opts)
> >>  {
> >> -	if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
> >> -		const struct sock *ssk = (const struct sock *)tp;
> >> -		struct mptcp_subflow_context *subflow;
> >> -
> >> -		subflow = mptcp_subflow_ctx(ssk);
> >> -		subflow->send_mp_fail = 0;
> >> -
> >> -		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
> >> -				      TCPOLEN_MPTCP_FAIL,
> >> -				      0, 0);
> >> -		put_unaligned_be64(opts->fail_seq, ptr);
> >> -		ptr += 2;
> >> -	}
> > 
> > Hi Matt,
> > 
> > Sorry, this patch doesn't work on my test. We write both DSS + MP_FAIL,
> > but MP_FAIL is lost on the received side. Only DSS is received.
> > 
> > If we write MP_FAIL + DSS like the orignal code, MP_FAIL and DSS will be
> > received correctly.
> > 
> > I haven't figured out why yet.
> > 
> > Could we just keep this MP_FAIL writting code still at the beginning of
> > the function just like the orignal code did?
> 
> Thank you for having tested and reviewed the series!
> 
> Is this issue here fixed by your patch you sent a couple of minutes ago
> (mptcp: fix a DSS option writting error)

No, the commit (mptcp: fix a DSS option writting error) can't fix this
issue. I haven't found a solution yet. But I noticed if we move the
MP_FAIL writting at the beginning of the function like the orignal code,
it works.

(MP_FAIL + DSS) works, but (DSS + MP_FAIL) doesn't.

> 
> That would make sense as the only thing I did was to send the MP_FAIL
> after the DSS option.
> 
> Do you mind if I send a v8 series with your new fix the missing 'ptr +=
> 2;' below and have the code moving the writing of the MP_FAIL option
> later in a dedicated commit?

Please send a new version, thanks very much.

By the way, you can use the commit ([mptcp-next,v5] selftests: mptcp: add
mp_fail testcases) to test this patch.

Run this command:

./mptcp_join.sh -F

In the multiple subflows testcase, make sure MP_FAIL + MP_RST are sent and
received correctly.

In the single subflow testcase, make sure MP_FAIL + DSS are sent and
received.

Thanks,
-Geliang

> 
> > 
> >> -
> >> -	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and RST are mutually exclusive,
> >> +	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and FAIL are mutually exclusive,
> >>  	 * see mptcp_established_options*()
> >>  	 */
> >>  	if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
> >> @@ -1328,6 +1318,10 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >>  						   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
> >>  			}
> >>  		}
> >> +
> >> +		/* We might need to add MP_FAIL options in rare cases */
> >> +		if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions))
> >> +			goto mp_fail;
> >>  	} else if (OPTIONS_MPTCP_MPC & opts->suboptions) {
> >>  		u8 len, flag = MPTCP_CAP_HMAC_SHA256;
> >>  
> >> @@ -1460,19 +1454,41 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> >>  				ptr += 1;
> >>  			}
> >>  		}
> >> -	} else if (unlikely(OPTION_MPTCP_RST & opts->suboptions)) {
> >> -		/* RST is mutually exclusive with everything else */
> >> -		*ptr++ = mptcp_option(MPTCPOPT_RST,
> >> -				      TCPOLEN_MPTCP_RST,
> >> -				      opts->reset_transient,
> >> -				      opts->reset_reason);
> >> -		return;
> >>  	} else if (unlikely(OPTION_MPTCP_FASTCLOSE & opts->suboptions)) {
> >> -		/* FASTCLOSE is mutually exclusive with everything else */
> >> +		/* FASTCLOSE is mutually exclusive with others except RST */
> >>  		*ptr++ = mptcp_option(MPTCPOPT_MP_FASTCLOSE,
> >>  				      TCPOLEN_MPTCP_FASTCLOSE,
> >>  				      0, 0);
> >>  		put_unaligned_be64(opts->rcvr_key, ptr);
> > 
> > Here, 'ptr += 2;' is needed.
> 
> Good catch!
> 
> Note related to my modification but to "mptcp: implement fastclose xmit
> path" so it should be in this squash-to patch!
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
>