netdev - Re: [PATCH net-next 2/2] mptcp: add reset reason options in some places

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAL+tcoCSf=Gd1c_LiN1Bk5xKnJ92_NHyWRDTLEW80LzpSr7okg@mail.gmail.com>
Date: Fri, 5 Apr 2024 17:09:01 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	matttbe@...nel.org, martineau@...nel.org, geliang@...nel.org, 
	mptcp@...ts.linux.dev, netdev@...r.kernel.org, 
	Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next 2/2] mptcp: add reset reason options in some places

On Fri, Apr 5, 2024 at 4:16 PM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On Fri, 2024-04-05 at 10:39 +0800, Jason Xing wrote:
> > From: Jason Xing <kernelxing@...cent.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@...cent.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