On Wed, Aug 31, 2022 at 1:44 AM Shmulik Ladkani
<shmulik@xxxxxxxxxxxxxxxx> wrote:
>
> On Tue, 23 Aug 2022 00:59:01 -0700
> John Fastabend <john.fastabend@xxxxxxxxx> 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
I don't think you need a separate map to store the opts length. I
think you can store
struct tun_opts {
__u8 data[MAX_OPT_SZ];
__u32 len;
};
in the map, and then do something like:
struct bpf_dynptr ptr;
struct tun_opts *opts;
opts = bpf_map_lookup_elem(&opts_map, &the_flow_key);
bpf_skb_set_tunnel_opt_dynptr(skb, &ptr);
where inside the bpf_skb_set_tunnel_opt_dynptr() helper, you can parse
the dynptr data back to a "struct tun_opts" (after doing requisite
checks, eg making sure dynptr size matches sizeof(struct tun_opts))
and then access tun_opts->data and tun_opts->len accordingly.
What are your thoughts?
>
> 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
>
