On Mon, Apr 24, 2023 at 10:29 PM Joanne Koong <joannelkoong@xxxxxxxxx> wrote:
>
> On Mon, Apr 24, 2023 at 12:46 PM John Fastabend
> <john.fastabend@xxxxxxxxx> wrote:
> >
> > Joanne Koong wrote:
> > > Add a new kfunc
> > >
> > > int bpf_dynptr_adjust(struct bpf_dynptr_kern *ptr, u32 start, u32 end);
> > >
> > > which adjusts the dynptr to reflect the new [start, end) interval.
> > > In particular, it advances the offset of the dynptr by "start" bytes,
> > > and if end is less than the size of the dynptr, then this will trim the
> > > dynptr accordingly.
> > >
> > > Adjusting the dynptr interval 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, adjust can be used to narrow in on the
> > > specific region to hash.
> >
> > Would you want to prohibit creating an empty dynptr with [start, start)?
>
> I'm open to either :) I don't reallysee a use case for creating an
> empty dynptr, but I think the concept of an empty dynptr might be
> useful in general, so maybe we should let this be okay as well?
Yes, there is no need to artificially enforce a non-empty range. We
already use pointers to zero-sized memory region in verifier (e.g.,
Alexei's recent kfunc existence check changes). In general, empty
range is a valid range and we should strive to have that working
without assumptions on who and how would use that. As long as it's
conceptually safe, we should support it.
>
> >
> > >
> > > Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> > > ---
> > > kernel/bpf/helpers.c | 26 ++++++++++++++++++++++++++
> > > 1 file changed, 26 insertions(+)
> > >
> > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > > index 00e5fb0682ac..7ddf63ac93ce 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;
> > > @@ -2297,6 +2304,24 @@ __bpf_kfunc void *bpf_dynptr_slice_rdwr(const struct bpf_dynptr_kern *ptr, u32 o
> > > return bpf_dynptr_slice(ptr, offset, buffer, buffer__szk);
> > > }
> > >
> > > +__bpf_kfunc int bpf_dynptr_adjust(struct bpf_dynptr_kern *ptr, u32 start, u32 end)
> > > +{
> > > + u32 size;
> > > +
> > > + if (!ptr->data || start > end)
> > > + return -EINVAL;
> > > +
> > > + size = bpf_dynptr_get_size(ptr);
> > > +
> > > + if (start > size || end > size)
> > > + return -ERANGE;
> >
> > maybe 'start >= size'? OTOH if the verifier doesn't mind I guess its OK
> > to create the thing even if it doesn't make much sense.
>
> I think there might be use cases where this is useful even though the
> zero-sized dynptr can't do anything. for example, if there's a helper
> function in a program that takes in a dynptr, parses some fixed-size
> chunk of its data, and then advances it, it might be useful to have
> the concept of a zero-sized dynptr, so that if we're parsing the last
> chunk of the data, then the last call to bpf_dynptr_adjust() will
> still succeed and the dynptr will be of size 0, which signals
> completion.
>
+1, empty range does happen in practice naturally, and having to
special-case them is a hindrance. Let's keep it possible.
> >
> > > +
> > > + ptr->offset += start;
> > > + bpf_dynptr_set_size(ptr, end - start);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
> > > {
> > > return obj;
> > > @@ -2369,6 +2394,7 @@ 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_adjust)
> > > BTF_SET8_END(common_btf_ids)
> > >
> > > static const struct btf_kfunc_id_set common_kfunc_set = {
> > > --
> > > 2.34.1
> > >
