| 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
| ||
|
Message-ID: <784f0110-092f-16b6-14fc-003679d9d4db@netronome.com>
Date: Thu, 20 Dec 2018 18:45:01 +0000
From: Quentin Monnet <quentin.monnet@...ronome.com>
To: Stanislav Fomichev <sdf@...ichev.me>
Cc: Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
oss-drivers@...ronome.com,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Jesper Dangaard Brouer <brouer@...hat.com>,
Stanislav Fomichev <sdf@...gle.com>
Subject: Re: [RFC PATCH bpf-next v2 5/9] tools: bpftool: add probes for eBPF
map types
2018-12-20 10:42 UTC-0800 ~ Stanislav Fomichev <sdf@...ichev.me>
> On 12/20, Quentin Monnet wrote:
>> 2018-12-20 10:18 UTC-0800 ~ Stanislav Fomichev <sdf@...ichev.me>
>>> On 12/20, Quentin Monnet wrote:
>>>> 2018-12-20 09:47 UTC-0800 ~ Stanislav Fomichev <sdf@...ichev.me>
>>>>> On 12/20, Quentin Monnet wrote:
>>>>>> Add new probes for eBPF map types, to detect what are the ones available
>>>>>> on the system. Try creating one map of each type, and see if the kernel
>>>>>> complains.
>>>>>>
>>>>>> Sample output:
>>>>>>
>>>>>> # bpftool feature probe kernel
>>>>>> ...
>>>>>> Scanning eBPF map types...
>>>>>> eBPF map_type hash is available
>>>>>> eBPF map_type array is available
>>>>>> eBPF map_type prog_array is available
>>>>>> ...
>>>>>>
>>>>>> # bpftool --json --pretty feature probe kernel
>>>>>> {
>>>>>> ...
>>>>>> "map_types": {
>>>>>> "have_hash_map_type": true,
>>>>>> "have_array_map_type": true,
>>>>>> "have_prog_array_map_type": true,
>>>>>> ...
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> v2:
>>>>>> - Move probes from bpftool to libbpf.
>>>>>> - Remove C-style macros output from this patch.
>>>>>>
>>>>>> Signed-off-by: Quentin Monnet <quentin.monnet@...ronome.com>
>>>>>> ---
>>>>>> tools/bpf/bpftool/feature.c | 26 +++++++++++++++
>>>>>> tools/bpf/bpftool/main.h | 3 ++
>>>>>> tools/bpf/bpftool/map.c | 4 ++-
>>>>>> tools/lib/bpf/libbpf.h | 1 +
>>>>>> tools/lib/bpf/libbpf.map | 1 +
>>>>>> tools/lib/bpf/libbpf_probes.c | 59 +++++++++++++++++++++++++++++++++++
>>>>>> 6 files changed, 93 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
>>>>>> index 3ba0a0a5904c..3c44953ded5a 100644
>>>>>> --- a/tools/bpf/bpftool/feature.c
>>>>>> +++ b/tools/bpf/bpftool/feature.c
>>>>>> @@ -398,6 +398,26 @@ probe_prog_type(enum bpf_prog_type prog_type, int kernel_version,
>>>>>> print_bool_feature(feat_name, plain_desc, res);
>>>>>> }
>>>>>> +static void probe_map_type(enum bpf_map_type map_type)
>>>>>> +{
>>>>>> + const char *plain_comment = "eBPF map_type ";
>>>>>> + char feat_name[128], plain_desc[128];
>>>>>> + size_t maxlen;
>>>>>> + bool res;
>>>>>> +
>>>>>> + res = bpf_probe_map_type(map_type, 0);
>>>>>> +
>>>>>> + maxlen = sizeof(plain_desc) - strlen(plain_comment) - 1;
>>>>>> + if (strlen(map_type_name[map_type]) > maxlen) {
>>>>>> + p_info("map type name too long");
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> + sprintf(feat_name, "have_%s_map_type", map_type_name[map_type]);
>>>>>> + sprintf(plain_desc, "%s%s", plain_comment, map_type_name[map_type]);
>>>>>> + print_bool_feature(feat_name, plain_desc, res);
>>>>>> +}
>>>>>> +
>>>>>> static int do_probe(int argc, char **argv)
>>>>>> {
>>>>>> enum probe_component target = COMPONENT_UNSPEC;
>>>>>> @@ -468,6 +488,12 @@ static int do_probe(int argc, char **argv)
>>>>>> for (i = BPF_PROG_TYPE_UNSPEC + 1; i < ARRAY_SIZE(prog_type_name); i++)
>>>>>> probe_prog_type(i, kernel_version, supported_types);
>>>>>> + print_end_then_start_section("map_types",
>>>>>> + "Scanning eBPF map types...");
>>>>>> +
>>>>>> + for (i = BPF_MAP_TYPE_UNSPEC + 1; i < map_type_name_size; i++)
>>>>>> + probe_map_type(i);
>>>>>> +
>>>>>> exit_close_json:
>>>>>> if (json_output) {
>>>>>> /* End current "section" of probes */
>>>>>> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
>>>>>> index 5cfc6601de9b..d7dd84d3c660 100644
>>>>>> --- a/tools/bpf/bpftool/main.h
>>>>>> +++ b/tools/bpf/bpftool/main.h
>>>>>> @@ -75,6 +75,9 @@ static const char * const prog_type_name[] = {
>>>>>> [BPF_PROG_TYPE_FLOW_DISSECTOR] = "flow_dissector",
>>>>>> };
>>>>>> +extern const char * const map_type_name[];
>>>>>> +extern const size_t map_type_name_size;
>>>>>> +
>>>>>> enum bpf_obj_type {
>>>>>> BPF_OBJ_UNKNOWN,
>>>>>> BPF_OBJ_PROG,
>>>>>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>>>>>> index 2037e3dc864b..b73985589929 100644
>>>>>> --- a/tools/bpf/bpftool/map.c
>>>>>> +++ b/tools/bpf/bpftool/map.c
>>>>>> @@ -21,7 +21,7 @@
>>>>>> #include "json_writer.h"
>>>>>> #include "main.h"
>>>>>> -static const char * const map_type_name[] = {
>>>>>> +const char * const map_type_name[] = {
>>>>>> [BPF_MAP_TYPE_UNSPEC] = "unspec",
>>>>>> [BPF_MAP_TYPE_HASH] = "hash",
>>>>>> [BPF_MAP_TYPE_ARRAY] = "array",
>>>>>> @@ -48,6 +48,8 @@ static const char * const map_type_name[] = {
>>>>>> [BPF_MAP_TYPE_STACK] = "stack",
>>>>>> };
>>>>>> +const size_t map_type_name_size = ARRAY_SIZE(map_type_name);
>>>>>> +
>>>>>> static bool map_is_per_cpu(__u32 type)
>>>>>> {
>>>>>> return type == BPF_MAP_TYPE_PERCPU_HASH ||
>>>>>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>>>>>> index f4bb2764ca9a..202c1ee5c579 100644
>>>>>> --- a/tools/lib/bpf/libbpf.h
>>>>>> +++ b/tools/lib/bpf/libbpf.h
>>>>>> @@ -360,6 +360,7 @@ bpf_prog_linfo__lfind(const struct bpf_prog_linfo *prog_linfo,
>>>>>> */
>>>>>> LIBBPF_API bool bpf_probe_prog_type(enum bpf_prog_type prog_type,
>>>>>> int kernel_version, __u32 ifindex);
>>>>>> +LIBBPF_API bool bpf_probe_map_type(enum bpf_map_type map_type, __u32 ifindex);
>>>>>> #ifdef __cplusplus
>>>>>> } /* extern "C" */
>>>>>> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
>>>>>> index 6355e4c80a86..c08f4c726e8e 100644
>>>>>> --- a/tools/lib/bpf/libbpf.map
>>>>>> +++ b/tools/lib/bpf/libbpf.map
>>>>>> @@ -56,6 +56,7 @@ LIBBPF_0.0.1 {
>>>>>> bpf_object__unpin_maps;
>>>>>> bpf_object__unpin_programs;
>>>>>> bpf_perf_event_read_simple;
>>>>>> + bpf_probe_map_type;
>>>>>> bpf_probe_prog_type;
>>>>>> bpf_prog_attach;
>>>>>> bpf_prog_detach;
>>>>>> diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
>>>>>> index 2c5e0cdc9f2f..796fe1e66169 100644
>>>>>> --- a/tools/lib/bpf/libbpf_probes.c
>>>>>> +++ b/tools/lib/bpf/libbpf_probes.c
>>>>>> @@ -53,3 +53,62 @@ bool bpf_probe_prog_type(enum bpf_prog_type prog_type, int kernel_version,
>>>>>> return errno != EINVAL && errno != EOPNOTSUPP;
>>>>>> }
>>>>>> +
>>>>>> +bool bpf_probe_map_type(enum bpf_map_type map_type, __u32 ifindex)
>>>>>> +{
>>>>>> + int key_size, value_size, max_entries, map_flags;
>>>>>> + struct bpf_create_map_attr attr = {};
>>>>>> + int fd = -1, fd_inner;
>>>>>> +
>>>>>> + key_size = sizeof(__u32);
>>>>>> + value_size = sizeof(__u32);
>>>>>> + max_entries = 1;
>>>>>> + map_flags = 0;
>>>>>> +
>>>>>> + if (map_type == BPF_MAP_TYPE_LPM_TRIE) {
>>>>> How about using switch here? So it yells and complains when the new item
>>>>> is added to the bpf_map_type enum.
>>>>>> + key_size = sizeof(__u64);
>>>>>> + value_size = sizeof(__u64);
>>>>>> + map_flags = BPF_F_NO_PREALLOC;
>>>>>> + } else if (map_type == BPF_MAP_TYPE_STACK_TRACE) {
>>>>>> + value_size = sizeof(__u64);
>>>>>> + } else if (map_type == BPF_MAP_TYPE_CGROUP_STORAGE ||
>>>>>> + map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE) {
>>>>>> + key_size = sizeof(struct bpf_cgroup_storage_key);
>>>>>> + value_size = sizeof(__u64);
>>>>>> + max_entries = 0;
>>>>>> + } else if (map_type == BPF_MAP_TYPE_QUEUE ||
>>>>>> + map_type == BPF_MAP_TYPE_STACK) {
>>>>>> + key_size = 0;
>>>>>> + }
>>>>
>>>> Hmm so I had a switch in the first place and it complained even with a
>>>> "default" label because -Wno-switch-enum is used for compiling. So you would
>>>> have me use a switch with all existing map types as explicit labels, even if
>>>> we do not use them? To limit the risk of forgetting to update for new map
>>>> types that would need specific parameters for the probe, is that correct?
>>> Correct. You'd have to list all enum items to pass -Wno-switch-enum. It
>>> looks more rigid, it makes sure we never forget to update the probes
>>> when adding new map types.
>>
>> True... If it is acceptable to take the risk of breaking compilation for
>> libbpf if we forget to update the list when new map types are added (I mean,
>> this is the desired effect for keeping the list of parameters up-to-date,
>> but people e.g. just trying to compile bpftool might have issues in such a
>> case), I can change my code to use a switch.
> I think we already have a precedent, see bpf_prog_type__needs_kver in
> libbpf ;-)
True, thanks! Although that function might well disappear in the future.
Powered by blists - more mailing lists