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