On 9/1/22 8:10 AM, Shmulik Ladkani wrote:
On Wed, 31 Aug 2022 22:46:15 +0200 Daniel Borkmann <daniel@xxxxxxxxxxxxx> 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?
Yes, the argumentation sounds reasonable to me. I've added this note to the commit message to reflect the design decision, and applied it to bpf-next. Thanks Shmulik!
