Re: [PATCH v1 bpf-next 00/11] bpf: tcp: Add SYN Cookie generation/validation SOCK_OPS hooks.

From: Kuniyuki Iwashima <kuniyu@amazon.com>
To: <martin.lau@linux.dev>
Cc: <andrii@kernel.org>, <ast@kernel.org>, <bpf@vger.kernel.org>,
	<daniel@iogearbox.net>, <davem@davemloft.net>,
	<dsahern@kernel.org>, <edumazet@google.com>, <haoluo@google.com>,
	<john.fastabend@gmail.com>, <jolsa@kernel.org>,
	<kpsingh@kernel.org>, <kuba@kernel.org>, <kuni1840@gmail.com>,
	<kuniyu@amazon.com>, <mykolal@fb.com>, <netdev@vger.kernel.org>,
	<pabeni@redhat.com>, <sdf@google.com>, <song@kernel.org>,
	<yonghong.song@linux.dev>
Subject: Re: [PATCH v1 bpf-next 00/11] bpf: tcp: Add SYN Cookie generation/validation SOCK_OPS hooks.
Date: Tue, 17 Oct 2023 09:48:07 -0700	[thread overview]
Message-ID: <20231017164807.19824-1-kuniyu@amazon.com> (raw)
In-Reply-To: <9666242b-d899-c428-55bd-14f724cc4ffd@linux.dev>

From: Martin KaFai Lau <martin.lau@linux.dev>
Date: Mon, 16 Oct 2023 22:53:15 -0700
> On 10/13/23 3:04 PM, Kuniyuki Iwashima wrote:
> > Under SYN Flood, the TCP stack generates SYN Cookie to remain stateless
> > After 3WHS, the proxy restores SYN and forwards it and ACK to the backend
> > server.  Our kernel module works at Netfilter input/output hooks and first
> > feeds SYN to the TCP stack to initiate 3WHS.  When the module is triggered
> > for SYN+ACK, it looks up the corresponding request socket and overwrites
> > tcp_rsk(req)->snt_isn with the proxy's cookie.  Then, the module can
> > complete 3WHS with the original ACK as is.
> 
> Does the current kernel module also use the timestamp bits differently? 
> (something like patch 8 and patch 10 trying to do)

Our SYN Proxy uses TS as is.  The proxy nodes generate a random number
if TS is in SYN.

But I thought someone would suggest making TS available so that we can
mock the default behaviour at least, and it would be more acceptable.

The selftest uses TS just to strengthen security by validating 32-bits
hash.  Dropping a part of hash makes collision easier to happen, but
24-bits were sufficient for us to reduce SYN flood to the managable
level at the backend.

> 
> > 
> > This way, our SYN Proxy does not manage the ISN mappings and can stay
> > stateless.  It's working very well for high-bandwidth services like
> > multiple Tbps, but we are looking for a way to drop the dirty hack and
> > further optimise the sequences.
> > 
> > If we could validate an arbitrary SYN Cookie on the backend server with
> > BPF, the proxy would need not restore SYN nor pass it.  After validating
> > ACK, the proxy node just needs to forward it, and then the server can do
> > the lightweight validation (e.g. check if ACK came from proxy nodes, etc)
> > and create a connection from the ACK.
> > 
> > This series adds two SOCK_OPS hooks to generate and validate arbitrary
> > SYN Cookie.  Each hook is invoked if BPF_SOCK_OPS_SYNCOOKIE_CB_FLAG is
> > set to the listening socket in advance by bpf_sock_ops_cb_flags_set().
> > 
> > The user interface looks like this:
> > 
> >    BPF_SOCK_OPS_GEN_SYNCOOKIE_CB
> > 
> >      input
> >      |- bpf_sock_ops.sk           : 4-tuple
> >      |- bpf_sock_ops.skb          : TCP header
> >      |- bpf_sock_ops.args[0]      : MSS
> >      `- bpf_sock_ops.args[1]      : BPF_SYNCOOKIE_XXX flags
> > 
> >      output
> >      |- bpf_sock_ops.replylong[0] : ISN (SYN Cookie) ------.
> >      `- bpf_sock_ops.replylong[1] : TS value -----------.  |
> >                                                         |  |
> >    BPF_SOCK_OPS_CHECK_SYNCOOKIE_CB                      |  |
> >                                                         |  |
> >      input                                              |  |
> >      |- bpf_sock_ops.sk           : 4-tuple             |  |
> >      |- bpf_sock_ops.skb          : TCP header          |  |
> >      |- bpf_sock_ops.args[0]      : ISN (SYN Cookie) <-----'
> >      `- bpf_sock_ops.args[1]      : TS value <----------'
> > 
> >      output
> >      |- bpf_sock_ops.replylong[0] : MSS
> >      `- bpf_sock_ops.replylong[1] : BPF_SYNCOOKIE_XXX flags
> > 
> > To establish a connection from SYN Cookie, BPF_SOCK_OPS_CHECK_SYNCOOKIE_CB
> > hook must set a valid MSS to bpf_sock_ops.replylong[0], meaning that
> > BPF_SOCK_OPS_GEN_SYNCOOKIE_CB hook must encode MSS to ISN or TS val to be
> > restored in the validation hook.
> > 
> > If WScale, SACK, and ECN are detected to be available in SYN packet, the
> > corresponding flags are passed to args[0] of BPF_SOCK_OPS_GEN_SYNCOOKIE_CB
> > so that bpf prog need not parse the TCP header.  The same flags can be set
> > to replylong[0] of BPF_SOCK_OPS_CHECK_SYNCOOKIE_CB to enable each feature
> > on the connection.
> > 
> > For details, please see each patch.  Here's an overview:
> > 
> >    patch 1 - 4 : Misc cleanup
> >    patch 5, 6  : Add SOCK_OPS hook (only ISN is available here)
> >    patch 7, 8  : Make TS val available as the second cookie storage
> >    patch 9, 10 : Make WScale, SACK, and ECN configurable from ACK
> >    patch 11    : selftest, need some help from BPF experts...
> 
> I cannot reprod the issue. Commented in patch 11.
> 
> I only scanned through the high level of the patchset. will take a closer look. 
> Thanks.

I'll wait your review before posting v2.
Thank you!