From: Paolo Abeni <pabeni@xxxxxxxxxx>
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@xxxxxxxxxx>
> > > Date: Thu, 14 Dec 2023 17:31:15 +0100
> > > > On Thu, Dec 14, 2023 at 4:56 PM Kuniyuki Iwashima <kuniyu@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > > ---
> > > > > 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@xxxxxxxxxx/
> >
> > 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?
