[PATCH mptcp-next v3 0/2] send MP_FAIL with MP_RST and others

[PATCH mptcp-next v3 0/2] send MP_FAIL with MP_RST and others

[Geliang Tang]

v3:
 - update patch 1 as Matt suggested.
 - do more cleanups in patch 1.

v2:
 - rename patch 1 as a squash-to patch.
 - print out reset_transient in patch 2 too. Please keep patch 2 as a
standalone patch, not a squash-to patch.

Two small patches about sending MP_FAIL with MP_RST.

Geliang Tang (2):
  Squash to "mptcp: implement fastclose xmit path"
  mptcp: print out reset infos of MP_RST

 net/mptcp/options.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

-- 
2.31.1

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

[Geliang Tang]

As described in RFC8684, 3.7 Fallback:

'''
Therefore, it is not possible to recover the subflow, and the affected
subflow must be immediately closed with a RST that includes an MP_FAIL
option (Figure 16), which defines the data sequence number at the start
of the segment (defined by the Data Sequence Mapping) that had the
checksum failure.
'''

MP_FAIL could be sent with MP_RST at the same time. This patch fixed it.

This patch also did a small cleanup, to keep the options order in
mptcp_write_options() as this comment said:

'''
/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and RST are mutually exclusive,
'''

This order matches the options order in RFC8684, 7.2. MPTCP Option Subtypes
too.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/options.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 8a1020e4285c..35425fb4b179 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -828,8 +828,11 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
 		return false;
 
 	if (unlikely(skb && TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST)) {
+		if (mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts)) {
+			*size += opt_size;
+			remaining -= opt_size;
+		}
 		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)) {
 			*size += opt_size;
 			remaining -= opt_size;
@@ -1458,13 +1461,6 @@ 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 */
 		*ptr++ = mptcp_option(MPTCPOPT_MP_FASTCLOSE,
@@ -1472,6 +1468,13 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 				      0, 0);
 		put_unaligned_be64(opts->rcvr_key, ptr);
 		return;
+	} 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;
 	}
 
 	if (OPTION_MPTCP_PRIO & opts->suboptions) {
-- 
2.31.1

[PATCH mptcp-next v3 2/2] mptcp: print out reset infos of MP_RST

[Geliang Tang]

This patch printed out the reset infos, reset_transient and reset_reason,
of MP_RST in mptcp_parse_option() to show that MP_RST is received.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/options.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 35425fb4b179..f38220f30d7a 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -336,6 +336,8 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 		flags = *ptr++;
 		mp_opt->reset_transient = flags & MPTCP_RST_TRANSIENT;
 		mp_opt->reset_reason = *ptr;
+		pr_debug("MP_RST: transient=%u reason=%u",
+			 mp_opt->reset_transient, mp_opt->reset_reason);
 		break;
 
 	case MPTCPOPT_MP_FAIL:
-- 
2.31.1

Re: [PATCH mptcp-next v3 1/2] Squash to "mptcp: implement fastclose xmit path"

[Matthieu Baerts]

Hi Geliang,

Adding Paolo in cc as he wrote the original commit.

On 30/11/2021 04:21, Geliang Tang wrote:
> As described in RFC8684, 3.7 Fallback:
> 
> '''
> Therefore, it is not possible to recover the subflow, and the affected
> subflow must be immediately closed with a RST that includes an MP_FAIL
> option (Figure 16), which defines the data sequence number at the start
> of the segment (defined by the Data Sequence Mapping) that had the
> checksum failure.
> '''
> 
> MP_FAIL could be sent with MP_RST at the same time. This patch fixed it.
> 
> This patch also did a small cleanup, to keep the options order in
> mptcp_write_options() as this comment said:
> 
> '''
> /* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and RST are mutually exclusive,
> '''
> 
> This order matches the options order in RFC8684, 7.2. MPTCP Option Subtypes
> too.
> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/options.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 8a1020e4285c..35425fb4b179 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -828,8 +828,11 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
>  		return false;
>  
>  	if (unlikely(skb && TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST)) {
> +		if (mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts)) {
> +			*size += opt_size;
> +			remaining -= opt_size;
> +		}
>  		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)) {

Thank you for the new version.

This is now aligned with what we can find in mptcp_write_options() but
is it correct?

Here you say:

* FAIL + RST:    OK
* FAIL + FCLOSE: OK
* RST  + FCLOSE: KO

But is it correct? Is it not:

* FAIL + RST:    OK
* FAIL + FCLOSE: KO
* RST  + FCLOSE: OK

No? It sounds more logic to me to add more info (MP_RST) in an MP_FAIL
or MP_FASTCLOSE if there is room. Can we have both an MP_FAIL and an
MP_FASTCLOSE in the same packet?

This really shows we need a packetdrill test to agree on that and
validate this behaviour :)

