| Message ID | 20220806014603.1771-4-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Don't reinit map value in prealloc_lru_pop | expand |
On Sat, 6 Aug 2022 at 03:46, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > Add a regression test to check against invalid check_and_init_map_value > call inside prealloc_lru_pop. > > To actually observe a kind of problem this can cause, set debug to 1 > when running the test locally without the fix. Then, just observe the > refcount which keeps increasing on each run of the test. With timers or > spin locks, it would cause unpredictable results when racing. > > ... > > bash-5.1# ./test_progs -t lru_bug > test_progs-192 [000] d..21 354.838821: bpf_trace_printk: ref: 4 > test_progs-192 [000] d..21 354.842824: bpf_trace_printk: ref: 5 > bash-5.1# ./test_pogs -t lru_bug > test_progs-193 [000] d..21 356.722813: bpf_trace_printk: ref: 5 > test_progs-193 [000] d..21 356.727071: bpf_trace_printk: ref: 6 > > ... and so on. > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > .../selftests/bpf/prog_tests/lru_bug.c | 19 ++++++ > tools/testing/selftests/bpf/progs/lru_bug.c | 67 +++++++++++++++++++ > 2 files changed, 86 insertions(+) > create mode 100644 tools/testing/selftests/bpf/prog_tests/lru_bug.c > create mode 100644 tools/testing/selftests/bpf/progs/lru_bug.c > > diff --git a/tools/testing/selftests/bpf/prog_tests/lru_bug.c b/tools/testing/selftests/bpf/prog_tests/lru_bug.c > new file mode 100644 > index 000000000000..e77b2d9469cb > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/lru_bug.c > @@ -0,0 +1,19 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <test_progs.h> > + > +#include "lru_bug.skel.h" > + > +void test_lru_bug(void) CI is failing because map_kptr and this test both want to observe refcount when it is not being touched by either, so marking this serial_ would fix it (map_kptr takes time so it is better for it to run in parallel mode). I will wait for the discussion to conclude before respinning.
diff --git a/tools/testing/selftests/bpf/prog_tests/lru_bug.c b/tools/testing/selftests/bpf/prog_tests/lru_bug.c new file mode 100644 index 000000000000..e77b2d9469cb --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/lru_bug.c @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <test_progs.h> + +#include "lru_bug.skel.h" + +void test_lru_bug(void) +{ + struct lru_bug *skel; + int ret; + + skel = lru_bug__open_and_load(); + if (!ASSERT_OK_PTR(skel, "lru_bug__open_and_load")) + return; + ret = lru_bug__attach(skel); + if (!ASSERT_OK(ret, "lru_bug__attach")) + return; + usleep(1); + ASSERT_OK(skel->data->result, "prealloc_lru_pop doesn't call check_and_init_map_value"); +} diff --git a/tools/testing/selftests/bpf/progs/lru_bug.c b/tools/testing/selftests/bpf/progs/lru_bug.c new file mode 100644 index 000000000000..35cbbe7aba9e --- /dev/null +++ b/tools/testing/selftests/bpf/progs/lru_bug.c @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <vmlinux.h> +#include <bpf/bpf_tracing.h> +#include <bpf/bpf_helpers.h> + +struct map_value { + struct prog_test_ref_kfunc __kptr_ref *ptr; +}; + +struct { + __uint(type, BPF_MAP_TYPE_LRU_HASH); + __uint(max_entries, 1); + __type(key, int); + __type(value, struct map_value); +} lru_map SEC(".maps"); + +extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym; +extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *s) __ksym; + +int pid = 0; +const volatile int debug = 0; +int result = 1; + +SEC("fentry/bpf_ktime_get_ns") +int printk(void *ctx) +{ + struct map_value v = {}; + + if (pid == bpf_get_current_task_btf()->pid) + bpf_map_update_elem(&lru_map, &(int){0}, &v, 0); + return 0; +} + +SEC("fentry/do_nanosleep") +int nanosleep(void *ctx) +{ + struct map_value val = {}, *v; + struct prog_test_ref_kfunc *s; + unsigned long l = 0; + + bpf_map_update_elem(&lru_map, &(int){0}, &val, 0); + v = bpf_map_lookup_elem(&lru_map, &(int){0}); + if (!v) + return 0; + bpf_map_delete_elem(&lru_map, &(int){0}); + s = bpf_kfunc_call_test_acquire(&l); + if (!s) + return 0; + if (debug) + bpf_printk("ref: %d\n", s->cnt.refs.counter); + s = bpf_kptr_xchg(&v->ptr, s); + if (s) + bpf_kfunc_call_test_release(s); + pid = bpf_get_current_task_btf()->pid; + bpf_ktime_get_ns(); + if (debug) { + s = bpf_kfunc_call_test_acquire(&l); + if (!s) + return 0; + bpf_printk("ref: %d\n", s->cnt.refs.counter); + bpf_kfunc_call_test_release(s); + } + result = !v->ptr; + return 0; +} + +char _license[] SEC("license") = "GPL";
Add a regression test to check against invalid check_and_init_map_value call inside prealloc_lru_pop. To actually observe a kind of problem this can cause, set debug to 1 when running the test locally without the fix. Then, just observe the refcount which keeps increasing on each run of the test. With timers or spin locks, it would cause unpredictable results when racing. ... bash-5.1# ./test_progs -t lru_bug test_progs-192 [000] d..21 354.838821: bpf_trace_printk: ref: 4 test_progs-192 [000] d..21 354.842824: bpf_trace_printk: ref: 5 bash-5.1# ./test_pogs -t lru_bug test_progs-193 [000] d..21 356.722813: bpf_trace_printk: ref: 5 test_progs-193 [000] d..21 356.727071: bpf_trace_printk: ref: 6 ... and so on. Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- .../selftests/bpf/prog_tests/lru_bug.c | 19 ++++++ tools/testing/selftests/bpf/progs/lru_bug.c | 67 +++++++++++++++++++ 2 files changed, 86 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/lru_bug.c create mode 100644 tools/testing/selftests/bpf/progs/lru_bug.c