RE: [PATCH v5 bpf-next 1/4] bpf: Add 'bpf_dynptr_get_data' helper

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

Shmulik Ladkani wrote:
> The task of calculating bpf_dynptr_kern's available size, and the
> current (offset) data pointer is common for BPF functions working with
> ARG_PTR_TO_DYNPTR parameters.
> 
> Introduce 'bpf_dynptr_get_data' which returns the current data
> (with properer offset), and the number of usable bytes it has.
> 
> This will void callers from directly calculating bpf_dynptr_kern's
> data, offset and size.
> 
> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
> ---
> v5:
> - fix bpf_dynptr_get_data's incorrect usage of bpf_dynptr_kern's size
>   spotted by Joanne Koong <joannelkoong@gmail.com>
> ---
>  include/linux/bpf.h  | 1 +
>  kernel/bpf/helpers.c | 8 ++++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 99fc7a64564f..d84d37bda87f 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2577,6 +2577,7 @@ void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data,
>  		     enum bpf_dynptr_type type, u32 offset, u32 size);
>  void bpf_dynptr_set_null(struct bpf_dynptr_kern *ptr);
>  int bpf_dynptr_check_size(u32 size);
> +void *bpf_dynptr_get_data(struct bpf_dynptr_kern *ptr, u32 *avail_bytes);
>  
>  #ifdef CONFIG_BPF_LSM
>  void bpf_cgroup_atype_get(u32 attach_btf_id, int cgroup_atype);
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index fc08035f14ed..96ff93941cae 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1427,6 +1427,14 @@ void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data,
>  	bpf_dynptr_set_type(ptr, type);
>  }
>  
> +void *bpf_dynptr_get_data(struct bpf_dynptr_kern *ptr, u32 *avail_bytes)
> +{
> +	if (!ptr->data)
> +		return NULL;
> +	*avail_bytes = bpf_dynptr_get_size(ptr);
> +	return ptr->data + ptr->offset;
> +}
> +
>  void bpf_dynptr_set_null(struct bpf_dynptr_kern *ptr)
>  {
>  	memset(ptr, 0, sizeof(*ptr));
> -- 
> 2.37.2
> 

As a bit of an addmitedly nitpick I just wonder if having the avail_bytes
passed through like this is much use anymore? For example,

+BPF_CALL_3(bpf_skb_set_tunnel_opt_dynptr, struct sk_buff *, skb,
+	   struct bpf_dynptr_kern *, ptr, u32, len)
+{
+	const u8 *from;
+	u32 avail;
+
-       if (!ptr->data)
-		return -EFAULT;
-       avail = bpf_dynptr_get_size(ptr)
+	from = bpf_dynptr_get_data(ptr, &avail);
+	if (unlikely(len > avail))
+		return -EINVAL;
+	return __bpf_skb_set_tunopt(skb, from, len);
+}
+

seems just about as compact to me and then drop the null check from the
helper so we have a bpf_dynptr_get_data(*ptr) that just does the
data+offset arithmatic. Then it could also be used in a few other
spots where that calculation seems common.

I find it easier to read at least and think the helper would get
more use, also land it in one of the .h files. And avoids bouncing
avail around.

Bit of a gripe but what do you think?