| 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: <e45e2200-931e-117f-d427-d6dcd5214e00@netronome.com>
Date: Fri, 21 Dec 2018 14:47:16 +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 v2] libbpf: Show supported ELF section names on
when failed to guess a prog/attach type
2018-12-21 22:22 UTC+0900 ~ Taeung Song <treeze.taeung@...il.com>
> We need to let users check their wrong ELF section name
> with proper ELF section names when failed to get a prog/attach type from it.
> Because users can't realize libbpf guess prog/attach types from
> given ELF section names.
> 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 ELF section name cgroup
>
> After:
>
> libbpf: failed to guess program type based on ELF section name 'cgroup'
> libbpf: supported 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
>
> In addtion, add pr_out printing without the "libbpf: " prefix.
>
> Cc: Jakub Kicinski <jakub.kicinski@...ronome.com>
> Cc: Quentin Monnet <quentin.monnet@...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 | 31 ++++++++++++++-----
> tools/lib/bpf/libbpf.h | 3 +-
> tools/lib/bpf/test_libbpf.cpp | 2 +-
> tools/perf/util/bpf-loader.c | 4 +--
> tools/testing/selftests/bpf/test_btf.c | 2 +-
> .../testing/selftests/bpf/test_libbpf_open.c | 4 +--
> tools/testing/selftests/bpf/test_progs.c | 4 +--
> .../selftests/bpf/test_socket_cookie.c | 4 +--
> 9 files changed, 38 insertions(+), 26 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 169e347c76f6..55cb3f7cb3ee 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -68,24 +68,30 @@ static int __base_pr(const char *format, ...)
> static __printf(1, 2) libbpf_print_fn_t __pr_warning = __base_pr;
> static __printf(1, 2) libbpf_print_fn_t __pr_info = __base_pr;
> static __printf(1, 2) libbpf_print_fn_t __pr_debug;
> +static __printf(1, 2) libbpf_print_fn_t __pr_out = __base_pr;
> +
> +#define pr_fmt(fmt) "libbpf: " fmt
>
> #define __pr(func, fmt, ...) \
> do { \
> if ((func)) \
> - (func)("libbpf: " fmt, ##__VA_ARGS__); \
> + (func)(fmt, ##__VA_ARGS__); \
> } while (0)
>
> -#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_warning(fmt, ...) __pr(__pr_warning, pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_info(fmt, ...) __pr(__pr_info, pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_debug(fmt, ...) __pr(__pr_debug, pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_out(fmt, ...) __pr(__pr_out, fmt, ##__VA_ARGS__)
>
> void libbpf_set_print(libbpf_print_fn_t warn,
> libbpf_print_fn_t info,
> - libbpf_print_fn_t debug)
> + libbpf_print_fn_t debug,
> + libbpf_print_fn_t out)
> {
> __pr_warning = warn;
> __pr_info = info;
> __pr_debug = debug;
> + __pr_out = out;
> }
>
> #define STRERR_BUFSIZE 128
> @@ -2682,6 +2688,12 @@ 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 ELF section name '%s'\n", name);
> + pr_info("supported section(type) names are:");
> + for (i = 0; i < ARRAY_SIZE(section_names); i++)
> + pr_out(" %s", section_names[i].sec);
> + pr_out("\n");
> return -EINVAL;
> }
>
> @@ -2701,6 +2713,13 @@ 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 ELF 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);
> + pr_out("\n");
> return -EINVAL;
> }
Thanks for the fixes!
However I don't see an easy way to integrate this pr_out() function to
the list of output facilities in libbpf. I have several concerns here:
- "pr_out()" is not a name that means much by itself, since all
functions are used to print out messages (also the names for "fmt" and
"pr_fmt" are not really explicit either).
- Even if we can find a name that better fits the "no prefix" aspect, we
still have three functions that are distinguished by importance level
(error, warn, info), while this one cannot be combined with them and
just exists for formatting purposes. I mean, as a user, when should I
allow for pr_out() output to be printed? If I want info messages, but
not debug messages, should I print it? Or is that lower importance than
debug?...
- Worse, libbpf_set_print() has been UAPI for several versions now, and
I do not believe we can change it at all. Having an additional function
would be possible, but would look cumbersome maybe.
Why not do something else instead, when trying to attach or load the
programs, for example:
char buf[1024] = {};
/* Forge string buf with all available names */
for (i = 0; i < ARRAY_SIZE(section_names); i++) {
if (strlen(buf) + strlen(" ") +
strlen(section_names[i]) + 1 > sizeof(buf))
break;
strcat(buf, " ");
strcat(buf, sections_names[i]);
}
pr_warning("failed to guess attach type...");
/* Now print all names at once in the same p_info() */
pr_info("attachable section(type) names are: %s\n" buf);
So you wouldn't need to add any additional pr_*() function?
Also for your information, bpf-next tree closed yesterday for the merge
window.
>
> @@ -2907,8 +2926,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/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 5f68d7b75215..930c75012785 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -57,7 +57,8 @@ typedef int (*libbpf_print_fn_t)(const char *, ...)
>
> LIBBPF_API void libbpf_set_print(libbpf_print_fn_t warn,
> libbpf_print_fn_t info,
> - libbpf_print_fn_t debug);
> + libbpf_print_fn_t debug,
> + libbpf_print_fn_t out);
Powered by blists - more mailing lists