| Message ID | 20240405023914.54872-3-kerneljasonxing@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | mptcp: add reset reasons in skb in more cases | expand |
On Fri, 2024-04-05 at 10:39 +0800, Jason Xing wrote: > From: Jason Xing <kernelxing@tencent.com> > > The reason codes are handled in two ways nowadays (quoting Mat Martineau): > 1. Sending in the MPTCP option on RST packets when there is no subflow > context available (these use subflow_add_reset_reason() and directly call > a TCP-level send_reset function) > 2. The "normal" way via subflow->reset_reason. This will propagate to both > the outgoing reset packet and to a local path manager process via netlink > in mptcp_event_sub_closed() > > RFC 8684 defines the skb reset reason behaviour which is not required > even though in some places: > > A host sends a TCP RST in order to close a subflow or reject > an attempt to open a subflow (MP_JOIN). In order to let the > receiving host know why a subflow is being closed or rejected, > the TCP RST packet MAY include the MP_TCPRST option (Figure 15). > The host MAY use this information to decide, for example, whether > it tries to re-establish the subflow immediately, later, or never. > > Since the commit dc87efdb1a5cd ("mptcp: add mptcp reset option support") > introduced this feature about three years ago, we can fully use it. > There remains some places where we could insert reason into skb as > we can see in this patch. > > Many thanks to Mat for help:) > > Signed-off-by: Jason Xing <kernelxing@tencent.com> > --- > net/mptcp/subflow.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c > index 1626dd20c68f..49f746d91884 100644 > --- a/net/mptcp/subflow.c > +++ b/net/mptcp/subflow.c > @@ -301,8 +301,13 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk, > return dst; > > dst_release(dst); > - if (!req->syncookie) > + if (!req->syncookie) { > + struct mptcp_ext *mpext = mptcp_get_ext(skb); > + > + if (mpext) > + subflow_add_reset_reason(skb, mpext->reset_reason); uhm? subflow_add_reset_reason() will do: mptcp_ext_add(skb)->reset_reason = mpext->reset_reason The above looks like a no-op. Possibly we should instead ensure that subflow_check_req() calls subflow_add_reset_reason() with reasonable arguments on all the error paths?!? Something alike the (completely untested) following Cheers, Paolo --- diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index 6042a47da61b..298c6342a78c 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -150,8 +150,10 @@ static int subflow_check_req(struct request_sock *req, /* no MPTCP if MD5SIG is enabled on this socket or we may run out of * TCP option space. */ - if (rcu_access_pointer(tcp_sk(sk_listener)->md5sig_info)) + if (rcu_access_pointer(tcp_sk(sk_listener)->md5sig_info)) { + subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP); return -EINVAL; + } #endif mptcp_get_options(skb, &mp_opt); @@ -219,6 +221,7 @@ static int subflow_check_req(struct request_sock *req, ntohs(inet_sk((struct sock *)subflow_req->msk)->inet_sport)); if (!mptcp_pm_sport_in_anno_list(subflow_req->msk, sk_listener)) { SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MISMATCHPORTSYNRX); + subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT); return -EPERM; } SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINPORTSYNRX); @@ -227,10 +230,12 @@ static int subflow_check_req(struct request_sock *req, subflow_req_create_thmac(subflow_req); if (unlikely(req->syncookie)) { - if (mptcp_can_accept_new_subflow(subflow_req->msk)) - subflow_init_req_cookie_join_save(subflow_req, skb); - else + if (!mptcp_can_accept_new_subflow(subflow_req->msk)) { + subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT); return -EPERM; + } + + subflow_init_req_cookie_join_save(subflow_req, skb); } pr_debug("token=%u, remote_nonce=%u msk=%p", subflow_req->token,
On Fri, Apr 5, 2024 at 4:16 PM Paolo Abeni <pabeni@redhat.com> wrote: > > On Fri, 2024-04-05 at 10:39 +0800, Jason Xing wrote: > > From: Jason Xing <kernelxing@tencent.com> > > > > The reason codes are handled in two ways nowadays (quoting Mat Martineau): > > 1. Sending in the MPTCP option on RST packets when there is no subflow > > context available (these use subflow_add_reset_reason() and directly call > > a TCP-level send_reset function) > > 2. The "normal" way via subflow->reset_reason. This will propagate to both > > the outgoing reset packet and to a local path manager process via netlink > > in mptcp_event_sub_closed() > > > > RFC 8684 defines the skb reset reason behaviour which is not required > > even though in some places: > > > > A host sends a TCP RST in order to close a subflow or reject > > an attempt to open a subflow (MP_JOIN). In order to let the > > receiving host know why a subflow is being closed or rejected, > > the TCP RST packet MAY include the MP_TCPRST option (Figure 15). > > The host MAY use this information to decide, for example, whether > > it tries to re-establish the subflow immediately, later, or never. > > > > Since the commit dc87efdb1a5cd ("mptcp: add mptcp reset option support") > > introduced this feature about three years ago, we can fully use it. > > There remains some places where we could insert reason into skb as > > we can see in this patch. > > > > Many thanks to Mat for help:) > > > > Signed-off-by: Jason Xing <kernelxing@tencent.com> > > --- > > net/mptcp/subflow.c | 21 ++++++++++++++++++--- > > 1 file changed, 18 insertions(+), 3 deletions(-) > > > > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c > > index 1626dd20c68f..49f746d91884 100644 > > --- a/net/mptcp/subflow.c > > +++ b/net/mptcp/subflow.c > > @@ -301,8 +301,13 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk, > > return dst; > > > > dst_release(dst); > > - if (!req->syncookie) > > + if (!req->syncookie) { > > + struct mptcp_ext *mpext = mptcp_get_ext(skb); > > + > > + if (mpext) > > + subflow_add_reset_reason(skb, mpext->reset_reason); > > uhm? subflow_add_reset_reason() will do: > > mptcp_ext_add(skb)->reset_reason = mpext->reset_reason > > The above looks like a no-op. Ah, my bad. Actually I didn't add the mpext test statement in my original patch. Yes, you're right. The 'if (mpext)' is totally unnecessary. > > Possibly we should instead ensure that subflow_check_req() calls > subflow_add_reset_reason() with reasonable arguments on all the error > paths?!? Absolutely yes, it would be great. The reason I didn't touch them is I'm still studying how to specify the error kind. > > Something alike the (completely untested) following > > Cheers, > > Paolo > --- > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c > index 6042a47da61b..298c6342a78c 100644 > --- a/net/mptcp/subflow.c > +++ b/net/mptcp/subflow.c > @@ -150,8 +150,10 @@ static int subflow_check_req(struct request_sock *req, > /* no MPTCP if MD5SIG is enabled on this socket or we may run out of > * TCP option space. > */ > - if (rcu_access_pointer(tcp_sk(sk_listener)->md5sig_info)) > + if (rcu_access_pointer(tcp_sk(sk_listener)->md5sig_info)) { > + subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP); > return -EINVAL; > + } > #endif > > mptcp_get_options(skb, &mp_opt); > @@ -219,6 +221,7 @@ static int subflow_check_req(struct request_sock *req, > ntohs(inet_sk((struct sock *)subflow_req->msk)->inet_sport)); > if (!mptcp_pm_sport_in_anno_list(subflow_req->msk, sk_listener)) { > SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MISMATCHPORTSYNRX); > + subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT); > return -EPERM; > } > SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINPORTSYNRX); > @@ -227,10 +230,12 @@ static int subflow_check_req(struct request_sock *req, > subflow_req_create_thmac(subflow_req); > > if (unlikely(req->syncookie)) { > - if (mptcp_can_accept_new_subflow(subflow_req->msk)) > - subflow_init_req_cookie_join_save(subflow_req, skb); > - else > + if (!mptcp_can_accept_new_subflow(subflow_req->msk)) { > + subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT); > return -EPERM; > + } > + > + subflow_init_req_cookie_join_save(subflow_req, skb); > } > > pr_debug("token=%u, remote_nonce=%u msk=%p", subflow_req->token, > Great! You complete it! Thanks for your instructions. I'll test it and update it soon. Thanks, Jason
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index 1626dd20c68f..49f746d91884 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -301,8 +301,13 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk, return dst; dst_release(dst); - if (!req->syncookie) + if (!req->syncookie) { + struct mptcp_ext *mpext = mptcp_get_ext(skb); + + if (mpext) + subflow_add_reset_reason(skb, mpext->reset_reason); tcp_request_sock_ops.send_reset(sk, skb); + } return NULL; } @@ -368,8 +373,13 @@ static struct dst_entry *subflow_v6_route_req(const struct sock *sk, return dst; dst_release(dst); - if (!req->syncookie) + if (!req->syncookie) { + struct mptcp_ext *mpext = mptcp_get_ext(skb); + + if (mpext) + subflow_add_reset_reason(skb, mpext->reset_reason); tcp6_request_sock_ops.send_reset(sk, skb); + } return NULL; } #endif @@ -873,13 +883,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); + subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT); goto dispose_child; } SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINPORTACKRX); } - if (!mptcp_finish_join(child)) + if (!mptcp_finish_join(child)) { + struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(child); + + subflow_add_reset_reason(skb, subflow->reset_reason); goto dispose_child; + } SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINACKRX); tcp_rsk(req)->drop_req = true;