Hello Mat, On Fri, Apr 5, 2024 at 4:33 AM Mat Martineau <martin...@kernel.org> wrote: > > On Thu, 4 Apr 2024, Jason Xing wrote: > > > From: Jason Xing <kernelx...@tencent.com> > > > > It relys on what reset options in MPTCP does as rfc8684 says. Reusing > > this logic can save us much energy. This patch replaces all the prior > > NOT_SPECIFIED reasons. > > > > Signed-off-by: Jason Xing <kernelx...@tencent.com> > > --- > > net/mptcp/subflow.c | 26 ++++++++++++++++++++------ > > 1 file changed, 20 insertions(+), 6 deletions(-) > > > > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c > > index a68d5d0f3e2a..24668d3020aa 100644 > > --- a/net/mptcp/subflow.c > > +++ b/net/mptcp/subflow.c > > @@ -304,7 +304,10 @@ static struct dst_entry *subflow_v4_route_req(const > > struct sock *sk, > > > > dst_release(dst); > > if (!req->syncookie) > > - tcp_request_sock_ops.send_reset(sk, skb, > > SK_RST_REASON_NOT_SPECIFIED); > > + /* According to RFC 8684, 3.2. Starting a New Subflow, > > + * we should use an "MPTCP specific error" reason code. > > + */ > > + tcp_request_sock_ops.send_reset(sk, skb, > > SK_RST_REASON_MPTCP_RST_EMPTCP); > > Hi Jason - > > In this case, the MPTCP reset reason is set in subflow_check_req(). Looks > like it uses EMPTCP but that isn't guaranteed to stay the same. I think it > would be better to extract the reset reason from the skb extension or the > subflow context "reset_reason" field.
Good suggestions :) > > > > return NULL; > > } > > > > @@ -371,7 +374,10 @@ static struct dst_entry *subflow_v6_route_req(const > > struct sock *sk, > > > > dst_release(dst); > > if (!req->syncookie) > > - tcp6_request_sock_ops.send_reset(sk, skb, > > SK_RST_REASON_NOT_SPECIFIED); > > + /* According to RFC 8684, 3.2. Starting a New Subflow, > > + * we should use an "MPTCP specific error" reason code. > > + */ > > + tcp6_request_sock_ops.send_reset(sk, skb, > > SK_RST_REASON_MPTCP_RST_EMPTCP); > > Same issue here. Got it. > > > return NULL; > > } > > #endif > > @@ -778,6 +784,7 @@ static struct sock *subflow_syn_recv_sock(const struct > > sock *sk, > > bool fallback, fallback_is_fatal; > > struct mptcp_sock *owner; > > struct sock *child; > > + int reason; > > > > pr_debug("listener=%p, req=%p, conn=%p", listener, req, > > listener->conn); > > > > @@ -833,7 +840,8 @@ static struct sock *subflow_syn_recv_sock(const struct > > sock *sk, > > */ > > if (!ctx || fallback) { > > if (fallback_is_fatal) { > > - subflow_add_reset_reason(skb, > > MPTCP_RST_EMPTCP); > > + reason = MPTCP_RST_EMPTCP; > > + subflow_add_reset_reason(skb, reason); > > goto dispose_child; > > } > > goto fallback; > > @@ -861,7 +869,8 @@ static struct sock *subflow_syn_recv_sock(const struct > > sock *sk, > > } else if (ctx->mp_join) { > > owner = subflow_req->msk; > > if (!owner) { > > - subflow_add_reset_reason(skb, > > MPTCP_RST_EPROHIBIT); > > + reason = MPTCP_RST_EPROHIBIT; > > + subflow_add_reset_reason(skb, reason); > > goto dispose_child; > > } > > > > @@ -875,13 +884,18 @@ static struct sock *subflow_syn_recv_sock(const > > struct sock *sk, > > ntohs(inet_sk((struct sock > > *)owner)->inet_sport)); > > if (!mptcp_pm_sport_in_anno_list(owner, sk)) { > > SUBFLOW_REQ_INC_STATS(req, > > MPTCP_MIB_MISMATCHPORTACKRX); > > + reason = MPTCP_RST_EUNSPEC; > > I think the MPTCP code here should have been using MPTCP_RST_EPROHIBIT. I'll update in the V2 of the patch. Thanks, Jason