Date: Fri, 16 Feb 2024 12:41:33 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: davem@...emloft.net, dsahern@...nel.org, edumazet@...gle.com, 
	kernelxing@...cent.com, kuba@...nel.org, netdev@...r.kernel.org, 
	pabeni@...hat.com
Subject: Re: [PATCH net-next v5 03/11] tcp: use drop reasons in cookie check
 for ipv4

On Fri, Feb 16, 2024 at 12:11 PM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> From: Jason Xing <kerneljasonxing@...il.com>
> Date: Fri, 16 Feb 2024 11:50:45 +0800
> > On Fri, Feb 16, 2024 at 11:03 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
> > >
> > > From: Jason Xing <kerneljasonxing@...il.com>
> > > Date: Fri, 16 Feb 2024 09:28:26 +0800
> > > > On Fri, Feb 16, 2024 at 5:09 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
> > > > >
> > > > > From: Jason Xing <kerneljasonxing@...il.com>
> > > > > Date: Thu, 15 Feb 2024 09:20:19 +0800
> > > > > > From: Jason Xing <kernelxing@...cent.com>
> > > > > >
> > > > > > Now it's time to use the prepared definitions to refine this part.
> > > > > > Four reasons used might enough for now, I think.
> > > > > >
> > > > > > Signed-off-by: Jason Xing <kernelxing@...cent.com>
> > > > > > --
> > > > > > v5:
> > > > > > Link: https://lore.kernel.org/netdev/CANn89i+iELpsoea6+C-08m6+=JkneEEM=nAj-28eNtcOCkwQjw@mail.gmail.com/
> > > > > > Link: https://lore.kernel.org/netdev/632c6fd4-e060-4b8e-a80e-5d545a6c6b6c@kernel.org/
> > > > > > 1. Use SKB_DROP_REASON_IP_OUTNOROUTES instead of introducing a new one (Eric, David)
> > > > > > 2. Reuse SKB_DROP_REASON_NOMEM to handle failure of request socket allocation (Eric)
> > > > > > 3. Reuse NO_SOCKET instead of introducing COOKIE_NOCHILD
> > > > > > ---
> > > > > >  net/ipv4/syncookies.c | 18 +++++++++++++-----
> > > > > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> > > > > > index 38f331da6677..aeb61c880fbd 100644
> > > > > > --- a/net/ipv4/syncookies.c
> > > > > > +++ b/net/ipv4/syncookies.c
> > > > > > @@ -421,8 +421,10 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
> > > > > >               if (IS_ERR(req))
> > > > > >                       goto out;
> > >
> > > I noticed in this case (ret == sk) we can set drop reason in
> > > tcp_v4_do_rcv() as INVALID_COOKIE or something else.
> >
> > If cookie_v4_check() returns the sk which is the same as the first
> > parameter of its caller (tcp_v4_do_rcv()), then we cannot directly
> > drop it
>
> No, I meant we can just set drop reason, not calling kfree_skb_reason()
> just after cookie_v4_check().
>
> Then, in tcp_v4_do_rcv(), the default reason is NOT_SPECIFIED, but
> INVALID_COOKIE only when cookie_v4_check() returns the listener.
>
> ---8<---
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index 0c50c5a32b84..05cd697a7c07 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1923,6 +1923,8 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
>                         }
>                         return 0;
>                 }
> +
> +               reason = SKB_DROP_REASON_INVALID_COOKIE;
>         } else
>                 sock_rps_save_rxhash(sk, skb);

After this, it will go into 'reason = tcp_rcv_state_process()' which
will replace INVALID_COOKIE reason if the kernel is shipped with this
series.

