Re: [PATCH bpf-next 04/10] bpf: tcp: Allow bpf prog to write and parse BPF TCP header option

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Martin KaFai Lau <kafai@fb.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Eric Dumazet <edumazet@google.com>,
	kernel-team@fb.com, Lawrence Brakmo <brakmo@fb.com>,
	Neal Cardwell <ncardwell@google.com>,
	netdev@vger.kernel.org, Yuchung Cheng <ycheng@google.com>
Subject: Re: [PATCH bpf-next 04/10] bpf: tcp: Allow bpf prog to write and parse BPF TCP header option
Date: Sun, 28 Jun 2020 11:24:27 -0700	[thread overview]
Message-ID: <20200628182427.qt7vpjohwkxvixfi@ast-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <20200626175526.1461133-1-kafai@fb.com>

On Fri, Jun 26, 2020 at 10:55:26AM -0700, Martin KaFai Lau wrote:
> 
> Parsing BPF Header Option
> ─────────────────────────
> 
> As mentioned earlier, the received SYN/SYNACK/ACK during the 3WHS
> will be available to some specific CB (e.g. the *_ESTABLISHED_CB)
> 
> For established connection, if the kernel finds a bpf header
> option (i.e. option with kind:254 and magic:0xeB9F) and the
> the "PARSE_HDR_OPT_CB_FLAG" flag is set,  the
> bpf prog will be called in the "BPF_SOCK_OPS_PARSE_HDR_OPT_CB" op.
> The received skb will be available through sock_ops->skb_data
> and the bpf header option offset will also be specified
> in sock_ops->skb_bpf_hdr_opt_off.

TCP noob question:
- can tcp header have two or more options with the same kind and magic?
I scanned draft-ietf-tcpm-experimental-options-00.txt and it seems
it's not prohibiting collisions.
So should be ok?
Why I'm asking... I think existing bpf_sock_ops style of running
multiple bpf progs is gonna be awkward to use.
Picking the max of bpf_reserve_hdr_opt() from many calls and let
parent bpf progs override children written headers feels a bit hackish.
I feel the users will thank us if we design the progs to be more
isolated and independent somehow.
I was thinking may be each bpf prog will bpf_reserve_hdr_opt()
and bpf_store_hdr_opt() into their own option?
Then during option writing side the tcp header will have two or more
options with the same kind and magic.
Obviously it creates a headache during parsing. Which bpf prog
should be called for each option?

I suspect tcp draft actually prefers all options to have unique kind+magic.
Can we add an attribute to prog load time that will request particular magic ?
Then only that _one_ program will be called for the given kind+magic.
We can still have multiple progs attached to a cgroup (likely root cgroup)
and different progs will take care of parsing and writing their own option.
cgroup attaching side can make sure that multi progs have different magics.

Saving multiple bpf_hdr_opt_off in patch 2 for different magics becomes
problematic. So clearly the implementation complexity shots through the roof
with above proposal, but it feels more flexible and more user friendly?
Not a strong opinion. The feature is great as-is.