Let me send a patch of what I have in mind. (it is too long to be
readable here I think).

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

[PATCH mptcp-next] Squash to "mptcp: implement fastclose xmit path"

[Matthieu Baerts]

This is linked to Geliang's patch with the same title.

If I understand the RFC properly, we can use MP_RST with an MP_FASTCLOSE
and an MP_FAIL (or none of them) but we cannot use MP_FASTCLOSE and
MP_FAIL together.

This patch implements this logic in both mptcp_established_options() and
mptcp_write_options(). Before this patch, the first function was not
allowing any of these 3 options to be used at the same time while the
second one was allowing MP_FAIL to be used with either MP_FASTCLOSE or
MP_RST.

Small note: I tried to keep Paolo's idea of reducing conditions for
"normal" options by moving MP_RST at the end and allow to jump there
from MP_FAIL and MP_FASTCLOSE but I'm not sure it makes it easy to read
and really improves perfs. We can also move MP_RST check at the
beginning.

@Geliang: feel free to re-use this patch if it looks OK to you.

PS: I didn't try this patch, just quickly wrote it to better explain
what I had in mind.

Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Geliang Tang <geliang.tang@suse.com>
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---

Notes:
    to be squashed in "mptcp: implement fastclose xmit path"

 net/mptcp/options.c | 62 +++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 25 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 8a1020e4285c..07091971a51a 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -829,11 +829,17 @@ 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;
+		}
+
 		return true;
 	}
 
