* [PATCH bpf-next] bpf/selftests: Test bpf_d_path on rdonly_mem. @ 2021-12-20 20:12 Hao Luo 2021-12-21 4:28 ` Yonghong Song 0 siblings, 1 reply; 6+ messages in thread From: Hao Luo @ 2021-12-20 20:12 UTC (permalink / raw) To: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann Cc: Martin KaFai Lau, Song Liu, Yonghong Song, KP Singh, bpf, Hao Luo The second parameter of bpf_d_path() can only accept writable memories. rdonly_mem obtained from bpf_per_cpu_ptr() can not be passed into bpf_d_path for modification. This patch adds a selftest to verify this behavior. Signed-off-by: Hao Luo <haoluo@google.com> --- .../testing/selftests/bpf/prog_tests/d_path.c | 22 +++++++++++++- .../bpf/progs/test_d_path_check_rdonly_mem.c | 30 +++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c index 0a577a248d34..f8d8c5a5dfba 100644 --- a/tools/testing/selftests/bpf/prog_tests/d_path.c +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c @@ -9,6 +9,7 @@ #define MAX_FILES 7 #include "test_d_path.skel.h" +#include "test_d_path_check_rdonly_mem.skel.h" static int duration; @@ -99,7 +100,7 @@ static int trigger_fstat_events(pid_t pid) return ret; } -void test_d_path(void) +static void test_d_path_basic(void) { struct test_d_path__bss *bss; struct test_d_path *skel; @@ -155,3 +156,22 @@ void test_d_path(void) cleanup: test_d_path__destroy(skel); } + +static void test_d_path_check_rdonly_mem(void) +{ + struct test_d_path_check_rdonly_mem *skel; + + skel = test_d_path_check_rdonly_mem__open_and_load(); + ASSERT_ERR_PTR(skel, "unexpected load of a prog using d_path to write rdonly_mem\n"); + + test_d_path_check_rdonly_mem__destroy(skel); +} + +void test_d_path(void) +{ + if (test__start_subtest("basic")) + test_d_path_basic(); + + if (test__start_subtest("check_rdonly_mem")) + test_d_path_check_rdonly_mem(); +} diff --git a/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c new file mode 100644 index 000000000000..c7a9655d5850 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c @@ -0,0 +1,30 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2021 Google */ + +#include "vmlinux.h" + +#include "vmlinux.h" +#include <bpf/bpf_helpers.h> +#include <bpf/bpf_tracing.h> + +extern const int bpf_prog_active __ksym; + +SEC("fentry/security_inode_getattr") +int BPF_PROG(d_path_check_rdonly_mem, struct path *path, struct kstat *stat, + __u32 request_mask, unsigned int query_flags) +{ + char *active; + __u32 cpu; + + cpu = bpf_get_smp_processor_id(); + active = (char *)bpf_per_cpu_ptr(&bpf_prog_active, cpu); + if (active) { + /* FAIL here! 'active' is a rdonly_mem. bpf helpers that + * update its arguments can not write into it. + */ + bpf_d_path(path, active, sizeof(int)); + } + return 0; +} + +char _license[] SEC("license") = "GPL"; -- 2.34.1.307.g9b7440fafd-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf/selftests: Test bpf_d_path on rdonly_mem. 2021-12-20 20:12 [PATCH bpf-next] bpf/selftests: Test bpf_d_path on rdonly_mem Hao Luo @ 2021-12-21 4:28 ` Yonghong Song 2021-12-21 20:16 ` Hao Luo 0 siblings, 1 reply; 6+ messages in thread From: Yonghong Song @ 2021-12-21 4:28 UTC (permalink / raw) To: Hao Luo, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann Cc: Martin KaFai Lau, Song Liu, KP Singh, bpf On 12/20/21 12:12 PM, Hao Luo wrote: > The second parameter of bpf_d_path() can only accept writable > memories. rdonly_mem obtained from bpf_per_cpu_ptr() can not > be passed into bpf_d_path for modification. This patch adds > a selftest to verify this behavior. > > Signed-off-by: Hao Luo <haoluo@google.com> > --- > .../testing/selftests/bpf/prog_tests/d_path.c | 22 +++++++++++++- > .../bpf/progs/test_d_path_check_rdonly_mem.c | 30 +++++++++++++++++++ > 2 files changed, 51 insertions(+), 1 deletion(-) > create mode 100644 tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c > > diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c > index 0a577a248d34..f8d8c5a5dfba 100644 > --- a/tools/testing/selftests/bpf/prog_tests/d_path.c > +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c > @@ -9,6 +9,7 @@ > #define MAX_FILES 7 > > #include "test_d_path.skel.h" > +#include "test_d_path_check_rdonly_mem.skel.h" > > static int duration; > > @@ -99,7 +100,7 @@ static int trigger_fstat_events(pid_t pid) > return ret; > } > > -void test_d_path(void) > +static void test_d_path_basic(void) > { > struct test_d_path__bss *bss; > struct test_d_path *skel; > @@ -155,3 +156,22 @@ void test_d_path(void) > cleanup: > test_d_path__destroy(skel); > } > + > +static void test_d_path_check_rdonly_mem(void) > +{ > + struct test_d_path_check_rdonly_mem *skel; > + > + skel = test_d_path_check_rdonly_mem__open_and_load(); > + ASSERT_ERR_PTR(skel, "unexpected load of a prog using d_path to write rdonly_mem\n"); > + > + test_d_path_check_rdonly_mem__destroy(skel); You shouldn't call test_d_path_check_rdonly_mem__destroy(skel) if skel is an ERR_PTR. Maybe if (!ASSERT_ERR_PTR(...)) test_d_path_check_rdonly_mem__destroy(skel); > +} > + > +void test_d_path(void) > +{ > + if (test__start_subtest("basic")) > + test_d_path_basic(); > + > + if (test__start_subtest("check_rdonly_mem")) > + test_d_path_check_rdonly_mem(); > +} > diff --git a/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c > new file mode 100644 > index 000000000000..c7a9655d5850 > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c > @@ -0,0 +1,30 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (c) 2021 Google */ > + > +#include "vmlinux.h" > + > +#include "vmlinux.h" duplicated vmlinux.h. > +#include <bpf/bpf_helpers.h> > +#include <bpf/bpf_tracing.h> > + > +extern const int bpf_prog_active __ksym; > + > +SEC("fentry/security_inode_getattr") > +int BPF_PROG(d_path_check_rdonly_mem, struct path *path, struct kstat *stat, > + __u32 request_mask, unsigned int query_flags) > +{ > + char *active; int *active? It may not matter since the program is rejected by the kernel but with making it conforms to kernel definition we have one less thing to worry about the verification. > + __u32 cpu; > + > + cpu = bpf_get_smp_processor_id(); > + active = (char *)bpf_per_cpu_ptr(&bpf_prog_active, cpu); int * > + if (active) { > + /* FAIL here! 'active' is a rdonly_mem. bpf helpers that 'active' points to readonly memory. > + * update its arguments can not write into it. > + */ > + bpf_d_path(path, active, sizeof(int)); > + } > + return 0; > +} > + > +char _license[] SEC("license") = "GPL"; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf/selftests: Test bpf_d_path on rdonly_mem. 2021-12-21 4:28 ` Yonghong Song @ 2021-12-21 20:16 ` Hao Luo 2021-12-21 22:29 ` Yonghong Song 2021-12-22 0:24 ` Andrii Nakryiko 0 siblings, 2 replies; 6+ messages in thread From: Hao Luo @ 2021-12-21 20:16 UTC (permalink / raw) To: Yonghong Song Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Martin KaFai Lau, Song Liu, KP Singh, bpf On Mon, Dec 20, 2021 at 8:28 PM Yonghong Song <yhs@fb.com> wrote: > > > > On 12/20/21 12:12 PM, Hao Luo wrote: > > The second parameter of bpf_d_path() can only accept writable > > memories. rdonly_mem obtained from bpf_per_cpu_ptr() can not > > be passed into bpf_d_path for modification. This patch adds > > a selftest to verify this behavior. > > > > Signed-off-by: Hao Luo <haoluo@google.com> > > --- > > .../testing/selftests/bpf/prog_tests/d_path.c | 22 +++++++++++++- > > .../bpf/progs/test_d_path_check_rdonly_mem.c | 30 +++++++++++++++++++ > > 2 files changed, 51 insertions(+), 1 deletion(-) > > create mode 100644 tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c > > index 0a577a248d34..f8d8c5a5dfba 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/d_path.c > > +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c > > @@ -9,6 +9,7 @@ > > #define MAX_FILES 7 > > > > #include "test_d_path.skel.h" > > +#include "test_d_path_check_rdonly_mem.skel.h" > > > > static int duration; > > > > @@ -99,7 +100,7 @@ static int trigger_fstat_events(pid_t pid) > > return ret; > > } > > > > -void test_d_path(void) > > +static void test_d_path_basic(void) > > { > > struct test_d_path__bss *bss; > > struct test_d_path *skel; > > @@ -155,3 +156,22 @@ void test_d_path(void) > > cleanup: > > test_d_path__destroy(skel); > > } > > + > > +static void test_d_path_check_rdonly_mem(void) > > +{ > > + struct test_d_path_check_rdonly_mem *skel; > > + > > + skel = test_d_path_check_rdonly_mem__open_and_load(); > > + ASSERT_ERR_PTR(skel, "unexpected load of a prog using d_path to write rdonly_mem\n"); > > + > > + test_d_path_check_rdonly_mem__destroy(skel); > > You shouldn't call test_d_path_check_rdonly_mem__destroy(skel) if skel > is an ERR_PTR. Maybe > if (!ASSERT_ERR_PTR(...)) > test_d_path_check_rdonly_mem__destroy(skel); > Ack. Will change that. I don't know if it's only me: I find it confusing when figuring out what ASSERT_ERR_PTR(ptr) returns. Is the returned value 'ptr'? or 'ptr != NULL'? or 'err != 0'? I used to think ASSERT-like function/macro returns nothing. I noticed that xxx__destroy has a check for NULL, so I put the destroy function unconditionally. > > +} > > + > > +void test_d_path(void) > > +{ > > + if (test__start_subtest("basic")) > > + test_d_path_basic(); > > + > > + if (test__start_subtest("check_rdonly_mem")) > > + test_d_path_check_rdonly_mem(); > > +} > > diff --git a/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c > > new file mode 100644 > > index 000000000000..c7a9655d5850 > > --- /dev/null > > +++ b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c > > @@ -0,0 +1,30 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* Copyright (c) 2021 Google */ > > + > > +#include "vmlinux.h" > > + > > +#include "vmlinux.h" > > duplicated vmlinux.h. > Thanks. Will fix that. > > +#include <bpf/bpf_helpers.h> > > +#include <bpf/bpf_tracing.h> > > + > > +extern const int bpf_prog_active __ksym; > > + > > +SEC("fentry/security_inode_getattr") > > +int BPF_PROG(d_path_check_rdonly_mem, struct path *path, struct kstat *stat, > > + __u32 request_mask, unsigned int query_flags) > > +{ > > + char *active; > > int *active? > It may not matter since the program is rejected by the kernel but > with making it conforms to kernel definition we have one less thing > to worry about the verification. > Because bpf_d_path() accepts 'char *' instead of 'int *', I need to cast 'active' to 'char *' somewhere, otherwise the compiler will issue a warning. To combine with your comment, maybe the following: int *active; active = (int *)bpf_per_cpu_ptr(...); ... bpf_d_path(path, (char *)active, sizeof(int)); > > + __u32 cpu; > > + > > + cpu = bpf_get_smp_processor_id(); > > + active = (char *)bpf_per_cpu_ptr(&bpf_prog_active, cpu); > > int * > > > + if (active) { > > + /* FAIL here! 'active' is a rdonly_mem. bpf helpers that > > 'active' points to readonly memory. > Ack. > > + * update its arguments can not write into it. > > + */ > > + bpf_d_path(path, active, sizeof(int)); > > + } > > + return 0; > > +} > > + > > +char _license[] SEC("license") = "GPL"; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf/selftests: Test bpf_d_path on rdonly_mem. 2021-12-21 20:16 ` Hao Luo @ 2021-12-21 22:29 ` Yonghong Song 2021-12-22 0:24 ` Andrii Nakryiko 1 sibling, 0 replies; 6+ messages in thread From: Yonghong Song @ 2021-12-21 22:29 UTC (permalink / raw) To: Hao Luo Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Martin KaFai Lau, Song Liu, KP Singh, bpf On 12/21/21 12:16 PM, Hao Luo wrote: > On Mon, Dec 20, 2021 at 8:28 PM Yonghong Song <yhs@fb.com> wrote: >> >> >> >> On 12/20/21 12:12 PM, Hao Luo wrote: >>> The second parameter of bpf_d_path() can only accept writable >>> memories. rdonly_mem obtained from bpf_per_cpu_ptr() can not >>> be passed into bpf_d_path for modification. This patch adds >>> a selftest to verify this behavior. >>> >>> Signed-off-by: Hao Luo <haoluo@google.com> >>> --- >>> .../testing/selftests/bpf/prog_tests/d_path.c | 22 +++++++++++++- >>> .../bpf/progs/test_d_path_check_rdonly_mem.c | 30 +++++++++++++++++++ >>> 2 files changed, 51 insertions(+), 1 deletion(-) >>> create mode 100644 tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c >>> >>> diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c >>> index 0a577a248d34..f8d8c5a5dfba 100644 >>> --- a/tools/testing/selftests/bpf/prog_tests/d_path.c >>> +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c >>> @@ -9,6 +9,7 @@ >>> #define MAX_FILES 7 >>> >>> #include "test_d_path.skel.h" >>> +#include "test_d_path_check_rdonly_mem.skel.h" >>> >>> static int duration; >>> >>> @@ -99,7 +100,7 @@ static int trigger_fstat_events(pid_t pid) >>> return ret; >>> } >>> [...] >>> + >>> +extern const int bpf_prog_active __ksym; >>> + >>> +SEC("fentry/security_inode_getattr") >>> +int BPF_PROG(d_path_check_rdonly_mem, struct path *path, struct kstat *stat, >>> + __u32 request_mask, unsigned int query_flags) >>> +{ >>> + char *active; >> >> int *active? >> It may not matter since the program is rejected by the kernel but >> with making it conforms to kernel definition we have one less thing >> to worry about the verification. >> > > Because bpf_d_path() accepts 'char *' instead of 'int *', I need to > cast 'active' to 'char *' somewhere, otherwise the compiler will issue > a warning. To combine with your comment, maybe the following: > > int *active; > active = (int *)bpf_per_cpu_ptr(...); > ... > bpf_d_path(path, (char *)active, sizeof(int)); This is fine. Thanks! > >>> + __u32 cpu; >>> + >>> + cpu = bpf_get_smp_processor_id(); >>> + active = (char *)bpf_per_cpu_ptr(&bpf_prog_active, cpu); >> >> int * >> >>> + if (active) { >>> + /* FAIL here! 'active' is a rdonly_mem. bpf helpers that >> >> 'active' points to readonly memory. >> > > Ack. > [...] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf/selftests: Test bpf_d_path on rdonly_mem. 2021-12-21 20:16 ` Hao Luo 2021-12-21 22:29 ` Yonghong Song @ 2021-12-22 0:24 ` Andrii Nakryiko 2021-12-22 1:05 ` Hao Luo 1 sibling, 1 reply; 6+ messages in thread From: Andrii Nakryiko @ 2021-12-22 0:24 UTC (permalink / raw) To: Hao Luo Cc: Yonghong Song, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Martin KaFai Lau, Song Liu, KP Singh, bpf On Tue, Dec 21, 2021 at 12:16 PM Hao Luo <haoluo@google.com> wrote: > > On Mon, Dec 20, 2021 at 8:28 PM Yonghong Song <yhs@fb.com> wrote: > > > > > > > > On 12/20/21 12:12 PM, Hao Luo wrote: > > > The second parameter of bpf_d_path() can only accept writable > > > memories. rdonly_mem obtained from bpf_per_cpu_ptr() can not > > > be passed into bpf_d_path for modification. This patch adds > > > a selftest to verify this behavior. > > > > > > Signed-off-by: Hao Luo <haoluo@google.com> > > > --- > > > .../testing/selftests/bpf/prog_tests/d_path.c | 22 +++++++++++++- > > > .../bpf/progs/test_d_path_check_rdonly_mem.c | 30 +++++++++++++++++++ > > > 2 files changed, 51 insertions(+), 1 deletion(-) > > > create mode 100644 tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c > > > index 0a577a248d34..f8d8c5a5dfba 100644 > > > --- a/tools/testing/selftests/bpf/prog_tests/d_path.c > > > +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c > > > @@ -9,6 +9,7 @@ > > > #define MAX_FILES 7 > > > > > > #include "test_d_path.skel.h" > > > +#include "test_d_path_check_rdonly_mem.skel.h" > > > > > > static int duration; > > > > > > @@ -99,7 +100,7 @@ static int trigger_fstat_events(pid_t pid) > > > return ret; > > > } > > > > > > -void test_d_path(void) > > > +static void test_d_path_basic(void) > > > { > > > struct test_d_path__bss *bss; > > > struct test_d_path *skel; > > > @@ -155,3 +156,22 @@ void test_d_path(void) > > > cleanup: > > > test_d_path__destroy(skel); > > > } > > > + > > > +static void test_d_path_check_rdonly_mem(void) > > > +{ > > > + struct test_d_path_check_rdonly_mem *skel; > > > + > > > + skel = test_d_path_check_rdonly_mem__open_and_load(); > > > + ASSERT_ERR_PTR(skel, "unexpected load of a prog using d_path to write rdonly_mem\n"); > > > + > > > + test_d_path_check_rdonly_mem__destroy(skel); > > > > You shouldn't call test_d_path_check_rdonly_mem__destroy(skel) if skel > > is an ERR_PTR. Maybe > > if (!ASSERT_ERR_PTR(...)) > > test_d_path_check_rdonly_mem__destroy(skel); > > > > Ack. Will change that. no need, __destroy() handles NULLs and ERR_PTR just fine, the way you wrote it is totally correct (that's a deliberate nice feature of libbpf's "destructor" APIs) > > I don't know if it's only me: I find it confusing when figuring out > what ASSERT_ERR_PTR(ptr) returns. Is the returned value 'ptr'? or 'ptr > != NULL'? or 'err != 0'? I used to think ASSERT-like function/macro > returns nothing. > You haven't looked at many other selftests, I presume. All the ASSERT_xxx() macros return true/false depending whether the assertion holds or not. ASSERT_ERR_PTR() checks that ptr *is* erroneous (which is NULL and ERR_PTR). If it's not, it returns false. So if (!ASSERT_ERR_PTR(ptr, "short_descriptor")) /* do something if assertion failed */ is a common pattern. Note also "short_descriptor", it's not supposed to be a long descriptive sentences, it's sort of a "codename" of the particular check. It's not illegal to use space-separated sentence, but better to keep it short and identifier-like. > I noticed that xxx__destroy has a check for NULL, so I put the destroy > function unconditionally. > > > > +} > > > + > > > +void test_d_path(void) > > > +{ > > > + if (test__start_subtest("basic")) > > > + test_d_path_basic(); > > > + > > > + if (test__start_subtest("check_rdonly_mem")) > > > + test_d_path_check_rdonly_mem(); > > > +} > > > diff --git a/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c > > > new file mode 100644 > > > index 000000000000..c7a9655d5850 > > > --- /dev/null > > > +++ b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c > > > @@ -0,0 +1,30 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* Copyright (c) 2021 Google */ > > > + > > > +#include "vmlinux.h" > > > + > > > +#include "vmlinux.h" > > > > duplicated vmlinux.h. > > > > Thanks. Will fix that. > > > > +#include <bpf/bpf_helpers.h> > > > +#include <bpf/bpf_tracing.h> > > > + > > > +extern const int bpf_prog_active __ksym; > > > + > > > +SEC("fentry/security_inode_getattr") > > > +int BPF_PROG(d_path_check_rdonly_mem, struct path *path, struct kstat *stat, > > > + __u32 request_mask, unsigned int query_flags) > > > +{ > > > + char *active; > > > > int *active? > > It may not matter since the program is rejected by the kernel but > > with making it conforms to kernel definition we have one less thing > > to worry about the verification. > > > > Because bpf_d_path() accepts 'char *' instead of 'int *', I need to > cast 'active' to 'char *' somewhere, otherwise the compiler will issue > a warning. To combine with your comment, maybe the following: > > int *active; > active = (int *)bpf_per_cpu_ptr(...); > ... > bpf_d_path(path, (char *)active, sizeof(int)); > why not `void *`? > > > + __u32 cpu; > > > + > > > + cpu = bpf_get_smp_processor_id(); > > > + active = (char *)bpf_per_cpu_ptr(&bpf_prog_active, cpu); > > > > int * > > > > > + if (active) { > > > + /* FAIL here! 'active' is a rdonly_mem. bpf helpers that > > > > 'active' points to readonly memory. > > > > Ack. > > > > + * update its arguments can not write into it. > > > + */ > > > + bpf_d_path(path, active, sizeof(int)); > > > + } > > > + return 0; > > > +} > > > + > > > +char _license[] SEC("license") = "GPL"; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf/selftests: Test bpf_d_path on rdonly_mem. 2021-12-22 0:24 ` Andrii Nakryiko @ 2021-12-22 1:05 ` Hao Luo 0 siblings, 0 replies; 6+ messages in thread From: Hao Luo @ 2021-12-22 1:05 UTC (permalink / raw) To: Andrii Nakryiko Cc: Yonghong Song, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Martin KaFai Lau, Song Liu, KP Singh, bpf On Tue, Dec 21, 2021 at 4:24 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Dec 21, 2021 at 12:16 PM Hao Luo <haoluo@google.com> wrote: > > > > On Mon, Dec 20, 2021 at 8:28 PM Yonghong Song <yhs@fb.com> wrote: > > > > > > > > > > > > On 12/20/21 12:12 PM, Hao Luo wrote: > > > > The second parameter of bpf_d_path() can only accept writable > > > > memories. rdonly_mem obtained from bpf_per_cpu_ptr() can not > > > > be passed into bpf_d_path for modification. This patch adds > > > > a selftest to verify this behavior. > > > > > > > > Signed-off-by: Hao Luo <haoluo@google.com> > > > > --- > > > > .../testing/selftests/bpf/prog_tests/d_path.c | 22 +++++++++++++- > > > > .../bpf/progs/test_d_path_check_rdonly_mem.c | 30 +++++++++++++++++++ > > > > 2 files changed, 51 insertions(+), 1 deletion(-) > > > > create mode 100644 tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c > > > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c > > > > index 0a577a248d34..f8d8c5a5dfba 100644 > > > > --- a/tools/testing/selftests/bpf/prog_tests/d_path.c > > > > +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c > > > > @@ -9,6 +9,7 @@ > > > > #define MAX_FILES 7 > > > > > > > > #include "test_d_path.skel.h" > > > > +#include "test_d_path_check_rdonly_mem.skel.h" > > > > > > > > static int duration; > > > > > > > > @@ -99,7 +100,7 @@ static int trigger_fstat_events(pid_t pid) > > > > return ret; > > > > } > > > > > > > > -void test_d_path(void) > > > > +static void test_d_path_basic(void) > > > > { > > > > struct test_d_path__bss *bss; > > > > struct test_d_path *skel; > > > > @@ -155,3 +156,22 @@ void test_d_path(void) > > > > cleanup: > > > > test_d_path__destroy(skel); > > > > } > > > > + > > > > +static void test_d_path_check_rdonly_mem(void) > > > > +{ > > > > + struct test_d_path_check_rdonly_mem *skel; > > > > + > > > > + skel = test_d_path_check_rdonly_mem__open_and_load(); > > > > + ASSERT_ERR_PTR(skel, "unexpected load of a prog using d_path to write rdonly_mem\n"); > > > > + > > > > + test_d_path_check_rdonly_mem__destroy(skel); > > > > > > You shouldn't call test_d_path_check_rdonly_mem__destroy(skel) if skel > > > is an ERR_PTR. Maybe > > > if (!ASSERT_ERR_PTR(...)) > > > test_d_path_check_rdonly_mem__destroy(skel); > > > > > > > Ack. Will change that. > > no need, __destroy() handles NULLs and ERR_PTR just fine, the way you > wrote it is totally correct (that's a deliberate nice feature of > libbpf's "destructor" APIs) > Yep. That's also my understanding. > > > > I don't know if it's only me: I find it confusing when figuring out > > what ASSERT_ERR_PTR(ptr) returns. Is the returned value 'ptr'? or 'ptr > > != NULL'? or 'err != 0'? I used to think ASSERT-like function/macro > > returns nothing. > > > > You haven't looked at many other selftests, I presume. All the > ASSERT_xxx() macros return true/false depending whether the assertion > holds or not. ASSERT_ERR_PTR() checks that ptr *is* erroneous (which > is NULL and ERR_PTR). If it's not, it returns false. So > > if (!ASSERT_ERR_PTR(ptr, "short_descriptor")) > /* do something if assertion failed */ > > is a common pattern. > > Note also "short_descriptor", it's not supposed to be a long > descriptive sentences, it's sort of a "codename" of the particular > check. It's not illegal to use space-separated sentence, but better to > keep it short and identifier-like. > I see. Thanks for the explanation. > > I noticed that xxx__destroy has a check for NULL, so I put the destroy > > function unconditionally. > > > > > > +} > > > > + > > > > +void test_d_path(void) > > > > +{ > > > > + if (test__start_subtest("basic")) > > > > + test_d_path_basic(); > > > > + > > > > + if (test__start_subtest("check_rdonly_mem")) > > > > + test_d_path_check_rdonly_mem(); > > > > +} > > > > diff --git a/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c > > > > new file mode 100644 > > > > index 000000000000..c7a9655d5850 > > > > --- /dev/null > > > > +++ b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c > > > > @@ -0,0 +1,30 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > +/* Copyright (c) 2021 Google */ > > > > + > > > > +#include "vmlinux.h" > > > > + > > > > +#include "vmlinux.h" > > > > > > duplicated vmlinux.h. > > > > > > > Thanks. Will fix that. > > > > > > +#include <bpf/bpf_helpers.h> > > > > +#include <bpf/bpf_tracing.h> > > > > + > > > > +extern const int bpf_prog_active __ksym; > > > > + > > > > +SEC("fentry/security_inode_getattr") > > > > +int BPF_PROG(d_path_check_rdonly_mem, struct path *path, struct kstat *stat, > > > > + __u32 request_mask, unsigned int query_flags) > > > > +{ > > > > + char *active; > > > > > > int *active? > > > It may not matter since the program is rejected by the kernel but > > > with making it conforms to kernel definition we have one less thing > > > to worry about the verification. > > > > > > > Because bpf_d_path() accepts 'char *' instead of 'int *', I need to > > cast 'active' to 'char *' somewhere, otherwise the compiler will issue > > a warning. To combine with your comment, maybe the following: > > > > int *active; > > active = (int *)bpf_per_cpu_ptr(...); > > ... > > bpf_d_path(path, (char *)active, sizeof(int)); > > > > why not `void *`? > 'void *' works. Just haven't thought about that. > > > > + __u32 cpu; > > > > + > > > > + cpu = bpf_get_smp_processor_id(); > > > > + active = (char *)bpf_per_cpu_ptr(&bpf_prog_active, cpu); > > > > > > int * > > > > > > > + if (active) { > > > > + /* FAIL here! 'active' is a rdonly_mem. bpf helpers that > > > > > > 'active' points to readonly memory. > > > > > > > Ack. > > > > > > + * update its arguments can not write into it. > > > > + */ > > > > + bpf_d_path(path, active, sizeof(int)); > > > > + } > > > > + return 0; > > > > +} > > > > + > > > > +char _license[] SEC("license") = "GPL"; ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-12-22 1:06 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-20 20:12 [PATCH bpf-next] bpf/selftests: Test bpf_d_path on rdonly_mem Hao Luo 2021-12-21 4:28 ` Yonghong Song 2021-12-21 20:16 ` Hao Luo 2021-12-21 22:29 ` Yonghong Song 2021-12-22 0:24 ` Andrii Nakryiko 2021-12-22 1:05 ` Hao Luo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).