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?

Best
Shmulik