[PATCH v1 bpf-next 0/5] Dynptr convenience helpers

[PATCH v1 bpf-next 0/5] Dynptr convenience helpers

[Joanne Koong]

This patchset is the 3rd in the dynptr series. The 1st (dynptr
fundamentals) can be found here [0] and the second (skb + xdp dynptrs)
can be found here [1].

This patchset adds the following convenience helpers for interacting
with dynptrs:

int bpf_dynptr_trim(struct bpf_dynptr *ptr, __u32 len) __ksym;
int bpf_dynptr_advance(struct bpf_dynptr *ptr, __u32 len) __ksym;
int bpf_dynptr_is_null(const struct bpf_dynptr *ptr) __ksym;
int bpf_dynptr_is_rdonly(const struct bpf_dynptr *ptr) __ksym;
__u32 bpf_dynptr_get_size(const struct bpf_dynptr *ptr) __ksym;
__u32 bpf_dynptr_get_offset(const struct bpf_dynptr *ptr) __ksym;
int bpf_dynptr_clone(const struct bpf_dynptr *ptr, struct bpf_dynptr *clone__init) __ksym;

[0] https://lore.kernel.org/bpf/20220523210712.3641569-1-joannelkoong@gmail.com/
[1] https://lore.kernel.org/bpf/20230301154953.641654-1-joannelkoong@gmail.com/

Joanne Koong (5):
  bpf: Add bpf_dynptr_trim and bpf_dynptr_advance
  bpf: Add bpf_dynptr_is_null and bpf_dynptr_is_rdonly
  bpf: Add bpf_dynptr_get_size and bpf_dynptr_get_offset
  bpf: Add bpf_dynptr_clone
  selftests/bpf: add tests for dynptr convenience helpers

 include/linux/bpf.h                           |   2 +-
 kernel/bpf/helpers.c                          | 108 +++++-
 kernel/bpf/verifier.c                         | 125 ++++++-
 kernel/trace/bpf_trace.c                      |   4 +-
 tools/testing/selftests/bpf/bpf_kfuncs.h      |   8 +
 .../testing/selftests/bpf/prog_tests/dynptr.c |   6 +
 .../testing/selftests/bpf/progs/dynptr_fail.c | 313 +++++++++++++++++
 .../selftests/bpf/progs/dynptr_success.c      | 320 ++++++++++++++++++
 8 files changed, 864 insertions(+), 22 deletions(-)

-- 
2.34.1

[PATCH v1 bpf-next 1/5] bpf: Add bpf_dynptr_trim and bpf_dynptr_advance

[Joanne Koong]

bpf_dynptr_trim decreases the size of a dynptr by the specified
number of bytes (offset remains the same). bpf_dynptr_advance advances
the offset of the dynptr by the specified number of bytes (size
decreases correspondingly).

Trimming or advancing the dynptr may be useful in certain situations.
For example, when hashing which takes in generic dynptrs, if the dynptr
points to a struct but only a certain memory region inside the struct
should be hashed, advance/trim can be used to narrow in on the
specific region to hash.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 kernel/bpf/helpers.c | 49 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index b6a5cda5bb59..51b4c4b5dbed 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1448,6 +1448,13 @@ u32 bpf_dynptr_get_size(const struct bpf_dynptr_kern *ptr)
 	return ptr->size & DYNPTR_SIZE_MASK;
 }
 
