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

[Paolo Abeni <pabeni@redhat.com>]

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.

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

Thanks!

Paolo