Message-ID: <CAL+tcoCrqUbHp1k9giaJL5AmGtsumVpMYDdcaXuNqpyKihLbkg@mail.gmail.com>
Date: Fri, 16 Feb 2024 11:50:45 +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 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 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.

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