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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 30 Apr 2020 07:05:33 +0000
From:   Song Liu <songliubraving@...com>
To:     Yonghong Song <yhs@...com>
CC:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        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 Apr 30, 2020, at 12:02 AM, Yonghong Song <yhs@...com> wrote:
> 
> 
> 
> On 4/29/20 10:12 PM, Song Liu 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?
>> Would "__u64 count;" work?
> 
> It is because of "static". This static variable has file scope and the
> compiler COULD remove count+=1 since it does not have any other effect
> other than incrementting itself and nobody uses 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;
> 
> I think this is okay.
> 
> For llvm10, you have to use `__u64 count = 0`.
> For llvm11, you can use "__u64 count", the compiler changed global "common" variable treatment default from as a "common" var
> to as a "bss" var.
> 
> In selftest, we have numerous cases for `__u64 count = 0` style
> definitions and I recommend to use it as well since probably
> quite some people uses llvm10 to compile/run selftests.

Thanks for the explanation. Will send fixed version shortly.

Song