Re: [PATCH v1 bpf-next 10/11] bpf: tcp: Make WS, SACK, ECN configurable from BPF SYN Cookie.

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Martin KaFai Lau <martin.lau@linux.dev>
Date: Tue, 17 Oct 2023 18:08:34 -0700
> On 10/13/23 3:04 PM, Kuniyuki Iwashima wrote:
> > This patch allows BPF_SOCK_OPS_CHECK_SYNCOOKIE_CB hook to enable WScale,
> > SACK, and ECN by passing corresponding flags to bpf_sock_ops.replylong[1].
> > 
> > The same flags are passed to BPF_SOCK_OPS_GEN_SYNCOOKIE_CB hook as
> > bpf_sock_ops.args[1] so that the BPF prog need not parse the TCP header to
> > check if WScale, SACK, ECN, and TS are available in SYN.
> > 
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > ---
> >   include/uapi/linux/bpf.h       | 18 ++++++++++++++++++
> >   net/ipv4/syncookies.c          | 20 ++++++++++++++++++++
> >   net/ipv4/tcp_input.c           | 11 +++++++++++
> >   tools/include/uapi/linux/bpf.h | 18 ++++++++++++++++++
> >   4 files changed, 67 insertions(+)
> > 
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 24f673d88c0d..cdae4dd5d797 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -6869,6 +6869,7 @@ enum {
> >   					 * option.
> >   					 *
> >   					 * args[0]: MSS
> > +					 * args[1]: BPF_SYNCOOKIE_XXX
> >   					 *
> >   					 * replylong[0]: ISN
> >   					 * replylong[1]: TS
> > @@ -6883,6 +6884,7 @@ enum {
> >   					 * args[1]: TS
> >   					 *
> >   					 * replylong[0]: MSS
> > +					 * replylong[1]: BPF_SYNCOOKIE_XXX
> >   					 */
> >   };
> >   
> > @@ -6970,6 +6972,22 @@ enum {
> >   						 */
> >   };
> >   
> > +/* arg[1] value for BPF_SOCK_OPS_GEN_SYNCOOKIE_CB and
> > + * replylong[1] for BPF_SOCK_OPS_CHECK_SYNCOOKIE_CB.
> > + *
> > + * MSB                                LSB
> > + * | 31 ... | 6  | 5   | 4    | 3 2 1 0 |
> > + * |    ... | TS | ECN | SACK | WScale  |
> > + */
> > +enum {
> > +	/* 0xf is invalid thus means that SYN did not have WScale. */
> > +	BPF_SYNCOOKIE_WSCALE_MASK	= (1 << 4) - 1,
> > +	BPF_SYNCOOKIE_SACK		= (1 << 4),
> > +	BPF_SYNCOOKIE_ECN		= (1 << 5),
> > +	/* Only available for BPF_SOCK_OPS_GEN_SYNCOOKIE_CB to check if SYN has TS */
> > +	BPF_SYNCOOKIE_TS		= (1 << 6),
> > +};
> 
> This details should not be exposed to uapi (more below).
> 
> > +
> >   struct bpf_perf_event_value {
> >   	__u64 counter;
> >   	__u64 enabled;
> > diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> > index ff979cc314da..22353a9af52d 100644
> > --- a/net/ipv4/syncookies.c
> > +++ b/net/ipv4/syncookies.c
> > @@ -286,6 +286,7 @@ int bpf_skops_cookie_check(struct sock *sk, struct request_sock *req, struct sk_
> >   {
> >   	struct bpf_sock_ops_kern sock_ops;
> >   	struct net *net = sock_net(sk);
> > +	u32 options;
> >   
> >   	if (tcp_opt->saw_tstamp) {
> >   		if (!READ_ONCE(net->ipv4.sysctl_tcp_timestamps))
> > @@ -309,6 +310,25 @@ int bpf_skops_cookie_check(struct sock *sk, struct request_sock *req, struct sk_
> >   	if (!sock_ops.replylong[0])
> >   		goto err;
> >   
> > +	options = sock_ops.replylong[1];
> > +
> > +	if ((options & BPF_SYNCOOKIE_WSCALE_MASK) != BPF_SYNCOOKIE_WSCALE_MASK) {
> > +		if (!READ_ONCE(net->ipv4.sysctl_tcp_window_scaling))
> > +			goto err;
> > +
> > +		tcp_opt->wscale_ok = 1;
> > +		tcp_opt->snd_wscale = options & BPF_SYNCOOKIE_WSCALE_MASK;
> > +	}
> > +
> > +	if (options & BPF_SYNCOOKIE_SACK) {
> > +		if (!READ_ONCE(net->ipv4.sysctl_tcp_sack))
> > +			goto err;
> > +
> > +		tcp_opt->sack_ok = 1;
> > +	}
> > +
> > +	inet_rsk(req)->ecn_ok = options & BPF_SYNCOOKIE_ECN;
> > +
> >   	__NET_INC_STATS(sock_net(sk), LINUX_MIB_SYNCOOKIESRECV);
> >   
> >   	return sock_ops.replylong[0];
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index feb44bff29ef..483e2f36afe5 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -6970,14 +6970,25 @@ EXPORT_SYMBOL_GPL(tcp_get_syncookie_mss);
> >   static int bpf_skops_cookie_init_sequence(struct sock *sk, struct request_sock *req,
> >   					  struct sk_buff *skb, __u32 *isn)
> >   {
> > +	struct inet_request_sock *ireq = inet_rsk(req);
> >   	struct bpf_sock_ops_kern sock_ops;
> > +	u32 options;
> >   	int ret;
> >   
> > +	options = ireq->wscale_ok ? ireq->snd_wscale : BPF_SYNCOOKIE_WSCALE_MASK;
> > +	if (ireq->sack_ok)
> > +		options |= BPF_SYNCOOKIE_SACK;
> > +	if (ireq->ecn_ok)
> > +		options |= BPF_SYNCOOKIE_ECN;
> > +	if (ireq->tstamp_ok)
> > +		options |= BPF_SYNCOOKIE_TS;
> 
> No need to set "options" (which becomes args[1]). sock_ops.sk is available to 
> the bpf prog. The bpf prog can directly read it. The recent AF_UNIX bpf support 
> could be a reference on how the bpf_cast_to_kern_ctx() and bpf_rdonly_cast() are 
> used.
> 
> https://lore.kernel.org/bpf/20231011185113.140426-10-daan.j.demeyer@gmail.com/

I just tried bpf_cast_to_kern_ctx() and bpf_rdonly_cast() and found
it's quite useful, thanks!

If we want to set {sack_ok,ecn_ok,snd_wscale} in one shot, it would
be good to expose such flags and a helper.