@@ -1257,21 +1263,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)) {
@@ -1458,19 +1450,39 @@ 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 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.32.0

Re: Squash to "mptcp: implement fastclose xmit path": Build Failure

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

But sadly, our CI spotted some issues with it when trying to build it.

You can find more details there:

  https://patchwork.kernel.org/project/mptcp/patch/20211130165527.1506827-1-matthieu.baerts@tessares.net/
  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/1521989225

Status: failure
Initiator: MPTCPimporter
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/6efac2b03aba

Feel free to reply to this email if you cannot access logs, if you need
some support to fix the error, if this doesn't seem to be caused by your
modifications or if the error is a false positive one.

Cheers,
MPTCP GH Action bot

Re: [PATCH mptcp-next] Squash to "mptcp: implement fastclose xmit path"

[Paolo Abeni]

On Tue, 2021-11-30 at 17:55 +0100, Matthieu Baerts wrote:
> This is linked to Geliang's patch with the same title.
> 
> If I understand the RFC properly, we can use MP_RST with an MP_FASTCLOSE
> and an MP_FAIL (or none of them) but we cannot use MP_FASTCLOSE and
> MP_FAIL together.
> 
> This patch implements this logic in both mptcp_established_options() and
> mptcp_write_options(). Before this patch, the first function was not
> allowing any of these 3 options to be used at the same time while the
> second one was allowing MP_FAIL to be used with either MP_FASTCLOSE or
> MP_RST.
> 
> Small note: I tried to keep Paolo's idea of reducing conditions for
> "normal" options by moving MP_RST at the end and allow to jump there
> from MP_FAIL and MP_FASTCLOSE but I'm not sure it makes it easy to read
> and really improves perfs. We can also move MP_RST check at the
> beginning.
> 
> @Geliang: feel free to re-use this patch if it looks OK to you.
> 
> PS: I didn't try this patch, just quickly wrote it to better explain
> what I had in mind.
> 
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Geliang Tang <geliang.tang@suse.com>
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
> 
> Notes:
>     to be squashed in "mptcp: implement fastclose xmit path"
> 
>  net/mptcp/options.c | 62 +++++++++++++++++++++++++++------------------
>  1 file changed, 37 insertions(+), 25 deletions(-)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 8a1020e4285c..07091971a51a 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -829,11 +829,17 @@ 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;
> +		}
> +
>  		return true;
>  	}
>  
> @@ -1257,21 +1263,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)) {
> @@ -1458,19 +1450,39 @@ 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 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

As the CI spotted, missing ';' here.

Otherwise LGTM!

Thanks,

Paolo

Re: [PATCH mptcp-next] Squash to "mptcp: implement fastclose xmit path"

[Matthieu Baerts]

Hi Paolo, Geliang,

On 01/12/2021 11:40, Paolo Abeni wrote:
> On Tue, 2021-11-30 at 17:55 +0100, Matthieu Baerts wrote:
>> This is linked to Geliang's patch with the same title.
>>
>> If I understand the RFC properly, we can use MP_RST with an MP_FASTCLOSE
>> and an MP_FAIL (or none of them) but we cannot use MP_FASTCLOSE and
>> MP_FAIL together.
>>
>> This patch implements this logic in both mptcp_established_options() and
>> mptcp_write_options(). Before this patch, the first function was not
>> allowing any of these 3 options to be used at the same time while the
>> second one was allowing MP_FAIL to be used with either MP_FASTCLOSE or
>> MP_RST.
>>
>> Small note: I tried to keep Paolo's idea of reducing conditions for
>> "normal" options by moving MP_RST at the end and allow to jump there
>> from MP_FAIL and MP_FASTCLOSE but I'm not sure it makes it easy to read
>> and really improves perfs. We can also move MP_RST check at the
>> beginning.
>>
>> @Geliang: feel free to re-use this patch if it looks OK to you.
>>
>> PS: I didn't try this patch, just quickly wrote it to better explain
>> what I had in mind.
>>
>> Cc: Paolo Abeni <pabeni@redhat.com>
>> Cc: Geliang Tang <geliang.tang@suse.com>
>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>> ---
>>
>> Notes:
>>     to be squashed in "mptcp: implement fastclose xmit path"
>>
>>  net/mptcp/options.c | 62 +++++++++++++++++++++++++++------------------
>>  1 file changed, 37 insertions(+), 25 deletions(-)
>>
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index 8a1020e4285c..07091971a51a 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -829,11 +829,17 @@ 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;
>> +		}
>> +
>>  		return true;
>>  	}
>>  
>> @@ -1257,21 +1263,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)) {
>> @@ -1458,19 +1450,39 @@ 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 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
> 
> As the CI spotted, missing ';' here.

Yes sorry, I quickly did it before having to go.

> Otherwise LGTM!

Thank you for the review!

@Geliang: do you prefer to use this diff -- without the missing ';' --
in a new version or do you prefer if I apply it with the fix directly?

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

Re: [PATCH mptcp-next] Squash to "mptcp: implement fastclose xmit path"

[Mat Martineau]

On Wed, 1 Dec 2021, Matthieu Baerts wrote:

> Hi Paolo, Geliang,
>
> On 01/12/2021 11:40, Paolo Abeni wrote:
>> On Tue, 2021-11-30 at 17:55 +0100, Matthieu Baerts wrote:
>>> This is linked to Geliang's patch with the same title.
>>>
>>> If I understand the RFC properly, we can use MP_RST with an MP_FASTCLOSE
>>> and an MP_FAIL (or none of them) but we cannot use MP_FASTCLOSE and
>>> MP_FAIL together.
>>>
>>> This patch implements this logic in both mptcp_established_options() and
>>> mptcp_write_options(). Before this patch, the first function was not
>>> allowing any of these 3 options to be used at the same time while the
>>> second one was allowing MP_FAIL to be used with either MP_FASTCLOSE or
>>> MP_RST.
>>>
>>> Small note: I tried to keep Paolo's idea of reducing conditions for
>>> "normal" options by moving MP_RST at the end and allow to jump there
>>> from MP_FAIL and MP_FASTCLOSE but I'm not sure it makes it easy to read
>>> and really improves perfs. We can also move MP_RST check at the
>>> beginning.
>>>
>>> @Geliang: feel free to re-use this patch if it looks OK to you.
>>>
>>> PS: I didn't try this patch, just quickly wrote it to better explain
>>> what I had in mind.
>>>
>>> Cc: Paolo Abeni <pabeni@redhat.com>
>>> Cc: Geliang Tang <geliang.tang@suse.com>
>>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>>> ---
>>>
>>> Notes:
>>>     to be squashed in "mptcp: implement fastclose xmit path"
>>>
>>>  net/mptcp/options.c | 62 +++++++++++++++++++++++++++------------------
>>>  1 file changed, 37 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>> index 8a1020e4285c..07091971a51a 100644
>>> --- a/net/mptcp/options.c
>>> +++ b/net/mptcp/options.c
>>> @@ -829,11 +829,17 @@ 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;
>>> +		}
>>> +
>>>  		return true;
>>>  	}
>>>
>>> @@ -1257,21 +1263,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)) {
>>> @@ -1458,19 +1450,39 @@ 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 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
>>
>> As the CI spotted, missing ';' here.
>
> Yes sorry, I quickly did it before having to go.
>
>> Otherwise LGTM!
>
> Thank you for the review!
>
> @Geliang: do you prefer to use this diff -- without the missing ';' --
> in a new version or do you prefer if I apply it with the fix directly?
>

Matthieu -

Since you were handling Geliang's revisions of this series while I was 
away, I assume you are not waiting to hear from me, but anyway: these 
changes (plus ';') combined with Geliang's v3 all look good to me too.

--
Mat Martineau
Intel