| 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: <27c167d8-af20-e1d5-104c-92a427c9c4b4@gmail.com>
Date: Mon, 21 Jan 2019 21:47:45 +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 RESEND bpf-next v3] libbpf: Show supported ELF section
names on when failed to guess a prog/attach type
On 1/21/19 8:45 PM, Quentin Monnet wrote:
> 2019-01-21 16:25 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,
>> show available ELF section names(types).
>>
>> 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
>>
>> Reviewed-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
>> Reviewed-by: Quentin Monnet <quentin.monnet@...ronome.com>
>
> Please do not add "Reviewed-by" tags unless offered by the reviewers. I
> cannot remember either Jakub or I giving the tag for this series so far.
> Just in case, more details on the usage of this tag are available in
> Documentation/process/submitting-patches.rst, in section
> "Using Reported-by:, Tested-by:, Reviewed-by:, Suggested-by: and Fixes:"
>
I got it, thanks for pointing out this.
>> Cc: Andrey Ignatov <rdna@...com>
>> Signed-off-by: Taeung Song <treeze.taeung@...il.com>
>
> This version of the patch looks better, thanks! A few additional
> comments below.
>
>> ---
>> tools/bpf/bpftool/prog.c | 10 ++----
>> tools/lib/bpf/libbpf.c | 34 +++++++++++++++++--
>> .../selftests/bpf/test_socket_cookie.c | 4 +--
>> 3 files changed, 36 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..ea44d7a05149 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -2667,10 +2667,31 @@ static const struct {
>> #undef BPF_EAPROG_SEC
>> #undef BPF_APROG_COMPAT
>>
>> +#define MAX_TYPE_NAME_SIZE 32
>> +
>> +static char *libbpf_get_type_names(bool attach_type)
>> +{
>> + int i, len = ARRAY_SIZE(section_names) * MAX_TYPE_NAME_SIZE;
>> + char *buf = malloc(len);
>
> I'd rather have the malloc() on a line separate from variable
> declaration, like this:
>
> char *buf;
>
> buf = malloc(len);
>
> Also, can you please make sure that malloc() succeeds? (if (!buf) ...)
>
OK, will check it.
>> +
>> + buf[0] = '\0';
>> + /* Forge string buf with all available names */
>> + for (i = 0; i < ARRAY_SIZE(section_names); i++) {
>
> Could you please add a check on "buf"'s length to make sure there is no
> overflow? It seems very unlikely that we add a section name so long as
> to make this happen, but just in case.
>
>> + if (attach_type && !section_names[i].is_attachable)
>> + continue;
>> +
>> + strcat(buf, " ");
>> + strcat(buf, section_names[i].sec);
>> + }
>> +
>> + return buf;
>
> The code in this function uses a mix of tabs and spaces for indents.
> Please stick to tabs.
>
Oops, I modified it.
>> +}
>> +
>> int libbpf_prog_type_by_name(const char *name, enum bpf_prog_type *prog_type,
>> enum bpf_attach_type *expected_attach_type)
>> {
>> int i;
>> + char *type_names;
>
> Please declare variables in reverse-Christmas-tree order.
>
>>
>> if (!name)
>> return -EINVAL;
>> @@ -2682,6 +2703,11 @@ 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);
>> + type_names = libbpf_get_type_names(false);
>> + pr_info("supported section(type) names are:%s\n", type_names);
>> + free(type_names);
>> +
>> return -EINVAL;
>> }
>>
>> @@ -2689,6 +2715,7 @@ int libbpf_attach_type_by_name(const char *name,
>> enum bpf_attach_type *attach_type)
>> {
>> int i;
>> + char *type_names;
>
> (same comment on variables' order)
>
OK, will send v4 soon !
Thanks,
Taeung
>>
>> if (!name)
>> return -EINVAL;
>> @@ -2701,6 +2728,11 @@ 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);
>> + type_names = libbpf_get_type_names(true);
>> + pr_info("attachable section(type) names are:%s\n", type_names);
>> + free(type_names);
>> +
>> return -EINVAL;
>> }
>>
>> @@ -2907,8 +2939,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 fc7832ee566b..e51d63786ff8 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