| 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: <c38e90ab-4071-6e4e-70b1-f701cd32fe43@netronome.com>
Date: Thu, 20 Dec 2018 18:27:41 +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: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.
Powered by blists - more mailing lists