| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 10 Sep 2020 15:22:10 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Jiri Olsa <jolsa@...nel.org>
Cc: Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andriin@...com>,
Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
Martin KaFai Lau <kafai@...com>,
Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...omium.org>
Subject: Re: [PATCH bpf-next] selftests/bpf: Check trampoline execution in
d_path test
On Thu, Sep 10, 2020 at 5:25 AM Jiri Olsa <jolsa@...nel.org> wrote:
>
> Some kernels builds might inline vfs_getattr call within
> fstat syscall code path, so fentry/vfs_getattr trampoline
> is not called.
>
> I'm not sure how to handle this in some generic way other
> than use some other function, but that might get inlined at
> some point as well.
>
> Adding flags that indicate trampolines were called and failing
> the test if neither of them got called.
>
> $ sudo ./test_progs -t d_path
> test_d_path:PASS:setup 0 nsec
> ...
> trigger_fstat_events:PASS:trigger 0 nsec
> test_d_path:FAIL:124 trampolines not called
> #22 d_path:FAIL
> Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED
>
> If only one trampoline is called, it's still enough to test
> the helper, so only warn about missing trampoline call and
> continue in test.
>
> $ sudo ./test_progs -t d_path -v
> test_d_path:PASS:setup 0 nsec
> ...
> trigger_fstat_events:PASS:trigger 0 nsec
> fentry/vfs_getattr not called
> #22 d_path:OK
> Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
>
> Signed-off-by: Jiri Olsa <jolsa@...hat.com>
> ---
Acked-by: Andrii Nakryiko <andriin@...com>
> .../testing/selftests/bpf/prog_tests/d_path.c | 25 +++++++++++++++----
> .../testing/selftests/bpf/progs/test_d_path.c | 7 ++++++
> 2 files changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
> index fc12e0d445ff..ec15f7d1dd0a 100644
> --- a/tools/testing/selftests/bpf/prog_tests/d_path.c
> +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
> @@ -120,26 +120,41 @@ void test_d_path(void)
> if (err < 0)
> goto cleanup;
>
> + if (!bss->called_stat && !bss->called_close) {
> + PRINT_FAIL("trampolines not called\n");
> + goto cleanup;
> + }
> +
> + if (!bss->called_stat) {
> + fprintf(stdout, "fentry/vfs_getattr not called\n");
> + goto cleanup;
> + }
> +
> + if (!bss->called_close) {
> + fprintf(stdout, "fentry/filp_close not called\n");
> + goto cleanup;
> + }
not sure why you didn't go with `if (CHECK(!bss->called_close, ...`
for these checks, would even save you some typing.
> +
> for (int i = 0; i < MAX_FILES; i++) {
> - CHECK(strncmp(src.paths[i], bss->paths_stat[i], MAX_PATH_LEN),
> + CHECK(bss->called_stat && strncmp(src.paths[i], bss->paths_stat[i], MAX_PATH_LEN),
> "check",
> "failed to get stat path[%d]: %s vs %s\n",
> i, src.paths[i], bss->paths_stat[i]);
> - CHECK(strncmp(src.paths[i], bss->paths_close[i], MAX_PATH_LEN),
> + CHECK(bss->called_close && strncmp(src.paths[i], bss->paths_close[i], MAX_PATH_LEN),
> "check",
> "failed to get close path[%d]: %s vs %s\n",
> i, src.paths[i], bss->paths_close[i]);
> /* The d_path helper returns size plus NUL char, hence + 1 */
> - CHECK(bss->rets_stat[i] != strlen(bss->paths_stat[i]) + 1,
> + CHECK(bss->called_stat && bss->rets_stat[i] != strlen(bss->paths_stat[i]) + 1,
> "check",
> "failed to match stat return [%d]: %d vs %zd [%s]\n",
> i, bss->rets_stat[i], strlen(bss->paths_stat[i]) + 1,
> bss->paths_stat[i]);
> - CHECK(bss->rets_close[i] != strlen(bss->paths_stat[i]) + 1,
> + CHECK(bss->called_close && bss->rets_close[i] != strlen(bss->paths_close[i]) + 1,
> "check",
> "failed to match stat return [%d]: %d vs %zd [%s]\n",
> i, bss->rets_close[i], strlen(bss->paths_close[i]) + 1,
> - bss->paths_stat[i]);
> + bss->paths_close[i]);
those `bss->called_xxx` guard conditions are a bit lost on reading, if
you reordered CHECKs, you could be more explicit:
if (bss->called_stat) {
CHECK(...);
CHECK(...);
}
if (bss->called_close) { ... }
> }
>
> cleanup:
> diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c b/tools/testing/selftests/bpf/progs/test_d_path.c
> index 61f007855649..9e7223b4a555 100644
> --- a/tools/testing/selftests/bpf/progs/test_d_path.c
> +++ b/tools/testing/selftests/bpf/progs/test_d_path.c
> @@ -15,6 +15,9 @@ char paths_close[MAX_FILES][MAX_PATH_LEN] = {};
> int rets_stat[MAX_FILES] = {};
> int rets_close[MAX_FILES] = {};
>
> +int called_stat = 0;
> +int called_close = 0;
> +
> SEC("fentry/vfs_getattr")
> int BPF_PROG(prog_stat, struct path *path, struct kstat *stat,
> __u32 request_mask, unsigned int query_flags)
> @@ -23,6 +26,8 @@ int BPF_PROG(prog_stat, struct path *path, struct kstat *stat,
> __u32 cnt = cnt_stat;
> int ret;
>
> + called_stat = 1;
> +
> if (pid != my_pid)
> return 0;
>
> @@ -42,6 +47,8 @@ int BPF_PROG(prog_close, struct file *file, void *id)
> __u32 cnt = cnt_close;
> int ret;
>
> + called_close = 1;
> +
> if (pid != my_pid)
> return 0;
>
> --
> 2.26.2
>
Powered by blists - more mailing lists