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

[Matthieu Baerts <matthieu.baerts@tessares.net>]

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