netdev - Re: [PATCH bpf-next 1/6] selftests/bpf: add map/prog type probe helpers

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]

Date:   Fri, 14 Dec 2018 18:37:33 +0000
From:   Quentin Monnet <quentin.monnet@...ronome.com>
To:     Stanislav Fomichev <sdf@...ichev.me>
Cc:     Stanislav Fomichev <sdf@...gle.com>, netdev@...r.kernel.org,
        ast@...nel.org, davem@...emloft.net, daniel@...earbox.net,
        ecree@...arflare.com,
        OSS-drivers Netronome <oss-drivers@...ronome.com>
Subject: Re: [PATCH bpf-next 1/6] selftests/bpf: add map/prog type probe
 helpers

2018-12-14 10:16 UTC-0800 ~ Stanislav Fomichev <sdf@...ichev.me>
> On 12/14, Quentin Monnet wrote:
>> Hi Stanislav,
>>
>> 2018-12-13 11:02 UTC-0800 ~ Stanislav Fomichev <sdf@...gle.com>
>>> Export bpf_map_type_supported() and bpf_prog_type_supported() which
>>> return true/false to indicate kernel support for the appropriate
>>> program or map type. These helpers will be used in the next commits
>>> to selectively skip test_verifier/test_maps tests.
>>>
>>> bpf_map_type_supported() supports only limited set of maps for which we
>>> do fixups in the test_verifier, for unknown maps it falls back to
>>> 'supported'.
>>
>> Why would you fall back on “supported” if it does not know about them?
>> Would that be worth having an enum as a return type (..._SUPPORTED,
>> ..._UNSUPPORTED, ..._UNKNOWN) maybe? Or default to not supported?
> I thought that it's safer for verifier to FAIL in case we forgot to add
> a specific map support to bpf_map_type_supported(). This is not the case
> if we were to use your version where you try to support every map type.
> 
>> Not related - We would need to put a warning somewhere, maybe a comment
>> in the header, that using probes repeatedly in a short amount of time
>> needs to update resources limits (setrlimit()), otherwise probes won't
>> work correctly.
> If we were to move this to libbpf, yes. For tests, I think we include
> bpr_rlimit.h everywhere and things just work :-)

Hmm. I was so focused on bpftool and libbpf that somehow I read you
patch as a proposal to include these probes directly into libbpf. Which,
as you explain (and as I should have read), is not the case. So please
accept my apologies, in this case your decisions (here and in the rest
of the patch) make sense to me :).