All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	 Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	 Martin KaFai Lau <martin.lau@kernel.org>,
	Dave Marchevsky <davemarchevsky@meta.com>
Subject: Re: [PATCH bpf-next v10 22/24] selftests/bpf: Add BPF linked list API tests
Date: Fri, 20 Oct 2023 16:51:35 +0200	[thread overview]
Message-ID: <CAP01T75YbRK65vi9zgZpZmqrkd0bFWhm3ydKL3Snt2ueb7H7RQ@mail.gmail.com> (raw)
In-Reply-To: <CAEf4BzZZrRW7MQOEhJ28sx0N_F-o0OzT+EEnvKh1h3inXhpYEQ@mail.gmail.com>

On Fri, 20 Oct 2023 at 02:15, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Oct 11, 2023 at 4:02 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > On Thu, 12 Oct 2023 at 00:44, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Thu, Nov 17, 2022 at 5:57 PM Kumar Kartikeya Dwivedi
> > > <memxor@gmail.com> wrote:
> > > >
> > > > Include various tests covering the success and failure cases. Also, run
> > > > the success cases at runtime to verify correctness of linked list
> > > > manipulation routines, in addition to ensuring successful verification.
> > > >
> > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > > ---
> > > >  tools/testing/selftests/bpf/DENYLIST.aarch64  |   1 +
> > > >  tools/testing/selftests/bpf/DENYLIST.s390x    |   1 +
> > > >  .../selftests/bpf/prog_tests/linked_list.c    | 255 ++++++++
> > > >  .../testing/selftests/bpf/progs/linked_list.c | 370 +++++++++++
> > > >  .../testing/selftests/bpf/progs/linked_list.h |  56 ++
> > > >  .../selftests/bpf/progs/linked_list_fail.c    | 581 ++++++++++++++++++
> > > >  6 files changed, 1264 insertions(+)
> > > >  create mode 100644 tools/testing/selftests/bpf/prog_tests/linked_list.c
> > > >  create mode 100644 tools/testing/selftests/bpf/progs/linked_list.c
> > > >  create mode 100644 tools/testing/selftests/bpf/progs/linked_list.h
> > > >  create mode 100644 tools/testing/selftests/bpf/progs/linked_list_fail.c
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/DENYLIST.aarch64 b/tools/testing/selftests/bpf/DENYLIST.aarch64
> > > > index 09416d5d2e33..affc5aebbf0f 100644
> > > > --- a/tools/testing/selftests/bpf/DENYLIST.aarch64
> > > > +++ b/tools/testing/selftests/bpf/DENYLIST.aarch64
> > > > @@ -38,6 +38,7 @@ kprobe_multi_test/skel_api                       # kprobe_multi__attach unexpect
> > > >  ksyms_module/libbpf                              # 'bpf_testmod_ksym_percpu': not found in kernel BTF
> > > >  ksyms_module/lskel                               # test_ksyms_module_lskel__open_and_load unexpected error: -2
> > > >  libbpf_get_fd_by_id_opts                         # test_libbpf_get_fd_by_id_opts__attach unexpected error: -524 (errno 524)
> > > > +linked_list
> > > >  lookup_key                                       # test_lookup_key__attach unexpected error: -524 (errno 524)
> > > >  lru_bug                                          # lru_bug__attach unexpected error: -524 (errno 524)
> > > >  modify_return                                    # modify_return__attach failed unexpected error: -524 (errno 524)
> > > > diff --git a/tools/testing/selftests/bpf/DENYLIST.s390x b/tools/testing/selftests/bpf/DENYLIST.s390x
> > > > index be4e3d47ea3e..072243af93b0 100644
> > > > --- a/tools/testing/selftests/bpf/DENYLIST.s390x
> > > > +++ b/tools/testing/selftests/bpf/DENYLIST.s390x
> > > > @@ -33,6 +33,7 @@ ksyms_module                             # test_ksyms_module__open_and_load unex
> > > >  ksyms_module_libbpf                      # JIT does not support calling kernel function                                (kfunc)
> > > >  ksyms_module_lskel                       # test_ksyms_module_lskel__open_and_load unexpected error: -9                 (?)
> > > >  libbpf_get_fd_by_id_opts                 # failed to attach: ERROR: strerror_r(-524)=22                                (trampoline)
> > > > +linked_list                             # JIT does not support calling kernel function                                (kfunc)
> > > >  lookup_key                               # JIT does not support calling kernel function                                (kfunc)
> > > >  lru_bug                                  # prog 'printk': failed to auto-attach: -524
> > > >  map_kptr                                 # failed to open_and_load program: -524 (trampoline)
> > > > diff --git a/tools/testing/selftests/bpf/prog_tests/linked_list.c b/tools/testing/selftests/bpf/prog_tests/linked_list.c
> > > > new file mode 100644
> > > > index 000000000000..41e588807321
> > > > --- /dev/null
> > > > +++ b/tools/testing/selftests/bpf/prog_tests/linked_list.c
> > > > @@ -0,0 +1,255 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +#include <test_progs.h>
> > > > +#include <network_helpers.h>
> > > > +
> > > > +#include "linked_list.skel.h"
> > > > +#include "linked_list_fail.skel.h"
> > > > +
> > > > +static char log_buf[1024 * 1024];
> > > > +
> > > > +static struct {
> > > > +       const char *prog_name;
> > > > +       const char *err_msg;
> > > > +} linked_list_fail_tests[] = {
> > > > +#define TEST(test, off) \
> > > > +       { #test "_missing_lock_push_front", \
> > > > +         "bpf_spin_lock at off=" #off " must be held for bpf_list_head" }, \
> > > > +       { #test "_missing_lock_push_back", \
> > > > +         "bpf_spin_lock at off=" #off " must be held for bpf_list_head" }, \
> > > > +       { #test "_missing_lock_pop_front", \
> > > > +         "bpf_spin_lock at off=" #off " must be held for bpf_list_head" }, \
> > > > +       { #test "_missing_lock_pop_back", \
> > > > +         "bpf_spin_lock at off=" #off " must be held for bpf_list_head" },
> > > > +       TEST(kptr, 32)
> > > > +       TEST(global, 16)
> > > > +       TEST(map, 0)
> > > > +       TEST(inner_map, 0)
> > > > +#undef TEST
> > > > +#define TEST(test, op) \
> > > > +       { #test "_kptr_incorrect_lock_" #op, \
> > > > +         "held lock and object are not in the same allocation\n" \
> > > > +         "bpf_spin_lock at off=32 must be held for bpf_list_head" }, \
> > > > +       { #test "_global_incorrect_lock_" #op, \
> > > > +         "held lock and object are not in the same allocation\n" \
> > > > +         "bpf_spin_lock at off=16 must be held for bpf_list_head" }, \
> > > > +       { #test "_map_incorrect_lock_" #op, \
> > > > +         "held lock and object are not in the same allocation\n" \
> > > > +         "bpf_spin_lock at off=0 must be held for bpf_list_head" }, \
> > > > +       { #test "_inner_map_incorrect_lock_" #op, \
> > > > +         "held lock and object are not in the same allocation\n" \
> > > > +         "bpf_spin_lock at off=0 must be held for bpf_list_head" },
> > > > +       TEST(kptr, push_front)
> > > > +       TEST(kptr, push_back)
> > > > +       TEST(kptr, pop_front)
> > > > +       TEST(kptr, pop_back)
> > > > +       TEST(global, push_front)
> > > > +       TEST(global, push_back)
> > > > +       TEST(global, pop_front)
> > > > +       TEST(global, pop_back)
> > > > +       TEST(map, push_front)
> > > > +       TEST(map, push_back)
> > > > +       TEST(map, pop_front)
> > > > +       TEST(map, pop_back)
> > > > +       TEST(inner_map, push_front)
> > > > +       TEST(inner_map, push_back)
> > > > +       TEST(inner_map, pop_front)
> > > > +       TEST(inner_map, pop_back)
> > > > +#undef TEST
> > > > +       { "map_compat_kprobe", "tracing progs cannot use bpf_list_head yet" },
> > > > +       { "map_compat_kretprobe", "tracing progs cannot use bpf_list_head yet" },
> > > > +       { "map_compat_tp", "tracing progs cannot use bpf_list_head yet" },
> > > > +       { "map_compat_perf", "tracing progs cannot use bpf_list_head yet" },
> > > > +       { "map_compat_raw_tp", "tracing progs cannot use bpf_list_head yet" },
> > > > +       { "map_compat_raw_tp_w", "tracing progs cannot use bpf_list_head yet" },
> > > > +       { "obj_type_id_oor", "local type ID argument must be in range [0, U32_MAX]" },
> > > > +       { "obj_new_no_composite", "bpf_obj_new type ID argument must be of a struct" },
> > > > +       { "obj_new_no_struct", "bpf_obj_new type ID argument must be of a struct" },
> > > > +       { "obj_drop_non_zero_off", "R1 must have zero offset when passed to release func" },
> > > > +       { "new_null_ret", "R0 invalid mem access 'ptr_or_null_'" },
> > > > +       { "obj_new_acq", "Unreleased reference id=" },
> > > > +       { "use_after_drop", "invalid mem access 'scalar'" },
> > > > +       { "ptr_walk_scalar", "type=scalar expected=percpu_ptr_" },
> > > > +       { "direct_read_lock", "direct access to bpf_spin_lock is disallowed" },
> > > > +       { "direct_write_lock", "direct access to bpf_spin_lock is disallowed" },
> > > > +       { "direct_read_head", "direct access to bpf_list_head is disallowed" },
> > > > +       { "direct_write_head", "direct access to bpf_list_head is disallowed" },
> > > > +       { "direct_read_node", "direct access to bpf_list_node is disallowed" },
> > > > +       { "direct_write_node", "direct access to bpf_list_node is disallowed" },
> > > > +       { "write_after_push_front", "only read is supported" },
> > > > +       { "write_after_push_back", "only read is supported" },
> > > > +       { "use_after_unlock_push_front", "invalid mem access 'scalar'" },
> > > > +       { "use_after_unlock_push_back", "invalid mem access 'scalar'" },
> > > > +       { "double_push_front", "arg#1 expected pointer to allocated object" },
> > > > +       { "double_push_back", "arg#1 expected pointer to allocated object" },
> > > > +       { "no_node_value_type", "bpf_list_node not found at offset=0" },
> > > > +       { "incorrect_value_type",
> > > > +         "operation on bpf_list_head expects arg#1 bpf_list_node at offset=0 in struct foo, "
> > > > +         "but arg is at offset=0 in struct bar" },
> > > > +       { "incorrect_node_var_off", "variable ptr_ access var_off=(0x0; 0xffffffff) disallowed" },
> > > > +       { "incorrect_node_off1", "bpf_list_node not found at offset=1" },
> > > > +       { "incorrect_node_off2", "arg#1 offset=40, but expected bpf_list_node at offset=0 in struct foo" },
> > > > +       { "no_head_type", "bpf_list_head not found at offset=0" },
> > > > +       { "incorrect_head_var_off1", "R1 doesn't have constant offset" },
> > > > +       { "incorrect_head_var_off2", "variable ptr_ access var_off=(0x0; 0xffffffff) disallowed" },
> > > > +       { "incorrect_head_off1", "bpf_list_head not found at offset=17" },
> > > > +       { "incorrect_head_off2", "bpf_list_head not found at offset=1" },
> > > > +       { "pop_front_off",
> > > > +         "15: (bf) r1 = r6                      ; R1_w=ptr_or_null_foo(id=4,ref_obj_id=4,off=40,imm=0) "
> > > > +         "R6_w=ptr_or_null_foo(id=4,ref_obj_id=4,off=40,imm=0) refs=2,4\n"
> > > > +         "16: (85) call bpf_this_cpu_ptr#154\nR1 type=ptr_or_null_ expected=percpu_ptr_" },
> > > > +       { "pop_back_off",
> > > > +         "15: (bf) r1 = r6                      ; R1_w=ptr_or_null_foo(id=4,ref_obj_id=4,off=40,imm=0) "
> > > > +         "R6_w=ptr_or_null_foo(id=4,ref_obj_id=4,off=40,imm=0) refs=2,4\n"
> > > > +         "16: (85) call bpf_this_cpu_ptr#154\nR1 type=ptr_or_null_ expected=percpu_ptr_" },
> > > > +};
> > > > +
> > >
> > > Hey Kumar,
> > >
> > > pop_front_off/pop_back_off validation seems to rely on exact register
> > > usage (r6 in this case) generated by the compiler, while the test
> > > itself is written in C, so really nothing is guaranteed. And that's
> > > exactly what seems to happen to me locally, as in my case compiler
> > > chose to use r7 in this particular spot (see logs below).
> > >
> > > Can you please take a look and try to make it more robust? Ideally we
> > > should probably rewrite BPF program to use inline assembly if we are
> > > to check the exact instruction index and registers.
> >
> > Thanks for the report Andrii.
> > I'll take a look and send a patch to address this.
>
> Friendly ping! Did you get a chance to look at this?
>

Hi Andrii, sorry for the delay. The fix should be relatively simple,
we just need to check whether the offset on the returned pointer is
48.
I have posted a fix here, PTAL. Thanks.

https://lore.kernel.org/bpf/20231020144839.2734006-1-memxor@gmail.com

  reply	other threads:[~2023-10-20 14:52 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-18  1:55 [PATCH bpf-next v10 00/24] Allocated objects, BPF linked lists Kumar Kartikeya Dwivedi
2022-11-18  1:55 ` [PATCH bpf-next v10 01/24] bpf: Fix early return in map_check_btf Kumar Kartikeya Dwivedi
2022-11-18  1:55 ` [PATCH bpf-next v10 02/24] bpf: Do btf_record_free outside map_free callback Kumar Kartikeya Dwivedi
2022-11-18  1:55 ` [PATCH bpf-next v10 03/24] bpf: Free inner_map_meta when btf_record_dup fails Kumar Kartikeya Dwivedi
2022-11-18  1:55 ` [PATCH bpf-next v10 04/24] bpf: Populate field_offs for inner_map_meta Kumar Kartikeya Dwivedi
2022-11-18  1:55 ` [PATCH bpf-next v10 05/24] bpf: Introduce allocated objects support Kumar Kartikeya Dwivedi
2022-11-18  1:55 ` [PATCH bpf-next v10 06/24] bpf: Recognize lock and list fields in allocated objects Kumar Kartikeya Dwivedi
2022-11-18  1:55 ` [PATCH bpf-next v10 07/24] bpf: Verify ownership relationships for user BTF types Kumar Kartikeya Dwivedi
2022-11-18  1:55 ` [PATCH bpf-next v10 08/24] bpf: Allow locking bpf_spin_lock in allocated objects Kumar Kartikeya Dwivedi
2022-11-18  1:55 ` [PATCH bpf-next v10 09/24] bpf: Allow locking bpf_spin_lock global variables Kumar Kartikeya Dwivedi
2022-11-18  1:56 ` [PATCH bpf-next v10 10/24] bpf: Allow locking bpf_spin_lock in inner map values Kumar Kartikeya Dwivedi
2022-11-18  1:56 ` [PATCH bpf-next v10 11/24] bpf: Rewrite kfunc argument handling Kumar Kartikeya Dwivedi
2022-11-18  3:34   ` Alexei Starovoitov
2022-11-18 10:37     ` Kumar Kartikeya Dwivedi
2022-11-18 18:02       ` Alexei Starovoitov
2022-11-18 19:00         ` Kumar Kartikeya Dwivedi
2022-11-18 18:08       ` Alexei Starovoitov
2022-11-18 19:40   ` David Vernet
2022-11-20 19:25     ` Kumar Kartikeya Dwivedi
2022-11-18  1:56 ` [PATCH bpf-next v10 12/24] bpf: Support constant scalar arguments for kfuncs Kumar Kartikeya Dwivedi
2022-11-18  1:56 ` [PATCH bpf-next v10 13/24] bpf: Introduce bpf_obj_new Kumar Kartikeya Dwivedi
2022-11-18  1:56 ` [PATCH bpf-next v10 14/24] bpf: Introduce bpf_obj_drop Kumar Kartikeya Dwivedi
2022-11-18  1:56 ` [PATCH bpf-next v10 15/24] bpf: Permit NULL checking pointer with non-zero fixed offset Kumar Kartikeya Dwivedi
2022-11-18  1:56 ` [PATCH bpf-next v10 16/24] bpf: Introduce single ownership BPF linked list API Kumar Kartikeya Dwivedi
2022-11-21 18:34   ` Nathan Chancellor
2022-11-18  1:56 ` [PATCH bpf-next v10 17/24] bpf: Add 'release on unlock' logic for bpf_list_push_{front,back} Kumar Kartikeya Dwivedi
2022-11-18  1:56 ` [PATCH bpf-next v10 18/24] bpf: Add comments for map BTF matching requirement for bpf_list_head Kumar Kartikeya Dwivedi
2022-11-18  1:56 ` [PATCH bpf-next v10 19/24] selftests/bpf: Add __contains macro to bpf_experimental.h Kumar Kartikeya Dwivedi
2022-11-18  1:56 ` [PATCH bpf-next v10 20/24] selftests/bpf: Update spinlock selftest Kumar Kartikeya Dwivedi
2022-11-18  1:56 ` [PATCH bpf-next v10 21/24] selftests/bpf: Add failure test cases for spin lock pairing Kumar Kartikeya Dwivedi
2022-11-18  1:56 ` [PATCH bpf-next v10 22/24] selftests/bpf: Add BPF linked list API tests Kumar Kartikeya Dwivedi
2023-10-11 22:44   ` Andrii Nakryiko
2023-10-11 23:02     ` Kumar Kartikeya Dwivedi
2023-10-20  0:15       ` Andrii Nakryiko
2023-10-20 14:51         ` Kumar Kartikeya Dwivedi [this message]
2022-11-18  1:56 ` [PATCH bpf-next v10 23/24] selftests/bpf: Add BTF sanity tests Kumar Kartikeya Dwivedi
2022-11-18  1:56 ` [PATCH bpf-next v10 24/24] selftests/bpf: Temporarily disable linked list tests Kumar Kartikeya Dwivedi
2022-11-22 17:24   ` Alexei Starovoitov
2022-11-18  3:40 ` [PATCH bpf-next v10 00/24] Allocated objects, BPF linked lists 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=CAP01T75YbRK65vi9zgZpZmqrkd0bFWhm3ydKL3Snt2ueb7H7RQ@mail.gmail.com \
    --to=memxor@gmail.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=davemarchevsky@meta.com \
    --cc=martin.lau@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.