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.
@Eric, were you looking to something else and/or more specific?
Thanks!
Paolo
