Re: [PATCH v1 bpf-next 05/11] bpf: tcp: Add SYN Cookie generation SOCK_OPS hook.

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Martin KaFai Lau <martin.lau@linux.dev>
Date: Tue, 17 Oct 2023 17:54:53 -0700
> On 10/13/23 3:04 PM, Kuniyuki Iwashima wrote:
> > This patch adds a new SOCK_OPS hook to generate arbitrary SYN Cookie.
> > 
> > When the kernel sends SYN Cookie to a client, the hook is invoked with
> > bpf_sock_ops.op == BPF_SOCK_OPS_GEN_SYNCOOKIE_CB if the listener has
> > BPF_SOCK_OPS_SYNCOOKIE_CB_FLAG set by bpf_sock_ops_cb_flags_set().
> > 
> > The BPF program can access the following information to encode into
> > ISN:
> > 
> >    bpf_sock_ops.sk      : 4-tuple
> >    bpf_sock_ops.skb     : TCP header
> >    bpf_sock_ops.args[0] : MSS
> > 
> > The program must encode MSS and set it to bpf_sock_ops.replylong[0],
> > which will be looped back to the paired hook added in the following
> > patch.
> > 
> > Note that we do not call tcp_synq_overflow() so that the BPF program
> > can set its own expiration period.
> > 
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > ---
> >   include/uapi/linux/bpf.h       | 18 +++++++++++++++-
> >   net/ipv4/tcp_input.c           | 38 +++++++++++++++++++++++++++++++++-
> >   tools/include/uapi/linux/bpf.h | 18 +++++++++++++++-
> >   3 files changed, 71 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 7ba61b75bc0e..d3cc530613c0 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -6738,8 +6738,17 @@ enum {
> >   	 * options first before the BPF program does.
> >   	 */
> >   	BPF_SOCK_OPS_WRITE_HDR_OPT_CB_FLAG = (1<<6),
> > +	/* Call bpf when the kernel generates SYN Cookie (ISN) for SYN+ACK.
> > +	 *
> > +	 * The bpf prog will be called to encode MSS into SYN Cookie with
> > +	 * sock_ops->op == BPF_SOCK_OPS_GEN_SYNCOOKIE_CB.
> > +	 *
> > +	 * Please refer to the comment in BPF_SOCK_OPS_GEN_SYNCOOKIE_CB for
> > +	 * input and output.
> > +	 */
> > +	BPF_SOCK_OPS_SYNCOOKIE_CB_FLAG = (1<<7),
> >   /* Mask of all currently supported cb flags */
> > -	BPF_SOCK_OPS_ALL_CB_FLAGS       = 0x7F,
> > +	BPF_SOCK_OPS_ALL_CB_FLAGS       = 0xFF,
> >   };
> >   
> >   /* List of known BPF sock_ops operators.
> > @@ -6852,6 +6861,13 @@ enum {
> >   					 * by the kernel or the
> >   					 * earlier bpf-progs.
> >   					 */
> > +	BPF_SOCK_OPS_GEN_SYNCOOKIE_CB,	/* Generate SYN Cookie (ISN of
> > +					 * SYN+ACK).
> > +					 *
> > +					 * args[0]: MSS
> > +					 *
> > +					 * replylong[0]: ISN
> > +					 */
> >   };
> >   
> >   /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index 584825ddd0a0..c86a737e4fe6 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -6966,6 +6966,37 @@ u16 tcp_get_syncookie_mss(struct request_sock_ops *rsk_ops,
> >   }
> >   EXPORT_SYMBOL_GPL(tcp_get_syncookie_mss);
> >   
> > +#if IS_ENABLED(CONFIG_CGROUP_BPF) && IS_ENABLED(CONFIG_SYN_COOKIES)
> > +static int bpf_skops_cookie_init_sequence(struct sock *sk, struct request_sock *req,
> > +					  struct sk_buff *skb, __u32 *isn)
> > +{
> > +	struct bpf_sock_ops_kern sock_ops;
> > +	int ret;
> > +
> > +	memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, temp));
> > +
> > +	sock_ops.op = BPF_SOCK_OPS_GEN_SYNCOOKIE_CB;
> > +	sock_ops.sk = req_to_sk(req);
> > +	sock_ops.args[0] = req->mss;
> > +
> > +	bpf_skops_init_skb(&sock_ops, skb, tcp_hdrlen(skb));
> > +
> > +	ret = BPF_CGROUP_RUN_PROG_SOCK_OPS_SK(&sock_ops, sk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	*isn = sock_ops.replylong[0];
> 
> sock_ops.{replylong,reply} cannot be used. afaik, no existing sockops hook 
> relies on {replylong,reply}. It is a union of args[4]. There could be a few 
> skops bpf in the same cgrp and each of them will be run one after another. (eg. 
> two skops progs want to generate cookie).

Ah, I missed that case.  Looking at bpf_prog_run_array_cg(), multiple
SOCK_OPS prog can be attached and args[] are reused.  Then, we cannot
use replylong[] for interface from bpf prog.


> 
> I don't prefer to extend the uapi 'struct bpf_sock_ops' and then the 
> sock_ops_convert_ctx_access(). Adding member to the kernel 'struct 
> bpf_sock_addr_kern' could still be considered if it is really needed.
> 
> One option is to add kfunc to allow the bpf prog to directly update the value of 
> the kernel obj (e.g. tcp_rsk(req)->snt_isn here).

Yes, we need to set snt_isn, mss, sack_ok etc based on _CB (if we
continue with SOCK_OPS).


> 
> Also, we need to allow a bpf prog to selectively generate custom cookie for one 
> SYN but fall-through to the kernel cookie for another SYN.

Initially I implemented the fallback but the validation hook looked bit
ugly (because of reqsk allocation) and removed the fallback flow.

Also, I thought it can be done with other hooks so that such SYN will be
distributed to another listener.