[PATCH mptcp-next v7 0/4] Clarify when options can be used

[PATCH mptcp-next v7 0/4] Clarify when options can be used

[Matthieu Baerts]

This is a new version of Geliang's series called "send MP_FAIL with MP_RST and
others".

The Squash-to patch is the same as the draft I previously sent but fixing a
compilation issue: MP_RST can be sent with MP_FASTCLOSE and MP_FAIL but
MP_FASTCLOSE cannot be sent with MP_FAIL.

Patch 2/4 has not been modified.

Here, you might want to start looking at "mptcp: clarify when options can be
used" and check if everything is OK for you related to when options can be used
together.

If this is OK, the logic behind the Squash-to patch should be OK. Of course, the
implementation might not be OK but that's different :)

Patch 4/4 is there just to have mptcp_established_options() in sync with
mptcp_write_options() (and the RFC).

Happy New Year!

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

Matthieu Baerts (2):
  mptcp: clarify when options can be used
  mptcp: allow sending both ADD_ADDR and RM_ADDR

 net/mptcp/options.c | 94 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 67 insertions(+), 27 deletions(-)

-- 
2.33.1

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

[Matthieu Baerts]

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

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

[Matthieu Baerts]

From: Geliang Tang <geliang.tang@suse.com>

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 9d2c1c9edbe6..3427b31c0da7 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.33.1

[PATCH mptcp-next v7 3/4] mptcp: clarify when options can be used

[Matthieu Baerts]

RFC8684 doesn't seem to clearly specify which MPTCP options can be used
together.

Some options are mutually exclusive -- e.g. MP_CAPABLE and MP_JOIN --,
some can be used together -- e.g. DSS + MP_PRIO --, some can but we
prefer not to -- e.g. DSS + ADD_ADDR -- and some have to be used
together at some points -- e.g. MP_FAIL and DSS.

We need to clarify this as a base before allowing other modifications.

For example, does it make sense to send a RM_ADDR with an MPC or MPJ?
This remains open for possible future discussions.

Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/options.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 3427b31c0da7..d06855e66007 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1263,8 +1263,27 @@ 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)
 {
-	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and FAIL are mutually exclusive,
-	 * see mptcp_established_options*()
+	/* Which options can be used together?
+	 *
+	 * X: mutually exclusive
+	 * O: often used together
+	 * C: can be used together in some cases
+	 * P: could be used together but we prefer not to (optimisations)
+	 *
+	 *  Opt: | MPC  | MPJ  | DSS  | ADD  |  RM  | PRIO | FAIL |  FC  |
+	 * ------|------|------|------|------|------|------|------|------|
+	 *  MPC  |------|------|------|------|------|------|------|------|
+	 *  MPJ  |  X   |------|------|------|------|------|------|------|
+	 *  DSS  |  X   |  X   |------|------|------|------|------|------|
+	 *  ADD  |  X   |  X   |  P   |------|------|------|------|------|
+	 *  RM   |  C   |  C   |  C   |  C   |------|------|------|------|
+	 *  PRIO |  X   |  C   |  C   |  C   |  C   |------|------|------|
+	 *  FAIL |  X   |  X   |  C   |  X   |  X   |  X   |------|------|
+	 *  FC   |  X   |  X   |  X   |  X   |  X   |  X   |  X   |------|
+	 *  RST  |  X   |  X   |  X   |  X   |  X   |  X   |  O   |  O   |
+	 * ------|------|------|------|------|------|------|------|------|
+	 *
+	 * The same applies in mptcp_established_options() function.
 	 */
 	if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
 		struct mptcp_ext *mpext = &opts->ext_copy;
-- 
2.33.1

[PATCH mptcp-next v7 4/4] mptcp: allow sending both ADD_ADDR and RM_ADDR

[Matthieu Baerts]

This is mainly to be sync with what is done in mptcp_write_options()
where after having written the ADD_ADDR option, we check if the RM_ADDR
option also needs to be added.

But if we have room, it makes sense to write the two options in the same
packet. So best to remove the 'else'.

While at it, also mention that mptcp_established_options_add_addr() can
remove options.

Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/options.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index d06855e66007..c08d4b04458b 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -863,11 +863,14 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
 
 	*size += opt_size;
 	remaining -= opt_size;
+	/* Note: this can remove previously set options in some conditions */
 	if (mptcp_established_options_add_addr(sk, skb, &opt_size, remaining, opts)) {
 		*size += opt_size;
 		remaining -= opt_size;
 		ret = true;
-	} else if (mptcp_established_options_rm_addr(sk, &opt_size, remaining, opts)) {
+	}
+
+	if (mptcp_established_options_rm_addr(sk, &opt_size, remaining, opts)) {
 		*size += opt_size;
 		remaining -= opt_size;
 		ret = true;
-- 
2.33.1

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

[Geliang Tang]

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"

[Matthieu Baerts]

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"

[Geliang Tang]

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
> 

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

[Geliang Tang]

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"

[Matthieu Baerts]

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