On Wed, Sep 7, 2022 at 5:07 PM Joanne Koong <joannelkoong@xxxxxxxxx> wrote:
>
> Add a new helper function, bpf_dynptr_iterator:
>
> long bpf_dynptr_iterator(struct bpf_dynptr *ptr, void *callback_fn,
> void *callback_ctx, u64 flags)
>
> where callback_fn is defined as:
>
> long (*callback_fn)(struct bpf_dynptr *ptr, void *ctx)
>
> and callback_fn returns the number of bytes to advance the
> dynptr by (or an error code in the case of error). The iteration
> will stop if the callback_fn returns 0 or an error or tries to
> advance by more bytes than available.
>
> Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> ---
> include/uapi/linux/bpf.h | 20 ++++++++++++++
> kernel/bpf/helpers.c | 48 +++++++++++++++++++++++++++++++---
> kernel/bpf/verifier.c | 27 +++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 20 ++++++++++++++
> 4 files changed, 111 insertions(+), 4 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 16973fa4d073..ff78a94c262a 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -5531,6 +5531,25 @@ union bpf_attr {
> * losing access to the original view of the dynptr.
> * Return
> * 0 on success, -EINVAL if the dynptr to clone is invalid.
> + *
> + * long bpf_dynptr_iterator(struct bpf_dynptr *ptr, void *callback_fn, void *callback_ctx, u64 flags)
> + * Description
> + * Iterate through the dynptr data, calling **callback_fn** on each
> + * iteration with **callback_ctx** as the context parameter.
> + * The **callback_fn** should be a static function and
> + * the **callback_ctx** should be a pointer to the stack.
> + * Currently **flags** is unused and must be 0.
> + *
> + * long (\*callback_fn)(struct bpf_dynptr \*ptr, void \*ctx);
> + *
> + * where **callback_fn** returns the number of bytes to advance
> + * the dynptr by or an error. The iteration will stop if **callback_fn**
> + * returns 0 or an error or tries to advance by more bytes than the
> + * size of the dynptr.
> + * Return
> + * 0 on success, -EINVAL if the dynptr is invalid or **flags** is not 0,
> + * -ERANGE if attempting to iterate more bytes than available, or other
> + * negative error if **callback_fn** returns an error.
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -5752,6 +5771,7 @@ union bpf_attr {
> FN(dynptr_get_size), \
> FN(dynptr_get_offset), \
> FN(dynptr_clone), \
> + FN(dynptr_iterator), \
> /* */
>
> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 667f1e213a61..519b3da06d49 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1653,13 +1653,11 @@ static const struct bpf_func_proto bpf_dynptr_data_proto = {
> .arg3_type = ARG_CONST_ALLOC_SIZE_OR_ZERO,
> };
>
> -BPF_CALL_2(bpf_dynptr_advance, struct bpf_dynptr_kern *, ptr, u32, len)
> +/* *ptr* should always be a valid dynptr */
> +static int __bpf_dynptr_advance(struct bpf_dynptr_kern *ptr, u32 len)
> {
> u32 size;
>
> - if (!ptr->data)
> - return -EINVAL;
> -
> size = __bpf_dynptr_get_size(ptr);
>
> if (len > size)
> @@ -1672,6 +1670,14 @@ BPF_CALL_2(bpf_dynptr_advance, struct bpf_dynptr_kern *, ptr, u32, len)
> return 0;
> }
>
> +BPF_CALL_2(bpf_dynptr_advance, struct bpf_dynptr_kern *, ptr, u32, len)
> +{
> + if (!ptr->data)
> + return -EINVAL;
> +
> + return __bpf_dynptr_advance(ptr, len);
> +}
> +
> static const struct bpf_func_proto bpf_dynptr_advance_proto = {
> .func = bpf_dynptr_advance,
> .gpl_only = false,
> @@ -1783,6 +1789,38 @@ static const struct bpf_func_proto bpf_dynptr_clone_proto = {
> .arg2_type = ARG_PTR_TO_DYNPTR | MEM_UNINIT,
> };
>
> +BPF_CALL_4(bpf_dynptr_iterator, struct bpf_dynptr_kern *, ptr, void *, callback_fn,
> + void *, callback_ctx, u64, flags)
> +{
> + bpf_callback_t callback = (bpf_callback_t)callback_fn;
> + int nr_bytes, err;
> +
> + if (!ptr->data || flags)
> + return -EINVAL;
> +
> + while (ptr->size > 0) {
> + nr_bytes = callback((u64)(long)ptr, (u64)(long)callback_ctx, 0, 0, 0);
> + if (nr_bytes <= 0)
> + return nr_bytes;
callback is defined as returning long but here you are silently
truncating it to int. Let's either stick to longs or to ints.
> +
> + err = __bpf_dynptr_advance(ptr, nr_bytes);
as Kumar pointed out, you can't modify ptr in place, you have to
create a local copy and bpf_dynptr_clone() it (so that for MALLOC
you'll bump refcnt). Then advance and pass it to callback. David has
such local dynptr use case in bpf_user_ringbuf_drain() helper.
> + if (err)
> + return err;
> + }
> +
> + return 0;
> +}
> +
[...]