+static void bpf_dynptr_set_size(struct bpf_dynptr_kern *ptr, u32 new_size)
+{
+	u32 metadata = ptr->size & ~DYNPTR_SIZE_MASK;
+
+	ptr->size = new_size | metadata;
+}
+
 int bpf_dynptr_check_size(u32 size)
 {
 	return size > DYNPTR_MAX_SIZE ? -E2BIG : 0;
@@ -2275,6 +2282,46 @@ __bpf_kfunc void *bpf_dynptr_slice_rdwr(const struct bpf_dynptr_kern *ptr, u32 o
 	return bpf_dynptr_slice(ptr, offset, buffer, buffer__szk);
 }
 
+/* For dynptrs, the offset may only be advanced and the size may only be decremented */
+static int bpf_dynptr_adjust(struct bpf_dynptr_kern *ptr, u32 off_inc, u32 sz_dec)
+{
+	u32 size;
+
+	if (!ptr->data)
+		return -EINVAL;
+
+	size = bpf_dynptr_get_size(ptr);
+
+	if (sz_dec > size)
+		return -ERANGE;
+
+	if (off_inc) {
+		u32 new_off;
+
+		if (off_inc > size)
+			return -ERANGE;
+
+		if (check_add_overflow(ptr->offset, off_inc, &new_off))
+			return -ERANGE;
+
+		ptr->offset = new_off;
+	}
+
+	bpf_dynptr_set_size(ptr, size - sz_dec);
+
+	return 0;
+}
+
+__bpf_kfunc int bpf_dynptr_advance(struct bpf_dynptr_kern *ptr, u32 len)
+{
+	return bpf_dynptr_adjust(ptr, len, len);
+}
+
+__bpf_kfunc int bpf_dynptr_trim(struct bpf_dynptr_kern *ptr, u32 len)
+{
+	return bpf_dynptr_adjust(ptr, 0, len);
+}
+
 __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
 {
 	return obj;
@@ -2347,6 +2394,8 @@ BTF_ID_FLAGS(func, bpf_dynptr_slice_rdwr, KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_iter_num_new, KF_ITER_NEW)
 BTF_ID_FLAGS(func, bpf_iter_num_next, KF_ITER_NEXT | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_iter_num_destroy, KF_ITER_DESTROY)
+BTF_ID_FLAGS(func, bpf_dynptr_trim)
+BTF_ID_FLAGS(func, bpf_dynptr_advance)
 BTF_SET8_END(common_btf_ids)
 
 static const struct btf_kfunc_id_set common_kfunc_set = {
-- 
2.34.1

Re: [PATCH v1 bpf-next 1/5] bpf: Add bpf_dynptr_trim and bpf_dynptr_advance

[Andrii Nakryiko]

On Sat, Apr 8, 2023 at 8:34 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> bpf_dynptr_trim decreases the size of a dynptr by the specified
> number of bytes (offset remains the same). bpf_dynptr_advance advances
> the offset of the dynptr by the specified number of bytes (size
> decreases correspondingly).
>
> Trimming or advancing the dynptr may be useful in certain situations.
> For example, when hashing which takes in generic dynptrs, if the dynptr
> points to a struct but only a certain memory region inside the struct
> should be hashed, advance/trim can be used to narrow in on the
> specific region to hash.
>
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  kernel/bpf/helpers.c | 49 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index b6a5cda5bb59..51b4c4b5dbed 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1448,6 +1448,13 @@ u32 bpf_dynptr_get_size(const struct bpf_dynptr_kern *ptr)
>         return ptr->size & DYNPTR_SIZE_MASK;
>  }
>
> +static void bpf_dynptr_set_size(struct bpf_dynptr_kern *ptr, u32 new_size)
> +{
> +       u32 metadata = ptr->size & ~DYNPTR_SIZE_MASK;
> +
> +       ptr->size = new_size | metadata;
> +}
> +
>  int bpf_dynptr_check_size(u32 size)
>  {
>         return size > DYNPTR_MAX_SIZE ? -E2BIG : 0;
> @@ -2275,6 +2282,46 @@ __bpf_kfunc void *bpf_dynptr_slice_rdwr(const struct bpf_dynptr_kern *ptr, u32 o
>         return bpf_dynptr_slice(ptr, offset, buffer, buffer__szk);
>  }
>
> +/* For dynptrs, the offset may only be advanced and the size may only be decremented */
> +static int bpf_dynptr_adjust(struct bpf_dynptr_kern *ptr, u32 off_inc, u32 sz_dec)

it feels like this helper just makes it a bit harder to follow what's
going on. Half of this function isn't actually executed for
bpf_dynptr_trim, so I don't think we are saving all that much code,
maybe let's code each of advance and trim explicitly?

> +{
> +       u32 size;
> +
> +       if (!ptr->data)
> +               return -EINVAL;
> +
> +       size = bpf_dynptr_get_size(ptr);
> +
> +       if (sz_dec > size)
> +               return -ERANGE;
> +
> +       if (off_inc) {
> +               u32 new_off;
> +
> +               if (off_inc > size)

like here it becomes confusing if off_inc includes sz_dec, or they
should be added to each other. I think it's convoluted as is.


> +                       return -ERANGE;
> +
> +               if (check_add_overflow(ptr->offset, off_inc, &new_off))

why do we need to worry about overflow, we checked all the error
conditions above?..

> +                       return -ERANGE;
> +
> +               ptr->offset = new_off;
> +       }
> +
> +       bpf_dynptr_set_size(ptr, size - sz_dec);
> +
> +       return 0;
> +}
> +
> +__bpf_kfunc int bpf_dynptr_advance(struct bpf_dynptr_kern *ptr, u32 len)
> +{
> +       return bpf_dynptr_adjust(ptr, len, len);
> +}
> +
> +__bpf_kfunc int bpf_dynptr_trim(struct bpf_dynptr_kern *ptr, u32 len)

I'm also wondering if trim operation is a bit unusual for dealing
ranges? Instead of a relative size decrement, maybe it's more
straightforward to have bpf_dynptr_resize() to set new desired size?
So if someone has original dynptr with 100 bytes but wants to have
dynptr for bytes [10, 30), they'd do a pretty natural:

bpf_dynptr_advance(&dynptr, 10);
bpf_dynptr_resize(&dynptr, 20);

?

> +{
> +       return bpf_dynptr_adjust(ptr, 0, len);
> +}
> +
>  __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
>  {
>         return obj;
> @@ -2347,6 +2394,8 @@ BTF_ID_FLAGS(func, bpf_dynptr_slice_rdwr, KF_RET_NULL)
>  BTF_ID_FLAGS(func, bpf_iter_num_new, KF_ITER_NEW)
>  BTF_ID_FLAGS(func, bpf_iter_num_next, KF_ITER_NEXT | KF_RET_NULL)
>  BTF_ID_FLAGS(func, bpf_iter_num_destroy, KF_ITER_DESTROY)
> +BTF_ID_FLAGS(func, bpf_dynptr_trim)
> +BTF_ID_FLAGS(func, bpf_dynptr_advance)
>  BTF_SET8_END(common_btf_ids)
>
>  static const struct btf_kfunc_id_set common_kfunc_set = {
> --
> 2.34.1
>

Re: [PATCH v1 bpf-next 1/5] bpf: Add bpf_dynptr_trim and bpf_dynptr_advance

[Joanne Koong]

On Wed, Apr 12, 2023 at 2:46 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Sat, Apr 8, 2023 at 8:34 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > bpf_dynptr_trim decreases the size of a dynptr by the specified
> > number of bytes (offset remains the same). bpf_dynptr_advance advances
> > the offset of the dynptr by the specified number of bytes (size
> > decreases correspondingly).
> >
> > Trimming or advancing the dynptr may be useful in certain situations.
> > For example, when hashing which takes in generic dynptrs, if the dynptr
> > points to a struct but only a certain memory region inside the struct
> > should be hashed, advance/trim can be used to narrow in on the
> > specific region to hash.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > ---
> >  kernel/bpf/helpers.c | 49 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index b6a5cda5bb59..51b4c4b5dbed 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -1448,6 +1448,13 @@ u32 bpf_dynptr_get_size(const struct bpf_dynptr_kern *ptr)
> >         return ptr->size & DYNPTR_SIZE_MASK;
> >  }
> >
> > +static void bpf_dynptr_set_size(struct bpf_dynptr_kern *ptr, u32 new_size)
> > +{
> > +       u32 metadata = ptr->size & ~DYNPTR_SIZE_MASK;
> > +
> > +       ptr->size = new_size | metadata;
> > +}
> > +
> >  int bpf_dynptr_check_size(u32 size)
> >  {
> >         return size > DYNPTR_MAX_SIZE ? -E2BIG : 0;
> > @@ -2275,6 +2282,46 @@ __bpf_kfunc void *bpf_dynptr_slice_rdwr(const struct bpf_dynptr_kern *ptr, u32 o
> >         return bpf_dynptr_slice(ptr, offset, buffer, buffer__szk);
> >  }
> >
> > +/* For dynptrs, the offset may only be advanced and the size may only be decremented */
> > +static int bpf_dynptr_adjust(struct bpf_dynptr_kern *ptr, u32 off_inc, u32 sz_dec)
>
> it feels like this helper just makes it a bit harder to follow what's
> going on. Half of this function isn't actually executed for
> bpf_dynptr_trim, so I don't think we are saving all that much code,
> maybe let's code each of advance and trim explicitly?
>

Sounds good, I will change this in v2 to handle advance and trim separately

> > +{
> > +       u32 size;
> > +
> > +       if (!ptr->data)
> > +               return -EINVAL;
> > +
> > +       size = bpf_dynptr_get_size(ptr);
> > +
> > +       if (sz_dec > size)
> > +               return -ERANGE;
> > +
> > +       if (off_inc) {
> > +               u32 new_off;
> > +
> > +               if (off_inc > size)
>
> like here it becomes confusing if off_inc includes sz_dec, or they
> should be added to each other. I think it's convoluted as is.
>
>
> > +                       return -ERANGE;
> > +
> > +               if (check_add_overflow(ptr->offset, off_inc, &new_off))
>
> why do we need to worry about overflow, we checked all the error
> conditions above?..

Ahh you're right, this cant overflow u32. The dynptr max supported
size is 2^24 - 1 as well

>
> > +                       return -ERANGE;
> > +
> > +               ptr->offset = new_off;
> > +       }
> > +
> > +       bpf_dynptr_set_size(ptr, size - sz_dec);
> > +
> > +       return 0;
> > +}
> > +
> > +__bpf_kfunc int bpf_dynptr_advance(struct bpf_dynptr_kern *ptr, u32 len)
> > +{
> > +       return bpf_dynptr_adjust(ptr, len, len);
> > +}
> > +
> > +__bpf_kfunc int bpf_dynptr_trim(struct bpf_dynptr_kern *ptr, u32 len)
>
> I'm also wondering if trim operation is a bit unusual for dealing
> ranges? Instead of a relative size decrement, maybe it's more
> straightforward to have bpf_dynptr_resize() to set new desired size?
> So if someone has original dynptr with 100 bytes but wants to have
> dynptr for bytes [10, 30), they'd do a pretty natural:
>
> bpf_dynptr_advance(&dynptr, 10);
> bpf_dynptr_resize(&dynptr, 20);
>
> ?
>

Yeah! I like this idea a lot, that way they dont' need to know the
current size of the dynptr before they trim. This seems a lot more
ergonomic

> > +{
> > +       return bpf_dynptr_adjust(ptr, 0, len);
> > +}
> > +
> >  __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
> >  {
> >         return obj;
> > @@ -2347,6 +2394,8 @@ BTF_ID_FLAGS(func, bpf_dynptr_slice_rdwr, KF_RET_NULL)
> >  BTF_ID_FLAGS(func, bpf_iter_num_new, KF_ITER_NEW)
> >  BTF_ID_FLAGS(func, bpf_iter_num_next, KF_ITER_NEXT | KF_RET_NULL)
> >  BTF_ID_FLAGS(func, bpf_iter_num_destroy, KF_ITER_DESTROY)
> > +BTF_ID_FLAGS(func, bpf_dynptr_trim)
> > +BTF_ID_FLAGS(func, bpf_dynptr_advance)
> >  BTF_SET8_END(common_btf_ids)
> >
> >  static const struct btf_kfunc_id_set common_kfunc_set = {
> > --
> > 2.34.1
> >

Re: [PATCH v1 bpf-next 1/5] bpf: Add bpf_dynptr_trim and bpf_dynptr_advance

[Andrii Nakryiko]

On Thu, Apr 13, 2023 at 10:15 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Wed, Apr 12, 2023 at 2:46 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Sat, Apr 8, 2023 at 8:34 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> > >
> > > bpf_dynptr_trim decreases the size of a dynptr by the specified
> > > number of bytes (offset remains the same). bpf_dynptr_advance advances
> > > the offset of the dynptr by the specified number of bytes (size
> > > decreases correspondingly).
> > >
> > > Trimming or advancing the dynptr may be useful in certain situations.
> > > For example, when hashing which takes in generic dynptrs, if the dynptr
> > > points to a struct but only a certain memory region inside the struct
> > > should be hashed, advance/trim can be used to narrow in on the
> > > specific region to hash.
> > >
> > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > ---
> > >  kernel/bpf/helpers.c | 49 ++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 49 insertions(+)
> > >
> > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > > index b6a5cda5bb59..51b4c4b5dbed 100644
> > > --- a/kernel/bpf/helpers.c
> > > +++ b/kernel/bpf/helpers.c
> > > @@ -1448,6 +1448,13 @@ u32 bpf_dynptr_get_size(const struct bpf_dynptr_kern *ptr)
> > >         return ptr->size & DYNPTR_SIZE_MASK;
> > >  }
> > >
> > > +static void bpf_dynptr_set_size(struct bpf_dynptr_kern *ptr, u32 new_size)
> > > +{
> > > +       u32 metadata = ptr->size & ~DYNPTR_SIZE_MASK;
> > > +
> > > +       ptr->size = new_size | metadata;
> > > +}
> > > +
> > >  int bpf_dynptr_check_size(u32 size)
> > >  {
> > >         return size > DYNPTR_MAX_SIZE ? -E2BIG : 0;
> > > @@ -2275,6 +2282,46 @@ __bpf_kfunc void *bpf_dynptr_slice_rdwr(const struct bpf_dynptr_kern *ptr, u32 o
> > >         return bpf_dynptr_slice(ptr, offset, buffer, buffer__szk);
> > >  }
> > >
> > > +/* For dynptrs, the offset may only be advanced and the size may only be decremented */
> > > +static int bpf_dynptr_adjust(struct bpf_dynptr_kern *ptr, u32 off_inc, u32 sz_dec)
> >
> > it feels like this helper just makes it a bit harder to follow what's
> > going on. Half of this function isn't actually executed for
> > bpf_dynptr_trim, so I don't think we are saving all that much code,
> > maybe let's code each of advance and trim explicitly?
> >
>
> Sounds good, I will change this in v2 to handle advance and trim separately
>
> > > +{
> > > +       u32 size;
> > > +
> > > +       if (!ptr->data)
> > > +               return -EINVAL;
> > > +
> > > +       size = bpf_dynptr_get_size(ptr);
> > > +
> > > +       if (sz_dec > size)
> > > +               return -ERANGE;
> > > +
> > > +       if (off_inc) {
> > > +               u32 new_off;
> > > +
> > > +               if (off_inc > size)
> >
> > like here it becomes confusing if off_inc includes sz_dec, or they
> > should be added to each other. I think it's convoluted as is.
> >
> >
> > > +                       return -ERANGE;
> > > +
> > > +               if (check_add_overflow(ptr->offset, off_inc, &new_off))
> >
> > why do we need to worry about overflow, we checked all the error
> > conditions above?..
>
> Ahh you're right, this cant overflow u32. The dynptr max supported
> size is 2^24 - 1 as well
>
> >
> > > +                       return -ERANGE;
> > > +
> > > +               ptr->offset = new_off;
> > > +       }
> > > +
> > > +       bpf_dynptr_set_size(ptr, size - sz_dec);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +__bpf_kfunc int bpf_dynptr_advance(struct bpf_dynptr_kern *ptr, u32 len)
> > > +{
> > > +       return bpf_dynptr_adjust(ptr, len, len);
> > > +}
> > > +
> > > +__bpf_kfunc int bpf_dynptr_trim(struct bpf_dynptr_kern *ptr, u32 len)
> >
> > I'm also wondering if trim operation is a bit unusual for dealing
> > ranges? Instead of a relative size decrement, maybe it's more
> > straightforward to have bpf_dynptr_resize() to set new desired size?
> > So if someone has original dynptr with 100 bytes but wants to have
> > dynptr for bytes [10, 30), they'd do a pretty natural:
> >
> > bpf_dynptr_advance(&dynptr, 10);
> > bpf_dynptr_resize(&dynptr, 20);
> >
> > ?
> >
>
> Yeah! I like this idea a lot, that way they dont' need to know the
> current size of the dynptr before they trim. This seems a lot more
> ergonomic

Thinking a bit more, I'm now wondering if we should actually merge
those two into one API to allow adjust both at the same time.
Similarly how langauges like Go and Rust allow to adjust array slices
by specifying new [start, end) offsets, should we have just one:

bpf_dynptr_adjust(&dynptr, 10, 30);

bpf_dynptr_advance() could be expressed as:

bpf_dynptr_adjust(&dynptr, 10, bpf_dynptr_size(&dynptr) - 10);

I suspect full adjust with custom [start, end) will be actually more
common than just advancing offset.

>
> > > +{
> > > +       return bpf_dynptr_adjust(ptr, 0, len);
> > > +}
> > > +
> > >  __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
> > >  {
> > >         return obj;
> > > @@ -2347,6 +2394,8 @@ BTF_ID_FLAGS(func, bpf_dynptr_slice_rdwr, KF_RET_NULL)
> > >  BTF_ID_FLAGS(func, bpf_iter_num_new, KF_ITER_NEW)
> > >  BTF_ID_FLAGS(func, bpf_iter_num_next, KF_ITER_NEXT | KF_RET_NULL)
> > >  BTF_ID_FLAGS(func, bpf_iter_num_destroy, KF_ITER_DESTROY)
> > > +BTF_ID_FLAGS(func, bpf_dynptr_trim)
> > > +BTF_ID_FLAGS(func, bpf_dynptr_advance)
> > >  BTF_SET8_END(common_btf_ids)
> > >
> > >  static const struct btf_kfunc_id_set common_kfunc_set = {
> > > --
> > > 2.34.1
> > >

Re: [PATCH v1 bpf-next 1/5] bpf: Add bpf_dynptr_trim and bpf_dynptr_advance

[Joanne Koong]

On Mon, Apr 17, 2023 at 4:36 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Apr 13, 2023 at 10:15 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > On Wed, Apr 12, 2023 at 2:46 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Sat, Apr 8, 2023 at 8:34 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> > > >
> > > > bpf_dynptr_trim decreases the size of a dynptr by the specified
> > > > number of bytes (offset remains the same). bpf_dynptr_advance advances
> > > > the offset of the dynptr by the specified number of bytes (size
> > > > decreases correspondingly).
> > > >
> > > > Trimming or advancing the dynptr may be useful in certain situations.
> > > > For example, when hashing which takes in generic dynptrs, if the dynptr
> > > > points to a struct but only a certain memory region inside the struct
> > > > should be hashed, advance/trim can be used to narrow in on the
> > > > specific region to hash.
> > > >
> > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > ---
> > > >  kernel/bpf/helpers.c | 49 ++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 49 insertions(+)
> > > >
> > > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > > > index b6a5cda5bb59..51b4c4b5dbed 100644
> > > > --- a/kernel/bpf/helpers.c
> > > > +++ b/kernel/bpf/helpers.c
> > > > @@ -1448,6 +1448,13 @@ u32 bpf_dynptr_get_size(const struct bpf_dynptr_kern *ptr)
> > > >         return ptr->size & DYNPTR_SIZE_MASK;
> > > >  }
> > > >
> > > > +static void bpf_dynptr_set_size(struct bpf_dynptr_kern *ptr, u32 new_size)
> > > > +{
> > > > +       u32 metadata = ptr->size & ~DYNPTR_SIZE_MASK;
> > > > +
> > > > +       ptr->size = new_size | metadata;
> > > > +}
> > > > +
> > > >  int bpf_dynptr_check_size(u32 size)
> > > >  {
> > > >         return size > DYNPTR_MAX_SIZE ? -E2BIG : 0;
> > > > @@ -2275,6 +2282,46 @@ __bpf_kfunc void *bpf_dynptr_slice_rdwr(const struct bpf_dynptr_kern *ptr, u32 o
> > > >         return bpf_dynptr_slice(ptr, offset, buffer, buffer__szk);
> > > >  }
> > > >
> > > > +/* For dynptrs, the offset may only be advanced and the size may only be decremented */
> > > > +static int bpf_dynptr_adjust(struct bpf_dynptr_kern *ptr, u32 off_inc, u32 sz_dec)
> > >
> > > it feels like this helper just makes it a bit harder to follow what's
> > > going on. Half of this function isn't actually executed for
> > > bpf_dynptr_trim, so I don't think we are saving all that much code,
> > > maybe let's code each of advance and trim explicitly?
> > >
> >
> > Sounds good, I will change this in v2 to handle advance and trim separately
> >
> > > > +{
> > > > +       u32 size;
> > > > +
> > > > +       if (!ptr->data)
> > > > +               return -EINVAL;
> > > > +
> > > > +       size = bpf_dynptr_get_size(ptr);
> > > > +
> > > > +       if (sz_dec > size)
> > > > +               return -ERANGE;
> > > > +
> > > > +       if (off_inc) {
> > > > +               u32 new_off;
> > > > +
> > > > +               if (off_inc > size)
> > >
> > > like here it becomes confusing if off_inc includes sz_dec, or they
> > > should be added to each other. I think it's convoluted as is.
> > >
> > >
> > > > +                       return -ERANGE;
> > > > +
> > > > +               if (check_add_overflow(ptr->offset, off_inc, &new_off))
> > >
> > > why do we need to worry about overflow, we checked all the error
> > > conditions above?..
> >
> > Ahh you're right, this cant overflow u32. The dynptr max supported
> > size is 2^24 - 1 as well
> >
> > >
> > > > +                       return -ERANGE;
> > > > +
> > > > +               ptr->offset = new_off;
> > > > +       }
> > > > +
> > > > +       bpf_dynptr_set_size(ptr, size - sz_dec);
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +__bpf_kfunc int bpf_dynptr_advance(struct bpf_dynptr_kern *ptr, u32 len)
> > > > +{
> > > > +       return bpf_dynptr_adjust(ptr, len, len);
> > > > +}
> > > > +
> > > > +__bpf_kfunc int bpf_dynptr_trim(struct bpf_dynptr_kern *ptr, u32 len)
> > >
> > > I'm also wondering if trim operation is a bit unusual for dealing
> > > ranges? Instead of a relative size decrement, maybe it's more
> > > straightforward to have bpf_dynptr_resize() to set new desired size?
> > > So if someone has original dynptr with 100 bytes but wants to have
> > > dynptr for bytes [10, 30), they'd do a pretty natural:
> > >
> > > bpf_dynptr_advance(&dynptr, 10);
> > > bpf_dynptr_resize(&dynptr, 20);
> > >
> > > ?
> > >
> >
> > Yeah! I like this idea a lot, that way they dont' need to know the
> > current size of the dynptr before they trim. This seems a lot more
> > ergonomic
>
> Thinking a bit more, I'm now wondering if we should actually merge
> those two into one API to allow adjust both at the same time.
> Similarly how langauges like Go and Rust allow to adjust array slices
> by specifying new [start, end) offsets, should we have just one:
>
> bpf_dynptr_adjust(&dynptr, 10, 30);
>
> bpf_dynptr_advance() could be expressed as:
>
> bpf_dynptr_adjust(&dynptr, 10, bpf_dynptr_size(&dynptr) - 10);
>
I think for expressing advance where only start offset changes, end
needs to be "bpf_dynptr_size(&dynptr)" (no minus 10) here?

> I suspect full adjust with custom [start, end) will be actually more
> common than just advancing offset.
>

I think this might get quickly cumbersome for the use cases where the
user just wants to parse through the data with only adjusting start
offset, for example parsing an skb's header options. maybe there's
some way to combine the two?:

bpf_dynptr_adjust(&dynptr, start, end);
where if end is -1 or some #define macro set to u32_max or something
like that then that signifies dont' modify the end offset, just modify
the start? That way the user can just advance instead of needing to
know its size every time. I don't know if that makes the interface
uglier / more confusing though. WDYT?

> >
> > > > +{
> > > > +       return bpf_dynptr_adjust(ptr, 0, len);
> > > > +}
> > > > +
> > > >  __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
> > > >  {
> > > >         return obj;
> > > > @@ -2347,6 +2394,8 @@ BTF_ID_FLAGS(func, bpf_dynptr_slice_rdwr, KF_RET_NULL)
> > > >  BTF_ID_FLAGS(func, bpf_iter_num_new, KF_ITER_NEW)
> > > >  BTF_ID_FLAGS(func, bpf_iter_num_next, KF_ITER_NEXT | KF_RET_NULL)
> > > >  BTF_ID_FLAGS(func, bpf_iter_num_destroy, KF_ITER_DESTROY)
> > > > +BTF_ID_FLAGS(func, bpf_dynptr_trim)
> > > > +BTF_ID_FLAGS(func, bpf_dynptr_advance)
> > > >  BTF_SET8_END(common_btf_ids)
> > > >
> > > >  static const struct btf_kfunc_id_set common_kfunc_set = {
> > > > --
> > > > 2.34.1
> > > >

Re: [PATCH v1 bpf-next 1/5] bpf: Add bpf_dynptr_trim and bpf_dynptr_advance

[Andrii Nakryiko]

On Tue, Apr 18, 2023 at 11:22 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Mon, Apr 17, 2023 at 4:36 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, Apr 13, 2023 at 10:15 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> > >
> > > On Wed, Apr 12, 2023 at 2:46 PM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > On Sat, Apr 8, 2023 at 8:34 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> > > > >
> > > > > bpf_dynptr_trim decreases the size of a dynptr by the specified
> > > > > number of bytes (offset remains the same). bpf_dynptr_advance advances
> > > > > the offset of the dynptr by the specified number of bytes (size
> > > > > decreases correspondingly).
> > > > >
> > > > > Trimming or advancing the dynptr may be useful in certain situations.
> > > > > For example, when hashing which takes in generic dynptrs, if the dynptr
> > > > > points to a struct but only a certain memory region inside the struct
> > > > > should be hashed, advance/trim can be used to narrow in on the
> > > > > specific region to hash.
> > > > >
> > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > > ---
> > > > >  kernel/bpf/helpers.c | 49 ++++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 49 insertions(+)
> > > > >
> > > > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > > > > index b6a5cda5bb59..51b4c4b5dbed 100644
> > > > > --- a/kernel/bpf/helpers.c
> > > > > +++ b/kernel/bpf/helpers.c
> > > > > @@ -1448,6 +1448,13 @@ u32 bpf_dynptr_get_size(const struct bpf_dynptr_kern *ptr)
> > > > >         return ptr->size & DYNPTR_SIZE_MASK;
> > > > >  }
> > > > >
> > > > > +static void bpf_dynptr_set_size(struct bpf_dynptr_kern *ptr, u32 new_size)
> > > > > +{
> > > > > +       u32 metadata = ptr->size & ~DYNPTR_SIZE_MASK;
> > > > > +
> > > > > +       ptr->size = new_size | metadata;
> > > > > +}
> > > > > +
> > > > >  int bpf_dynptr_check_size(u32 size)
> > > > >  {
> > > > >         return size > DYNPTR_MAX_SIZE ? -E2BIG : 0;
> > > > > @@ -2275,6 +2282,46 @@ __bpf_kfunc void *bpf_dynptr_slice_rdwr(const struct bpf_dynptr_kern *ptr, u32 o
> > > > >         return bpf_dynptr_slice(ptr, offset, buffer, buffer__szk);
> > > > >  }
> > > > >
> > > > > +/* For dynptrs, the offset may only be advanced and the size may only be decremented */
> > > > > +static int bpf_dynptr_adjust(struct bpf_dynptr_kern *ptr, u32 off_inc, u32 sz_dec)
> > > >
> > > > it feels like this helper just makes it a bit harder to follow what's
> > > > going on. Half of this function isn't actually executed for
> > > > bpf_dynptr_trim, so I don't think we are saving all that much code,
> > > > maybe let's code each of advance and trim explicitly?
> > > >
> > >
> > > Sounds good, I will change this in v2 to handle advance and trim separately
> > >
> > > > > +{
> > > > > +       u32 size;
> > > > > +
> > > > > +       if (!ptr->data)
> > > > > +               return -EINVAL;
> > > > > +
> > > > > +       size = bpf_dynptr_get_size(ptr);
> > > > > +
> > > > > +       if (sz_dec > size)
> > > > > +               return -ERANGE;
> > > > > +
> > > > > +       if (off_inc) {
> > > > > +               u32 new_off;
> > > > > +
> > > > > +               if (off_inc > size)
> > > >
> > > > like here it becomes confusing if off_inc includes sz_dec, or they
> > > > should be added to each other. I think it's convoluted as is.
> > > >
> > > >
> > > > > +                       return -ERANGE;
> > > > > +
> > > > > +               if (check_add_overflow(ptr->offset, off_inc, &new_off))
> > > >
> > > > why do we need to worry about overflow, we checked all the error
> > > > conditions above?..
> > >
> > > Ahh you're right, this cant overflow u32. The dynptr max supported
> > > size is 2^24 - 1 as well
> > >
> > > >
> > > > > +                       return -ERANGE;
> > > > > +
> > > > > +               ptr->offset = new_off;
> > > > > +       }
> > > > > +
> > > > > +       bpf_dynptr_set_size(ptr, size - sz_dec);
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > > +__bpf_kfunc int bpf_dynptr_advance(struct bpf_dynptr_kern *ptr, u32 len)
> > > > > +{
> > > > > +       return bpf_dynptr_adjust(ptr, len, len);
> > > > > +}
> > > > > +
> > > > > +__bpf_kfunc int bpf_dynptr_trim(struct bpf_dynptr_kern *ptr, u32 len)
> > > >
> > > > I'm also wondering if trim operation is a bit unusual for dealing
> > > > ranges? Instead of a relative size decrement, maybe it's more
> > > > straightforward to have bpf_dynptr_resize() to set new desired size?
> > > > So if someone has original dynptr with 100 bytes but wants to have
> > > > dynptr for bytes [10, 30), they'd do a pretty natural:
> > > >
> > > > bpf_dynptr_advance(&dynptr, 10);
> > > > bpf_dynptr_resize(&dynptr, 20);
> > > >
> > > > ?
> > > >
> > >
> > > Yeah! I like this idea a lot, that way they dont' need to know the
> > > current size of the dynptr before they trim. This seems a lot more
> > > ergonomic
> >
> > Thinking a bit more, I'm now wondering if we should actually merge
> > those two into one API to allow adjust both at the same time.
> > Similarly how langauges like Go and Rust allow to adjust array slices
> > by specifying new [start, end) offsets, should we have just one:
> >
> > bpf_dynptr_adjust(&dynptr, 10, 30);
> >
> > bpf_dynptr_advance() could be expressed as:
> >
> > bpf_dynptr_adjust(&dynptr, 10, bpf_dynptr_size(&dynptr) - 10);
> >
> I think for expressing advance where only start offset changes, end
> needs to be "bpf_dynptr_size(&dynptr)" (no minus 10) here?

yep, you are right! it's end offset, so no need to adjust for 10. So
even better:

bpf_dynptr_adjust(&dynptr, 10, bpf_dynptr_size(&dynptr));

>
> > I suspect full adjust with custom [start, end) will be actually more
> > common than just advancing offset.
> >
>
> I think this might get quickly cumbersome for the use cases where the
> user just wants to parse through the data with only adjusting start
> offset, for example parsing an skb's header options. maybe there's
> some way to combine the two?:
>
> bpf_dynptr_adjust(&dynptr, start, end);
> where if end is -1 or some #define macro set to u32_max or something
> like that then that signifies dont' modify the end offset, just modify
> the start? That way the user can just advance instead of needing to
> know its size every time. I don't know if that makes the interface
> uglier / more confusing though. WDYT?

I think it does make it more cumbersome, I'd keep it as [start, end)
offset always. We can inline bpf_dynptr_size() if there is a
performance concern.

At least I'd start there, and if there is demand we can also add -1 as
a special case later.

>
> > >
> > > > > +{
> > > > > +       return bpf_dynptr_adjust(ptr, 0, len);
> > > > > +}
> > > > > +
> > > > >  __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
> > > > >  {
> > > > >         return obj;
> > > > > @@ -2347,6 +2394,8 @@ BTF_ID_FLAGS(func, bpf_dynptr_slice_rdwr, KF_RET_NULL)
> > > > >  BTF_ID_FLAGS(func, bpf_iter_num_new, KF_ITER_NEW)
> > > > >  BTF_ID_FLAGS(func, bpf_iter_num_next, KF_ITER_NEXT | KF_RET_NULL)
> > > > >  BTF_ID_FLAGS(func, bpf_iter_num_destroy, KF_ITER_DESTROY)
> > > > > +BTF_ID_FLAGS(func, bpf_dynptr_trim)
> > > > > +BTF_ID_FLAGS(func, bpf_dynptr_advance)
> > > > >  BTF_SET8_END(common_btf_ids)
> > > > >
> > > > >  static const struct btf_kfunc_id_set common_kfunc_set = {
> > > > > --
> > > > > 2.34.1
> > > > >

[PATCH v1 bpf-next 2/5] bpf: Add bpf_dynptr_is_null and bpf_dynptr_is_rdonly

[Joanne Koong]

bpf_dynptr_is_null returns true if the dynptr is null / invalid
(determined by whether ptr->data is NULL), else false if
the dynptr is a valid dynptr.

bpf_dynptr_is_rdonly returns true if the dynptr is read-only,
else false if the dynptr is read-writable.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 kernel/bpf/helpers.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 51b4c4b5dbed..e4e84e92a4c6 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1423,7 +1423,7 @@ static const struct bpf_func_proto bpf_kptr_xchg_proto = {
 #define DYNPTR_SIZE_MASK	0xFFFFFF
 #define DYNPTR_RDONLY_BIT	BIT(31)
 
-static bool bpf_dynptr_is_rdonly(const struct bpf_dynptr_kern *ptr)
+static bool __bpf_dynptr_is_rdonly(const struct bpf_dynptr_kern *ptr)
 {
 	return ptr->size & DYNPTR_RDONLY_BIT;
 }
@@ -1570,7 +1570,7 @@ BPF_CALL_5(bpf_dynptr_write, const struct bpf_dynptr_kern *, dst, u32, offset, v
 	enum bpf_dynptr_type type;
 	int err;
 
-	if (!dst->data || bpf_dynptr_is_rdonly(dst))
+	if (!dst->data || __bpf_dynptr_is_rdonly(dst))
 		return -EINVAL;
 
 	err = bpf_dynptr_check_off_len(dst, offset, len);
@@ -1626,7 +1626,7 @@ BPF_CALL_3(bpf_dynptr_data, const struct bpf_dynptr_kern *, ptr, u32, offset, u3
 	if (err)
 		return 0;
 
-	if (bpf_dynptr_is_rdonly(ptr))
+	if (__bpf_dynptr_is_rdonly(ptr))
 		return 0;
 
 	type = bpf_dynptr_get_type(ptr);
@@ -2254,7 +2254,7 @@ __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset
 __bpf_kfunc void *bpf_dynptr_slice_rdwr(const struct bpf_dynptr_kern *ptr, u32 offset,
 					void *buffer, u32 buffer__szk)
 {
-	if (!ptr->data || bpf_dynptr_is_rdonly(ptr))
+	if (!ptr->data || __bpf_dynptr_is_rdonly(ptr))
 		return NULL;
 
 	/* bpf_dynptr_slice_rdwr is the same logic as bpf_dynptr_slice.
@@ -2322,6 +2322,19 @@ __bpf_kfunc int bpf_dynptr_trim(struct bpf_dynptr_kern *ptr, u32 len)
 	return bpf_dynptr_adjust(ptr, 0, len);
 }
 
+__bpf_kfunc bool bpf_dynptr_is_null(struct bpf_dynptr_kern *ptr)
+{
+	return !ptr->data;
+}
+
+__bpf_kfunc bool bpf_dynptr_is_rdonly(struct bpf_dynptr_kern *ptr)
+{
+	if (!ptr->data)
+		return false;
+
+	return __bpf_dynptr_is_rdonly(ptr);
+}
+
 __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
 {
 	return obj;
@@ -2396,6 +2409,8 @@ BTF_ID_FLAGS(func, bpf_iter_num_next, KF_ITER_NEXT | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_iter_num_destroy, KF_ITER_DESTROY)
 BTF_ID_FLAGS(func, bpf_dynptr_trim)
 BTF_ID_FLAGS(func, bpf_dynptr_advance)
+BTF_ID_FLAGS(func, bpf_dynptr_is_null)
+BTF_ID_FLAGS(func, bpf_dynptr_is_rdonly)
 BTF_SET8_END(common_btf_ids)
 
 static const struct btf_kfunc_id_set common_kfunc_set = {
-- 
2.34.1

Re: [PATCH v1 bpf-next 2/5] bpf: Add bpf_dynptr_is_null and bpf_dynptr_is_rdonly

[Andrii Nakryiko]

On Sat, Apr 8, 2023 at 8:34 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> bpf_dynptr_is_null returns true if the dynptr is null / invalid
> (determined by whether ptr->data is NULL), else false if
> the dynptr is a valid dynptr.
>
> bpf_dynptr_is_rdonly returns true if the dynptr is read-only,
> else false if the dynptr is read-writable.
>
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  kernel/bpf/helpers.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 51b4c4b5dbed..e4e84e92a4c6 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1423,7 +1423,7 @@ static const struct bpf_func_proto bpf_kptr_xchg_proto = {
>  #define DYNPTR_SIZE_MASK       0xFFFFFF
>  #define DYNPTR_RDONLY_BIT      BIT(31)
>
> -static bool bpf_dynptr_is_rdonly(const struct bpf_dynptr_kern *ptr)
> +static bool __bpf_dynptr_is_rdonly(const struct bpf_dynptr_kern *ptr)
>  {
>         return ptr->size & DYNPTR_RDONLY_BIT;
>  }
> @@ -1570,7 +1570,7 @@ BPF_CALL_5(bpf_dynptr_write, const struct bpf_dynptr_kern *, dst, u32, offset, v
>         enum bpf_dynptr_type type;
>         int err;
>
> -       if (!dst->data || bpf_dynptr_is_rdonly(dst))
> +       if (!dst->data || __bpf_dynptr_is_rdonly(dst))
>                 return -EINVAL;
>
>         err = bpf_dynptr_check_off_len(dst, offset, len);
> @@ -1626,7 +1626,7 @@ BPF_CALL_3(bpf_dynptr_data, const struct bpf_dynptr_kern *, ptr, u32, offset, u3
>         if (err)
>                 return 0;
>
> -       if (bpf_dynptr_is_rdonly(ptr))
> +       if (__bpf_dynptr_is_rdonly(ptr))
>                 return 0;
>
>         type = bpf_dynptr_get_type(ptr);
> @@ -2254,7 +2254,7 @@ __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset
>  __bpf_kfunc void *bpf_dynptr_slice_rdwr(const struct bpf_dynptr_kern *ptr, u32 offset,
>                                         void *buffer, u32 buffer__szk)
>  {
> -       if (!ptr->data || bpf_dynptr_is_rdonly(ptr))
> +       if (!ptr->data || __bpf_dynptr_is_rdonly(ptr))

seems like all the uses of __bpf_dynptr_is_rdonly check !ptr->data
explicitly, so maybe move that ptr->data check inside and simplify all
the callers?

Regardless, looks good:

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>                 return NULL;
>
>         /* bpf_dynptr_slice_rdwr is the same logic as bpf_dynptr_slice.
> @@ -2322,6 +2322,19 @@ __bpf_kfunc int bpf_dynptr_trim(struct bpf_dynptr_kern *ptr, u32 len)
>         return bpf_dynptr_adjust(ptr, 0, len);
>  }
>
> +__bpf_kfunc bool bpf_dynptr_is_null(struct bpf_dynptr_kern *ptr)
> +{
> +       return !ptr->data;
> +}
> +
> +__bpf_kfunc bool bpf_dynptr_is_rdonly(struct bpf_dynptr_kern *ptr)
> +{
> +       if (!ptr->data)
> +               return false;
> +
> +       return __bpf_dynptr_is_rdonly(ptr);
> +}
> +
>  __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
>  {
>         return obj;
> @@ -2396,6 +2409,8 @@ BTF_ID_FLAGS(func, bpf_iter_num_next, KF_ITER_NEXT | KF_RET_NULL)
>  BTF_ID_FLAGS(func, bpf_iter_num_destroy, KF_ITER_DESTROY)
>  BTF_ID_FLAGS(func, bpf_dynptr_trim)
>  BTF_ID_FLAGS(func, bpf_dynptr_advance)
> +BTF_ID_FLAGS(func, bpf_dynptr_is_null)
> +BTF_ID_FLAGS(func, bpf_dynptr_is_rdonly)
>  BTF_SET8_END(common_btf_ids)
>
>  static const struct btf_kfunc_id_set common_kfunc_set = {
> --
> 2.34.1
>

Re: [PATCH v1 bpf-next 2/5] bpf: Add bpf_dynptr_is_null and bpf_dynptr_is_rdonly

[Joanne Koong]

On Wed, Apr 12, 2023 at 2:50 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Sat, Apr 8, 2023 at 8:34 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > bpf_dynptr_is_null returns true if the dynptr is null / invalid
> > (determined by whether ptr->data is NULL), else false if
> > the dynptr is a valid dynptr.
> >
> > bpf_dynptr_is_rdonly returns true if the dynptr is read-only,
> > else false if the dynptr is read-writable.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > ---
> >  kernel/bpf/helpers.c | 23 +++++++++++++++++++----
> >  1 file changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index 51b4c4b5dbed..e4e84e92a4c6 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -1423,7 +1423,7 @@ static const struct bpf_func_proto bpf_kptr_xchg_proto = {
> >  #define DYNPTR_SIZE_MASK       0xFFFFFF
> >  #define DYNPTR_RDONLY_BIT      BIT(31)
> >
> > -static bool bpf_dynptr_is_rdonly(const struct bpf_dynptr_kern *ptr)
> > +static bool __bpf_dynptr_is_rdonly(const struct bpf_dynptr_kern *ptr)
> >  {
> >         return ptr->size & DYNPTR_RDONLY_BIT;
> >  }
> > @@ -1570,7 +1570,7 @@ BPF_CALL_5(bpf_dynptr_write, const struct bpf_dynptr_kern *, dst, u32, offset, v
> >         enum bpf_dynptr_type type;
> >         int err;
> >
> > -       if (!dst->data || bpf_dynptr_is_rdonly(dst))
> > +       if (!dst->data || __bpf_dynptr_is_rdonly(dst))
> >                 return -EINVAL;
> >
> >         err = bpf_dynptr_check_off_len(dst, offset, len);
> > @@ -1626,7 +1626,7 @@ BPF_CALL_3(bpf_dynptr_data, const struct bpf_dynptr_kern *, ptr, u32, offset, u3
> >         if (err)
> >                 return 0;
> >
> > -       if (bpf_dynptr_is_rdonly(ptr))
> > +       if (__bpf_dynptr_is_rdonly(ptr))
> >                 return 0;
> >
> >         type = bpf_dynptr_get_type(ptr);
> > @@ -2254,7 +2254,7 @@ __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset
> >  __bpf_kfunc void *bpf_dynptr_slice_rdwr(const struct bpf_dynptr_kern *ptr, u32 offset,
> >                                         void *buffer, u32 buffer__szk)
> >  {
> > -       if (!ptr->data || bpf_dynptr_is_rdonly(ptr))
> > +       if (!ptr->data || __bpf_dynptr_is_rdonly(ptr))
>
> seems like all the uses of __bpf_dynptr_is_rdonly check !ptr->data
> explicitly, so maybe move that ptr->data check inside and simplify all
> the callers?

i think combining it gets confusing in the case where ptr->data is
null, as to how the invoked places interpret the return value. I think
having the check spelled out more explicitly in the invoked places (eg
bpf_dynptr_write, bpf_dynptr_data, ...) makes it more clear as well
where the check for null is happening. for v2 I will leave this as is,
but also happy to change it if you prefer the two be combined

>
> Regardless, looks good:
>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>
> >                 return NULL;
> >
> >         /* bpf_dynptr_slice_rdwr is the same logic as bpf_dynptr_slice.
> > @@ -2322,6 +2322,19 @@ __bpf_kfunc int bpf_dynptr_trim(struct bpf_dynptr_kern *ptr, u32 len)
> >         return bpf_dynptr_adjust(ptr, 0, len);
> >  }
> >
> > +__bpf_kfunc bool bpf_dynptr_is_null(struct bpf_dynptr_kern *ptr)
> > +{
> > +       return !ptr->data;
> > +}
> > +
> > +__bpf_kfunc bool bpf_dynptr_is_rdonly(struct bpf_dynptr_kern *ptr)
> > +{
> > +       if (!ptr->data)
> > +               return false;
> > +
> > +       return __bpf_dynptr_is_rdonly(ptr);
> > +}
> > +
> >  __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
> >  {
> >         return obj;
> > @@ -2396,6 +2409,8 @@ BTF_ID_FLAGS(func, bpf_iter_num_next, KF_ITER_NEXT | KF_RET_NULL)
> >  BTF_ID_FLAGS(func, bpf_iter_num_destroy, KF_ITER_DESTROY)
> >  BTF_ID_FLAGS(func, bpf_dynptr_trim)
> >  BTF_ID_FLAGS(func, bpf_dynptr_advance)
> > +BTF_ID_FLAGS(func, bpf_dynptr_is_null)
> > +BTF_ID_FLAGS(func, bpf_dynptr_is_rdonly)
> >  BTF_SET8_END(common_btf_ids)
> >
> >  static const struct btf_kfunc_id_set common_kfunc_set = {
> > --
> > 2.34.1
> >

[PATCH v1 bpf-next 3/5] bpf: Add bpf_dynptr_get_size and bpf_dynptr_get_offset

[Joanne Koong]

bpf_dynptr_get_size returns the number of useable bytes in a dynptr and
bpf_dynptr_get_offset returns the current offset into the dynptr.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 include/linux/bpf.h      |  2 +-
 kernel/bpf/helpers.c     | 24 +++++++++++++++++++++---
 kernel/trace/bpf_trace.c |  4 ++--
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 002a811b6b90..2a73ddd06e55 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1167,7 +1167,7 @@ enum bpf_dynptr_type {
 };
 
 int bpf_dynptr_check_size(u32 size);
-u32 bpf_dynptr_get_size(const struct bpf_dynptr_kern *ptr);
+u32 __bpf_dynptr_get_size(const struct bpf_dynptr_kern *ptr);
 
 #ifdef CONFIG_BPF_JIT
 int bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr);
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index e4e84e92a4c6..bac4c6fe49f0 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1443,7 +1443,7 @@ static enum bpf_dynptr_type bpf_dynptr_get_type(const struct bpf_dynptr_kern *pt
 	return (ptr->size & ~(DYNPTR_RDONLY_BIT)) >> DYNPTR_TYPE_SHIFT;
 }
 
-u32 bpf_dynptr_get_size(const struct bpf_dynptr_kern *ptr)
+u32 __bpf_dynptr_get_size(const struct bpf_dynptr_kern *ptr)
 {
 	return ptr->size & DYNPTR_SIZE_MASK;
 }
@@ -1476,7 +1476,7 @@ void bpf_dynptr_set_null(struct bpf_dynptr_kern *ptr)
 
 static int bpf_dynptr_check_off_len(const struct bpf_dynptr_kern *ptr, u32 offset, u32 len)
 {
-	u32 size = bpf_dynptr_get_size(ptr);
+	u32 size = __bpf_dynptr_get_size(ptr);
 
 	if (len > size || offset > size - len)
 		return -E2BIG;
@@ -2290,7 +2290,7 @@ static int bpf_dynptr_adjust(struct bpf_dynptr_kern *ptr, u32 off_inc, u32 sz_de
 	if (!ptr->data)
 		return -EINVAL;
 
-	size = bpf_dynptr_get_size(ptr);
+	size = __bpf_dynptr_get_size(ptr);
 
 	if (sz_dec > size)
 		return -ERANGE;
@@ -2335,6 +2335,22 @@ __bpf_kfunc bool bpf_dynptr_is_rdonly(struct bpf_dynptr_kern *ptr)
 	return __bpf_dynptr_is_rdonly(ptr);
 }
 
+__bpf_kfunc __u32 bpf_dynptr_get_size(const struct bpf_dynptr_kern *ptr)
+{
+	if (!ptr->data)
+		return -EINVAL;
+
+	return __bpf_dynptr_get_size(ptr);
+}
+
+__bpf_kfunc __u32 bpf_dynptr_get_offset(const struct bpf_dynptr_kern *ptr)
+{
+	if (!ptr->data)
+		return -EINVAL;
+
+	return ptr->offset;
+}
+
 __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
 {
 	return obj;
@@ -2411,6 +2427,8 @@ BTF_ID_FLAGS(func, bpf_dynptr_trim)
 BTF_ID_FLAGS(func, bpf_dynptr_advance)
 BTF_ID_FLAGS(func, bpf_dynptr_is_null)
 BTF_ID_FLAGS(func, bpf_dynptr_is_rdonly)
+BTF_ID_FLAGS(func, bpf_dynptr_get_size)
+BTF_ID_FLAGS(func, bpf_dynptr_get_offset)
 BTF_SET8_END(common_btf_ids)
 
 static const struct btf_kfunc_id_set common_kfunc_set = {
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index bcf91bc7bf71..f30bdc72d26c 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1349,9 +1349,9 @@ __bpf_kfunc int bpf_verify_pkcs7_signature(struct bpf_dynptr_kern *data_ptr,
 	}
 
 	return verify_pkcs7_signature(data_ptr->data,
-				      bpf_dynptr_get_size(data_ptr),
+				      __bpf_dynptr_get_size(data_ptr),
 				      sig_ptr->data,
-				      bpf_dynptr_get_size(sig_ptr),
+				      __bpf_dynptr_get_size(sig_ptr),
 				      trusted_keyring->key,
 				      VERIFYING_UNSPECIFIED_SIGNATURE, NULL,
 				      NULL);
-- 
2.34.1

Re: [PATCH v1 bpf-next 3/5] bpf: Add bpf_dynptr_get_size and bpf_dynptr_get_offset

[Andrii Nakryiko]

On Sat, Apr 8, 2023 at 8:34 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> bpf_dynptr_get_size returns the number of useable bytes in a dynptr and
> bpf_dynptr_get_offset returns the current offset into the dynptr.
>
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  include/linux/bpf.h      |  2 +-
>  kernel/bpf/helpers.c     | 24 +++++++++++++++++++++---
>  kernel/trace/bpf_trace.c |  4 ++--
>  3 files changed, 24 insertions(+), 6 deletions(-)
>

[...]

> +__bpf_kfunc __u32 bpf_dynptr_get_size(const struct bpf_dynptr_kern *ptr)
> +{
> +       if (!ptr->data)
> +               return -EINVAL;
> +
> +       return __bpf_dynptr_get_size(ptr);
> +}
> +
> +__bpf_kfunc __u32 bpf_dynptr_get_offset(const struct bpf_dynptr_kern *ptr)

I think get_offset is actually not essential and it's hard to think
about the case where this is going to be really necessary. Let's keep
only get_size for now?


> +{
> +       if (!ptr->data)
> +               return -EINVAL;
> +
> +       return ptr->offset;
> +}
> +

[...]

Re: [PATCH v1 bpf-next 3/5] bpf: Add bpf_dynptr_get_size and bpf_dynptr_get_offset

[Joanne Koong]

On Wed, Apr 12, 2023 at 2:52 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Sat, Apr 8, 2023 at 8:34 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > bpf_dynptr_get_size returns the number of useable bytes in a dynptr and
> > bpf_dynptr_get_offset returns the current offset into the dynptr.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > ---
> >  include/linux/bpf.h      |  2 +-
> >  kernel/bpf/helpers.c     | 24 +++++++++++++++++++++---
> >  kernel/trace/bpf_trace.c |  4 ++--
> >  3 files changed, 24 insertions(+), 6 deletions(-)
> >
>
> [...]
>
> > +__bpf_kfunc __u32 bpf_dynptr_get_size(const struct bpf_dynptr_kern *ptr)
> > +{
> > +       if (!ptr->data)
> > +               return -EINVAL;
> > +
> > +       return __bpf_dynptr_get_size(ptr);
> > +}
> > +
> > +__bpf_kfunc __u32 bpf_dynptr_get_offset(const struct bpf_dynptr_kern *ptr)
>
> I think get_offset is actually not essential and it's hard to think
> about the case where this is going to be really necessary. Let's keep
> only get_size for now?
>

Sounds good, I will remove this from v2 (i'll try to send v2 out next
week). Thanks for reviewing this patchset!

>
> > +{
> > +       if (!ptr->data)
> > +               return -EINVAL;
> > +
> > +       return ptr->offset;
> > +}
> > +
>
> [...]

[PATCH v1 bpf-next 4/5] bpf: Add bpf_dynptr_clone

[Joanne Koong]

The cloned dynptr will point to the same data as its parent dynptr,
with the same type, offset, size and read-only properties.

Any writes to a dynptr will be reflected across all instances
(by 'instance', this means any dynptrs that point to the same
underlying data).

Please note that data slice and dynptr invalidations will affect all
instances as well. For example, if bpf_dynptr_write() is called on an
skb-type dynptr, all data slices of dynptr instances to that skb
will be invalidated as well (eg data slices of any clones, parents,
grandparents, ...). Another example is if a ringbuf dynptr is submitted,
any instance of that dynptr will be invalidated.

Changing the view of the dynptr (eg advancing the offset or
trimming the size) will only affect that dynptr and not affect any
other instances.

One example use case where cloning may be helpful is for hashing or
iterating through dynptr data. Cloning will allow the user to maintain
the original view of the dynptr for future use, while also allowing
views to smaller subsets of the data after the offset is advanced or the
size is trimmed.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 kernel/bpf/helpers.c  |  14 +++++
 kernel/bpf/verifier.c | 125 +++++++++++++++++++++++++++++++++++++-----
 2 files changed, 126 insertions(+), 13 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index bac4c6fe49f0..108f3bcfa6da 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2351,6 +2351,19 @@ __bpf_kfunc __u32 bpf_dynptr_get_offset(const struct bpf_dynptr_kern *ptr)
 	return ptr->offset;
 }
 
+__bpf_kfunc int bpf_dynptr_clone(struct bpf_dynptr_kern *ptr,
+				 struct bpf_dynptr_kern *clone__uninit)
+{
+	if (!ptr->data) {
+		bpf_dynptr_set_null(clone__uninit);
+		return -EINVAL;
+	}
+
+	memcpy(clone__uninit, ptr, sizeof(*clone__uninit));
+
+	return 0;
+}
+
 __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
 {
 	return obj;
@@ -2429,6 +2442,7 @@ BTF_ID_FLAGS(func, bpf_dynptr_is_null)
 BTF_ID_FLAGS(func, bpf_dynptr_is_rdonly)
 BTF_ID_FLAGS(func, bpf_dynptr_get_size)
 BTF_ID_FLAGS(func, bpf_dynptr_get_offset)
+BTF_ID_FLAGS(func, bpf_dynptr_clone)
 BTF_SET8_END(common_btf_ids)
 
 static const struct btf_kfunc_id_set common_kfunc_set = {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 3660b573048a..804cb50050f9 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -302,6 +302,7 @@ struct bpf_kfunc_call_arg_meta {
 	struct {
 		enum bpf_dynptr_type type;
 		u32 id;
+		u32 ref_obj_id;
 	} initialized_dynptr;
 	struct {
 		u8 spi;
@@ -963,24 +964,15 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
 	return 0;
 }
 
-static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
+static void invalidate_dynptr(struct bpf_verifier_env *env, struct bpf_func_state *state, int spi)
 {
-	struct bpf_func_state *state = func(env, reg);
-	int spi, i;
-
-	spi = dynptr_get_spi(env, reg);
-	if (spi < 0)
-		return spi;
+	int i;
 
 	for (i = 0; i < BPF_REG_SIZE; i++) {
 		state->stack[spi].slot_type[i] = STACK_INVALID;
 		state->stack[spi - 1].slot_type[i] = STACK_INVALID;
 	}
 
-	/* Invalidate any slices associated with this dynptr */
-	if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type))
-		WARN_ON_ONCE(release_reference(env, state->stack[spi].spilled_ptr.ref_obj_id));
-
 	__mark_reg_not_init(env, &state->stack[spi].spilled_ptr);
 	__mark_reg_not_init(env, &state->stack[spi - 1].spilled_ptr);
 
@@ -1007,6 +999,51 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re
 	 */
 	state->stack[spi].spilled_ptr.live |= REG_LIVE_WRITTEN;
 	state->stack[spi - 1].spilled_ptr.live |= REG_LIVE_WRITTEN;
+}
+
+static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
+{
+	struct bpf_func_state *state = func(env, reg);
+	int spi;
+
+	spi = dynptr_get_spi(env, reg);
+	if (spi < 0)
+		return spi;
+
+	if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type)) {
+		int ref_obj_id = state->stack[spi].spilled_ptr.ref_obj_id;
+		int i;
+
+		/* If the dynptr has a ref_obj_id, then we need to invaldiate
+		 * two things:
+		 *
+		 * 1) Any dynptrs with a matching ref_obj_id (clones)
+		 * 2) Any slices associated with the ref_obj_id
+		 */
+
+		/* Invalidate any slices associated with this dynptr */
+		WARN_ON_ONCE(release_reference(env, ref_obj_id));
+
+		/* Invalidate any dynptr clones */
+		for (i = 1; i < state->allocated_stack / BPF_REG_SIZE; i++) {
+			if (state->stack[i].spilled_ptr.ref_obj_id == ref_obj_id) {
+				/* it should always be the case that if the ref obj id
+				 * matches then the stack slot also belongs to a
+				 * dynptr
+				 */
+				if (state->stack[i].slot_type[0] != STACK_DYNPTR) {
+					verbose(env, "verifier internal error: misconfigured ref_obj_id\n");
+					return -EFAULT;
+				}
+				if (state->stack[i].spilled_ptr.dynptr.first_slot)
+					invalidate_dynptr(env, state, i);
+			}
+		}
+
+		return 0;
+	}
+
+	invalidate_dynptr(env, state, spi);
 
 	return 0;
 }
@@ -6967,6 +7004,50 @@ static int process_iter_next_call(struct bpf_verifier_env *env, int insn_idx,
 	return 0;
 }
 
+static int handle_dynptr_clone(struct bpf_verifier_env *env, enum bpf_arg_type arg_type,
+			       int regno, int insn_idx, struct bpf_kfunc_call_arg_meta *meta)
+{
+	struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
+	struct bpf_reg_state *first_reg_state, *second_reg_state;
+	struct bpf_func_state *state = func(env, reg);
+	enum bpf_dynptr_type dynptr_type = meta->initialized_dynptr.type;
+	int err, spi, ref_obj_id;
+
+	if (!dynptr_type) {
+		verbose(env, "verifier internal error: no dynptr type for bpf_dynptr_clone\n");
+		return -EFAULT;
+	}
+	arg_type |= get_dynptr_type_flag(dynptr_type);
+
+	err = process_dynptr_func(env, regno, insn_idx, arg_type);
+	if (err < 0)
+		return err;
+
+	spi = dynptr_get_spi(env, reg);
+	if (spi < 0)
+		return spi;
+
+	first_reg_state = &state->stack[spi].spilled_ptr;
+	second_reg_state = &state->stack[spi - 1].spilled_ptr;
+	ref_obj_id = first_reg_state->ref_obj_id;
+
+	/* reassign the clone the same dynptr id as the original */
+	__mark_dynptr_reg(first_reg_state, dynptr_type, true, meta->initialized_dynptr.id);
+	__mark_dynptr_reg(second_reg_state, dynptr_type, false, meta->initialized_dynptr.id);
+
+	if (meta->initialized_dynptr.ref_obj_id) {
+		/* release the new ref obj id assigned during process_dynptr_func */
+		err = release_reference_state(cur_func(env), ref_obj_id);
+		if (err)
+			return err;
+		/* reassign the clone the same ref obj id as the original */
+		first_reg_state->ref_obj_id = meta->initialized_dynptr.ref_obj_id;
+		second_reg_state->ref_obj_id = meta->initialized_dynptr.ref_obj_id;
+	}
+
+	return 0;
+}
+
 static bool arg_type_is_mem_size(enum bpf_arg_type type)
 {
 	return type == ARG_CONST_SIZE ||
@@ -9615,6 +9696,7 @@ enum special_kfunc_type {
 	KF_bpf_dynptr_from_xdp,
 	KF_bpf_dynptr_slice,
 	KF_bpf_dynptr_slice_rdwr,
+	KF_bpf_dynptr_clone,
 };
 
 BTF_SET_START(special_kfunc_set)
@@ -9633,6 +9715,7 @@ BTF_ID(func, bpf_dynptr_from_skb)
 BTF_ID(func, bpf_dynptr_from_xdp)
 BTF_ID(func, bpf_dynptr_slice)
 BTF_ID(func, bpf_dynptr_slice_rdwr)
+BTF_ID(func, bpf_dynptr_clone)
 BTF_SET_END(special_kfunc_set)
 
 BTF_ID_LIST(special_kfunc_list)
@@ -9653,6 +9736,7 @@ BTF_ID(func, bpf_dynptr_from_skb)
 BTF_ID(func, bpf_dynptr_from_xdp)
 BTF_ID(func, bpf_dynptr_slice)
 BTF_ID(func, bpf_dynptr_slice_rdwr)
+BTF_ID(func, bpf_dynptr_clone)
 
 static bool is_kfunc_bpf_rcu_read_lock(struct bpf_kfunc_call_arg_meta *meta)
 {
@@ -10414,10 +10498,24 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
 			if (is_kfunc_arg_uninit(btf, &args[i]))
 				dynptr_arg_type |= MEM_UNINIT;
 
-			if (meta->func_id == special_kfunc_list[KF_bpf_dynptr_from_skb])
+			if (meta->func_id == special_kfunc_list[KF_bpf_dynptr_from_skb]) {
 				dynptr_arg_type |= DYNPTR_TYPE_SKB;
-			else if (meta->func_id == special_kfunc_list[KF_bpf_dynptr_from_xdp])
+			} else if (meta->func_id == special_kfunc_list[KF_bpf_dynptr_from_xdp]) {
 				dynptr_arg_type |= DYNPTR_TYPE_XDP;
+			} else if (meta->func_id == special_kfunc_list[KF_bpf_dynptr_clone] &&
+				   (dynptr_arg_type & MEM_UNINIT)) {
+				/* bpf_dynptr_clone is special.
+				 *
+				 * we need to assign the clone the same dynptr type and
+				 * the clone needs to have the same id and ref_obj_id as
+				 * the original dynptr
+				 */
+				ret = handle_dynptr_clone(env, dynptr_arg_type, regno, insn_idx, meta);
+				if (ret < 0)
+					return ret;
+
+				break;
+			}
 
 			ret = process_dynptr_func(env, regno, insn_idx, dynptr_arg_type);
 			if (ret < 0)
@@ -10432,6 +10530,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
 				}
 				meta->initialized_dynptr.id = id;
 				meta->initialized_dynptr.type = dynptr_get_type(env, reg);
+				meta->initialized_dynptr.ref_obj_id = dynptr_ref_obj_id(env, reg);
 			}
 
 			break;
-- 
2.34.1

Re: [PATCH v1 bpf-next 4/5] bpf: Add bpf_dynptr_clone

[Andrii Nakryiko]

On Sat, Apr 8, 2023 at 8:34 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> The cloned dynptr will point to the same data as its parent dynptr,
> with the same type, offset, size and read-only properties.
>
> Any writes to a dynptr will be reflected across all instances
> (by 'instance', this means any dynptrs that point to the same
> underlying data).
>
> Please note that data slice and dynptr invalidations will affect all
> instances as well. For example, if bpf_dynptr_write() is called on an
> skb-type dynptr, all data slices of dynptr instances to that skb
> will be invalidated as well (eg data slices of any clones, parents,
> grandparents, ...). Another example is if a ringbuf dynptr is submitted,
> any instance of that dynptr will be invalidated.
>
> Changing the view of the dynptr (eg advancing the offset or
> trimming the size) will only affect that dynptr and not affect any
> other instances.
>
> One example use case where cloning may be helpful is for hashing or
> iterating through dynptr data. Cloning will allow the user to maintain
> the original view of the dynptr for future use, while also allowing
> views to smaller subsets of the data after the offset is advanced or the
> size is trimmed.
>
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  kernel/bpf/helpers.c  |  14 +++++
>  kernel/bpf/verifier.c | 125 +++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 126 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index bac4c6fe49f0..108f3bcfa6da 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2351,6 +2351,19 @@ __bpf_kfunc __u32 bpf_dynptr_get_offset(const struct bpf_dynptr_kern *ptr)
>         return ptr->offset;
>  }
>
> +__bpf_kfunc int bpf_dynptr_clone(struct bpf_dynptr_kern *ptr,
> +                                struct bpf_dynptr_kern *clone__uninit)

I think most of uses for bpf_dynptr_clone() would be to get a partial
view (like you mentioned above, to, e.g., do a hashing of a part of
the memory range). So it feels it would be best UX if clone would
allow you to define a new range in one go. So e.g., to create a
"sub-dynptr" for range of bytes [10, 30), it should be enough to:

struct bpf_dynptr orig_ptr, new_ptr;

bpf_dynptr_clone(&orig_ptr, 10, 30, &new_ptr);

Instead of:

bpf_dynptr_clone(&orig_ptr, &new_ptr);
bpf_dynptr_advance(&new_ptr, 10);
bpf_dynptr_trim(&new_ptr, bpf_dynptr_get_size(&orig_ptr) - 30);


This, btw, shows the awkwardness of the bpf_dynptr_trim() approach.

If someone really wanted an exact full-sized copy, it's trivial:

bpf_dynptr_clone(&orig_ptr, 0, bpf_dynptr_get_size(&orig_ptr), &new_ptr);


BTW, let's rename bpf_dynptr_get_size -> bpf_dynptr_size()? That
"get_" is a sore in the eye, IMO.


> +{
> +       if (!ptr->data) {
> +               bpf_dynptr_set_null(clone__uninit);
> +               return -EINVAL;
> +       }
> +
> +       memcpy(clone__uninit, ptr, sizeof(*clone__uninit));

why memcpy instead of `*clone__uninit = *ptr`?

> +
> +       return 0;
> +}
> +
>  __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
>  {
>         return obj;
> @@ -2429,6 +2442,7 @@ BTF_ID_FLAGS(func, bpf_dynptr_is_null)
>  BTF_ID_FLAGS(func, bpf_dynptr_is_rdonly)
>  BTF_ID_FLAGS(func, bpf_dynptr_get_size)
>  BTF_ID_FLAGS(func, bpf_dynptr_get_offset)
> +BTF_ID_FLAGS(func, bpf_dynptr_clone)
>  BTF_SET8_END(common_btf_ids)
>
>  static const struct btf_kfunc_id_set common_kfunc_set = {
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 3660b573048a..804cb50050f9 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -302,6 +302,7 @@ struct bpf_kfunc_call_arg_meta {
>         struct {
>                 enum bpf_dynptr_type type;
>                 u32 id;
> +               u32 ref_obj_id;
>         } initialized_dynptr;
>         struct {
>                 u8 spi;
> @@ -963,24 +964,15 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
>         return 0;
>  }
>
> -static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> +static void invalidate_dynptr(struct bpf_verifier_env *env, struct bpf_func_state *state, int spi)
>  {
> -       struct bpf_func_state *state = func(env, reg);
> -       int spi, i;
> -
> -       spi = dynptr_get_spi(env, reg);
> -       if (spi < 0)
> -               return spi;
> +       int i;
>
>         for (i = 0; i < BPF_REG_SIZE; i++) {
>                 state->stack[spi].slot_type[i] = STACK_INVALID;
>                 state->stack[spi - 1].slot_type[i] = STACK_INVALID;
>         }
>
> -       /* Invalidate any slices associated with this dynptr */
> -       if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type))
> -               WARN_ON_ONCE(release_reference(env, state->stack[spi].spilled_ptr.ref_obj_id));
> -
>         __mark_reg_not_init(env, &state->stack[spi].spilled_ptr);
>         __mark_reg_not_init(env, &state->stack[spi - 1].spilled_ptr);
>
> @@ -1007,6 +999,51 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re
>          */
>         state->stack[spi].spilled_ptr.live |= REG_LIVE_WRITTEN;
>         state->stack[spi - 1].spilled_ptr.live |= REG_LIVE_WRITTEN;
> +}
> +
> +static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> +{
> +       struct bpf_func_state *state = func(env, reg);
> +       int spi;
> +
> +       spi = dynptr_get_spi(env, reg);
> +       if (spi < 0)
> +               return spi;
> +
> +       if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type)) {
> +               int ref_obj_id = state->stack[spi].spilled_ptr.ref_obj_id;
> +               int i;
> +
> +               /* If the dynptr has a ref_obj_id, then we need to invaldiate

typo: invalidate

> +                * two things:
> +                *
> +                * 1) Any dynptrs with a matching ref_obj_id (clones)
> +                * 2) Any slices associated with the ref_obj_id

I think this is where this dynptr_id confusion comes from. The rule
should be "any slices derived from this dynptr". But as mentioned on
another thread, it's a separate topic which we can address later.

> +                */
> +
> +               /* Invalidate any slices associated with this dynptr */
> +               WARN_ON_ONCE(release_reference(env, ref_obj_id));
> +
> +               /* Invalidate any dynptr clones */
> +               for (i = 1; i < state->allocated_stack / BPF_REG_SIZE; i++) {
> +                       if (state->stack[i].spilled_ptr.ref_obj_id == ref_obj_id) {

nit: invert if condition and continue to reduce nestedness of the rest
the loop body

> +                               /* it should always be the case that if the ref obj id
> +                                * matches then the stack slot also belongs to a
> +                                * dynptr
> +                                */
> +                               if (state->stack[i].slot_type[0] != STACK_DYNPTR) {
> +                                       verbose(env, "verifier internal error: misconfigured ref_obj_id\n");
> +                                       return -EFAULT;
> +                               }
> +                               if (state->stack[i].spilled_ptr.dynptr.first_slot)
> +                                       invalidate_dynptr(env, state, i);
> +                       }
> +               }
> +
> +               return 0;
> +       }
> +
> +       invalidate_dynptr(env, state, spi);
>
>         return 0;
>  }
> @@ -6967,6 +7004,50 @@ static int process_iter_next_call(struct bpf_verifier_env *env, int insn_idx,
>         return 0;
>  }
>
> +static int handle_dynptr_clone(struct bpf_verifier_env *env, enum bpf_arg_type arg_type,
> +                              int regno, int insn_idx, struct bpf_kfunc_call_arg_meta *meta)
> +{
> +       struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
> +       struct bpf_reg_state *first_reg_state, *second_reg_state;
> +       struct bpf_func_state *state = func(env, reg);
> +       enum bpf_dynptr_type dynptr_type = meta->initialized_dynptr.type;
> +       int err, spi, ref_obj_id;
> +
> +       if (!dynptr_type) {
> +               verbose(env, "verifier internal error: no dynptr type for bpf_dynptr_clone\n");
> +               return -EFAULT;
> +       }
> +       arg_type |= get_dynptr_type_flag(dynptr_type);


what about MEM_RDONLY and MEM_UNINIT flags, do we need to derive and add them?

> +
> +       err = process_dynptr_func(env, regno, insn_idx, arg_type);
> +       if (err < 0)
> +               return err;
> +
> +       spi = dynptr_get_spi(env, reg);
> +       if (spi < 0)
> +               return spi;
> +
> +       first_reg_state = &state->stack[spi].spilled_ptr;
> +       second_reg_state = &state->stack[spi - 1].spilled_ptr;
> +       ref_obj_id = first_reg_state->ref_obj_id;
> +
> +       /* reassign the clone the same dynptr id as the original */
> +       __mark_dynptr_reg(first_reg_state, dynptr_type, true, meta->initialized_dynptr.id);
> +       __mark_dynptr_reg(second_reg_state, dynptr_type, false, meta->initialized_dynptr.id);

I'm not sure why clone should have the same dynptr_id? Isn't it a new
instance of a dynptr? I get preserving ref_obj_id (if refcounted), but
why reusing dynptr_id?..


> +
> +       if (meta->initialized_dynptr.ref_obj_id) {
> +               /* release the new ref obj id assigned during process_dynptr_func */
> +               err = release_reference_state(cur_func(env), ref_obj_id);
> +               if (err)
> +                       return err;

ugh... this is not good to create reference just to release. If we
need to reuse logic, let's reuse the logic without parts that
shouldn't happen. Please check if we can split process_dynptr_func()
appropriately to allow this.

> +               /* reassign the clone the same ref obj id as the original */
> +               first_reg_state->ref_obj_id = meta->initialized_dynptr.ref_obj_id;
> +               second_reg_state->ref_obj_id = meta->initialized_dynptr.ref_obj_id;
> +       }
> +
> +       return 0;
> +}
> +

[...]

Re: [PATCH v1 bpf-next 4/5] bpf: Add bpf_dynptr_clone

[Joanne Koong]

On Wed, Apr 12, 2023 at 3:12 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Sat, Apr 8, 2023 at 8:34 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > The cloned dynptr will point to the same data as its parent dynptr,
> > with the same type, offset, size and read-only properties.
> >
> > Any writes to a dynptr will be reflected across all instances
> > (by 'instance', this means any dynptrs that point to the same
> > underlying data).
> >
> > Please note that data slice and dynptr invalidations will affect all
> > instances as well. For example, if bpf_dynptr_write() is called on an
> > skb-type dynptr, all data slices of dynptr instances to that skb
> > will be invalidated as well (eg data slices of any clones, parents,
> > grandparents, ...). Another example is if a ringbuf dynptr is submitted,
> > any instance of that dynptr will be invalidated.
> >
> > Changing the view of the dynptr (eg advancing the offset or
> > trimming the size) will only affect that dynptr and not affect any
> > other instances.
> >
> > One example use case where cloning may be helpful is for hashing or
> > iterating through dynptr data. Cloning will allow the user to maintain
> > the original view of the dynptr for future use, while also allowing
> > views to smaller subsets of the data after the offset is advanced or the
> > size is trimmed.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > ---
> >  kernel/bpf/helpers.c  |  14 +++++
> >  kernel/bpf/verifier.c | 125 +++++++++++++++++++++++++++++++++++++-----
> >  2 files changed, 126 insertions(+), 13 deletions(-)
> >
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index bac4c6fe49f0..108f3bcfa6da 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -2351,6 +2351,19 @@ __bpf_kfunc __u32 bpf_dynptr_get_offset(const struct bpf_dynptr_kern *ptr)
> >         return ptr->offset;
> >  }
> >
> > +__bpf_kfunc int bpf_dynptr_clone(struct bpf_dynptr_kern *ptr,
> > +                                struct bpf_dynptr_kern *clone__uninit)
>
> I think most of uses for bpf_dynptr_clone() would be to get a partial
> view (like you mentioned above, to, e.g., do a hashing of a part of
> the memory range). So it feels it would be best UX if clone would
> allow you to define a new range in one go. So e.g., to create a
> "sub-dynptr" for range of bytes [10, 30), it should be enough to:
>
> struct bpf_dynptr orig_ptr, new_ptr;
>
> bpf_dynptr_clone(&orig_ptr, 10, 30, &new_ptr);
>
> Instead of:
>
> bpf_dynptr_clone(&orig_ptr, &new_ptr);
> bpf_dynptr_advance(&new_ptr, 10);
> bpf_dynptr_trim(&new_ptr, bpf_dynptr_get_size(&orig_ptr) - 30);
>

I don't think we can do this because we can't have bpf_dynptr_clone()
fail (which might happen if we take in a range, if the range is
invalid). This is because in the case where the clone is of a
reference-counted dynptr (eg like a ringbuf-type dynptr), the clone
even if it's invalid must be treated by the verifier as a legit dynptr
(since the verifier can't know ahead of time whether the clone call
will succeed or not) which means if the invalid clone dynptr is then
passed into a reference-releasing function, the verifier will release
the reference but the actual reference won't be released at runtime
(since the clone dynptr is invalid), which would leak the reference.
An example is something like:

 // invalid range is passed, error is returned and new_ptr is invalid
bpf_dynptr_clone(&ringbuf_ptr, 9999999, 9999999, &new_ptr);
// this releases the reference and invalidates both new_ptr and ringbuf_ptr
bpf_ringbuf_discard_dynptr(&new_ptr, 0);

At runtime, bpf_ringbuf_discard_dynptr() will be a no-op since new_ptr
is invalid - the ringbuf record still needs to be submitted/discarded,
but the verifier will think this already happened

>
> This, btw, shows the awkwardness of the bpf_dynptr_trim() approach.
>
> If someone really wanted an exact full-sized copy, it's trivial:
>
> bpf_dynptr_clone(&orig_ptr, 0, bpf_dynptr_get_size(&orig_ptr), &new_ptr);
>
>
> BTW, let's rename bpf_dynptr_get_size -> bpf_dynptr_size()? That
> "get_" is a sore in the eye, IMO.

Will do!
>
>
> > +{
> > +       if (!ptr->data) {
> > +               bpf_dynptr_set_null(clone__uninit);
> > +               return -EINVAL;
> > +       }
> > +
> > +       memcpy(clone__uninit, ptr, sizeof(*clone__uninit));
>
> why memcpy instead of `*clone__uninit = *ptr`?
>
No good reason :) I'll change this for v2
> > +
> > +       return 0;
> > +}
> > +
> >  __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
> >  {
> >         return obj;
> > @@ -2429,6 +2442,7 @@ BTF_ID_FLAGS(func, bpf_dynptr_is_null)
> >  BTF_ID_FLAGS(func, bpf_dynptr_is_rdonly)
> >  BTF_ID_FLAGS(func, bpf_dynptr_get_size)
> >  BTF_ID_FLAGS(func, bpf_dynptr_get_offset)
> > +BTF_ID_FLAGS(func, bpf_dynptr_clone)
> >  BTF_SET8_END(common_btf_ids)
> >
> >  static const struct btf_kfunc_id_set common_kfunc_set = {
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 3660b573048a..804cb50050f9 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -302,6 +302,7 @@ struct bpf_kfunc_call_arg_meta {
> >         struct {
> >                 enum bpf_dynptr_type type;
> >                 u32 id;
> > +               u32 ref_obj_id;
> >         } initialized_dynptr;
> >         struct {
> >                 u8 spi;
> > @@ -963,24 +964,15 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
> >         return 0;
> >  }
> >
> > -static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> > +static void invalidate_dynptr(struct bpf_verifier_env *env, struct bpf_func_state *state, int spi)
> >  {
> > -       struct bpf_func_state *state = func(env, reg);
> > -       int spi, i;
> > -
> > -       spi = dynptr_get_spi(env, reg);
> > -       if (spi < 0)
> > -               return spi;
> > +       int i;
> >
> >         for (i = 0; i < BPF_REG_SIZE; i++) {
> >                 state->stack[spi].slot_type[i] = STACK_INVALID;
> >                 state->stack[spi - 1].slot_type[i] = STACK_INVALID;
> >         }
> >
> > -       /* Invalidate any slices associated with this dynptr */
> > -       if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type))
> > -               WARN_ON_ONCE(release_reference(env, state->stack[spi].spilled_ptr.ref_obj_id));
> > -
> >         __mark_reg_not_init(env, &state->stack[spi].spilled_ptr);
> >         __mark_reg_not_init(env, &state->stack[spi - 1].spilled_ptr);
> >
> > @@ -1007,6 +999,51 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re
> >          */
> >         state->stack[spi].spilled_ptr.live |= REG_LIVE_WRITTEN;
> >         state->stack[spi - 1].spilled_ptr.live |= REG_LIVE_WRITTEN;
> > +}
> > +
> > +static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> > +{
> > +       struct bpf_func_state *state = func(env, reg);
> > +       int spi;
> > +
> > +       spi = dynptr_get_spi(env, reg);
> > +       if (spi < 0)
> > +               return spi;
> > +
> > +       if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type)) {
> > +               int ref_obj_id = state->stack[spi].spilled_ptr.ref_obj_id;
> > +               int i;
> > +
> > +               /* If the dynptr has a ref_obj_id, then we need to invaldiate
>
> typo: invalidate
>
> > +                * two things:
> > +                *
> > +                * 1) Any dynptrs with a matching ref_obj_id (clones)
> > +                * 2) Any slices associated with the ref_obj_id
>
> I think this is where this dynptr_id confusion comes from. The rule
> should be "any slices derived from this dynptr". But as mentioned on
> another thread, it's a separate topic which we can address later.
>
If there's a parent and a clone and slices derived from the parent and
slices derived from the clone, if the clone is invalidated then we
need to invalidate slices associated with the parent as well. So
shouldn't it be "any slices associated with the ref obj id" not "any
slices derived from this dynptr"? (also just a note, parent/clone
slices will share the same ref obj id and the same dynptr id, so
checking against either does the same thing)

> > +                */
> > +
> > +               /* Invalidate any slices associated with this dynptr */
> > +               WARN_ON_ONCE(release_reference(env, ref_obj_id));
> > +
> > +               /* Invalidate any dynptr clones */
> > +               for (i = 1; i < state->allocated_stack / BPF_REG_SIZE; i++) {
> > +                       if (state->stack[i].spilled_ptr.ref_obj_id == ref_obj_id) {
>
> nit: invert if condition and continue to reduce nestedness of the rest
> the loop body

Ooh good idea
>
> > +                               /* it should always be the case that if the ref obj id
> > +                                * matches then the stack slot also belongs to a
> > +                                * dynptr
> > +                                */
> > +                               if (state->stack[i].slot_type[0] != STACK_DYNPTR) {
> > +                                       verbose(env, "verifier internal error: misconfigured ref_obj_id\n");
> > +                                       return -EFAULT;
> > +                               }
> > +                               if (state->stack[i].spilled_ptr.dynptr.first_slot)
> > +                                       invalidate_dynptr(env, state, i);
> > +                       }
> > +               }
> > +
> > +               return 0;
> > +       }
> > +
> > +       invalidate_dynptr(env, state, spi);
> >
> >         return 0;
> >  }
> > @@ -6967,6 +7004,50 @@ static int process_iter_next_call(struct bpf_verifier_env *env, int insn_idx,
> >         return 0;
> >  }
> >
> > +static int handle_dynptr_clone(struct bpf_verifier_env *env, enum bpf_arg_type arg_type,
> > +                              int regno, int insn_idx, struct bpf_kfunc_call_arg_meta *meta)
> > +{
> > +       struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
> > +       struct bpf_reg_state *first_reg_state, *second_reg_state;
> > +       struct bpf_func_state *state = func(env, reg);
> > +       enum bpf_dynptr_type dynptr_type = meta->initialized_dynptr.type;
> > +       int err, spi, ref_obj_id;
> > +
> > +       if (!dynptr_type) {
> > +               verbose(env, "verifier internal error: no dynptr type for bpf_dynptr_clone\n");
> > +               return -EFAULT;
> > +       }
> > +       arg_type |= get_dynptr_type_flag(dynptr_type);
>
>
> what about MEM_RDONLY and MEM_UNINIT flags, do we need to derive and add them?

I don't think we need MEM_UNINIT because we can't clone an
uninitialized dynptr. But yes, definitely MEM_RDONLY. I missed that, I
will add it in in v2

>
> > +
> > +       err = process_dynptr_func(env, regno, insn_idx, arg_type);
> > +       if (err < 0)
> > +               return err;
> > +
> > +       spi = dynptr_get_spi(env, reg);
> > +       if (spi < 0)
> > +               return spi;
> > +
> > +       first_reg_state = &state->stack[spi].spilled_ptr;
> > +       second_reg_state = &state->stack[spi - 1].spilled_ptr;
> > +       ref_obj_id = first_reg_state->ref_obj_id;
> > +
> > +       /* reassign the clone the same dynptr id as the original */
> > +       __mark_dynptr_reg(first_reg_state, dynptr_type, true, meta->initialized_dynptr.id);
> > +       __mark_dynptr_reg(second_reg_state, dynptr_type, false, meta->initialized_dynptr.id);
>
> I'm not sure why clone should have the same dynptr_id? Isn't it a new
> instance of a dynptr? I get preserving ref_obj_id (if refcounted), but
> why reusing dynptr_id?..
>
I think we need to also copy over the dynptr id because in the case of
a non-reference counted dynptr, if the parent (or clone) is
invalidated (eg overwriting bytes of the dynptr on the stack), we must
also invalidate the slices of the clone (or parent)
>
> > +
> > +       if (meta->initialized_dynptr.ref_obj_id) {
> > +               /* release the new ref obj id assigned during process_dynptr_func */
> > +               err = release_reference_state(cur_func(env), ref_obj_id);
> > +               if (err)
> > +                       return err;
>
> ugh... this is not good to create reference just to release. If we
> need to reuse logic, let's reuse the logic without parts that
> shouldn't happen. Please check if we can split process_dynptr_func()
> appropriately to allow this.

I'll change this for v2. I think the simplest approach would be having
mark_stack_slots_dynptr() take in a boolean or something that'll
indicate whether it should acquire a new reference state or not
>
> > +               /* reassign the clone the same ref obj id as the original */
> > +               first_reg_state->ref_obj_id = meta->initialized_dynptr.ref_obj_id;
> > +               second_reg_state->ref_obj_id = meta->initialized_dynptr.ref_obj_id;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
>
> [...]

Re: [PATCH v1 bpf-next 4/5] bpf: Add bpf_dynptr_clone

[Andrii Nakryiko]

On Thu, Apr 13, 2023 at 11:03 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Wed, Apr 12, 2023 at 3:12 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Sat, Apr 8, 2023 at 8:34 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> > >
> > > The cloned dynptr will point to the same data as its parent dynptr,
> > > with the same type, offset, size and read-only properties.
> > >
> > > Any writes to a dynptr will be reflected across all instances
> > > (by 'instance', this means any dynptrs that point to the same
> > > underlying data).
> > >
> > > Please note that data slice and dynptr invalidations will affect all
> > > instances as well. For example, if bpf_dynptr_write() is called on an
> > > skb-type dynptr, all data slices of dynptr instances to that skb
> > > will be invalidated as well (eg data slices of any clones, parents,
> > > grandparents, ...). Another example is if a ringbuf dynptr is submitted,
> > > any instance of that dynptr will be invalidated.
> > >
> > > Changing the view of the dynptr (eg advancing the offset or
> > > trimming the size) will only affect that dynptr and not affect any
> > > other instances.
> > >
> > > One example use case where cloning may be helpful is for hashing or
> > > iterating through dynptr data. Cloning will allow the user to maintain
> > > the original view of the dynptr for future use, while also allowing
> > > views to smaller subsets of the data after the offset is advanced or the
> > > size is trimmed.
> > >
> > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > ---
> > >  kernel/bpf/helpers.c  |  14 +++++
> > >  kernel/bpf/verifier.c | 125 +++++++++++++++++++++++++++++++++++++-----
> > >  2 files changed, 126 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > > index bac4c6fe49f0..108f3bcfa6da 100644
> > > --- a/kernel/bpf/helpers.c
> > > +++ b/kernel/bpf/helpers.c
> > > @@ -2351,6 +2351,19 @@ __bpf_kfunc __u32 bpf_dynptr_get_offset(const struct bpf_dynptr_kern *ptr)
> > >         return ptr->offset;
> > >  }
> > >
> > > +__bpf_kfunc int bpf_dynptr_clone(struct bpf_dynptr_kern *ptr,
> > > +                                struct bpf_dynptr_kern *clone__uninit)
> >
> > I think most of uses for bpf_dynptr_clone() would be to get a partial
> > view (like you mentioned above, to, e.g., do a hashing of a part of
> > the memory range). So it feels it would be best UX if clone would
> > allow you to define a new range in one go. So e.g., to create a
> > "sub-dynptr" for range of bytes [10, 30), it should be enough to:
> >
> > struct bpf_dynptr orig_ptr, new_ptr;
> >
> > bpf_dynptr_clone(&orig_ptr, 10, 30, &new_ptr);
> >
> > Instead of:
> >
> > bpf_dynptr_clone(&orig_ptr, &new_ptr);
> > bpf_dynptr_advance(&new_ptr, 10);
> > bpf_dynptr_trim(&new_ptr, bpf_dynptr_get_size(&orig_ptr) - 30);
> >
>
> I don't think we can do this because we can't have bpf_dynptr_clone()
> fail (which might happen if we take in a range, if the range is
> invalid). This is because in the case where the clone is of a
> reference-counted dynptr (eg like a ringbuf-type dynptr), the clone
> even if it's invalid must be treated by the verifier as a legit dynptr
> (since the verifier can't know ahead of time whether the clone call
> will succeed or not) which means if the invalid clone dynptr is then
> passed into a reference-releasing function, the verifier will release
> the reference but the actual reference won't be released at runtime
> (since the clone dynptr is invalid), which would leak the reference.
> An example is something like:
>
>  // invalid range is passed, error is returned and new_ptr is invalid
> bpf_dynptr_clone(&ringbuf_ptr, 9999999, 9999999, &new_ptr);
> // this releases the reference and invalidates both new_ptr and ringbuf_ptr
> bpf_ringbuf_discard_dynptr(&new_ptr, 0);
>
> At runtime, bpf_ringbuf_discard_dynptr() will be a no-op since new_ptr
> is invalid - the ringbuf record still needs to be submitted/discarded,
> but the verifier will think this already happened

Ah, tricky, good point. But ok, I guess with bpf_dynptr_adjust()
proposal in another email this would be ok:

bpf_dynptr_clone(..); /* always succeeds */
bpf_dynptr_adjust(&new_ptr, 10, 30); /* could fail to adjust, but
dynptr is left valid */

Right?

>
> >
> > This, btw, shows the awkwardness of the bpf_dynptr_trim() approach.
> >
> > If someone really wanted an exact full-sized copy, it's trivial:
> >
> > bpf_dynptr_clone(&orig_ptr, 0, bpf_dynptr_get_size(&orig_ptr), &new_ptr);
> >
> >
> > BTW, let's rename bpf_dynptr_get_size -> bpf_dynptr_size()? That
> > "get_" is a sore in the eye, IMO.
>
> Will do!
> >
> >
> > > +{
> > > +       if (!ptr->data) {
> > > +               bpf_dynptr_set_null(clone__uninit);
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       memcpy(clone__uninit, ptr, sizeof(*clone__uninit));
> >
> > why memcpy instead of `*clone__uninit = *ptr`?
> >
> No good reason :) I'll change this for v2
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
> > >  {
> > >         return obj;
> > > @@ -2429,6 +2442,7 @@ BTF_ID_FLAGS(func, bpf_dynptr_is_null)
> > >  BTF_ID_FLAGS(func, bpf_dynptr_is_rdonly)
> > >  BTF_ID_FLAGS(func, bpf_dynptr_get_size)
> > >  BTF_ID_FLAGS(func, bpf_dynptr_get_offset)
> > > +BTF_ID_FLAGS(func, bpf_dynptr_clone)
> > >  BTF_SET8_END(common_btf_ids)
> > >
> > >  static const struct btf_kfunc_id_set common_kfunc_set = {
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 3660b573048a..804cb50050f9 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -302,6 +302,7 @@ struct bpf_kfunc_call_arg_meta {
> > >         struct {
> > >                 enum bpf_dynptr_type type;
> > >                 u32 id;
> > > +               u32 ref_obj_id;
> > >         } initialized_dynptr;
> > >         struct {
> > >                 u8 spi;
> > > @@ -963,24 +964,15 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
> > >         return 0;
> > >  }
> > >
> > > -static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> > > +static void invalidate_dynptr(struct bpf_verifier_env *env, struct bpf_func_state *state, int spi)
> > >  {
> > > -       struct bpf_func_state *state = func(env, reg);
> > > -       int spi, i;
> > > -
> > > -       spi = dynptr_get_spi(env, reg);
> > > -       if (spi < 0)
> > > -               return spi;
> > > +       int i;
> > >
> > >         for (i = 0; i < BPF_REG_SIZE; i++) {
> > >                 state->stack[spi].slot_type[i] = STACK_INVALID;
> > >                 state->stack[spi - 1].slot_type[i] = STACK_INVALID;
> > >         }
> > >
> > > -       /* Invalidate any slices associated with this dynptr */
> > > -       if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type))
> > > -               WARN_ON_ONCE(release_reference(env, state->stack[spi].spilled_ptr.ref_obj_id));
> > > -
> > >         __mark_reg_not_init(env, &state->stack[spi].spilled_ptr);
> > >         __mark_reg_not_init(env, &state->stack[spi - 1].spilled_ptr);
> > >
> > > @@ -1007,6 +999,51 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re
> > >          */
> > >         state->stack[spi].spilled_ptr.live |= REG_LIVE_WRITTEN;
> > >         state->stack[spi - 1].spilled_ptr.live |= REG_LIVE_WRITTEN;
> > > +}
> > > +
> > > +static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> > > +{
> > > +       struct bpf_func_state *state = func(env, reg);
> > > +       int spi;
> > > +
> > > +       spi = dynptr_get_spi(env, reg);
> > > +       if (spi < 0)
> > > +               return spi;
> > > +
> > > +       if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type)) {
> > > +               int ref_obj_id = state->stack[spi].spilled_ptr.ref_obj_id;
> > > +               int i;
> > > +
> > > +               /* If the dynptr has a ref_obj_id, then we need to invaldiate
> >
> > typo: invalidate
> >
> > > +                * two things:
> > > +                *
> > > +                * 1) Any dynptrs with a matching ref_obj_id (clones)
> > > +                * 2) Any slices associated with the ref_obj_id
> >
> > I think this is where this dynptr_id confusion comes from. The rule
> > should be "any slices derived from this dynptr". But as mentioned on
> > another thread, it's a separate topic which we can address later.
> >
> If there's a parent and a clone and slices derived from the parent and
> slices derived from the clone, if the clone is invalidated then we
> need to invalidate slices associated with the parent as well. So
> shouldn't it be "any slices associated with the ref obj id" not "any
> slices derived from this dynptr"? (also just a note, parent/clone
> slices will share the same ref obj id and the same dynptr id, so
> checking against either does the same thing)

So, we have a ringbuf dynptr with ref_obj_id=1, id=2, ok? We clone it,
clone gets ref_obj_id=1, id=3. If either original dynptr or clone
dynptr is released due to bpf_ringbuf_discard_dynptr(), we invalidate
all the dynptrs with ref_obj_id=1. During invalidation of each dynptr,
we invalidate all the slices with ref_obj_id==dynptr's id. So we'll
invalidate slices derived from dynptr with id=2 (original dynptr), and
then all the slices derived from dynptr with id=3?

>
> > > +                */
> > > +
> > > +               /* Invalidate any slices associated with this dynptr */
> > > +               WARN_ON_ONCE(release_reference(env, ref_obj_id));
> > > +
> > > +               /* Invalidate any dynptr clones */
> > > +               for (i = 1; i < state->allocated_stack / BPF_REG_SIZE; i++) {
> > > +                       if (state->stack[i].spilled_ptr.ref_obj_id == ref_obj_id) {
> >
> > nit: invert if condition and continue to reduce nestedness of the rest
> > the loop body
>
> Ooh good idea
> >
> > > +                               /* it should always be the case that if the ref obj id
> > > +                                * matches then the stack slot also belongs to a
> > > +                                * dynptr
> > > +                                */
> > > +                               if (state->stack[i].slot_type[0] != STACK_DYNPTR) {
> > > +                                       verbose(env, "verifier internal error: misconfigured ref_obj_id\n");
> > > +                                       return -EFAULT;
> > > +                               }
> > > +                               if (state->stack[i].spilled_ptr.dynptr.first_slot)
> > > +                                       invalidate_dynptr(env, state, i);
> > > +                       }
> > > +               }
> > > +
> > > +               return 0;
> > > +       }
> > > +
> > > +       invalidate_dynptr(env, state, spi);
> > >
> > >         return 0;
> > >  }
> > > @@ -6967,6 +7004,50 @@ static int process_iter_next_call(struct bpf_verifier_env *env, int insn_idx,
> > >         return 0;
> > >  }
> > >
> > > +static int handle_dynptr_clone(struct bpf_verifier_env *env, enum bpf_arg_type arg_type,
> > > +                              int regno, int insn_idx, struct bpf_kfunc_call_arg_meta *meta)
> > > +{
> > > +       struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
> > > +       struct bpf_reg_state *first_reg_state, *second_reg_state;
> > > +       struct bpf_func_state *state = func(env, reg);
> > > +       enum bpf_dynptr_type dynptr_type = meta->initialized_dynptr.type;
> > > +       int err, spi, ref_obj_id;
> > > +
> > > +       if (!dynptr_type) {
> > > +               verbose(env, "verifier internal error: no dynptr type for bpf_dynptr_clone\n");
> > > +               return -EFAULT;
> > > +       }
> > > +       arg_type |= get_dynptr_type_flag(dynptr_type);
> >
> >
> > what about MEM_RDONLY and MEM_UNINIT flags, do we need to derive and add them?
>
> I don't think we need MEM_UNINIT because we can't clone an
> uninitialized dynptr. But yes, definitely MEM_RDONLY. I missed that, I
> will add it in in v2
>
> >
> > > +
> > > +       err = process_dynptr_func(env, regno, insn_idx, arg_type);
> > > +       if (err < 0)
> > > +               return err;
> > > +
> > > +       spi = dynptr_get_spi(env, reg);
> > > +       if (spi < 0)
> > > +               return spi;
> > > +
> > > +       first_reg_state = &state->stack[spi].spilled_ptr;
> > > +       second_reg_state = &state->stack[spi - 1].spilled_ptr;
> > > +       ref_obj_id = first_reg_state->ref_obj_id;
> > > +
> > > +       /* reassign the clone the same dynptr id as the original */
> > > +       __mark_dynptr_reg(first_reg_state, dynptr_type, true, meta->initialized_dynptr.id);
> > > +       __mark_dynptr_reg(second_reg_state, dynptr_type, false, meta->initialized_dynptr.id);
> >
> > I'm not sure why clone should have the same dynptr_id? Isn't it a new
> > instance of a dynptr? I get preserving ref_obj_id (if refcounted), but
> > why reusing dynptr_id?..
> >
> I think we need to also copy over the dynptr id because in the case of
> a non-reference counted dynptr, if the parent (or clone) is
> invalidated (eg overwriting bytes of the dynptr on the stack), we must
> also invalidate the slices of the clone (or parent)

yep, right now we'll have to do that because we have dynptr_id. But if
we get rid of it and stick to ref_obj_id and id, then clone would need
to get a new id, but keep ref_obj_id, right?

> >
> > > +
> > > +       if (meta->initialized_dynptr.ref_obj_id) {
> > > +               /* release the new ref obj id assigned during process_dynptr_func */
> > > +               err = release_reference_state(cur_func(env), ref_obj_id);
> > > +               if (err)
> > > +                       return err;
> >
> > ugh... this is not good to create reference just to release. If we
> > need to reuse logic, let's reuse the logic without parts that
> > shouldn't happen. Please check if we can split process_dynptr_func()
> > appropriately to allow this.
>
> I'll change this for v2. I think the simplest approach would be having
> mark_stack_slots_dynptr() take in a boolean or something that'll
> indicate whether it should acquire a new reference state or not
> >
> > > +               /* reassign the clone the same ref obj id as the original */
> > > +               first_reg_state->ref_obj_id = meta->initialized_dynptr.ref_obj_id;
> > > +               second_reg_state->ref_obj_id = meta->initialized_dynptr.ref_obj_id;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> >
> > [...]

Re: [PATCH v1 bpf-next 4/5] bpf: Add bpf_dynptr_clone

[Joanne Koong]

On Mon, Apr 17, 2023 at 4:46 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Apr 13, 2023 at 11:03 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > On Wed, Apr 12, 2023 at 3:12 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Sat, Apr 8, 2023 at 8:34 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> > > >
> > > > The cloned dynptr will point to the same data as its parent dynptr,
> > > > with the same type, offset, size and read-only properties.
> > > >
> > > > Any writes to a dynptr will be reflected across all instances
> > > > (by 'instance', this means any dynptrs that point to the same
> > > > underlying data).
> > > >
> > > > Please note that data slice and dynptr invalidations will affect all
> > > > instances as well. For example, if bpf_dynptr_write() is called on an
> > > > skb-type dynptr, all data slices of dynptr instances to that skb
> > > > will be invalidated as well (eg data slices of any clones, parents,
> > > > grandparents, ...). Another example is if a ringbuf dynptr is submitted,
> > > > any instance of that dynptr will be invalidated.
> > > >
> > > > Changing the view of the dynptr (eg advancing the offset or
> > > > trimming the size) will only affect that dynptr and not affect any
> > > > other instances.
> > > >
> > > > One example use case where cloning may be helpful is for hashing or
> > > > iterating through dynptr data. Cloning will allow the user to maintain
> > > > the original view of the dynptr for future use, while also allowing
> > > > views to smaller subsets of the data after the offset is advanced or the
> > > > size is trimmed.
> > > >
> > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > ---
> > > >  kernel/bpf/helpers.c  |  14 +++++
> > > >  kernel/bpf/verifier.c | 125 +++++++++++++++++++++++++++++++++++++-----
> > > >  2 files changed, 126 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > > > index bac4c6fe49f0..108f3bcfa6da 100644
> > > > --- a/kernel/bpf/helpers.c
> > > > +++ b/kernel/bpf/helpers.c
> > > > @@ -2351,6 +2351,19 @@ __bpf_kfunc __u32 bpf_dynptr_get_offset(const struct bpf_dynptr_kern *ptr)
> > > >         return ptr->offset;
> > > >  }
> > > >
> > > > +__bpf_kfunc int bpf_dynptr_clone(struct bpf_dynptr_kern *ptr,
> > > > +                                struct bpf_dynptr_kern *clone__uninit)
> > >
> > > I think most of uses for bpf_dynptr_clone() would be to get a partial
> > > view (like you mentioned above, to, e.g., do a hashing of a part of
> > > the memory range). So it feels it would be best UX if clone would
> > > allow you to define a new range in one go. So e.g., to create a
> > > "sub-dynptr" for range of bytes [10, 30), it should be enough to:
> > >
> > > struct bpf_dynptr orig_ptr, new_ptr;
> > >
> > > bpf_dynptr_clone(&orig_ptr, 10, 30, &new_ptr);
> > >
> > > Instead of:
> > >
> > > bpf_dynptr_clone(&orig_ptr, &new_ptr);
> > > bpf_dynptr_advance(&new_ptr, 10);
> > > bpf_dynptr_trim(&new_ptr, bpf_dynptr_get_size(&orig_ptr) - 30);
> > >
> >
> > I don't think we can do this because we can't have bpf_dynptr_clone()
> > fail (which might happen if we take in a range, if the range is
> > invalid). This is because in the case where the clone is of a
> > reference-counted dynptr (eg like a ringbuf-type dynptr), the clone
> > even if it's invalid must be treated by the verifier as a legit dynptr
> > (since the verifier can't know ahead of time whether the clone call
> > will succeed or not) which means if the invalid clone dynptr is then
> > passed into a reference-releasing function, the verifier will release
> > the reference but the actual reference won't be released at runtime
> > (since the clone dynptr is invalid), which would leak the reference.
> > An example is something like:
> >
> >  // invalid range is passed, error is returned and new_ptr is invalid
> > bpf_dynptr_clone(&ringbuf_ptr, 9999999, 9999999, &new_ptr);
> > // this releases the reference and invalidates both new_ptr and ringbuf_ptr
> > bpf_ringbuf_discard_dynptr(&new_ptr, 0);
> >
> > At runtime, bpf_ringbuf_discard_dynptr() will be a no-op since new_ptr
> > is invalid - the ringbuf record still needs to be submitted/discarded,
> > but the verifier will think this already happened
>
> Ah, tricky, good point. But ok, I guess with bpf_dynptr_adjust()
> proposal in another email this would be ok:
>
> bpf_dynptr_clone(..); /* always succeeds */
> bpf_dynptr_adjust(&new_ptr, 10, 30); /* could fail to adjust, but
> dynptr is left valid */
>
> Right?

Yes, this would be okay because if bpf_dynptr_adjust fails, the clone
is valid (it was unaffected) so submitting/discarding it later on
would correctly release the reference
>
> >
> > >
> > > This, btw, shows the awkwardness of the bpf_dynptr_trim() approach.
> > >
> > > If someone really wanted an exact full-sized copy, it's trivial:
> > >
> > > bpf_dynptr_clone(&orig_ptr, 0, bpf_dynptr_get_size(&orig_ptr), &new_ptr);
> > >
> > >
[...]
> > > > +static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> > > > +{
> > > > +       struct bpf_func_state *state = func(env, reg);
> > > > +       int spi;
> > > > +
> > > > +       spi = dynptr_get_spi(env, reg);
> > > > +       if (spi < 0)
> > > > +               return spi;
> > > > +
> > > > +       if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type)) {
> > > > +               int ref_obj_id = state->stack[spi].spilled_ptr.ref_obj_id;
> > > > +               int i;
> > > > +
> > > > +               /* If the dynptr has a ref_obj_id, then we need to invaldiate
> > >
> > > typo: invalidate
> > >
> > > > +                * two things:
> > > > +                *
> > > > +                * 1) Any dynptrs with a matching ref_obj_id (clones)
> > > > +                * 2) Any slices associated with the ref_obj_id
> > >
> > > I think this is where this dynptr_id confusion comes from. The rule
> > > should be "any slices derived from this dynptr". But as mentioned on
> > > another thread, it's a separate topic which we can address later.
> > >
> > If there's a parent and a clone and slices derived from the parent and
> > slices derived from the clone, if the clone is invalidated then we
> > need to invalidate slices associated with the parent as well. So
> > shouldn't it be "any slices associated with the ref obj id" not "any
> > slices derived from this dynptr"? (also just a note, parent/clone
> > slices will share the same ref obj id and the same dynptr id, so
> > checking against either does the same thing)
>
> So, we have a ringbuf dynptr with ref_obj_id=1, id=2, ok? We clone it,
> clone gets ref_obj_id=1, id=3. If either original dynptr or clone
> dynptr is released due to bpf_ringbuf_discard_dynptr(), we invalidate
> all the dynptrs with ref_obj_id=1. During invalidation of each dynptr,
> we invalidate all the slices with ref_obj_id==dynptr's id. So we'll
> invalidate slices derived from dynptr with id=2 (original dynptr), and
> then all the slices derived from dynptr with id=3?
>
When we create a slice for a dynptr (through bpf_dynptr_data()), the
slice's reg->ref_obj_id is set to the dynptr's ref_obj_id (not
dynptr's id). During invalidation of the dynptr, we invalidate all the
slices with that ref obj id, which means we invalidate all slices for
any parents/clones.
[...]
> > > > +
> > > > +       err = process_dynptr_func(env, regno, insn_idx, arg_type);
> > > > +       if (err < 0)
> > > > +               return err;
> > > > +
> > > > +       spi = dynptr_get_spi(env, reg);
> > > > +       if (spi < 0)
> > > > +               return spi;
> > > > +
> > > > +       first_reg_state = &state->stack[spi].spilled_ptr;
> > > > +       second_reg_state = &state->stack[spi - 1].spilled_ptr;
> > > > +       ref_obj_id = first_reg_state->ref_obj_id;
> > > > +
> > > > +       /* reassign the clone the same dynptr id as the original */
> > > > +       __mark_dynptr_reg(first_reg_state, dynptr_type, true, meta->initialized_dynptr.id);
> > > > +       __mark_dynptr_reg(second_reg_state, dynptr_type, false, meta->initialized_dynptr.id);
> > >
> > > I'm not sure why clone should have the same dynptr_id? Isn't it a new
> > > instance of a dynptr? I get preserving ref_obj_id (if refcounted), but
> > > why reusing dynptr_id?..
> > >
> > I think we need to also copy over the dynptr id because in the case of
> > a non-reference counted dynptr, if the parent (or clone) is
> > invalidated (eg overwriting bytes of the dynptr on the stack), we must
> > also invalidate the slices of the clone (or parent)
>
> yep, right now we'll have to do that because we have dynptr_id. But if
> we get rid of it and stick to ref_obj_id and id, then clone would need
> to get a new id, but keep ref_obj_id, right?
>
Thinking about this some more, I think you're right that we shouldn't
reuse the dynptr id. in fact, i think reusing it would lead to
incorrect behavior - in the example of a non-reference counted dynptr,
if the parent dynptr is overwritten on the stack (and thus
invalidated), that shouldn't invalidate the slices of the clone at
all. I'll change this in the next version
> > >
[...]

Re: [PATCH v1 bpf-next 4/5] bpf: Add bpf_dynptr_clone

[Andrii Nakryiko]

On Tue, Apr 18, 2023 at 11:56 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Mon, Apr 17, 2023 at 4:46 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, Apr 13, 2023 at 11:03 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> > >
> > > On Wed, Apr 12, 2023 at 3:12 PM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > On Sat, Apr 8, 2023 at 8:34 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> > > > >
> > > > > The cloned dynptr will point to the same data as its parent dynptr,
> > > > > with the same type, offset, size and read-only properties.
> > > > >
> > > > > Any writes to a dynptr will be reflected across all instances
> > > > > (by 'instance', this means any dynptrs that point to the same
> > > > > underlying data).
> > > > >
> > > > > Please note that data slice and dynptr invalidations will affect all
> > > > > instances as well. For example, if bpf_dynptr_write() is called on an
> > > > > skb-type dynptr, all data slices of dynptr instances to that skb
> > > > > will be invalidated as well (eg data slices of any clones, parents,
> > > > > grandparents, ...). Another example is if a ringbuf dynptr is submitted,
> > > > > any instance of that dynptr will be invalidated.
> > > > >
> > > > > Changing the view of the dynptr (eg advancing the offset or
> > > > > trimming the size) will only affect that dynptr and not affect any
> > > > > other instances.
> > > > >
> > > > > One example use case where cloning may be helpful is for hashing or
> > > > > iterating through dynptr data. Cloning will allow the user to maintain
> > > > > the original view of the dynptr for future use, while also allowing
> > > > > views to smaller subsets of the data after the offset is advanced or the
> > > > > size is trimmed.
> > > > >
> > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > > ---
> > > > >  kernel/bpf/helpers.c  |  14 +++++
> > > > >  kernel/bpf/verifier.c | 125 +++++++++++++++++++++++++++++++++++++-----
> > > > >  2 files changed, 126 insertions(+), 13 deletions(-)
> > > > >
> > > > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > > > > index bac4c6fe49f0..108f3bcfa6da 100644
> > > > > --- a/kernel/bpf/helpers.c
> > > > > +++ b/kernel/bpf/helpers.c
> > > > > @@ -2351,6 +2351,19 @@ __bpf_kfunc __u32 bpf_dynptr_get_offset(const struct bpf_dynptr_kern *ptr)
> > > > >         return ptr->offset;
> > > > >  }
> > > > >
> > > > > +__bpf_kfunc int bpf_dynptr_clone(struct bpf_dynptr_kern *ptr,
> > > > > +                                struct bpf_dynptr_kern *clone__uninit)
> > > >
> > > > I think most of uses for bpf_dynptr_clone() would be to get a partial
> > > > view (like you mentioned above, to, e.g., do a hashing of a part of
> > > > the memory range). So it feels it would be best UX if clone would
> > > > allow you to define a new range in one go. So e.g., to create a
> > > > "sub-dynptr" for range of bytes [10, 30), it should be enough to:
> > > >
> > > > struct bpf_dynptr orig_ptr, new_ptr;
> > > >
> > > > bpf_dynptr_clone(&orig_ptr, 10, 30, &new_ptr);
> > > >
> > > > Instead of:
> > > >
> > > > bpf_dynptr_clone(&orig_ptr, &new_ptr);
> > > > bpf_dynptr_advance(&new_ptr, 10);
> > > > bpf_dynptr_trim(&new_ptr, bpf_dynptr_get_size(&orig_ptr) - 30);
> > > >
> > >
> > > I don't think we can do this because we can't have bpf_dynptr_clone()
> > > fail (which might happen if we take in a range, if the range is
> > > invalid). This is because in the case where the clone is of a
> > > reference-counted dynptr (eg like a ringbuf-type dynptr), the clone
> > > even if it's invalid must be treated by the verifier as a legit dynptr
> > > (since the verifier can't know ahead of time whether the clone call
> > > will succeed or not) which means if the invalid clone dynptr is then
> > > passed into a reference-releasing function, the verifier will release
> > > the reference but the actual reference won't be released at runtime
> > > (since the clone dynptr is invalid), which would leak the reference.
> > > An example is something like:
> > >
> > >  // invalid range is passed, error is returned and new_ptr is invalid
> > > bpf_dynptr_clone(&ringbuf_ptr, 9999999, 9999999, &new_ptr);
> > > // this releases the reference and invalidates both new_ptr and ringbuf_ptr
> > > bpf_ringbuf_discard_dynptr(&new_ptr, 0);
> > >
> > > At runtime, bpf_ringbuf_discard_dynptr() will be a no-op since new_ptr
> > > is invalid - the ringbuf record still needs to be submitted/discarded,
> > > but the verifier will think this already happened
> >
> > Ah, tricky, good point. But ok, I guess with bpf_dynptr_adjust()
> > proposal in another email this would be ok:
> >
> > bpf_dynptr_clone(..); /* always succeeds */
> > bpf_dynptr_adjust(&new_ptr, 10, 30); /* could fail to adjust, but
> > dynptr is left valid */
> >
> > Right?
>
> Yes, this would be okay because if bpf_dynptr_adjust fails, the clone
> is valid (it was unaffected) so submitting/discarding it later on
> would correctly release the reference

ok, cool, let's do that and keep things simple

> >
> > >
> > > >
> > > > This, btw, shows the awkwardness of the bpf_dynptr_trim() approach.
> > > >
> > > > If someone really wanted an exact full-sized copy, it's trivial:
> > > >
> > > > bpf_dynptr_clone(&orig_ptr, 0, bpf_dynptr_get_size(&orig_ptr), &new_ptr);
> > > >
> > > >
> [...]
> > > > > +static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> > > > > +{
> > > > > +       struct bpf_func_state *state = func(env, reg);
> > > > > +       int spi;
> > > > > +
> > > > > +       spi = dynptr_get_spi(env, reg);
> > > > > +       if (spi < 0)
> > > > > +               return spi;
> > > > > +
> > > > > +       if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type)) {
> > > > > +               int ref_obj_id = state->stack[spi].spilled_ptr.ref_obj_id;
> > > > > +               int i;
> > > > > +
> > > > > +               /* If the dynptr has a ref_obj_id, then we need to invaldiate
> > > >
> > > > typo: invalidate
> > > >
> > > > > +                * two things:
> > > > > +                *
> > > > > +                * 1) Any dynptrs with a matching ref_obj_id (clones)
> > > > > +                * 2) Any slices associated with the ref_obj_id
> > > >
> > > > I think this is where this dynptr_id confusion comes from. The rule
> > > > should be "any slices derived from this dynptr". But as mentioned on
> > > > another thread, it's a separate topic which we can address later.
> > > >
> > > If there's a parent and a clone and slices derived from the parent and
> > > slices derived from the clone, if the clone is invalidated then we
> > > need to invalidate slices associated with the parent as well. So
> > > shouldn't it be "any slices associated with the ref obj id" not "any
> > > slices derived from this dynptr"? (also just a note, parent/clone
> > > slices will share the same ref obj id and the same dynptr id, so
> > > checking against either does the same thing)
> >
> > So, we have a ringbuf dynptr with ref_obj_id=1, id=2, ok? We clone it,
> > clone gets ref_obj_id=1, id=3. If either original dynptr or clone
> > dynptr is released due to bpf_ringbuf_discard_dynptr(), we invalidate
> > all the dynptrs with ref_obj_id=1. During invalidation of each dynptr,
> > we invalidate all the slices with ref_obj_id==dynptr's id. So we'll
> > invalidate slices derived from dynptr with id=2 (original dynptr), and
> > then all the slices derived from dynptr with id=3?
> >
> When we create a slice for a dynptr (through bpf_dynptr_data()), the
> slice's reg->ref_obj_id is set to the dynptr's ref_obj_id (not
> dynptr's id). During invalidation of the dynptr, we invalidate all the
> slices with that ref obj id, which means we invalidate all slices for
> any parents/clones.
> [...]

Yep, because of how we define that ref_obj_id should be in refs array.
What I'm saying is that we should probably change that to be more
general "ID of an object which lifetime we are associated with". That
would need some verifier internals adjustments.

So let's wrap up this particular ref_obj_id revamp discussion for now,
it's a separate topic that will be just distracting us.

> > > > > +
> > > > > +       err = process_dynptr_func(env, regno, insn_idx, arg_type);
> > > > > +       if (err < 0)
> > > > > +               return err;
> > > > > +
> > > > > +       spi = dynptr_get_spi(env, reg);
> > > > > +       if (spi < 0)
> > > > > +               return spi;
> > > > > +
> > > > > +       first_reg_state = &state->stack[spi].spilled_ptr;
> > > > > +       second_reg_state = &state->stack[spi - 1].spilled_ptr;
> > > > > +       ref_obj_id = first_reg_state->ref_obj_id;
> > > > > +
> > > > > +       /* reassign the clone the same dynptr id as the original */
> > > > > +       __mark_dynptr_reg(first_reg_state, dynptr_type, true, meta->initialized_dynptr.id);
> > > > > +       __mark_dynptr_reg(second_reg_state, dynptr_type, false, meta->initialized_dynptr.id);
> > > >
> > > > I'm not sure why clone should have the same dynptr_id? Isn't it a new
> > > > instance of a dynptr? I get preserving ref_obj_id (if refcounted), but
> > > > why reusing dynptr_id?..
> > > >
> > > I think we need to also copy over the dynptr id because in the case of
> > > a non-reference counted dynptr, if the parent (or clone) is
> > > invalidated (eg overwriting bytes of the dynptr on the stack), we must
> > > also invalidate the slices of the clone (or parent)
> >
> > yep, right now we'll have to do that because we have dynptr_id. But if
> > we get rid of it and stick to ref_obj_id and id, then clone would need
> > to get a new id, but keep ref_obj_id, right?
> >
> Thinking about this some more, I think you're right that we shouldn't
> reuse the dynptr id. in fact, i think reusing it would lead to
> incorrect behavior - in the example of a non-reference counted dynptr,
> if the parent dynptr is overwritten on the stack (and thus
> invalidated), that shouldn't invalidate the slices of the clone at
> all. I'll change this in the next version

ok

> > > >
> [...]

Re: [PATCH v1 bpf-next 4/5] bpf: Add bpf_dynptr_clone

[kernel test robot]

Hi Joanne,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Joanne-Koong/bpf-Add-bpf_dynptr_trim-and-bpf_dynptr_advance/20230409-113652
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20230409033431.3992432-5-joannelkoong%40gmail.com
patch subject: [PATCH v1 bpf-next 4/5] bpf: Add bpf_dynptr_clone
config: m68k-randconfig-s032-20230416 (https://download.01.org/0day-ci/archive/20230418/202304180233.Hk6WZE5M-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/d7830addcc26375f56b68655ddbfb44116b3e7f6
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Joanne-Koong/bpf-Add-bpf_dynptr_trim-and-bpf_dynptr_advance/20230409-113652
        git checkout d7830addcc26375f56b68655ddbfb44116b3e7f6
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=m68k olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=m68k SHELL=/bin/bash kernel/bpf/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304180233.Hk6WZE5M-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> kernel/bpf/verifier.c:7020:41: sparse: sparse: mixing different enum types:
>> kernel/bpf/verifier.c:7020:41: sparse:    unsigned int enum bpf_arg_type
>> kernel/bpf/verifier.c:7020:41: sparse:    unsigned int enum bpf_type_flag
   kernel/bpf/verifier.c:18042:38: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c: note: in included file (through include/linux/bpf.h, include/linux/bpf-cgroup.h):
   include/linux/bpfptr.h:65:40: sparse: sparse: cast to non-scalar
   include/linux/bpfptr.h:65:40: sparse: sparse: cast from non-scalar
   include/linux/bpfptr.h:65:40: sparse: sparse: cast to non-scalar
   include/linux/bpfptr.h:65:40: sparse: sparse: cast from non-scalar
   include/linux/bpfptr.h:65:40: sparse: sparse: cast to non-scalar
   include/linux/bpfptr.h:65:40: sparse: sparse: cast from non-scalar

vim +7020 kernel/bpf/verifier.c

  7006	
  7007	static int handle_dynptr_clone(struct bpf_verifier_env *env, enum bpf_arg_type arg_type,
  7008				       int regno, int insn_idx, struct bpf_kfunc_call_arg_meta *meta)
  7009	{
  7010		struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
  7011		struct bpf_reg_state *first_reg_state, *second_reg_state;
  7012		struct bpf_func_state *state = func(env, reg);
  7013		enum bpf_dynptr_type dynptr_type = meta->initialized_dynptr.type;
  7014		int err, spi, ref_obj_id;
  7015	
  7016		if (!dynptr_type) {
  7017			verbose(env, "verifier internal error: no dynptr type for bpf_dynptr_clone\n");
  7018			return -EFAULT;
  7019		}
> 7020		arg_type |= get_dynptr_type_flag(dynptr_type);
  7021	
  7022		err = process_dynptr_func(env, regno, insn_idx, arg_type);
  7023		if (err < 0)
  7024			return err;
  7025	
  7026		spi = dynptr_get_spi(env, reg);
  7027		if (spi < 0)
  7028			return spi;
  7029	
  7030		first_reg_state = &state->stack[spi].spilled_ptr;
  7031		second_reg_state = &state->stack[spi - 1].spilled_ptr;
  7032		ref_obj_id = first_reg_state->ref_obj_id;
  7033	
  7034		/* reassign the clone the same dynptr id as the original */
  7035		__mark_dynptr_reg(first_reg_state, dynptr_type, true, meta->initialized_dynptr.id);
  7036		__mark_dynptr_reg(second_reg_state, dynptr_type, false, meta->initialized_dynptr.id);
  7037	
  7038		if (meta->initialized_dynptr.ref_obj_id) {
  7039			/* release the new ref obj id assigned during process_dynptr_func */
  7040			err = release_reference_state(cur_func(env), ref_obj_id);
  7041			if (err)
  7042				return err;
  7043			/* reassign the clone the same ref obj id as the original */
  7044			first_reg_state->ref_obj_id = meta->initialized_dynptr.ref_obj_id;
  7045			second_reg_state->ref_obj_id = meta->initialized_dynptr.ref_obj_id;
  7046		}
  7047	
  7048		return 0;
  7049	}
  7050	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

[PATCH v1 bpf-next 5/5] selftests/bpf: add tests for dynptr convenience helpers

[Joanne Koong]

Add various tests for the added dynptr convenience helpers.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 tools/testing/selftests/bpf/bpf_kfuncs.h      |   8 +
 .../testing/selftests/bpf/prog_tests/dynptr.c |   6 +
 .../testing/selftests/bpf/progs/dynptr_fail.c | 313 +++++++++++++++++
 .../selftests/bpf/progs/dynptr_success.c      | 320 ++++++++++++++++++
 4 files changed, 647 insertions(+)

diff --git a/tools/testing/selftests/bpf/bpf_kfuncs.h b/tools/testing/selftests/bpf/bpf_kfuncs.h
index 8c993ec8ceea..f62f5acde611 100644
--- a/tools/testing/selftests/bpf/bpf_kfuncs.h
+++ b/tools/testing/selftests/bpf/bpf_kfuncs.h
@@ -35,4 +35,12 @@ extern void *bpf_dynptr_slice(const struct bpf_dynptr *ptr, __u32 offset,
 extern void *bpf_dynptr_slice_rdwr(const struct bpf_dynptr *ptr, __u32 offset,
 			      void *buffer, __u32 buffer__szk) __ksym;
 
+extern int bpf_dynptr_trim(struct bpf_dynptr *ptr, __u32 len) __ksym;
+extern int bpf_dynptr_advance(struct bpf_dynptr *ptr, __u32 len) __ksym;
+extern int bpf_dynptr_is_null(const struct bpf_dynptr *ptr) __ksym;
+extern int bpf_dynptr_is_rdonly(const struct bpf_dynptr *ptr) __ksym;
+extern __u32 bpf_dynptr_get_size(const struct bpf_dynptr *ptr) __ksym;
+extern __u32 bpf_dynptr_get_offset(const struct bpf_dynptr *ptr) __ksym;
+extern int bpf_dynptr_clone(const struct bpf_dynptr *ptr, struct bpf_dynptr *clone__init) __ksym;
+
 #endif
diff --git a/tools/testing/selftests/bpf/prog_tests/dynptr.c b/tools/testing/selftests/bpf/prog_tests/dynptr.c
index d176c34a7d2e..198850fc8982 100644
--- a/tools/testing/selftests/bpf/prog_tests/dynptr.c
+++ b/tools/testing/selftests/bpf/prog_tests/dynptr.c
@@ -20,6 +20,12 @@ static struct {
 	{"test_ringbuf", SETUP_SYSCALL_SLEEP},
 	{"test_skb_readonly", SETUP_SKB_PROG},
 	{"test_dynptr_skb_data", SETUP_SKB_PROG},
+	{"test_advance_trim", SETUP_SYSCALL_SLEEP},
+	{"test_advance_trim_err", SETUP_SYSCALL_SLEEP},
+	{"test_zero_size_dynptr", SETUP_SYSCALL_SLEEP},
+	{"test_dynptr_is_null", SETUP_SYSCALL_SLEEP},
+	{"test_dynptr_is_rdonly", SETUP_SKB_PROG},
+	{"test_dynptr_clone", SETUP_SKB_PROG},
 };
 
 static void verify_success(const char *prog_name, enum test_setup_type setup_type)
diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
index 759eb5c245cd..8bafa64f4600 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
@@ -1378,3 +1378,316 @@ int invalid_slice_rdwr_rdonly(struct __sk_buff *skb)
 
 	return 0;
 }
+
+/* bpf_dynptr_advance can only be called on initialized dynptrs */
+SEC("?raw_tp")
+__failure __msg("Expected an initialized dynptr as arg #1")
+int dynptr_advance_invalid(void *ctx)
+{
+	struct bpf_dynptr ptr;
+
+	/* this should fail */
+	bpf_dynptr_advance(&ptr, 1);
+
+	return 0;
+}
+
+/* bpf_dynptr_trim can only be called on initialized dynptrs */
+SEC("?raw_tp")
+__failure __msg("Expected an initialized dynptr as arg #1")
+int dynptr_trim_invalid(void *ctx)
+{
+	struct bpf_dynptr ptr;
+
+	/* this should fail */
+	bpf_dynptr_trim(&ptr, 1);
+
+	return 0;
+}
+
+/* bpf_dynptr_is_null can only be called on initialized dynptrs */
+SEC("?raw_tp")
+__failure __msg("Expected an initialized dynptr as arg #1")
+int dynptr_is_null_invalid(void *ctx)
+{
+	struct bpf_dynptr ptr;
+
+	/* this should fail */
+	bpf_dynptr_is_null(&ptr);
+
+	return 0;
+}
+
+/* bpf_dynptr_is_rdonly can only be called on initialized dynptrs */
+SEC("?raw_tp")
+__failure __msg("Expected an initialized dynptr as arg #1")
+int dynptr_is_rdonly_invalid(void *ctx)
+{
+	struct bpf_dynptr ptr;
+
+	/* this should fail */
+	bpf_dynptr_is_rdonly(&ptr);
+
+	return 0;
+}
+
+/* bpf_dynptr_get_size can only be called on initialized dynptrs */
+SEC("?raw_tp")
+__failure __msg("Expected an initialized dynptr as arg #1")
+int dynptr_get_size_invalid(void *ctx)
+{
+	struct bpf_dynptr ptr;
+
+	/* this should fail */
+	bpf_dynptr_get_size(&ptr);
+
+	return 0;
+}
+
+/* bpf_dynptr_get_offset can only be called on initialized dynptrs */
+SEC("?raw_tp")
+__failure __msg("Expected an initialized dynptr as arg #1")
+int dynptr_get_offset_invalid(void *ctx)
+{
+	struct bpf_dynptr ptr;
+
+	/* this should fail */
+	bpf_dynptr_get_offset(&ptr);
+
+	return 0;
+}
+
+/* Only initialized dynptrs can be cloned */
+SEC("?raw_tp")
+__failure __msg("Expected an initialized dynptr as arg #1")
+int clone_invalid1(void *ctx)
+{
+	struct bpf_dynptr ptr1;
+	struct bpf_dynptr ptr2;
+
+	/* this should fail */
+	bpf_dynptr_clone(&ptr1, &ptr2);
+
+	return 0;
+}
+
+/* Can't overwrite an existing dynptr when cloning */
+SEC("?xdp")
+__failure __msg("cannot overwrite referenced dynptr")
+int clone_invalid2(struct xdp_md *xdp)
+{
+	struct bpf_dynptr ptr1;
+	struct bpf_dynptr clone;
+
+	bpf_dynptr_from_xdp(xdp, 0, &ptr1);
+
+	bpf_ringbuf_reserve_dynptr(&ringbuf, 64, 0, &clone);
+
+	/* this should fail */
+	bpf_dynptr_clone(&ptr1, &clone);
+
+	bpf_ringbuf_submit_dynptr(&clone, 0);
+
+	return 0;
+}
+
+/* Invalidating a dynptr should invalidate its clones */
+SEC("?raw_tp")
+__failure __msg("Expected an initialized dynptr as arg #3")
+int clone_invalidate1(void *ctx)
+{
+	struct bpf_dynptr clone;
+	struct bpf_dynptr ptr;
+	char read_data[64];
+
+	bpf_ringbuf_reserve_dynptr(&ringbuf, val, 0, &ptr);
+
+	bpf_dynptr_clone(&ptr, &clone);
+
+	bpf_ringbuf_submit_dynptr(&ptr, 0);
+
+	/* this should fail */
+	bpf_dynptr_read(read_data, sizeof(read_data), &clone, 0, 0);
+
+	return 0;
+}
+
+/* Invalidating a dynptr should invalidate its parent */
+SEC("?raw_tp")
+__failure __msg("Expected an initialized dynptr as arg #3")
+int clone_invalidate2(void *ctx)
+{
+	struct bpf_dynptr ptr;
+	struct bpf_dynptr clone;
+	char read_data[64];
+
+	bpf_ringbuf_reserve_dynptr(&ringbuf, val, 0, &ptr);
+
+	bpf_dynptr_clone(&ptr, &clone);
+
+	bpf_ringbuf_submit_dynptr(&clone, 0);
+
+	/* this should fail */
+	bpf_dynptr_read(read_data, sizeof(read_data), &ptr, 0, 0);
+
+	return 0;
+}
+
+/* Invalidating a dynptr should invalidate its siblings */
+SEC("?raw_tp")
+__failure __msg("Expected an initialized dynptr as arg #3")
+int clone_invalidate3(void *ctx)
+{
+	struct bpf_dynptr ptr;
+	struct bpf_dynptr clone1;
+	struct bpf_dynptr clone2;
+	char read_data[64];
+
+	bpf_ringbuf_reserve_dynptr(&ringbuf, val, 0, &ptr);
+
+	bpf_dynptr_clone(&ptr, &clone1);
+
+	bpf_dynptr_clone(&ptr, &clone2);
+
+	bpf_ringbuf_submit_dynptr(&clone2, 0);
+
+	/* this should fail */
+	bpf_dynptr_read(read_data, sizeof(read_data), &clone1, 0, 0);
+
+	return 0;
+}
+
+/* Invalidating a dynptr should invalidate any data slices
+ * of its clones
+ */
+SEC("?raw_tp")
+__failure __msg("invalid mem access 'scalar'")
+int clone_invalidate4(void *ctx)
+{
+	struct bpf_dynptr ptr;
+	struct bpf_dynptr clone;
+	int *data;
+
+	bpf_ringbuf_reserve_dynptr(&ringbuf, val, 0, &ptr);
+
+	bpf_dynptr_clone(&ptr, &clone);
+	data = bpf_dynptr_data(&clone, 0, sizeof(val));
+	if (!data)
+		return 0;
+
+	bpf_ringbuf_submit_dynptr(&ptr, 0);
+
+	/* this should fail */
+	*data = 123;
+
+	return 0;
+}
+
+/* Invalidating a dynptr should invalidate any data slices
+ * of its parent
+ */
+SEC("?raw_tp")
+__failure __msg("invalid mem access 'scalar'")
+int clone_invalidate5(void *ctx)
+{
+	struct bpf_dynptr ptr;
+	struct bpf_dynptr clone;
+	int *data;
+
+	bpf_ringbuf_reserve_dynptr(&ringbuf, val, 0, &ptr);
+	data = bpf_dynptr_data(&ptr, 0, sizeof(val));
+	if (!data)
+		return 0;
+
+	bpf_dynptr_clone(&ptr, &clone);
+
+	bpf_ringbuf_submit_dynptr(&clone, 0);
+
+	/* this should fail */
+	*data = 123;
+
+	return 0;
+}
+
+/* Invalidating a dynptr should invalidate any data slices
+ * of its sibling
+ */
+SEC("?raw_tp")
+__failure __msg("invalid mem access 'scalar'")
+int clone_invalidate6(void *ctx)
+{
+	struct bpf_dynptr ptr;
+	struct bpf_dynptr clone1;
+	struct bpf_dynptr clone2;
+	int *data;
+
+	bpf_ringbuf_reserve_dynptr(&ringbuf, val, 0, &ptr);
+
+	bpf_dynptr_clone(&ptr, &clone1);
+
+	bpf_dynptr_clone(&ptr, &clone2);
+
+	data = bpf_dynptr_data(&clone1, 0, sizeof(val));
+	if (!data)
+		return 0;
+
+	bpf_ringbuf_submit_dynptr(&clone2, 0);
+
+	/* this should fail */
+	*data = 123;
+
+	return 0;
+}
+
+/* A skb clone's data slices should be invalid anytime packet data changes */
+SEC("?tc")
+__failure __msg("invalid mem access 'scalar'")
+int clone_skb_packet_data(struct __sk_buff *skb)
+{
+	char buffer[sizeof(__u32)] = {};
+	struct bpf_dynptr clone;
+	struct bpf_dynptr ptr;
+	__u32 *data;
+
+	bpf_dynptr_from_skb(skb, 0, &ptr);
+
+	bpf_dynptr_clone(&ptr, &clone);
+	data = bpf_dynptr_slice_rdwr(&clone, 0, buffer, sizeof(buffer));
+	if (!data)
+		return XDP_DROP;
+
+	if (bpf_skb_pull_data(skb, skb->len))
+		return SK_DROP;
+
+	/* this should fail */
+	*data = 123;
+
+	return 0;
+}
+
+/* A xdp clone's data slices should be invalid anytime packet data changes */
+SEC("?xdp")
+__failure __msg("invalid mem access 'scalar'")
+int clone_xdp_packet_data(struct xdp_md *xdp)
+{
+	char buffer[sizeof(__u32)] = {};
+	struct bpf_dynptr clone;
+	struct bpf_dynptr ptr;
+	struct ethhdr *hdr;
+	__u32 *data;
+
+	bpf_dynptr_from_xdp(xdp, 0, &ptr);
+
+	bpf_dynptr_clone(&ptr, &clone);
+	data = bpf_dynptr_slice_rdwr(&clone, 0, buffer, sizeof(buffer));
+	if (!data)
+		return XDP_DROP;
+
+	if (bpf_xdp_adjust_head(xdp, 0 - (int)sizeof(*hdr)))
+		return XDP_DROP;
+
+	/* this should fail */
+	*data = 123;
+
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/dynptr_success.c b/tools/testing/selftests/bpf/progs/dynptr_success.c
index b2fa6c47ecc0..19a62f743183 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_success.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_success.c
@@ -207,3 +207,323 @@ int test_dynptr_skb_data(struct __sk_buff *skb)
 
 	return 1;
 }
+
+SEC("tp/syscalls/sys_enter_nanosleep")
+int test_advance_trim(void *ctx)
+{
+	struct bpf_dynptr ptr;
+	__u32 bytes = 64;
+	__u32 off = 10;
+	__u32 trim = 5;
+
+	if (bpf_get_current_pid_tgid() >> 32 != pid)
+		return 0;
+
+	err = bpf_ringbuf_reserve_dynptr(&ringbuf, bytes, 0, &ptr);
+	if (err) {
+		err = 1;
+		goto done;
+	}
+
+	if (bpf_dynptr_get_size(&ptr) != bytes) {
+		err = 2;
+		goto done;
+	}
+
+	/* Advance the dynptr by off */
+	err = bpf_dynptr_advance(&ptr, off);
+	if (err) {
+		err = 3;
+		goto done;
+	}
+
+	/* Check that the dynptr off and size were adjusted correctly */
+	if (bpf_dynptr_get_offset(&ptr) != off) {
+		err = 4;
+		goto done;
+	}
+	if (bpf_dynptr_get_size(&ptr) != bytes - off) {
+		err = 5;
+		goto done;
+	}
+
+	/* Trim the dynptr */
+	err = bpf_dynptr_trim(&ptr, trim);
+	if (err) {
+		err = 6;
+		goto done;
+	}
+
+	/* Check that the off was unaffected */
+	if (bpf_dynptr_get_offset(&ptr) != off) {
+		err = 7;
+		goto done;
+	}
+	/* Check that the size was adjusted correctly */
+	if (bpf_dynptr_get_size(&ptr) != bytes - off - trim) {
+		err = 8;
+		goto done;
+	}
+
+done:
+	bpf_ringbuf_discard_dynptr(&ptr, 0);
+	return 0;
+}
+
+SEC("tp/syscalls/sys_enter_nanosleep")
+int test_advance_trim_err(void *ctx)
+{
+	char write_data[45] = "hello there, world!!";
+	struct bpf_dynptr ptr;
+	__u32 trim_size = 10;
+	__u32 size = 64;
+	__u32 off = 10;
+
+	if (bpf_get_current_pid_tgid() >> 32 != pid)
+		return 0;
+
+	if (bpf_ringbuf_reserve_dynptr(&ringbuf, size, 0, &ptr)) {
+		err = 1;
+		goto done;
+	}
+
+	/* Check that you can't advance beyond size of dynptr data */
+	if (bpf_dynptr_advance(&ptr, size + 1) != -ERANGE) {
+		err = 2;
+		goto done;
+	}
+
+	if (bpf_dynptr_advance(&ptr, off)) {
+		err = 3;
+		goto done;
+	}
+
+	/* Check that you can't trim more than size of dynptr data */
+	if (bpf_dynptr_trim(&ptr, size - off + 1) != -ERANGE) {
+		err = 4;
+		goto done;
+	}
+
+	/* Check that you can't write more bytes than available into the dynptr
+	 * after you've trimmed it
+	 */
+	if (bpf_dynptr_trim(&ptr, trim_size)) {
+		err = 5;
+		goto done;
+	}
+
+	if (bpf_dynptr_write(&ptr, 0, &write_data, sizeof(write_data), 0) != -E2BIG) {
+		err = 6;
+		goto done;
+	}
+
+	/* Check that even after advancing / trimming, submitting/discarding
+	 * a ringbuf dynptr works
+	 */
+	bpf_ringbuf_submit_dynptr(&ptr, 0);
+	return 0;
+
+done:
+	bpf_ringbuf_discard_dynptr(&ptr, 0);
+	return 0;
+}
+
+SEC("tp/syscalls/sys_enter_nanosleep")
+int test_zero_size_dynptr(void *ctx)
+{
+	char write_data = 'x', read_data;
+	struct bpf_dynptr ptr;
+	__u32 size = 64;
+
+	if (bpf_get_current_pid_tgid() >> 32 != pid)
+		return 0;
+
+	/* check that you can reserve a dynamic size reservation */
+	if (bpf_ringbuf_reserve_dynptr(&ringbuf, size, 0, &ptr)) {
+		err = 1;
+		goto done;
+	}
+
+	/* After this, the dynptr has a size of 0 */
+	if (bpf_dynptr_advance(&ptr, size)) {
+		err = 2;
+		goto done;
+	}
+
+	/* Test that reading + writing non-zero bytes is not ok */
+	if (bpf_dynptr_read(&read_data, sizeof(read_data), &ptr, 0, 0) != -E2BIG) {
+		err = 3;
+		goto done;
+	}
+
+	if (bpf_dynptr_write(&ptr, 0, &write_data, sizeof(write_data), 0) != -E2BIG) {
+		err = 4;
+		goto done;
+	}
+
+	/* Test that reading + writing 0 bytes from a 0-size dynptr is ok */
+	if (bpf_dynptr_read(&read_data, 0, &ptr, 0, 0)) {
+		err = 5;
+		goto done;
+	}
+
+	if (bpf_dynptr_write(&ptr, 0, &write_data, 0, 0)) {
+		err = 6;
+		goto done;
+	}
+
+	err = 0;
+
+done:
+	bpf_ringbuf_discard_dynptr(&ptr, 0);
+	return 0;
+}
+
+SEC("tp/syscalls/sys_enter_nanosleep")
+int test_dynptr_is_null(void *ctx)
+{
+	struct bpf_dynptr ptr1;
+	struct bpf_dynptr ptr2;
+	__u64 size = 4;
+
+	if (bpf_get_current_pid_tgid() >> 32 != pid)
+		return 0;
+
+	/* Pass in invalid flags, get back an invalid dynptr */
+	if (bpf_ringbuf_reserve_dynptr(&ringbuf, size, 123, &ptr1) != -EINVAL) {
+		err = 1;
+		goto exit_early;
+	}
+
+	/* Test that the invalid dynptr is null */
+	if (!bpf_dynptr_is_null(&ptr1)) {
+		err = 2;
+		goto exit_early;
+	}
+
+	/* Get a valid dynptr */
+	if (bpf_ringbuf_reserve_dynptr(&ringbuf, size, 0, &ptr2)) {
+		err = 3;
+		goto exit;
+	}
+
+	/* Test that the valid dynptr is not null */
+	if (bpf_dynptr_is_null(&ptr2)) {
+		err = 4;
+		goto exit;
+	}
+
+exit:
+	bpf_ringbuf_discard_dynptr(&ptr2, 0);
+exit_early:
+	bpf_ringbuf_discard_dynptr(&ptr1, 0);
+	return 0;
+}
+
+SEC("cgroup_skb/egress")
+int test_dynptr_is_rdonly(struct __sk_buff *skb)
+{
+	struct bpf_dynptr ptr1;
+	struct bpf_dynptr ptr2;
+	struct bpf_dynptr ptr3;
+
+	/* Pass in invalid flags, get back an invalid dynptr */
+	if (bpf_dynptr_from_skb(skb, 123, &ptr1) != -EINVAL) {
+		err = 1;
+		return 0;
+	}
+
+	/* Test that an invalid dynptr is_rdonly returns false */
+	if (bpf_dynptr_is_rdonly(&ptr1)) {
+		err = 2;
+		return 0;
+	}
+
+	/* Get a read-only dynptr */
+	if (bpf_dynptr_from_skb(skb, 0, &ptr2)) {
+		err = 3;
+		return 0;
+	}
+
+	/* Test that the dynptr is read-only */
+	if (!bpf_dynptr_is_rdonly(&ptr2)) {
+		err = 4;
+		return 0;
+	}
+
+	/* Get a read-writeable dynptr */
+	if (bpf_ringbuf_reserve_dynptr(&ringbuf, 64, 0, &ptr3)) {
+		err = 5;
+		goto done;
+	}
+
+	/* Test that the dynptr is read-only */
+	if (bpf_dynptr_is_rdonly(&ptr3)) {
+		err = 6;
+		goto done;
+	}
+
+done:
+	bpf_ringbuf_discard_dynptr(&ptr3, 0);
+	return 0;
+}
+
+SEC("cgroup_skb/egress")
+int test_dynptr_clone(struct __sk_buff *skb)
+{
+	struct bpf_dynptr ptr1;
+	struct bpf_dynptr ptr2;
+	__u32 off = 2, size;
+
+	/* Get a dynptr */
+	if (bpf_dynptr_from_skb(skb, 0, &ptr1)) {
+		err = 1;
+		return 0;
+	}
+
+	if (bpf_dynptr_advance(&ptr1, off)) {
+		err = 2;
+		return 0;
+	}
+
+	/* Clone the dynptr */
+	if (bpf_dynptr_clone(&ptr1, &ptr2)) {
+		err = 3;
+		return 0;
+	}
+
+	size = bpf_dynptr_get_size(&ptr1);
+
+	/* Check that the clone has the same offset, size, and rd-only */
+	if (bpf_dynptr_get_size(&ptr2) != size) {
+		err = 4;
+		return 0;
+	}
+
+	if (bpf_dynptr_get_offset(&ptr2) != off) {
+		err = 5;
+		return 0;
+	}
+
+	if (bpf_dynptr_is_rdonly(&ptr2) != bpf_dynptr_is_rdonly(&ptr1)) {
+		err = 6;
+		return 0;
+	}
+
+	/* Advance and trim the original dynptr */
+	bpf_dynptr_advance(&ptr1, 50);
+	bpf_dynptr_trim(&ptr1, 50);
+
+	/* Check that only original dynptr was affected, and the clone wasn't */
+	if (bpf_dynptr_get_offset(&ptr2) != off) {
+		err = 7;
+		return 0;
+	}
+
+	if (bpf_dynptr_get_size(&ptr2) != size) {
+		err = 8;
+		return 0;
+	}
+
+	return 0;
+}
-- 
2.34.1

Re: [PATCH v1 bpf-next 0/5] Dynptr convenience helpers

[Andrii Nakryiko]

On Sat, Apr 8, 2023 at 8:34 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> This patchset is the 3rd in the dynptr series. The 1st (dynptr
> fundamentals) can be found here [0] and the second (skb + xdp dynptrs)
> can be found here [1].
>
> This patchset adds the following convenience helpers for interacting
> with dynptrs:

"convenience helpers" doesn't do these APIs justice. They are
necessary APIs to make dynptrs a generic interface for working with
memory ranges. So it's dynptr cloning and adjustment helpers, but not
just a convenience APIs.

>
> int bpf_dynptr_trim(struct bpf_dynptr *ptr, __u32 len) __ksym;
> int bpf_dynptr_advance(struct bpf_dynptr *ptr, __u32 len) __ksym;
> int bpf_dynptr_is_null(const struct bpf_dynptr *ptr) __ksym;
> int bpf_dynptr_is_rdonly(const struct bpf_dynptr *ptr) __ksym;
> __u32 bpf_dynptr_get_size(const struct bpf_dynptr *ptr) __ksym;
> __u32 bpf_dynptr_get_offset(const struct bpf_dynptr *ptr) __ksym;
> int bpf_dynptr_clone(const struct bpf_dynptr *ptr, struct bpf_dynptr *clone__init) __ksym;
>
> [0] https://lore.kernel.org/bpf/20220523210712.3641569-1-joannelkoong@gmail.com/
> [1] https://lore.kernel.org/bpf/20230301154953.641654-1-joannelkoong@gmail.com/
>
> Joanne Koong (5):
>   bpf: Add bpf_dynptr_trim and bpf_dynptr_advance
>   bpf: Add bpf_dynptr_is_null and bpf_dynptr_is_rdonly
>   bpf: Add bpf_dynptr_get_size and bpf_dynptr_get_offset
>   bpf: Add bpf_dynptr_clone
>   selftests/bpf: add tests for dynptr convenience helpers
>
>  include/linux/bpf.h                           |   2 +-
>  kernel/bpf/helpers.c                          | 108 +++++-
>  kernel/bpf/verifier.c                         | 125 ++++++-
>  kernel/trace/bpf_trace.c                      |   4 +-
>  tools/testing/selftests/bpf/bpf_kfuncs.h      |   8 +
>  .../testing/selftests/bpf/prog_tests/dynptr.c |   6 +
>  .../testing/selftests/bpf/progs/dynptr_fail.c | 313 +++++++++++++++++
>  .../selftests/bpf/progs/dynptr_success.c      | 320 ++++++++++++++++++
>  8 files changed, 864 insertions(+), 22 deletions(-)
>
> --
> 2.34.1
>