On Sat, Mar 04, 2023 at 03:27:57PM -0800, Andrii Nakryiko wrote:
> On Sat, Mar 4, 2023 at 12:21 PM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Thu, Mar 02, 2023 at 03:50:12PM -0800, Andrii Nakryiko wrote:
> > >
> > > static enum kfunc_ptr_arg_type
> > > @@ -10278,7 +10288,17 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
> > > if (is_kfunc_arg_uninit(btf, &args[i]))
> > > iter_arg_type |= MEM_UNINIT;
> > >
> > > - ret = process_iter_arg(env, regno, insn_idx, iter_arg_type, meta);
> > > + if (meta->func_id == special_kfunc_list[KF_bpf_iter_num_new] ||
> > > + meta->func_id == special_kfunc_list[KF_bpf_iter_num_next]) {
> > > + iter_arg_type |= ITER_TYPE_NUM;
> > > + } else if (meta->func_id == special_kfunc_list[KF_bpf_iter_num_destroy]) {
> > > + iter_arg_type |= ITER_TYPE_NUM | OBJ_RELEASE;
> >
> > Since OBJ_RELEASE is set pretty late here and kfuncs are not marked with KF_RELEASE,
> > the arg_type_is_release() in check_func_arg_reg_off() won't trigger.
>
> yeah, I had troubles with doing this release using existing scheme.
> KF_RELEASE doesn't work, as it makes some extra assumptions about what
> was acquired, it didn't fit iters. And I didn't have a precedent in
> dynptr to learn from, as RINGBUF dynptr is "acquired" and "released"
> using helper. Basically, we don't have dynptr release kfunc yet.
>
> So I set the OBJ_RELEASE flag for process_iter_arg to do an explicit release.
>
> I'd appreciate guidance on how to do this cleaner. Naive attempt to
> set KF_ACQUIRE for bpf_iter_num_new() and KF_RELEASE for
> bpf_iter_num_destroy() didn't work.
yep. KF_ACQUIRE and KF_RELEASE don't fit here, since they need the register
to be ref_obj_id-ed while here it's in the stack.
The current patch is fine. We can generalize iter and dynptr later in the follow up.
>
> > So I'm confused why there is:
> > + if (arg_type_is_iter(arg_type))
> > + return 0;
> > in the previous patch.
> > Will it ever trigger?
>
> maybe not, just followed what dynptr is doing
>
> >
> > Separate question: What happens when the user does:
> > bpf_iter_destroy(&it);
> > bpf_iter_destroy(&it);
>
> After the first destroy stack slots are marked STACK_INVALID, so next
> bpf_iter_destroy(&it) will complain about not seeing the initialized
> iterator.
>
> >
> > + if (!is_iter_reg_valid_init(env, reg)) {
> > + verbose(env, "expected an initialized iter as arg #%d\n", regno);
> > will trigger, right?
> > I didn't find such selftest.
>
> yep, that's the idea, I just checked, I do have such test, it's in
> iters_state_safety.c:
>
> __failure __msg("expected an initialized iter as arg #1")
> int double_destroy_fail(void *ctx)
See it now. Thanks for checking.
> There is also next_after_destroy_fail, next_without_new_fail, and
> other obvious error conditions. But it would be good for few people to
> check that with a fresh eye. I added them a long time ago, and might
> have missed something.
