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

[Shmulik Ladkani <shmulik@xxxxxxxxxxxxxxxx>]

On Wed, 31 Aug 2022 12:07:28 -0700
Joanne Koong <joannelkoong@xxxxxxxxx> wrote:

> > 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  
> 
> I don't think you need a separate map to store the opts length.

You are correct, I was under the impression that 'bpf_dynptr_from_mem'
will *only* accept the exact pointer to the map element, due to the
verifier checking this:

	case BPF_FUNC_dynptr_from_mem:
		if (regs[BPF_REG_1].type != PTR_TO_MAP_VALUE) {
			verbose(env, "Unsupported reg type %s for bpf_dynptr_from_mem data\n",

However I found out this is also allowed by the verifier:

struct tun_opts {
    __u32 len;
    __u8 data[MAX_OPT_SZ];
};

  opts = bpf_map_lookup_elem(&opts_map, &the_flow_key);
  bpf_dynptr_from_mem(opts->data, sizeof(opts->data), 0, &dptr);
  bpf_dynptr_trim(&dptr, opts->len);
  bpf_skb_set_tunnel_opt_dynptr(skb, &dptr);

Nevertheless, IMO it doesn't change the argument about the better
readability and simplicity of the "bpf_skb_set_var_tunnel_opt" approach:

  opts = bpf_map_lookup_elem(&opts_map, &the_flow_key);
  bpf_skb_set_var_tunnel_opt(skb, opts->data, sizeof(opts->data), opts->len);

which is more straight forward.
                       
It even allows the bpf program to set tunnel options *without* using a
map element at all, e.g. this usecase:

  __u8 data[MAX_OPT_SZ];

  len = bpf_skb_get_tunnel_opt(skb, data, sizeof(data));
  ...
  // fiddle with the skb, the tunnel key, the tunnel remote, or whatever.
  // then send again on a collect-md interface, setting same tunnel
  // opts as seen.
  ...
  bpf_skb_set_var_tunnel_opt(skb, data, sizeof(data), len);
  bpf_redirect()

Best,
Shmulik