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

[Geliang Tang <geliang.tang@suse.com>]

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
>