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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200429004340.5y2c3rkr64u43sfg@ast-mbp.dhcp.thefacebook.com>
Date:   Tue, 28 Apr 2020 17:43:40 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     Song Liu <songliubraving@...com>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
        kernel-team@...com
Subject: Re: [PATCH v6 bpf-next 3/3] bpf: add selftest for BPF_ENABLE_STATS

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?