>
> ---8<---
>
>
> > because it is against old behaviours and causes errors. It
> > should go into tcp_rcv_state_process() later. The similar mistake I
> > made was reported by Paolo in the patch [0/11] (see link[1] as below).
> >
> > link[1]: https://lore.kernel.org/netdev/c987d2c79e4a4655166eb8eafef473384edb37fb.camel@redhat.com/
> >
> > >
> > >
> > > > > >       }
> > > > > > -     if (!req)
> > > > > > +     if (!req) {
> > > > > > +             SKB_DR_SET(reason, NOMEM);
> > > > >
> > > > > NOMEM is not appropriate when mptcp_subflow_init_cookie_req() fails.
> > > >
> > > > Thanks for your careful check. It's true. I didn't check the MPTCP
> > > > path about how to handle it.
> > > >
> > > > It also means that what I did to the cookie_v6_check() is also wrong.
> > >
> > > Yes, same for the v6 change.
> > >
> > >
> > > >
> > > > [...]
> > > > > >       /* Try to redo what tcp_v4_send_synack did. */
> > > > > >       req->rsk_window_clamp = tp->window_clamp ? :dst_metric(&rt->dst, RTAX_WINDOW);
> > > > > > @@ -476,10 +482,12 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
> > > > > >       /* ip_queue_xmit() depends on our flow being setup
> > > > > >        * Normal sockets get it right from inet_csk_route_child_sock()
> > > > > >        */
> > > > > > -     if (ret)
> > > > > > +     if (ret) {
> > > > > >               inet_sk(ret)->cork.fl.u.ip4 = fl4;
> > > > > > -     else
> > > > > > +     } else {
> > > > > > +             SKB_DR_SET(reason, NO_SOCKET);
> > > > >
> > > > > This also seems wrong to me.
> > > > >
> > > > > e.g. syn_recv_sock() could fail with sk_acceptq_is_full(sk),
> > > > > then the listener is actually found.
> > > >
> > > > Initially I thought using a not-that-clear name could be helpfull,
> > > > though. NO_SOCKET here means no child socket can be used if I add a
> > > > new description to SKB_DROP_REASON_NO_SOCKET.
> > >
> > > Currently, NO_SOCKET is used only when sk lookup fails.  Mixing
> > > different reasons sounds like pushing it back to NOT_SPECIFIED.
> > > We could distinguish them by the caller IP though.
> >
> > It makes some sense, but I still think NO_SOCKET is just a mixture of
> > three kinds of cases (no sk during lookup process, no child sk, no
> > reqsk).
> > Let me think about it.
> >
> > >
> > >
> > > >
> > > > If the idea is proper, how about using NO_SOCKET for the first point
> > > > you said to explain that there is no request socket that can be used?
> > > >
> > > > If not, for both of the points you mentioned, it seems I have to add
> > > > back those two new reasons (perhaps with a better name updated)?
> > > > 1. Using SKB_DROP_REASON_REQSK_ALLOC for the first point (request
> > > > socket allocation in cookie_v4/6_check())
> > > > 2. Using SKB_DROP_REASON_GET_SOCK for the second point (child socket
> > > > fetching in cookie_v4/6_check())
> > > >
> > > > Now I'm struggling with the name and whether I should introduce some
> > > > new reasons like what I did in the old version of the series :S
> > >
> > > Why naming is hard would be because there are multiple reasons of
> > > failure.  One way to be more specific is moving kfree_skb_reason()
> > > into callee as you did in patch 2.
> > >
> > >
> > > > If someone comes up with a good name or a good way to explain them,
> > > > please tell me, thanks!
> > >
> > > For 1. no idea :p
> > >
> > > For 2. Maybe VALID_COOKIE ?  we drop the valid cookie in the same
> > > function, but due to LSM or L3 layer, so the reason could be said
> > > as L4-specific ?
> >
> > Thanks for your idea :)
> >
> > For 2, if we're on the same page and talk about how to handle
> > tcp_get_cookie_sock(), the name is not that appropriate because as you
> > said in your previous email it could fail due to full of accept queue
> > instead of cookie problem.
>
> That's why I wrote _VALID_ COOKIE.  Here we know the cookie was valid
> but somehow 3WHS failed.  If we want to be more specific, what is not
> appropriate would be the place where we set the reason or call kfree_skb().

Ah, I see. It does make sense if we use __VALID__. Let us hear more
voices, then I can accurately update the changes in the next version
:)

Really hope it can be done soon. It almost killed me :(

Thanks,
Jason

>
>
> >
> > If we're talking about cookie_tcp_check(), the name is also not that
> > good because the drop reason could be no memory which is unrelated to
> > cookie, right?
> >
> > COOKIE, it seems, cannot be the keyword/generic reason to conclude all
> > the reasons for either of them...
> >
> > Thanks,
> > Jason
> >
> > >
> > >
> > > >
> > > > also cc Eric, David
> > > >
> > > > Thanks,
> > > > Jason
> > > >

Powered by blists - more mailing lists