netdev - Re: [PATCH bpf-next v2] selftests/bpf: test for bpf_get_file

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4Bzand8qSxqmryyxMNg3FNL-pgokJ4taRrtGq07rdbEjsbA@mail.gmail.com>
Date:   Fri, 1 Nov 2019 22:48:49 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Wenbo Zhang <ethercflow@...il.com>
Cc:     bpf <bpf@...r.kernel.org>, Yonghong Song <yhs@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next v2] selftests/bpf: test for bpf_get_file_path()
 from raw tracepoint

On Fri, Nov 1, 2019 at 6:08 AM Wenbo Zhang <ethercflow@...il.com> wrote:
>
> trace fstat events by raw tracepoint sys_enter:newfstat, and handle events
> only produced by fd2path_loadgen, the fd2path_loadgen call fstat on several
> different types of files to test bpf_get_file_path's feature.
> ---

Unless there is a real reason for all this complexity (in which case,
please spell it out in commit or comments), I think this could be so
much simpler.

- you don't have to use perf_buffer to pass data back, just use global data;
- you can add a filter for PID to only capture data triggered by test
process and avoid the noise;
- why all those set_affinity dances? Is it just because you used
existing perf_buffer test which did that to specifically test
perf_buffer delivering data across every CPU core? Seems like your
test doesn't care about that...
- do we really need a separate binary generating hundreds of syscalls?
It's hard to synchronize with test and it seems much simpler to just
trigger necessary syscalls synchronously from the test itself, no?

I have a bunch of more minutia nits, but most of them will go away if
you simplify your testing approach anyway, so I'll postpone them till
then.

>  tools/testing/selftests/bpf/Makefile          |   8 +-
>  tools/testing/selftests/bpf/fd2path_loadgen.c |  75 ++++++++++
>  .../selftests/bpf/prog_tests/get_file_path.c  | 130 ++++++++++++++++++
>  .../selftests/bpf/progs/test_get_file_path.c  |  58 ++++++++
>  4 files changed, 269 insertions(+), 2 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/fd2path_loadgen.c
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/get_file_path.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_get_file_path.c
>

[...]