* [PATCH mptcp-next v3 0/2] send MP_FAIL with MP_RST and others @ 2021-11-30 3:21 Geliang Tang 2021-11-30 3:21 ` [PATCH mptcp-next v3 1/2] Squash to "mptcp: implement fastclose xmit path" Geliang Tang 2021-11-30 3:21 ` [PATCH mptcp-next v3 2/2] mptcp: print out reset infos of MP_RST Geliang Tang 0 siblings, 2 replies; 9+ messages in thread From: Geliang Tang @ 2021-11-30 3:21 UTC (permalink / raw) To: mptcp; +Cc: 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH mptcp-next v3 1/2] Squash to "mptcp: implement fastclose xmit path" 2021-11-30 3:21 [PATCH mptcp-next v3 0/2] send MP_FAIL with MP_RST and others Geliang Tang @ 2021-11-30 3:21 ` Geliang Tang 2021-11-30 16:52 ` Matthieu Baerts 2021-11-30 3:21 ` [PATCH mptcp-next v3 2/2] mptcp: print out reset infos of MP_RST Geliang Tang 1 sibling, 1 reply; 9+ messages in thread From: Geliang Tang @ 2021-11-30 3:21 UTC (permalink / raw) To: mptcp; +Cc: 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH mptcp-next v3 1/2] Squash to "mptcp: implement fastclose xmit path" 2021-11-30 3:21 ` [PATCH mptcp-next v3 1/2] Squash to "mptcp: implement fastclose xmit path" Geliang Tang @ 2021-11-30 16:52 ` Matthieu Baerts 2021-11-30 16:55 ` [PATCH mptcp-next] " Matthieu Baerts 0 siblings, 1 reply; 9+ messages in thread From: Matthieu Baerts @ 2021-11-30 16:52 UTC (permalink / raw) To: Geliang Tang; +Cc: mptcp, Paolo Abeni 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH mptcp-next] Squash to "mptcp: implement fastclose xmit path" 2021-11-30 16:52 ` Matthieu Baerts @ 2021-11-30 16:55 ` Matthieu Baerts 2021-11-30 17:02 ` Squash to "mptcp: implement fastclose xmit path": Build Failure MPTCP CI 2021-12-01 10:40 ` [PATCH mptcp-next] Squash to "mptcp: implement fastclose xmit path" Paolo Abeni 0 siblings, 2 replies; 9+ messages in thread From: Matthieu Baerts @ 2021-11-30 16:55 UTC (permalink / raw) To: mptcp; +Cc: Matthieu Baerts, Paolo Abeni, Geliang Tang 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: Squash to "mptcp: implement fastclose xmit path": Build Failure 2021-11-30 16:55 ` [PATCH mptcp-next] " Matthieu Baerts @ 2021-11-30 17:02 ` MPTCP CI 2021-12-01 10:40 ` [PATCH mptcp-next] Squash to "mptcp: implement fastclose xmit path" Paolo Abeni 1 sibling, 0 replies; 9+ messages in thread From: MPTCP CI @ 2021-11-30 17:02 UTC (permalink / raw) To: Matthieu Baerts; +Cc: mptcp 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH mptcp-next] Squash to "mptcp: implement fastclose xmit path" 2021-11-30 16:55 ` [PATCH mptcp-next] " Matthieu Baerts 2021-11-30 17:02 ` Squash to "mptcp: implement fastclose xmit path": Build Failure MPTCP CI @ 2021-12-01 10:40 ` Paolo Abeni 2021-12-01 11:23 ` Matthieu Baerts 1 sibling, 1 reply; 9+ messages in thread From: Paolo Abeni @ 2021-12-01 10:40 UTC (permalink / raw) To: Matthieu Baerts, mptcp; +Cc: Geliang Tang 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH mptcp-next] Squash to "mptcp: implement fastclose xmit path" 2021-12-01 10:40 ` [PATCH mptcp-next] Squash to "mptcp: implement fastclose xmit path" Paolo Abeni @ 2021-12-01 11:23 ` Matthieu Baerts 2021-12-01 18:34 ` Mat Martineau 0 siblings, 1 reply; 9+ messages in thread From: Matthieu Baerts @ 2021-12-01 11:23 UTC (permalink / raw) To: Paolo Abeni, Geliang Tang; +Cc: mptcp 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH mptcp-next] Squash to "mptcp: implement fastclose xmit path" 2021-12-01 11:23 ` Matthieu Baerts @ 2021-12-01 18:34 ` Mat Martineau 0 siblings, 0 replies; 9+ messages in thread From: Mat Martineau @ 2021-12-01 18:34 UTC (permalink / raw) To: Matthieu Baerts; +Cc: Paolo Abeni, Geliang Tang, mptcp 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH mptcp-next v3 2/2] mptcp: print out reset infos of MP_RST 2021-11-30 3:21 [PATCH mptcp-next v3 0/2] send MP_FAIL with MP_RST and others Geliang Tang 2021-11-30 3:21 ` [PATCH mptcp-next v3 1/2] Squash to "mptcp: implement fastclose xmit path" Geliang Tang @ 2021-11-30 3:21 ` Geliang Tang 1 sibling, 0 replies; 9+ messages in thread From: Geliang Tang @ 2021-11-30 3:21 UTC (permalink / raw) To: mptcp; +Cc: 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-12-01 18:39 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-30 3:21 [PATCH mptcp-next v3 0/2] send MP_FAIL with MP_RST and others Geliang Tang 2021-11-30 3:21 ` [PATCH mptcp-next v3 1/2] Squash to "mptcp: implement fastclose xmit path" Geliang Tang 2021-11-30 16:52 ` Matthieu Baerts 2021-11-30 16:55 ` [PATCH mptcp-next] " Matthieu Baerts 2021-11-30 17:02 ` Squash to "mptcp: implement fastclose xmit path": Build Failure MPTCP CI 2021-12-01 10:40 ` [PATCH mptcp-next] Squash to "mptcp: implement fastclose xmit path" Paolo Abeni 2021-12-01 11:23 ` Matthieu Baerts 2021-12-01 18:34 ` Mat Martineau 2021-11-30 3:21 ` [PATCH mptcp-next v3 2/2] mptcp: print out reset infos of MP_RST Geliang Tang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.