| Message ID | 20211130142215.1237217-6-houtao1@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | introduce bpf_strncmp() helper | expand |
On Tue, Nov 30, 2021 at 6:07 AM Hou Tao <houtao1@huawei.com> wrote: > > Four test cases are added: > (1) ensure the return value is expected > (2) ensure no const size is rejected > (3) ensure writable str is rejected > (4) ensure no null-terminated str is rejected > > Signed-off-by: Hou Tao <houtao1@huawei.com> > --- > .../selftests/bpf/prog_tests/test_strncmp.c | 170 ++++++++++++++++++ > .../selftests/bpf/progs/strncmp_test.c | 59 ++++++ > 2 files changed, 229 insertions(+) > create mode 100644 tools/testing/selftests/bpf/prog_tests/test_strncmp.c > create mode 100644 tools/testing/selftests/bpf/progs/strncmp_test.c > > diff --git a/tools/testing/selftests/bpf/prog_tests/test_strncmp.c b/tools/testing/selftests/bpf/prog_tests/test_strncmp.c > new file mode 100644 > index 000000000000..3ed54b55f96a > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/test_strncmp.c > @@ -0,0 +1,170 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (C) 2021. Huawei Technologies Co., Ltd */ > +#include <test_progs.h> > +#include "strncmp_test.skel.h" > + > +static struct strncmp_test *strncmp_test_open_and_disable_autoload(void) > +{ > + struct strncmp_test *skel; > + struct bpf_program *prog; > + > + skel = strncmp_test__open(); > + if (libbpf_get_error(skel)) > + return skel; > + > + bpf_object__for_each_program(prog, skel->obj) > + bpf_program__set_autoload(prog, false); I think this is a wrong "code economy". You save few lines of code, but make tests harder to follow. Just do 4 lines of code for each subtest: skel = strncmp_test__open(); if (!ASSERT_OK_PTR(skel, "skel_open")) return; bpf_object__for_each_program(prog, skel->obj) bpf_program__set_autoload(prog, false); It makes tests more self-contained and easier to follow. Also if some tests need to do something slightly different it's easier to modify them, as they are not coupled to some common helper. DRY is good where it makes sense, but it also increases code coupling and more "jumping around" in code, so it shouldn't be applied blindly. > + > + return skel; > +} > + > +static inline int to_tristate_ret(int ret) > +{ > + if (ret > 0) > + return 1; > + if (ret < 0) > + return -1; > + return 0; > +} > + > +static int trigger_strncmp(const struct strncmp_test *skel) > +{ > + struct timespec wait = {.tv_sec = 0, .tv_nsec = 1}; > + > + nanosleep(&wait, NULL); all the other tests are just doing usleep(1), why using this more verbose way? > + return to_tristate_ret(skel->bss->cmp_ret); > +} > + > +/* > + * Compare str and target after making str[i] != target[i]. > + * When exp is -1, make str[i] < target[i] and delta is -1. > + */ > +static void strncmp_full_str_cmp(struct strncmp_test *skel, const char *name, > + int exp) > +{ > + size_t nr = sizeof(skel->bss->str); > + char *str = skel->bss->str; > + int delta = exp; > + int got; > + size_t i; > + > + memcpy(str, skel->rodata->target, nr); > + for (i = 0; i < nr - 1; i++) { > + str[i] += delta; > + > + got = trigger_strncmp(skel); > + ASSERT_EQ(got, exp, name); > + > + str[i] -= delta; > + } > +} > + [...] > diff --git a/tools/testing/selftests/bpf/progs/strncmp_test.c b/tools/testing/selftests/bpf/progs/strncmp_test.c > new file mode 100644 > index 000000000000..8cdf950a0ce1 > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/strncmp_test.c > @@ -0,0 +1,59 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (C) 2021. Huawei Technologies Co., Ltd */ > +#include <stdbool.h> > +#include <linux/types.h> > +#include <linux/bpf.h> > +#include <bpf/bpf_helpers.h> > +#include <bpf/bpf_tracing.h> > + > +#define STRNCMP_STR_SZ 8 > + > +const char target[STRNCMP_STR_SZ] = "EEEEEEE"; > + > +char str[STRNCMP_STR_SZ]; > +int cmp_ret = 0; > +int target_pid = 0; > + > +char bad_target[STRNCMP_STR_SZ]; > +unsigned int bad_cmp_str_size = STRNCMP_STR_SZ; > + > +char _license[] SEC("license") = "GPL"; > + > +static __always_inline bool called_by_target_pid(void) > +{ > + __u32 pid = bpf_get_current_pid_tgid() >> 32; > + > + return pid == target_pid; > +} again, what's the point of this helper? it's used once and you'd actually save the code by doing the following inline: if ((bpf_get_current_pid_tgid() >> 32) != target_pid) return 0; > + > +SEC("tp/syscalls/sys_enter_nanosleep") > +int do_strncmp(void *ctx) > +{ > + if (!called_by_target_pid()) > + return 0; > + > + cmp_ret = bpf_strncmp(str, STRNCMP_STR_SZ, target); > + > + return 0; > +} > + > +SEC("tp/syscalls/sys_enter_nanosleep") > +int strncmp_bad_not_const_str_size(void *ctx) > +{ probably worth leaving a short comment explaining that this program should fail because ... > + cmp_ret = bpf_strncmp(str, bad_cmp_str_size, target); > + return 0; > +} > + > +SEC("tp/syscalls/sys_enter_nanosleep") > +int strncmp_bad_writable_target(void *ctx) > +{ > + cmp_ret = bpf_strncmp(str, STRNCMP_STR_SZ, bad_target); > + return 0; > +} > + > +SEC("tp/syscalls/sys_enter_nanosleep") > +int strncmp_bad_not_null_term_target(void *ctx) > +{ > + cmp_ret = bpf_strncmp(str, STRNCMP_STR_SZ, target); > + return 0; > +} > -- > 2.29.2 >
Hi, On 12/7/2021 11:09 AM, Andrii Nakryiko wrote: >> +static struct strncmp_test *strncmp_test_open_and_disable_autoload(void) >> +{ >> + struct strncmp_test *skel; >> + struct bpf_program *prog; >> + >> + skel = strncmp_test__open(); >> + if (libbpf_get_error(skel)) >> + return skel; >> + >> + bpf_object__for_each_program(prog, skel->obj) >> + bpf_program__set_autoload(prog, false); > I think this is a wrong "code economy". You save few lines of code, > but make tests harder to follow. Just do 4 lines of code for each > subtest: > > skel = strncmp_test__open(); > if (!ASSERT_OK_PTR(skel, "skel_open")) > return; > > bpf_object__for_each_program(prog, skel->obj) > bpf_program__set_autoload(prog, false); > > > It makes tests more self-contained and easier to follow. Also if some > tests need to do something slightly different it's easier to modify > them, as they are not coupled to some common helper. DRY is good where > it makes sense, but it also increases code coupling and more "jumping > around" in code, so it shouldn't be applied blindly. Thanks for your suggestion on DRY topic. Will do in v2. > + > +static int trigger_strncmp(const struct strncmp_test *skel) > +{ > + struct timespec wait = {.tv_sec = 0, .tv_nsec = 1}; > + > + nanosleep(&wait, NULL); > all the other tests are just doing usleep(1), why using this more verbose way? Will do in v2. >> + >> +static __always_inline bool called_by_target_pid(void) >> +{ >> + __u32 pid = bpf_get_current_pid_tgid() >> 32; >> + >> + return pid == target_pid; >> +} > again, what's the point of this helper? it's used once and you'd > actually save the code by doing the following inline: > > if ((bpf_get_current_pid_tgid() >> 32) != target_pid) > return 0; Will do in v2. >> + >> +SEC("tp/syscalls/sys_enter_nanosleep") >> +int do_strncmp(void *ctx) >> +{ >> + if (!called_by_target_pid()) >> + return 0; >> + >> + cmp_ret = bpf_strncmp(str, STRNCMP_STR_SZ, target); >> + >> + return 0; >> +} >> + >> +SEC("tp/syscalls/sys_enter_nanosleep") >> +int strncmp_bad_not_const_str_size(void *ctx) >> +{ > probably worth leaving a short comment explaining that this program > should fail because ... OK. Will do in v2. Regards, Tao
diff --git a/tools/testing/selftests/bpf/prog_tests/test_strncmp.c b/tools/testing/selftests/bpf/prog_tests/test_strncmp.c new file mode 100644 index 000000000000..3ed54b55f96a --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/test_strncmp.c @@ -0,0 +1,170 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (C) 2021. Huawei Technologies Co., Ltd */ +#include <test_progs.h> +#include "strncmp_test.skel.h" + +static struct strncmp_test *strncmp_test_open_and_disable_autoload(void) +{ + struct strncmp_test *skel; + struct bpf_program *prog; + + skel = strncmp_test__open(); + if (libbpf_get_error(skel)) + return skel; + + bpf_object__for_each_program(prog, skel->obj) + bpf_program__set_autoload(prog, false); + + return skel; +} + +static inline int to_tristate_ret(int ret) +{ + if (ret > 0) + return 1; + if (ret < 0) + return -1; + return 0; +} + +static int trigger_strncmp(const struct strncmp_test *skel) +{ + struct timespec wait = {.tv_sec = 0, .tv_nsec = 1}; + + nanosleep(&wait, NULL); + return to_tristate_ret(skel->bss->cmp_ret); +} + +/* + * Compare str and target after making str[i] != target[i]. + * When exp is -1, make str[i] < target[i] and delta is -1. + */ +static void strncmp_full_str_cmp(struct strncmp_test *skel, const char *name, + int exp) +{ + size_t nr = sizeof(skel->bss->str); + char *str = skel->bss->str; + int delta = exp; + int got; + size_t i; + + memcpy(str, skel->rodata->target, nr); + for (i = 0; i < nr - 1; i++) { + str[i] += delta; + + got = trigger_strncmp(skel); + ASSERT_EQ(got, exp, name); + + str[i] -= delta; + } +} + +static void test_strncmp_ret(void) +{ + struct strncmp_test *skel; + int err, got; + + skel = strncmp_test_open_and_disable_autoload(); + if (!ASSERT_OK_PTR(skel, "strncmp_test open")) + return; + + bpf_program__set_autoload(skel->progs.do_strncmp, true); + + err = strncmp_test__load(skel); + if (!ASSERT_EQ(err, 0, "strncmp_test load")) + goto out; + + err = strncmp_test__attach(skel); + if (!ASSERT_EQ(err, 0, "strncmp_test attach")) + goto out; + + skel->bss->target_pid = getpid(); + + /* Empty str */ + skel->bss->str[0] = '\0'; + got = trigger_strncmp(skel); + ASSERT_EQ(got, -1, "strncmp: empty str"); + + /* Same string */ + memcpy(skel->bss->str, skel->rodata->target, sizeof(skel->bss->str)); + got = trigger_strncmp(skel); + ASSERT_EQ(got, 0, "strncmp: same str"); + + /* Not-null-termainted string */ + memcpy(skel->bss->str, skel->rodata->target, sizeof(skel->bss->str)); + skel->bss->str[sizeof(skel->bss->str) - 1] = 'A'; + got = trigger_strncmp(skel); + ASSERT_EQ(got, 1, "strncmp: not-null-term str"); + + strncmp_full_str_cmp(skel, "strncmp: less than", -1); + strncmp_full_str_cmp(skel, "strncmp: greater than", 1); +out: + strncmp_test__destroy(skel); +} + +static void test_strncmp_bad_not_const_str_size(void) +{ + struct strncmp_test *skel; + int err; + + skel = strncmp_test_open_and_disable_autoload(); + if (!ASSERT_OK_PTR(skel, "strncmp_test open")) + return; + + bpf_program__set_autoload(skel->progs.strncmp_bad_not_const_str_size, + true); + + err = strncmp_test__load(skel); + ASSERT_ERR(err, "strncmp_test load bad_not_const_str_size"); + + strncmp_test__destroy(skel); +} + +static void test_strncmp_bad_writable_target(void) +{ + struct strncmp_test *skel; + int err; + + skel = strncmp_test_open_and_disable_autoload(); + if (!ASSERT_OK_PTR(skel, "strncmp_test open")) + return; + + bpf_program__set_autoload(skel->progs.strncmp_bad_writable_target, + true); + + err = strncmp_test__load(skel); + ASSERT_ERR(err, "strncmp_test load bad_writable_target"); + + strncmp_test__destroy(skel); +} + +static void test_strncmp_bad_not_null_term_target(void) +{ + struct strncmp_test *skel; + int err; + + skel = strncmp_test_open_and_disable_autoload(); + if (!ASSERT_OK_PTR(skel, "strncmp_test open")) + return; + + bpf_program__set_autoload(skel->progs.strncmp_bad_not_null_term_target, + true); + skel->rodata->target[sizeof(skel->rodata->target) - 1] = 'A'; + + err = strncmp_test__load(skel); + ASSERT_ERR(err, "strncmp_test load bad_not_null_term_target"); + + strncmp_test__destroy(skel); +} + +void test_test_strncmp(void) +{ + if (test__start_subtest("strncmp_ret")) + test_strncmp_ret(); + if (test__start_subtest("strncmp_bad_not_const_str_size")) + test_strncmp_bad_not_const_str_size(); + if (test__start_subtest("strncmp_bad_writable_target")) + test_strncmp_bad_writable_target(); + if (test__start_subtest("strncmp_bad_not_null_term_target")) + test_strncmp_bad_not_null_term_target(); +} diff --git a/tools/testing/selftests/bpf/progs/strncmp_test.c b/tools/testing/selftests/bpf/progs/strncmp_test.c new file mode 100644 index 000000000000..8cdf950a0ce1 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/strncmp_test.c @@ -0,0 +1,59 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (C) 2021. Huawei Technologies Co., Ltd */ +#include <stdbool.h> +#include <linux/types.h> +#include <linux/bpf.h> +#include <bpf/bpf_helpers.h> +#include <bpf/bpf_tracing.h> + +#define STRNCMP_STR_SZ 8 + +const char target[STRNCMP_STR_SZ] = "EEEEEEE"; + +char str[STRNCMP_STR_SZ]; +int cmp_ret = 0; +int target_pid = 0; + +char bad_target[STRNCMP_STR_SZ]; +unsigned int bad_cmp_str_size = STRNCMP_STR_SZ; + +char _license[] SEC("license") = "GPL"; + +static __always_inline bool called_by_target_pid(void) +{ + __u32 pid = bpf_get_current_pid_tgid() >> 32; + + return pid == target_pid; +} + +SEC("tp/syscalls/sys_enter_nanosleep") +int do_strncmp(void *ctx) +{ + if (!called_by_target_pid()) + return 0; + + cmp_ret = bpf_strncmp(str, STRNCMP_STR_SZ, target); + + return 0; +} + +SEC("tp/syscalls/sys_enter_nanosleep") +int strncmp_bad_not_const_str_size(void *ctx) +{ + cmp_ret = bpf_strncmp(str, bad_cmp_str_size, target); + return 0; +} + +SEC("tp/syscalls/sys_enter_nanosleep") +int strncmp_bad_writable_target(void *ctx) +{ + cmp_ret = bpf_strncmp(str, STRNCMP_STR_SZ, bad_target); + return 0; +} + +SEC("tp/syscalls/sys_enter_nanosleep") +int strncmp_bad_not_null_term_target(void *ctx) +{ + cmp_ret = bpf_strncmp(str, STRNCMP_STR_SZ, target); + return 0; +}
Four test cases are added: (1) ensure the return value is expected (2) ensure no const size is rejected (3) ensure writable str is rejected (4) ensure no null-terminated str is rejected Signed-off-by: Hou Tao <houtao1@huawei.com> --- .../selftests/bpf/prog_tests/test_strncmp.c | 170 ++++++++++++++++++ .../selftests/bpf/progs/strncmp_test.c | 59 ++++++ 2 files changed, 229 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/test_strncmp.c create mode 100644 tools/testing/selftests/bpf/progs/strncmp_test.c