| 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: <1abf90e1-c691-be47-44b5-95c9f5c71159@gmail.com>
Date: Mon, 21 Jan 2019 16:18:22 +0900
From: Taeung Song <treeze.taeung@...il.com>
To: Quentin Monnet <quentin.monnet@...ronome.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
On 12/21/18 11:47 PM, Quentin Monnet wrote:
> 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.
>
Thanks for detailed review.
I changed this patch as you said. At last, I send v3 soon.
Thanks,
Taeung
>>
>> @@ -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