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

Date:   Wed, 29 Apr 2020 05:32:05 +0000
From:   Song Liu <songliubraving@...com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
CC:     "bpf@...r.kernel.org" <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 v7 bpf-next 3/3] bpf: add selftest for BPF_ENABLE_STATS



> On Apr 28, 2020, at 9:57 PM, Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:
> 
> On Tue, Apr 28, 2020 at 08:58:41PM -0700, Song Liu wrote:
>> +
>> +	skel = test_enable_stats__open_and_load();
>> +	if (CHECK(!skel, "skel_open_and_load", "skeleton open/load failed\n"))
>> +		return;
>> +
>> +	stats_fd = bpf_enable_stats(BPF_STATS_RUNTIME_CNT);
> 
> Just realized that the name is wrong.
> The stats are enabling run_cnt and run_time_ns.
> runtime_cnt sounds like 'snark' from 'The Hunting of the Snark' :)
> May be BPF_STATS_RUN_TIME ?

Will fix. 

[...]

>> +
>> +	CHECK(info.run_cnt != count, "check_run_cnt_valid",
>> +	      "invalid run_cnt stats\n");
> 
> what happens if there are other syscalls during for(i<100) loop?
> The count will still match, right?
> Then why 100 ? and why usleep() at all?
> test_enable_stats__attach() will generate at least one syscall.

We don't really need usleep. I was thinking if it matches after 
many calls it really matches... I will remove it.

> 
>> +
>> +cleanup:
>> +	test_enable_stats__destroy(skel);
>> +	close(stats_fd);
> 
> May be close(stats_fd) first.
> Then test_enable_stats__attach(skel); again.
> Generate few more syscalls and check that 'count' incrementing,
> but info.run_cnt doesnt ?
> That check assumes that sysctl is off. Overkill?

I thought about this. However, close(stats_fd) cannot guarantee 
the stats is not enabled by other fd or the sysctl. I think this
will generate noise on specific systems. 

Thanks, 
Song