Re: [PATCH v6 bpf-next 3/6] bpf: tcp: Handle BPF SYN Cookie in skb_steal_sock().

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Paolo Abeni <pabeni@redhat.com>
Date: Wed, 20 Dec 2023 11:22:45 +0100
> On Tue, 2023-12-19 at 08:45 -0800, Mat Martineau wrote:
> > On Fri, 15 Dec 2023, Kuniyuki Iwashima wrote:
> > 
> > > From: Eric Dumazet <edumazet@google.com>
> > > Date: Thu, 14 Dec 2023 17:31:15 +0100
> > > > On Thu, Dec 14, 2023 at 4:56 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> > > > > 
> > > > > We will support arbitrary SYN Cookie with BPF.
> > > > > 
> > > > > If BPF prog validates ACK and kfunc allocates a reqsk, it will
> > > > > be carried to TCP stack as skb->sk with req->syncookie 1.  Also,
> > > > > the reqsk has its listener as req->rsk_listener with no refcnt
> > > > > taken.
> > > > > 
> > > > > When the TCP stack looks up a socket from the skb, we steal
> > > > > inet_reqsk(skb->sk)->rsk_listener in skb_steal_sock() so that
> > > > > the skb will be processed in cookie_v[46]_check() with the
> > > > > listener.
> > > > > 
> > > > > Note that we do not clear skb->sk and skb->destructor so that we
> > > > > can carry the reqsk to cookie_v[46]_check().
> > > > > 
> > > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > > > > ---
> > > > >  include/net/request_sock.h | 15 +++++++++++++--
> > > > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/include/net/request_sock.h b/include/net/request_sock.h
> > > > > index 26c630c40abb..8839133d6f6b 100644
> > > > > --- a/include/net/request_sock.h
> > > > > +++ b/include/net/request_sock.h
> > > > > @@ -101,10 +101,21 @@ static inline struct sock *skb_steal_sock(struct sk_buff *skb,
> > > > >         }
> > > > > 
> > > > >         *prefetched = skb_sk_is_prefetched(skb);
> > > > > -       if (*prefetched)
> > > > > +       if (*prefetched) {
> > > > > +#if IS_ENABLED(CONFIG_SYN_COOKIES)
> > > > > +               if (sk->sk_state == TCP_NEW_SYN_RECV && inet_reqsk(sk)->syncookie) {
> > > > > +                       struct request_sock *req = inet_reqsk(sk);
> > > > > +
> > > > > +                       *refcounted = false;
> > > > > +                       sk = req->rsk_listener;
> > > > > +                       req->rsk_listener = NULL;
> > > > 
> > > > I am not sure about interactions with MPTCP.
> > > > 
> > > > I would be nice to have their feedback.
> > > 
> > > Matthieu, Mat, Paolo, could you double check if the change
> > > above is sane ?
> > > https://lore.kernel.org/bpf/20231214155424.67136-4-kuniyu@amazon.com/
> > 
> > Hi Kuniyuki -
> > 
> > Yes, we will take a look. Haven't had time to look in detail yet but I 
> > wanted to let you know we saw your message and will follow up.
> 
> I'm sorry for the late reply.
> 
> AFAICS, from mptcp perspective, the main differences from built-in
> cookie validation are:
> 
> - cookie allocation via mptcp_subflow_reqsk_alloc() and cookie
> 'finalization' via cookie_tcp_reqsk_init() /
> mptcp_subflow_init_cookie_req(req, sk, skb) could refer 2 different
> listeners - within the same REUSEPORT group.
> 
> - incoming pure syn packets will not land into the TCP stack, so
> af_ops->route_req will not happen.
> 
> I think both the above are problematic form mptcp. 
> 
> Potentially we can have both mptcp-enabled and plain tcp socket with
> the same reuseport group. 
> 
> Currently the mptcp code assumes the listener is mptcp
> cookie_tcp_reqsk_init(), the req is mptcp, too. I think we could fix
> this at the mptcp level, but no patch ready at the moment.
> 
> Even the missing call to route_req() is problematic, as we use that to
> fetch required information from the initial syn for MP_JOIN subflows -
> yep, unfortunately mptcp needs to track of some state across MPJ syn
> and MPJ 3rd ack reception.
> 
> Fixing this last item looks more difficult. I think it would be safer
> and simpler to avoid mptcp support for generic syncookie and ev enable
> it later - after we address things on the mptcp side.

Thanks for checking, Paolo!

Ok, I will change kfunc in the next version so that it will return
-EINVAL for mptcp listener, and later the issues above are sorted out,
we can add mptcp support back.


> 
> @Eric, were you looking to something else and/or more specific?