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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

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