| Message ID | 20240423072137.65168-6-kerneljasonxing@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Implement reset reason mechanism to detect | expand |
Hi Jason, On 23/04/2024 09:21, Jason Xing wrote: > From: Jason Xing <kernelxing@tencent.com> > > It relys on what reset options in the skb are as rfc8684 says. Reusing (if you have something else to fix, 'checkpatch.pl --codespell' reported a warning here: s/relys/relies/) > this logic can save us much energy. This patch replaces most of the prior > NOT_SPECIFIED reasons. > > Signed-off-by: Jason Xing <kernelxing@tencent.com> > --- > net/mptcp/protocol.h | 28 ++++++++++++++++++++++++++++ > net/mptcp/subflow.c | 22 +++++++++++++++++----- > 2 files changed, 45 insertions(+), 5 deletions(-) > > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h > index fdfa843e2d88..bbcb8c068aae 100644 > --- a/net/mptcp/protocol.h > +++ b/net/mptcp/protocol.h > @@ -581,6 +581,34 @@ mptcp_subflow_ctx_reset(struct mptcp_subflow_context *subflow) > WRITE_ONCE(subflow->local_id, -1); > } > > +/* Convert reset reasons in MPTCP to enum sk_rst_reason type */ > +static inline enum sk_rst_reason > +sk_rst_convert_mptcp_reason(u32 reason) > +{ > + switch (reason) { > + case MPTCP_RST_EUNSPEC: > + return SK_RST_REASON_MPTCP_RST_EUNSPEC; > + case MPTCP_RST_EMPTCP: > + return SK_RST_REASON_MPTCP_RST_EMPTCP; > + case MPTCP_RST_ERESOURCE: > + return SK_RST_REASON_MPTCP_RST_ERESOURCE; > + case MPTCP_RST_EPROHIBIT: > + return SK_RST_REASON_MPTCP_RST_EPROHIBIT; > + case MPTCP_RST_EWQ2BIG: > + return SK_RST_REASON_MPTCP_RST_EWQ2BIG; > + case MPTCP_RST_EBADPERF: > + return SK_RST_REASON_MPTCP_RST_EBADPERF; > + case MPTCP_RST_EMIDDLEBOX: > + return SK_RST_REASON_MPTCP_RST_EMIDDLEBOX; > + default: > + /** I guess here as well, it should be '/*' instead of '/**'. But I guess that's fine, this file is probably not scanned. Anyway, if you have to send a new version, please fix this as well. (Also, this helper might require '#include <net/rstreason.h>', but our CI is fine with it, it is also added in the next commit, and probably already included via include/net/request_sock.h. So I guess that's fine.) Other than that, it looks good to me: Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> Cheers, Matt
Hello Matthieu, On Tue, Apr 23, 2024 at 6:02 PM Matthieu Baerts <matttbe@kernel.org> wrote: > > Hi Jason, > > On 23/04/2024 09:21, Jason Xing wrote: > > From: Jason Xing <kernelxing@tencent.com> > > > > It relys on what reset options in the skb are as rfc8684 says. Reusing > > (if you have something else to fix, 'checkpatch.pl --codespell' reported > a warning here: s/relys/relies/) Thanks. Will fix it. > > > this logic can save us much energy. This patch replaces most of the prior > > NOT_SPECIFIED reasons. > > > > Signed-off-by: Jason Xing <kernelxing@tencent.com> > > --- > > net/mptcp/protocol.h | 28 ++++++++++++++++++++++++++++ > > net/mptcp/subflow.c | 22 +++++++++++++++++----- > > 2 files changed, 45 insertions(+), 5 deletions(-) > > > > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h > > index fdfa843e2d88..bbcb8c068aae 100644 > > --- a/net/mptcp/protocol.h > > +++ b/net/mptcp/protocol.h > > @@ -581,6 +581,34 @@ mptcp_subflow_ctx_reset(struct mptcp_subflow_context *subflow) > > WRITE_ONCE(subflow->local_id, -1); > > } > > > > +/* Convert reset reasons in MPTCP to enum sk_rst_reason type */ > > +static inline enum sk_rst_reason > > +sk_rst_convert_mptcp_reason(u32 reason) > > +{ > > + switch (reason) { > > + case MPTCP_RST_EUNSPEC: > > + return SK_RST_REASON_MPTCP_RST_EUNSPEC; > > + case MPTCP_RST_EMPTCP: > > + return SK_RST_REASON_MPTCP_RST_EMPTCP; > > + case MPTCP_RST_ERESOURCE: > > + return SK_RST_REASON_MPTCP_RST_ERESOURCE; > > + case MPTCP_RST_EPROHIBIT: > > + return SK_RST_REASON_MPTCP_RST_EPROHIBIT; > > + case MPTCP_RST_EWQ2BIG: > > + return SK_RST_REASON_MPTCP_RST_EWQ2BIG; > > + case MPTCP_RST_EBADPERF: > > + return SK_RST_REASON_MPTCP_RST_EBADPERF; > > + case MPTCP_RST_EMIDDLEBOX: > > + return SK_RST_REASON_MPTCP_RST_EMIDDLEBOX; > > + default: > > + /** > > I guess here as well, it should be '/*' instead of '/**'. But I guess > that's fine, this file is probably not scanned. Anyway, if you have to > send a new version, please fix this as well. Thanks for your help. I will. > > (Also, this helper might require '#include <net/rstreason.h>', but our > CI is fine with it, it is also added in the next commit, and probably > already included via include/net/request_sock.h. So I guess that's fine.) Yes, If I need to submit the V9 patch, I will move it. > > > Other than that, it looks good to me: > > Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> Thanks for all the reviews :) Thanks, Jason > > Cheers, > Matt > -- > Sponsored by the NGI0 Core fund. >
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index fdfa843e2d88..bbcb8c068aae 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -581,6 +581,34 @@ mptcp_subflow_ctx_reset(struct mptcp_subflow_context *subflow) WRITE_ONCE(subflow->local_id, -1); } +/* Convert reset reasons in MPTCP to enum sk_rst_reason type */ +static inline enum sk_rst_reason +sk_rst_convert_mptcp_reason(u32 reason) +{ + switch (reason) { + case MPTCP_RST_EUNSPEC: + return SK_RST_REASON_MPTCP_RST_EUNSPEC; + case MPTCP_RST_EMPTCP: + return SK_RST_REASON_MPTCP_RST_EMPTCP; + case MPTCP_RST_ERESOURCE: + return SK_RST_REASON_MPTCP_RST_ERESOURCE; + case MPTCP_RST_EPROHIBIT: + return SK_RST_REASON_MPTCP_RST_EPROHIBIT; + case MPTCP_RST_EWQ2BIG: + return SK_RST_REASON_MPTCP_RST_EWQ2BIG; + case MPTCP_RST_EBADPERF: + return SK_RST_REASON_MPTCP_RST_EBADPERF; + case MPTCP_RST_EMIDDLEBOX: + return SK_RST_REASON_MPTCP_RST_EMIDDLEBOX; + default: + /** + * It should not happen, or else errors may occur + * in MPTCP layer + */ + return SK_RST_REASON_ERROR; + } +} + static inline u64 mptcp_subflow_get_map_offset(const struct mptcp_subflow_context *subflow) { diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index ac867d277860..fb7abf2d01ca 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -309,8 +309,13 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk, return dst; dst_release(dst); - if (!req->syncookie) - tcp_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED); + if (!req->syncookie) { + struct mptcp_ext *mpext = mptcp_get_ext(skb); + enum sk_rst_reason reason; + + reason = sk_rst_convert_mptcp_reason(mpext->reset_reason); + tcp_request_sock_ops.send_reset(sk, skb, reason); + } return NULL; } @@ -377,8 +382,13 @@ static struct dst_entry *subflow_v6_route_req(const struct sock *sk, return dst; dst_release(dst); - if (!req->syncookie) - tcp6_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED); + if (!req->syncookie) { + struct mptcp_ext *mpext = mptcp_get_ext(skb); + enum sk_rst_reason reason; + + reason = sk_rst_convert_mptcp_reason(mpext->reset_reason); + tcp6_request_sock_ops.send_reset(sk, skb, reason); + } return NULL; } #endif @@ -783,6 +793,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk, struct mptcp_subflow_request_sock *subflow_req; struct mptcp_options_received mp_opt; bool fallback, fallback_is_fatal; + enum sk_rst_reason reason; struct mptcp_sock *owner; struct sock *child; @@ -913,7 +924,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk, tcp_rsk(req)->drop_req = true; inet_csk_prepare_for_destroy_sock(child); tcp_done(child); - req->rsk_ops->send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED); + reason = sk_rst_convert_mptcp_reason(mptcp_get_ext(skb)->reset_reason); + req->rsk_ops->send_reset(sk, skb, reason); /* The last child reference will be released by the caller */ return child;