| 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: <20181220184205.GA21153@mini-arch.hsd1.ca.comcast.net>
Date: Thu, 20 Dec 2018 10:42:05 -0800
From: Stanislav Fomichev <sdf@...ichev.me>
To: Quentin Monnet <quentin.monnet@...ronome.com>
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
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 ;-)
Powered by blists - more mailing lists