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

Commit Message

Shmulik Ladkani Aug. 24, 2022, 4:41 a.m. UTC

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 <[e-mail]>
---
v5:
- fix bpf_dynptr_get_data's incorrect usage of bpf_dynptr_kern's size
  spotted by Joanne Koong <[e-mail]>
---
 include/linux/bpf.h  | 1 +
 kernel/bpf/helpers.c | 8 ++++++++
 2 files changed, 9 insertions(+)

Comments

John Fastabend Aug. 25, 2022, 4:14 p.m. UTC | #1

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 <[e-mail]>
> ---
> v5:
> - fix bpf_dynptr_get_data's incorrect usage of bpf_dynptr_kern's size
>   spotted by Joanne Koong <[e-mail]>
> ---
>  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?

Shmulik Ladkani Aug. 30, 2022, 4:57 p.m. UTC | #2

On Thu, 25 Aug 2022 09:14:50 -0700
John Fastabend <[e-mail]> wrote:

> 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?

Sure, I don't mind. 

My rationale extracting 'avail' was due to the fact the combo of 
"if !ptr->data + bpf_dynptr_get_size + bpf_dynptr_get_data" will be
repeated in future locations that need to access the stored data.
Therefore encapsulating this looked beneficial.

BTW, note we'll need to make bpf_dynptr_get_size public (currently it's
static).

Let me know your preference, I'm fine either way.

Patch

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));