Message ID | 20231203204851.388654-3-jolsa@kernel.org (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | BPF |
Headers | show |
Series | bpf: Fix map poke update | expand |
On 12/3/23 3:48 PM, Jiri Olsa wrote: > Adding test that tries to trigger the BUG_ON during early map update > in prog_array_map_poke_run function. > > The idea is to share prog array map between thread that constantly > updates it and another one loading a program that uses that prog > array. > > Eventually we will hit a place where the program is ok to be updated > (poke->tailcall_target_stable check) but the address is still not > registered in kallsyms, so the bpf_arch_text_poke returns -EINVAL > and cause imbalance for the next tail call update check, which will > fail with -EBUSY in bpf_arch_text_poke as described in previous fix. > > Acked-by: Ilya Leoshkevich <iii@linux.ibm.com> > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > .../selftests/bpf/prog_tests/tailcall_poke.c | 74 +++++++++++++++++++ > .../selftests/bpf/progs/tailcall_poke.c | 32 ++++++++ > 2 files changed, 106 insertions(+) > create mode 100644 tools/testing/selftests/bpf/prog_tests/tailcall_poke.c > create mode 100644 tools/testing/selftests/bpf/progs/tailcall_poke.c > > diff --git a/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c b/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c > new file mode 100644 > index 000000000000..f7e2c09fd772 > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c > @@ -0,0 +1,74 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <unistd.h> > +#include <test_progs.h> > +#include "tailcall_poke.skel.h" > + > +#define JMP_TABLE "/sys/fs/bpf/jmp_table" > + > +static int thread_exit; > + > +static void *update(void *arg) > +{ > + __u32 zero = 0, prog1_fd, prog2_fd, map_fd; > + struct tailcall_poke *call = arg; > + > + map_fd = bpf_map__fd(call->maps.jmp_table); > + prog1_fd = bpf_program__fd(call->progs.call1); > + prog2_fd = bpf_program__fd(call->progs.call2); > + > + while (!thread_exit) { > + bpf_map_update_elem(map_fd, &zero, &prog1_fd, BPF_ANY); > + bpf_map_update_elem(map_fd, &zero, &prog2_fd, BPF_ANY); > + } > + > + return NULL; > +} > + > +void test_tailcall_poke(void) > +{ > + struct tailcall_poke *call, *test; > + int err, cnt = 10; > + pthread_t thread; > + > + unlink(JMP_TABLE); > + > + call = tailcall_poke__open_and_load(); > + if (!ASSERT_OK_PTR(call, "tailcall_poke__open")) > + return; > + > + err = bpf_map__pin(call->maps.jmp_table, JMP_TABLE); > + if (!ASSERT_OK(err, "bpf_map__pin")) > + goto out; Just curious. What is the reason having bpf_map__pin() here and below? I tried and it looks like removing bpf_map__pin() and below bpf_map__set_pin_path() will make reproducing the failure hard/impossible. > + > + err = pthread_create(&thread, NULL, update, call); > + if (!ASSERT_OK(err, "new toggler")) > + goto out; > + > + while (cnt--) { > + test = tailcall_poke__open(); > + if (!ASSERT_OK_PTR(test, "tailcall_poke__open")) > + break; > + > + err = bpf_map__set_pin_path(test->maps.jmp_table, JMP_TABLE); > + if (!ASSERT_OK(err, "bpf_map__pin")) { > + tailcall_poke__destroy(test); > + break; > + } > + > + bpf_program__set_autoload(test->progs.test, true); > + bpf_program__set_autoload(test->progs.call1, false); > + bpf_program__set_autoload(test->progs.call2, false); > + > + err = tailcall_poke__load(test); > + tailcall_poke__destroy(test); > + if (!ASSERT_OK(err, "tailcall_poke__load")) > + break; > + } > + > + thread_exit = 1; > + ASSERT_OK(pthread_join(thread, NULL), "pthread_join"); > + > +out: > + bpf_map__unpin(call->maps.jmp_table, JMP_TABLE); > + tailcall_poke__destroy(call); > +} > diff --git a/tools/testing/selftests/bpf/progs/tailcall_poke.c b/tools/testing/selftests/bpf/progs/tailcall_poke.c > new file mode 100644 > index 000000000000..c78b94b75e83 > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/tailcall_poke.c > @@ -0,0 +1,32 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <linux/bpf.h> > +#include <bpf/bpf_helpers.h> > +#include <bpf/bpf_tracing.h> > + > +char _license[] SEC("license") = "GPL"; > + > +struct { > + __uint(type, BPF_MAP_TYPE_PROG_ARRAY); > + __uint(max_entries, 1); > + __uint(key_size, sizeof(__u32)); > + __uint(value_size, sizeof(__u32)); > +} jmp_table SEC(".maps"); > + > +SEC("?fentry/bpf_fentry_test1") > +int BPF_PROG(test, int a) > +{ > + bpf_tail_call_static(ctx, &jmp_table, 0); > + return 0; > +} > + > +SEC("fentry/bpf_fentry_test1") > +int BPF_PROG(call1, int a) > +{ > + return 0; > +} > + > +SEC("fentry/bpf_fentry_test1") > +int BPF_PROG(call2, int a) > +{ > + return 0; > +}
On Mon, Dec 04, 2023 at 09:16:52PM -0800, Yonghong Song wrote: > > On 12/3/23 3:48 PM, Jiri Olsa wrote: > > Adding test that tries to trigger the BUG_ON during early map update > > in prog_array_map_poke_run function. > > > > The idea is to share prog array map between thread that constantly > > updates it and another one loading a program that uses that prog > > array. > > > > Eventually we will hit a place where the program is ok to be updated > > (poke->tailcall_target_stable check) but the address is still not > > registered in kallsyms, so the bpf_arch_text_poke returns -EINVAL > > and cause imbalance for the next tail call update check, which will > > fail with -EBUSY in bpf_arch_text_poke as described in previous fix. > > > > Acked-by: Ilya Leoshkevich <iii@linux.ibm.com> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > .../selftests/bpf/prog_tests/tailcall_poke.c | 74 +++++++++++++++++++ > > .../selftests/bpf/progs/tailcall_poke.c | 32 ++++++++ > > 2 files changed, 106 insertions(+) > > create mode 100644 tools/testing/selftests/bpf/prog_tests/tailcall_poke.c > > create mode 100644 tools/testing/selftests/bpf/progs/tailcall_poke.c > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c b/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c > > new file mode 100644 > > index 000000000000..f7e2c09fd772 > > --- /dev/null > > +++ b/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c > > @@ -0,0 +1,74 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +#include <unistd.h> > > +#include <test_progs.h> > > +#include "tailcall_poke.skel.h" > > + > > +#define JMP_TABLE "/sys/fs/bpf/jmp_table" > > + > > +static int thread_exit; > > + > > +static void *update(void *arg) > > +{ > > + __u32 zero = 0, prog1_fd, prog2_fd, map_fd; > > + struct tailcall_poke *call = arg; > > + > > + map_fd = bpf_map__fd(call->maps.jmp_table); > > + prog1_fd = bpf_program__fd(call->progs.call1); > > + prog2_fd = bpf_program__fd(call->progs.call2); > > + > > + while (!thread_exit) { > > + bpf_map_update_elem(map_fd, &zero, &prog1_fd, BPF_ANY); > > + bpf_map_update_elem(map_fd, &zero, &prog2_fd, BPF_ANY); > > + } > > + > > + return NULL; > > +} > > + > > +void test_tailcall_poke(void) > > +{ > > + struct tailcall_poke *call, *test; > > + int err, cnt = 10; > > + pthread_t thread; > > + > > + unlink(JMP_TABLE); > > + > > + call = tailcall_poke__open_and_load(); > > + if (!ASSERT_OK_PTR(call, "tailcall_poke__open")) > > + return; > > + > > + err = bpf_map__pin(call->maps.jmp_table, JMP_TABLE); > > + if (!ASSERT_OK(err, "bpf_map__pin")) > > + goto out; > > Just curious. What is the reason having bpf_map__pin() here > and below? I tried and it looks like removing bpf_map__pin() > and below bpf_map__set_pin_path() will make reproducing > the failure hard/impossible. yes, it's there to share the jmp_table map between the two skeleton instances, so the update thread changes the same jmp_table map that's used in the skeleton we load in the while loop below I'll add some comments to the test jirka > > > + > > + err = pthread_create(&thread, NULL, update, call); > > + if (!ASSERT_OK(err, "new toggler")) > > + goto out; > > + > > + while (cnt--) { > > + test = tailcall_poke__open(); > > + if (!ASSERT_OK_PTR(test, "tailcall_poke__open")) > > + break; > > + > > + err = bpf_map__set_pin_path(test->maps.jmp_table, JMP_TABLE); > > + if (!ASSERT_OK(err, "bpf_map__pin")) { > > + tailcall_poke__destroy(test); > > + break; > > + } > > + > > + bpf_program__set_autoload(test->progs.test, true); > > + bpf_program__set_autoload(test->progs.call1, false); > > + bpf_program__set_autoload(test->progs.call2, false); > > + > > + err = tailcall_poke__load(test); > > + tailcall_poke__destroy(test); > > + if (!ASSERT_OK(err, "tailcall_poke__load")) > > + break; > > + } > > + > > + thread_exit = 1; > > + ASSERT_OK(pthread_join(thread, NULL), "pthread_join"); > > + > > +out: > > + bpf_map__unpin(call->maps.jmp_table, JMP_TABLE); > > + tailcall_poke__destroy(call); > > +} SNIP
On 12/5/23 3:43 AM, Jiri Olsa wrote: > On Mon, Dec 04, 2023 at 09:16:52PM -0800, Yonghong Song wrote: >> On 12/3/23 3:48 PM, Jiri Olsa wrote: >>> Adding test that tries to trigger the BUG_ON during early map update >>> in prog_array_map_poke_run function. >>> >>> The idea is to share prog array map between thread that constantly >>> updates it and another one loading a program that uses that prog >>> array. >>> >>> Eventually we will hit a place where the program is ok to be updated >>> (poke->tailcall_target_stable check) but the address is still not >>> registered in kallsyms, so the bpf_arch_text_poke returns -EINVAL >>> and cause imbalance for the next tail call update check, which will >>> fail with -EBUSY in bpf_arch_text_poke as described in previous fix. >>> >>> Acked-by: Ilya Leoshkevich <iii@linux.ibm.com> >>> Signed-off-by: Jiri Olsa <jolsa@kernel.org> >>> --- >>> .../selftests/bpf/prog_tests/tailcall_poke.c | 74 +++++++++++++++++++ >>> .../selftests/bpf/progs/tailcall_poke.c | 32 ++++++++ >>> 2 files changed, 106 insertions(+) >>> create mode 100644 tools/testing/selftests/bpf/prog_tests/tailcall_poke.c >>> create mode 100644 tools/testing/selftests/bpf/progs/tailcall_poke.c >>> >>> diff --git a/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c b/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c >>> new file mode 100644 >>> index 000000000000..f7e2c09fd772 >>> --- /dev/null >>> +++ b/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c >>> @@ -0,0 +1,74 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +#include <unistd.h> >>> +#include <test_progs.h> >>> +#include "tailcall_poke.skel.h" >>> + >>> +#define JMP_TABLE "/sys/fs/bpf/jmp_table" >>> + >>> +static int thread_exit; >>> + >>> +static void *update(void *arg) >>> +{ >>> + __u32 zero = 0, prog1_fd, prog2_fd, map_fd; >>> + struct tailcall_poke *call = arg; >>> + >>> + map_fd = bpf_map__fd(call->maps.jmp_table); >>> + prog1_fd = bpf_program__fd(call->progs.call1); >>> + prog2_fd = bpf_program__fd(call->progs.call2); >>> + >>> + while (!thread_exit) { >>> + bpf_map_update_elem(map_fd, &zero, &prog1_fd, BPF_ANY); >>> + bpf_map_update_elem(map_fd, &zero, &prog2_fd, BPF_ANY); >>> + } >>> + >>> + return NULL; >>> +} >>> + >>> +void test_tailcall_poke(void) >>> +{ >>> + struct tailcall_poke *call, *test; >>> + int err, cnt = 10; >>> + pthread_t thread; >>> + >>> + unlink(JMP_TABLE); >>> + >>> + call = tailcall_poke__open_and_load(); >>> + if (!ASSERT_OK_PTR(call, "tailcall_poke__open")) >>> + return; >>> + >>> + err = bpf_map__pin(call->maps.jmp_table, JMP_TABLE); >>> + if (!ASSERT_OK(err, "bpf_map__pin")) >>> + goto out; >> Just curious. What is the reason having bpf_map__pin() here >> and below? I tried and it looks like removing bpf_map__pin() >> and below bpf_map__set_pin_path() will make reproducing >> the failure hard/impossible. > yes, it's there to share the jmp_table map between the two > skeleton instances, so the update thread changes the same > jmp_table map that's used in the skeleton we load in the > while loop below This does make sense. > > I'll add some comments to the test Thanks for explanation. Some comments are definitely helpful! > > jirka > >>> + >>> + err = pthread_create(&thread, NULL, update, call); >>> + if (!ASSERT_OK(err, "new toggler")) >>> + goto out; >>> + >>> + while (cnt--) { >>> + test = tailcall_poke__open(); >>> + if (!ASSERT_OK_PTR(test, "tailcall_poke__open")) >>> + break; >>> + >>> + err = bpf_map__set_pin_path(test->maps.jmp_table, JMP_TABLE); >>> + if (!ASSERT_OK(err, "bpf_map__pin")) { >>> + tailcall_poke__destroy(test); >>> + break; >>> + } >>> + >>> + bpf_program__set_autoload(test->progs.test, true); >>> + bpf_program__set_autoload(test->progs.call1, false); >>> + bpf_program__set_autoload(test->progs.call2, false); >>> + >>> + err = tailcall_poke__load(test); >>> + tailcall_poke__destroy(test); >>> + if (!ASSERT_OK(err, "tailcall_poke__load")) >>> + break; >>> + } >>> + >>> + thread_exit = 1; >>> + ASSERT_OK(pthread_join(thread, NULL), "pthread_join"); >>> + >>> +out: >>> + bpf_map__unpin(call->maps.jmp_table, JMP_TABLE); >>> + tailcall_poke__destroy(call); >>> +} > SNIP
On Tue, Dec 05, 2023 at 08:00:48AM -0800, Yonghong Song wrote: SNIP > > > > +void test_tailcall_poke(void) > > > > +{ > > > > + struct tailcall_poke *call, *test; > > > > + int err, cnt = 10; > > > > + pthread_t thread; > > > > + > > > > + unlink(JMP_TABLE); > > > > + > > > > + call = tailcall_poke__open_and_load(); > > > > + if (!ASSERT_OK_PTR(call, "tailcall_poke__open")) > > > > + return; > > > > + > > > > + err = bpf_map__pin(call->maps.jmp_table, JMP_TABLE); > > > > + if (!ASSERT_OK(err, "bpf_map__pin")) > > > > + goto out; > > > Just curious. What is the reason having bpf_map__pin() here > > > and below? I tried and it looks like removing bpf_map__pin() > > > and below bpf_map__set_pin_path() will make reproducing > > > the failure hard/impossible. > > yes, it's there to share the jmp_table map between the two > > skeleton instances, so the update thread changes the same > > jmp_table map that's used in the skeleton we load in the > > while loop below > > This does make sense. > > > > > I'll add some comments to the test > > Thanks for explanation. Some comments are definitely helpful! np, also looks like I should move this to prog_tests/tailcalls.c, will send new version with that thanks, jirka > > > > > jirka > > > > > > + > > > > + err = pthread_create(&thread, NULL, update, call); > > > > + if (!ASSERT_OK(err, "new toggler")) > > > > + goto out; > > > > + > > > > + while (cnt--) { > > > > + test = tailcall_poke__open(); > > > > + if (!ASSERT_OK_PTR(test, "tailcall_poke__open")) > > > > + break; > > > > + > > > > + err = bpf_map__set_pin_path(test->maps.jmp_table, JMP_TABLE); > > > > + if (!ASSERT_OK(err, "bpf_map__pin")) { > > > > + tailcall_poke__destroy(test); > > > > + break; > > > > + } > > > > + > > > > + bpf_program__set_autoload(test->progs.test, true); > > > > + bpf_program__set_autoload(test->progs.call1, false); > > > > + bpf_program__set_autoload(test->progs.call2, false); > > > > + > > > > + err = tailcall_poke__load(test); > > > > + tailcall_poke__destroy(test); > > > > + if (!ASSERT_OK(err, "tailcall_poke__load")) > > > > + break; > > > > + } > > > > + > > > > + thread_exit = 1; > > > > + ASSERT_OK(pthread_join(thread, NULL), "pthread_join"); > > > > + > > > > +out: > > > > + bpf_map__unpin(call->maps.jmp_table, JMP_TABLE); > > > > + tailcall_poke__destroy(call); > > > > +} > > SNIP
diff --git a/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c b/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c new file mode 100644 index 000000000000..f7e2c09fd772 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <unistd.h> +#include <test_progs.h> +#include "tailcall_poke.skel.h" + +#define JMP_TABLE "/sys/fs/bpf/jmp_table" + +static int thread_exit; + +static void *update(void *arg) +{ + __u32 zero = 0, prog1_fd, prog2_fd, map_fd; + struct tailcall_poke *call = arg; + + map_fd = bpf_map__fd(call->maps.jmp_table); + prog1_fd = bpf_program__fd(call->progs.call1); + prog2_fd = bpf_program__fd(call->progs.call2); + + while (!thread_exit) { + bpf_map_update_elem(map_fd, &zero, &prog1_fd, BPF_ANY); + bpf_map_update_elem(map_fd, &zero, &prog2_fd, BPF_ANY); + } + + return NULL; +} + +void test_tailcall_poke(void) +{ + struct tailcall_poke *call, *test; + int err, cnt = 10; + pthread_t thread; + + unlink(JMP_TABLE); + + call = tailcall_poke__open_and_load(); + if (!ASSERT_OK_PTR(call, "tailcall_poke__open")) + return; + + err = bpf_map__pin(call->maps.jmp_table, JMP_TABLE); + if (!ASSERT_OK(err, "bpf_map__pin")) + goto out; + + err = pthread_create(&thread, NULL, update, call); + if (!ASSERT_OK(err, "new toggler")) + goto out; + + while (cnt--) { + test = tailcall_poke__open(); + if (!ASSERT_OK_PTR(test, "tailcall_poke__open")) + break; + + err = bpf_map__set_pin_path(test->maps.jmp_table, JMP_TABLE); + if (!ASSERT_OK(err, "bpf_map__pin")) { + tailcall_poke__destroy(test); + break; + } + + bpf_program__set_autoload(test->progs.test, true); + bpf_program__set_autoload(test->progs.call1, false); + bpf_program__set_autoload(test->progs.call2, false); + + err = tailcall_poke__load(test); + tailcall_poke__destroy(test); + if (!ASSERT_OK(err, "tailcall_poke__load")) + break; + } + + thread_exit = 1; + ASSERT_OK(pthread_join(thread, NULL), "pthread_join"); + +out: + bpf_map__unpin(call->maps.jmp_table, JMP_TABLE); + tailcall_poke__destroy(call); +} diff --git a/tools/testing/selftests/bpf/progs/tailcall_poke.c b/tools/testing/selftests/bpf/progs/tailcall_poke.c new file mode 100644 index 000000000000..c78b94b75e83 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/tailcall_poke.c @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <linux/bpf.h> +#include <bpf/bpf_helpers.h> +#include <bpf/bpf_tracing.h> + +char _license[] SEC("license") = "GPL"; + +struct { + __uint(type, BPF_MAP_TYPE_PROG_ARRAY); + __uint(max_entries, 1); + __uint(key_size, sizeof(__u32)); + __uint(value_size, sizeof(__u32)); +} jmp_table SEC(".maps"); + +SEC("?fentry/bpf_fentry_test1") +int BPF_PROG(test, int a) +{ + bpf_tail_call_static(ctx, &jmp_table, 0); + return 0; +} + +SEC("fentry/bpf_fentry_test1") +int BPF_PROG(call1, int a) +{ + return 0; +} + +SEC("fentry/bpf_fentry_test1") +int BPF_PROG(call2, int a) +{ + return 0; +}