| 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: <8ccf433f-c18c-5a42-9c99-b053f46d9209@netronome.com>
Date: Wed, 19 Dec 2018 11:26:12 +0000
From: Quentin Monnet <quentin.monnet@...ronome.com>
To: Taeung Song <treeze.taeung@...il.com>,
Daniel Borkmann <daniel@...earbox.net>,
Alexei Starovoitov <ast@...nel.org>
Cc: netdev@...r.kernel.org,
Jakub Kicinski <jakub.kicinski@...ronome.com>,
Andrey Ignatov <rdna@...com>
Subject: Re: [PATCH bpf-next] libbpf: Show possible section(type) names on
when failed to guess a type
2018-12-19 17:52 UTC+0900 ~ Taeung Song <treeze.taeung@...il.com>
> We need to let users check their wrong section name
> with proper section names when failed to get proper type from it.
> Because there is no knowing what kind of section name can be used.
> For example, when a 'cgroup' section name of a BPF program is used,
>
> Before:
>
> $ bpftool prog load bpf-prog.o /sys/fs/bpf/prog1
> Error: failed to guess program type based on section name cgroup
>
> After:
>
> libbpf: failed to guess program type based on section name 'cgroup'
> libbpf: possible section(type) names are: socket kprobe/ kretprobe/ classifier action tracepoint/ raw_tracepoint/ xdp perf_event lwt_in lwt_out lwt_xmit lwt_seg6local cgroup_skb/ingress cgroup_skb/egress cgroup/skb cgroup/sock cgroup/post_bind4 cgroup/post_bind6 cgroup/dev sockops sk_skb/stream_parser sk_skb/stream_verdict sk_skb sk_msg lirc_mode2 flow_dissector cgroup/bind4 cgroup/bind6 cgroup/connect4 cgroup/connect6 cgroup/sendmsg4 cgroup/sendmsg6
>
> Cc: Jakub Kicinski <jakub.kicinski@...ronome.com>
> Cc: Andrey Ignatov <rdna@...com>
> Signed-off-by: Taeung Song <treeze.taeung@...il.com>
> ---
> tools/bpf/bpftool/prog.c | 10 +++-------
> tools/lib/bpf/libbpf.c | 18 ++++++++++++++++--
> .../testing/selftests/bpf/test_socket_cookie.c | 4 +---
> 3 files changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index 2d1bb7d6ff51..0640e9bc0ada 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -930,10 +930,9 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> err = libbpf_prog_type_by_name(type, &attr.prog_type,
> &expected_attach_type);
> free(type);
> - if (err < 0) {
> - p_err("unknown program type '%s'", *argv);
> + if (err < 0)
> goto err_free_reuse_maps;
> - }
> +
> NEXT_ARG();
> } else if (is_prefix(*argv, "map")) {
> void *new_map_replace;
> @@ -1028,11 +1027,8 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
>
> err = libbpf_prog_type_by_name(sec_name, &prog_type,
> &expected_attach_type);
> - if (err < 0) {
> - p_err("failed to guess program type based on section name %s\n",
> - sec_name);
> + if (err < 0)
> goto err_close_obj;
> - }
> }
>
> bpf_program__set_ifindex(pos, ifindex);
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 169e347c76f6..ad274ac8d630 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -78,6 +78,7 @@ do { \
> #define pr_warning(fmt, ...) __pr(__pr_warning, fmt, ##__VA_ARGS__)
> #define pr_info(fmt, ...) __pr(__pr_info, fmt, ##__VA_ARGS__)
> #define pr_debug(fmt, ...) __pr(__pr_debug, fmt, ##__VA_ARGS__)
> +#define pr_out(fmt, ...) __base_pr(fmt, ##__VA_ARGS__)
Hi, thanks! Would be nice to get existing prog/attach types indeed.
Please can we avoid using a macro that unconditionally prints to stderr?
What will happen in your code below if any application uses
libbpf_set_print() to redefine pr_warning() and pr_info() is that only
the existing section names will be printed out, with no accompanying
message. Did you add pr_out() to avoid having the "libbpf: " prefix
printed in the middle of the list? If so, could we find something else
(e.g. alternative __pr() macro) and a way to replace that function by a
user-defined one (like libbpf_set_print() offers for the other ones)?
My main concern is to avoid breaking JSON output too much for bpftool.
Alternatively, would that make sense to have instead a function that
return a list of supported section names, and that could be called from
bpftool as a complement to the current error message? Just an idea.
>
> void libbpf_set_print(libbpf_print_fn_t warn,
> libbpf_print_fn_t info,
> @@ -2682,6 +2683,13 @@ int libbpf_prog_type_by_name(const char *name, enum bpf_prog_type *prog_type,
> *expected_attach_type = section_names[i].expected_attach_type;
> return 0;
> }
> +
> + pr_warning("failed to guess program type based on section name '%s'\n", name);
> + pr_info("possible section(type) names are:");
> + for (i = 0; i < ARRAY_SIZE(section_names); i++) {
> + pr_out(" %s", section_names[i].sec);
> + }
Nit: unnecessary brackets.
> + pr_out("\n");
> return -EINVAL;
> }
>
> @@ -2701,6 +2709,14 @@ int libbpf_attach_type_by_name(const char *name,
> *attach_type = section_names[i].attach_type;
> return 0;
> }
> +
> + pr_warning("failed to guess attach type based on section name '%s'\n", name);
> + pr_info("attachable section(type) names are:");
> + for (i = 0; i < ARRAY_SIZE(section_names); i++) {
> + if (section_names[i].is_attachable)
> + pr_out(" %s", section_names[i].sec);
> + }
Likewise.
> + pr_out("\n");
> return -EINVAL;
> }
>
> @@ -2907,8 +2923,6 @@ int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
> err = bpf_program__identify_section(prog, &prog_type,
> &expected_attach_type);
> if (err < 0) {
> - pr_warning("failed to guess program type based on section name %s\n",
> - prog->section_name);
> bpf_object__close(obj);
> return -EINVAL;
> }
> diff --git a/tools/testing/selftests/bpf/test_socket_cookie.c b/tools/testing/selftests/bpf/test_socket_cookie.c
> index b6c2c605d8c0..27c1576198d7 100644
> --- a/tools/testing/selftests/bpf/test_socket_cookie.c
> +++ b/tools/testing/selftests/bpf/test_socket_cookie.c
> @@ -158,10 +158,8 @@ static int run_test(int cgfd)
> bpf_object__for_each_program(prog, pobj) {
> prog_name = bpf_program__title(prog, /*needs_copy*/ false);
>
> - if (libbpf_attach_type_by_name(prog_name, &attach_type)) {
> - log_err("Unexpected prog: %s", prog_name);
> + if (libbpf_attach_type_by_name(prog_name, &attach_type))
> goto err;
> - }
>
> err = bpf_prog_attach(bpf_program__fd(prog), cgfd, attach_type,
> BPF_F_ALLOW_OVERRIDE);
>
Powered by blists - more mailing lists