Re: [PATCH v3 bpf-next 1/3] bpf: Support setting variable-length tunnel options

[Shmulik Ladkani <shmulik@metanetworks.com>]

> On Tue, 23 Aug 2022 00:59:01 -0700
> John Fastabend <john.fastabend@gmail.com> wrote:
> 
> > > + * long bpf_skb_set_var_tunnel_opt(struct sk_buff *skb, void *opt, u32 size, u32 len)
> > > + *	Description
> > > + *		Set tunnel options metadata for the packet associated to *skb*
> > > + *		to the variable length *len* bytes of option data contained in
> > > + *		the raw buffer *opt* sized *size*.
> > > + *
> > > + *		See also the description of the **bpf_skb_get_tunnel_opt**\ ()
> > > + *		helper for additional information.
> > > + *	Return
> > > + *		0 on success, or a negative error in case of failure.    
> > 
> > This API feels akward to me. Could you collapse this by using a dynamic pointer,
> > recently added? And drop the ptr_to_mem+const_size part at least? That seems
> > redundant with latest kernels.  
> 
> Revisiting this decision.
> 
> After following that path, seems to me that adding the newly proposed
> 'bpf_skb_set_tunnel_opt_dynptr' API creates awkwardness in user's bpf
> program.
> 
> Suppose user needs to hold a map of the options received on incoming
> traffic based on whatever 'bpf_skb_get_tunnel_opt' returns.
> 
> Then, when user needs to apply the options on the return traffic, we
> have the following two alternative APIs:
> 
> 
> option A: bpf_skb_set_var_tunnel_opt
> ------------------------------------
> 
> struct tun_opts {
>     __u8 data[MAX_OPT_SZ];
>     __u32 len;
> };
> BPF_MAP_TYPE_HASH opts_map; // __type(value, tun_opts)
> 
>   ...
> 
>   struct tun_opts *opts;
> 
>   opts = bpf_map_lookup_elem(&opts_map, &the_flow_key);
>   bpf_skb_set_var_tunnel_opt(skb, opts->data, sizeof(opts->data), opts->len);
> 
> 
> option B: bpf_skb_set_tunnel_opt_dynptr
> ---------------------------------------
> 
> struct tun_opts {
>     __u8 data[MAX_OPT_SZ];
> };
> BPF_MAP_TYPE_HASH opts_map;       // __type(value, tun_opts)
> BPF_MAP_TYPE_HASH opts_len_map;   // __type(value, __u32)
> 
>   ... 
> 
>   struct bpf_dynptr dptr;
>   struct tun_opts *opts;
>   __u32 *opts_len;
> 
>   opts = bpf_map_lookup_elem(&opts_map, &the_flow_key);
>   opts_len = bpf_map_lookup_elem(&opts_len_map, &the_flow_key);
> 
>   bpf_dynptr_from_mem(opts, sizeof(*opts), 0, &dptr);  // construct a dynptr from the raw option data
>   bpf_dynptr_trim(&dptr, opts_len);                    // trim it based on stored option len
>   bpf_skb_set_tunnel_opt_dynptr(skb, &dptr);
> 
> 
> IMO, the 2nd user program is less readable:
>  - need to store the received options length in a separate map
>  - 5 bpf function calls instead of 2
> 
> Despite the awkwardness of the 'bpf_skb_set_var_tunnel_opt' API (passing
> both constant size *and* dynamic len), it really creates more simple and
> readable ebpf programs.
> 
> WDYT?

John, Daniel, would appreciate your opinion re the prefered BPF API we
add to set var-length tunnel options. See the difference in bpf user
programs based on the 2 suggestions above.

Thanks,
Shmulik