Re: [PATCH bpf-next 1/2] bpf: Support getting tunnel flags

[Shmulik Ladkani <shmulik@metanetworks.com>]

On Wed, 31 Aug 2022 22:46:15 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:

> The bpf_skb_set_tunnel_key() helper has a number of flags we pass in, e.g.
> BPF_F_ZERO_CSUM_TX, BPF_F_DONT_FRAGMENT, BPF_F_SEQ_NUMBER, and then based on
> those flags we set:
> 
>    [...]
>    info->key.tun_flags = TUNNEL_KEY | TUNNEL_CSUM | TUNNEL_NOCACHE;
>    if (flags & BPF_F_DONT_FRAGMENT)
>            info->key.tun_flags |= TUNNEL_DONT_FRAGMENT;
>    if (flags & BPF_F_ZERO_CSUM_TX)
>            info->key.tun_flags &= ~TUNNEL_CSUM;
>    if (flags & BPF_F_SEQ_NUMBER)
>            info->key.tun_flags |= TUNNEL_SEQ;
>    [...]
> 
> Should we similarly only expose those which are interesting/relevant to BPF
> program authors as a __u16 tunnel_flags and not the whole set? Which ones
> do you have a need for? TUNNEL_SEQ, TUNNEL_CSUM, TUNNEL_KEY, and then the
> TUNNEL_OPTIONS_PRESENT?

Indeed, I noticed this and considered various approaches:

1. Convert the "interesting" internal TUNNEL_xxx flags back to BPF_F_yyy
and place into the new 'tunnel_flags' field.
This has 2 drawbacks:
 - The BPF_F_yyy flags are from *set_tunnel_key* enumeration space,
   e.g. BPF_F_ZERO_CSUM_TX.
   I find it awkward that it is "returned" into tunnel_flags from a
   *get_tunnel_key* call.
 - Not all "interesting" TUNNEL_xxx flags can be mapped to existing
   BPF_F_yyy flags, and it doesn't make sense to create new BPF_F_yyy
   flags just for purposes of the returned tunnel_flags.

2. Place key.tun_flags into 'tunnel_flags' but mask them, keeping only
   "interesting" flags.
   That's ok, but the drawback is that what's "intersting" for my
   usecase might be limiting for other usecases.

Therefore I decided to expose what's in key.tun_flags *as is*, which seems
most flexible. The bpf user can just choose to ingore bits he's not
interested in. The TUNNEL_xxx are uapi, so no harm exposing them back in
the get_tunnel_key call.

WDYT?

Best,
Shmulik