Date: Thu, 15 Feb 2024 20:11:36 -0800
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <kerneljasonxing@...il.com>
CC: <davem@...emloft.net>, <dsahern@...nel.org>, <edumazet@...gle.com>,
	<kernelxing@...cent.com>, <kuba@...nel.org>, <kuniyu@...zon.com>,
	<netdev@...r.kernel.org>, <pabeni@...hat.com>
Subject: Re: [PATCH net-next v5 03/11] tcp: use drop reasons in cookie check for ipv4

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);
 
---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().


> 
> 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