netdev - Re: [PATCH v6 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]

Message-ID: <78ED2E6A-BA51-4A1A-80C9-865215A25760@fb.com>
Date:   Wed, 29 Apr 2020 02:06:15 +0000
From:   Song Liu <songliubraving@...com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
CC:     Yonghong Song <yhs@...com>, bpf <bpf@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH v6 bpf-next 3/3] bpf: add selftest for BPF_ENABLE_STATS



> On Apr 28, 2020, at 5:43 PM, Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:
> 
> On Tue, Apr 28, 2020 at 05:33:54PM -0700, Yonghong Song wrote:
>> 
>> 
>> On 4/28/20 5:29 PM, Song Liu 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
>>> Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
> ...
>>> +static int val = 1;
>>> +
>>> +SEC("raw_tracepoint/sys_enter")
>>> +int test_enable_stats(void *ctx)
>>> +{
>>> +	__u32 key = 0;
>>> +	__u64 *val;
>> 
>> The above two declarations (key/val) are not needed,
>> esp. "val" is shadowing.
>> Maybe the maintainer can fix it up before merging
>> if there is no other changes for this patch set.
>> 
>>> +
>>> +	val += 1;
> 
> I think 'PASSED' above is quite misleading.
> How it can pass when it wasn't incremented?
> The user space test_enable_stats() doesn't check this val.
> Please fix.
> 
> usleep(1000); needs an explanation as well.
> Why 1000 ? It should work with any syscall. like getpid ?
> and with value 1 ?
> Since there is bpf_obj_get_info_by_fd() that usleep()
> is unnecessary. What am I missing?

This test currently doesn't test the value. It simply checks
run_time_ns is none zero. I guess it is good to actually 
test the value. Let me add that.

Thanks,
Son