netdev - Re: [PATCH v8 bpf-next 3/3] bpf: add selftest for BPF_ENABLE

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAEf4BzYeJZHK5hRPpETK+cJFTRx+nDFHv-dhjpOSt=-VP9T5Cg@mail.gmail.com>
Date:   Wed, 29 Apr 2020 23:55:26 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Song Liu <songliubraving@...com>
Cc:     bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH v8 bpf-next 3/3] bpf: add selftest for BPF_ENABLE_STATS

On Wed, Apr 29, 2020 at 10:12 PM Song Liu <songliubraving@...com> wrote:
>
>
>
> > On Apr 29, 2020, at 7:23 PM, Andrii Nakryiko <andrii.nakryiko@...il.com> wrote:
> >
> > On Tue, Apr 28, 2020 at 11:47 PM Song Liu <songliubraving@...com> wrote:
> >>
> >> Add test for BPF_ENABLE_STATS, which should enable run_time_ns stats.
> >>
> >> ~/selftests/bpf# ./test_progs -t enable_stats  -v
> >> test_enable_stats:PASS:skel_open_and_load 0 nsec
> >> test_enable_stats:PASS:get_stats_fd 0 nsec
> >> test_enable_stats:PASS:attach_raw_tp 0 nsec
> >> test_enable_stats:PASS:get_prog_info 0 nsec
> >> test_enable_stats:PASS:check_stats_enabled 0 nsec
> >> test_enable_stats:PASS:check_run_cnt_valid 0 nsec
> >> Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
> >>
> >> Signed-off-by: Song Liu <songliubraving@...com>
> >> ---
> >> .../selftests/bpf/prog_tests/enable_stats.c   | 46 +++++++++++++++++++
> >> .../selftests/bpf/progs/test_enable_stats.c   | 18 ++++++++
> >> 2 files changed, 64 insertions(+)
> >> create mode 100644 tools/testing/selftests/bpf/prog_tests/enable_stats.c
> >> create mode 100644 tools/testing/selftests/bpf/progs/test_enable_stats.c
> >>
> >> diff --git a/tools/testing/selftests/bpf/prog_tests/enable_stats.c b/tools/testing/selftests/bpf/prog_tests/enable_stats.c
> >> new file mode 100644
> >> index 000000000000..cb5e34dcfd42
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/bpf/prog_tests/enable_stats.c
> >> @@ -0,0 +1,46 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +#include <test_progs.h>
> >> +#include <sys/mman.h>
> >
> > is this header used for anything?
>
> Not really, will remove it.
>
> >
> >> +#include "test_enable_stats.skel.h"
> >> +
> >> +void test_enable_stats(void)
> >> +{
> >
> > [...]
> >
> >> +
> >> +char _license[] SEC("license") = "GPL";
> >> +
> >> +static __u64 count;
> >
> > this is actually very unreliable, because compiler might decide to
> > just remove this variable. It should be either `static volatile`, or
> > better use zero-initialized global variable:
> >
> > __u64 count = 0;
>
> Why would compile remove it? Is it because "static" or "no initialized?

because static, which makes compiler assume that no one else can
access it (which is not true for BPF programs).

> Would "__u64 count;" work?

unfortunately, no, libbpf enforces that all global variables are
initialized (uninitialized global variables go into special COM
section, libbpf doesn't support it).

>
> For "__u64 count = 0;", checkpatch.pl generates an error:
>
> ERROR: do not initialise globals to 0
> #92: FILE: tools/testing/selftests/bpf/progs/test_enable_stats.c:11:
> +__u64 count = 0;

ignore checkpatch.pl in this case?

>
> Thanks,
> Song