On Tue, Nov 23, 2021 at 10:34 AM Joanne Koong <joannekoong@xxxxxx> wrote:
>
> This patch adds the kernel-side and API changes for a new helper
> function, bpf_loop:
>
> long bpf_loop(u32 nr_loops, void *callback_fn, void *callback_ctx,
> u64 flags);
>
> where long (*callback_fn)(u32 index, void *ctx);
>
> bpf_loop invokes the "callback_fn" **nr_loops** times or until the
> callback_fn returns 1. The callback_fn can only return 0 or 1, and
> this is enforced by the verifier. The callback_fn index is zero-indexed.
>
> A few things to please note:
> ~ The "u64 flags" parameter is currently unused but is included in
> case a future use case for it arises.
> ~ In the kernel-side implementation of bpf_loop (kernel/bpf/bpf_iter.c),
> bpf_callback_t is used as the callback function cast.
> ~ A program can have nested bpf_loop calls but the program must
> still adhere to the verifier constraint of its stack depth (the stack depth
> cannot exceed MAX_BPF_STACK))
> ~ Recursive callback_fns do not pass the verifier, due to the call stack
> for these being too deep.
> ~ The next patch will include the tests and benchmark
>
> Signed-off-by: Joanne Koong <joannekoong@xxxxxx>
> ---
> include/linux/bpf.h | 1 +
> include/uapi/linux/bpf.h | 25 +++++++++
> kernel/bpf/bpf_iter.c | 35 ++++++++++++
> kernel/bpf/helpers.c | 2 +
> kernel/bpf/verifier.c | 97 +++++++++++++++++++++-------------
> tools/include/uapi/linux/bpf.h | 25 +++++++++
> 6 files changed, 148 insertions(+), 37 deletions(-)
>
[...]
> +/* maximum number of loops */
> +#define MAX_LOOPS BIT(23)
> +
> +BPF_CALL_4(bpf_loop, u32, nr_loops, void *, callback_fn, void *, callback_ctx,
> + u64, flags)
> +{
> + bpf_callback_t callback = (bpf_callback_t)callback_fn;
> + u64 ret;
> + u32 i;
> +
> + if (flags || nr_loops > MAX_LOOPS)
> + return -EINVAL;
nit: it's probably a good idea to return -E2BIG for nr_loops >
MAX_LOOPS? It will be more obvious for unsuspecting users that forgot
to read the documentation carefully :)
> +
> + for (i = 0; i < nr_loops; i++) {
> + ret = callback((u64)i, (u64)(long)callback_ctx, 0, 0, 0);
> + /* return value: 0 - continue, 1 - stop and return */
> + if (ret) {
> + i++;
> + break;
nit: could be just return i + 1;
> + }
> + }
> +
> + return i;
> +}
> +
[...]
> + case BPF_FUNC_get_local_storage:
> + /* check that flags argument in get_local_storage(map, flags) is 0,
> + * this is required because get_local_storage() can't return an error.
> + */
> + if (!register_is_null(®s[BPF_REG_2])) {
> + verbose(env, "get_local_storage() doesn't support non-zero flags\n");
> return -EINVAL;
> - }
> -
> - if (func_id == BPF_FUNC_timer_set_callback) {
> + }
> + err = 0;
err is guaranteed to be zero, no need to re-assign it
> + break;
> + case BPF_FUNC_for_each_map_elem:
> err = __check_func_call(env, insn, insn_idx_p, meta.subprogno,
> - set_timer_callback_state);
> - if (err < 0)
> - return -EINVAL;
> - }
> -
> - if (func_id == BPF_FUNC_find_vma) {
> + set_map_elem_callback_state) ? -EINVAL : 0;
I think it's actually good to propagate the original error code, so
let's not do `? -EINVAL : 0` logic
> + break;
> + case BPF_FUNC_timer_set_callback:
> err = __check_func_call(env, insn, insn_idx_p, meta.subprogno,
> - set_find_vma_callback_state);
> - if (err < 0)
> - return -EINVAL;
> - }
> -
> - if (func_id == BPF_FUNC_snprintf) {
> + set_timer_callback_state) ? -EINVAL : 0;
> + break;
> + case BPF_FUNC_find_vma:
> + err = __check_func_call(env, insn, insn_idx_p, meta.subprogno,
> + set_find_vma_callback_state) ? -EINVAL : 0;
> + break;
> + case BPF_FUNC_snprintf:
> err = check_bpf_snprintf_call(env, regs);
> - if (err < 0)
> - return err;
> + break;
> + case BPF_FUNC_loop:
> + err = __check_func_call(env, insn, insn_idx_p, meta.subprogno,
> + set_loop_callback_state) ? -EINVAL : 0;
> + break;
> + default:
> + err = 0;
same, err is already zero if no error so far was found
> }
>
> + if (err)
> + return err;
> +
[...]
