All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Andrii Nakryiko <andrii@kernel.org>,
	bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
	kernel-team@fb.com, Tejun Heo <tj@kernel.org>
Subject: Re: [PATCH bpf-next 14/17] bpf: implement number iterator
Date: Sat, 4 Mar 2023 15:27:57 -0800	[thread overview]
Message-ID: <CAEf4BzbNc24jVa5LG8Qag7wDWf-MCa+x4Gu6ecJw4tfRu-tyNA@mail.gmail.com> (raw)
In-Reply-To: <20230304202143.6d7dif64nhybxf6h@MacBook-Pro-6.local>

On Sat, Mar 4, 2023 at 12:21 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> 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.

  reply	other threads:[~2023-03-04 23:28 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-02 23:49 [PATCH bpf-next 00/17] BPF open-coded iterators Andrii Nakryiko
2023-03-02 23:49 ` [PATCH bpf-next 01/17] bpf: improve stack slot state printing Andrii Nakryiko
2023-03-02 23:50 ` [PATCH bpf-next 02/17] bpf: improve regsafe() checks for PTR_TO_{MEM,BUF,TP_BUFFER} Andrii Nakryiko
2023-03-02 23:50 ` [PATCH bpf-next 03/17] selftests/bpf: enhance align selftest's expected log matching Andrii Nakryiko
2023-03-02 23:50 ` [PATCH bpf-next 04/17] bpf: honor env->test_state_freq flag in is_state_visited() Andrii Nakryiko
2023-03-02 23:50 ` [PATCH bpf-next 05/17] selftests/bpf: adjust log_fixup's buffer size for proper truncation Andrii Nakryiko
2023-03-02 23:50 ` [PATCH bpf-next 06/17] bpf: clean up visit_insn()'s instruction processing Andrii Nakryiko
2023-03-02 23:50 ` [PATCH bpf-next 07/17] bpf: fix visit_insn()'s detection of BPF_FUNC_timer_set_callback helper Andrii Nakryiko
2023-03-02 23:50 ` [PATCH bpf-next 08/17] bpf: ensure that r0 is marked scratched after any function call Andrii Nakryiko
2023-03-02 23:50 ` [PATCH bpf-next 09/17] bpf: move kfunc_call_arg_meta higher in the file Andrii Nakryiko
2023-03-02 23:50 ` [PATCH bpf-next 10/17] bpf: mark PTR_TO_MEM as non-null register type Andrii Nakryiko
2023-03-02 23:50 ` [PATCH bpf-next 11/17] bpf: generalize dynptr_get_spi to be usable for iters Andrii Nakryiko
2023-03-02 23:50 ` [PATCH bpf-next 12/17] bpf: add support for fixed-size memory pointer returns for kfuncs Andrii Nakryiko
2023-03-02 23:50 ` [PATCH bpf-next 13/17] bpf: add support for open-coded iterator loops Andrii Nakryiko
2023-03-04 20:02   ` Alexei Starovoitov
2023-03-04 23:27     ` Andrii Nakryiko
2023-03-05 23:46       ` Alexei Starovoitov
2023-03-07 21:54         ` Andrii Nakryiko
2023-03-02 23:50 ` [PATCH bpf-next 14/17] bpf: implement number iterator Andrii Nakryiko
2023-03-04 20:21   ` Alexei Starovoitov
2023-03-04 23:27     ` Andrii Nakryiko [this message]
2023-03-05 23:49       ` Alexei Starovoitov
2023-03-02 23:50 ` [PATCH bpf-next 15/17] selftests/bpf: add bpf_for_each(), bpf_for(), and bpf_repeat() macros Andrii Nakryiko
2023-03-04 20:34   ` Alexei Starovoitov
2023-03-04 23:28     ` Andrii Nakryiko
2023-03-06  0:12       ` Alexei Starovoitov
2023-03-07 21:54         ` Andrii Nakryiko
2023-03-02 23:50 ` [PATCH bpf-next 16/17] selftests/bpf: add iterators tests Andrii Nakryiko
2023-03-04 20:39   ` Alexei Starovoitov
2023-03-04 23:29     ` Andrii Nakryiko
2023-03-06  0:14       ` Alexei Starovoitov
2023-03-04 21:09   ` Jiri Olsa
2023-03-04 23:29     ` Andrii Nakryiko
2023-03-02 23:50 ` [PATCH bpf-next 17/17] selftests/bpf: add number iterator tests Andrii Nakryiko
2023-03-04 19:30 ` [PATCH bpf-next 00/17] BPF open-coded iterators patchwork-bot+netdevbpf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAEf4BzbNc24jVa5LG8Qag7wDWf-MCa+x4Gu6ecJw4tfRu-tyNA@mail.gmail.com \
    --to=andrii.nakryiko@gmail.com \
    --cc=alexei.starovoitov@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=kernel-team@fb.com \
    --cc=tj@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.