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

[John Fastabend <john.fastabend@gmail.com>]

Shmulik Ladkani wrote:
> Existing 'bpf_skb_set_tunnel_opt' allows setting tunnel options given
> an option buffer (ARG_PTR_TO_MEM|MEM_RDONLY) and the compile-time
> fixed buffer size (ARG_CONST_SIZE).
> 
> However, in certain cases we wish to set tunnel options of dynamic
> length.
> 
> For example, we have an ebpf program that gets geneve options on
> incoming packets, stores them into a map (using a key representing
> the incoming flow), and later needs to assign *same* options to
> reply packets (belonging to same flow).
> 
> This is currently imposssibly without knowing sender's exact geneve
> options length, which unfortunately is dymamic.
> 
> Introduce 'skb_set_var_tunnel_opt'. This is a variant of
> 'bpf_skb_set_tunnel_opt' which gets an *additional* parameter 'len',
> which is the byte length from 'opt' buffer to copy into ip_tunnnel_info.
> 
> The 'size' parameter is kept ARG_CONST_SIZE. This way, verifier can still
> safe-guard buffer access. 'len' must never exceed 'size', o/w EINVAL is
> returned.
> 
> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
> ---
> v3: Avoid 'inline' for the __bpf_skb_set_tunopt helper function
> ---
>  include/uapi/linux/bpf.h       | 12 ++++++++++++
>  net/core/filter.c              | 34 +++++++++++++++++++++++++++++++---
>  tools/include/uapi/linux/bpf.h | 12 ++++++++++++
>  3 files changed, 55 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 934a2a8beb87..1b965dfd0c80 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -5355,6 +5355,17 @@ union bpf_attr {
>   *	Return
>   *		Current *ktime*.
>   *
> + * 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.

And then is there a case where size != len? Probably I guess? Anyways having
a signature like tunnel_otpion(skb, opt, len) looks a lot like memcpy to me
and feels familiar.

[...]

>  
> +static const struct bpf_func_proto bpf_skb_set_var_tunnel_opt_proto = {
> +	.func		= bpf_skb_set_var_tunnel_opt,
> +	.gpl_only	= false,
> +	.ret_type	= RET_INTEGER,
> +	.arg1_type	= ARG_PTR_TO_CTX,
> +	.arg2_type	= ARG_PTR_TO_MEM | MEM_RDONLY,
> +	.arg3_type	= ARG_CONST_SIZE,
> +	.arg4_type	= ARG_ANYTHING,
> +};
> +