| Message ID | 20190121072515.16522-1-treeze.taeung@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [RESEND,bpf-next,v3] libbpf: Show supported ELF section names on when failed to guess a prog/attach type | expand |
2019-01-21 16:25 UTC+0900 ~ Taeung Song <treeze.taeung@gmail.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@netronome.com> > Reviewed-by: Quentin Monnet <quentin.monnet@netronome.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:" > Cc: Andrey Ignatov <rdna@fb.com> > Signed-off-by: Taeung Song <treeze.taeung@gmail.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) ...) > + > + 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. > +} > + > 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) > > 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); >
On 1/21/19 8:45 PM, Quentin Monnet wrote: > 2019-01-21 16:25 UTC+0900 ~ Taeung Song <treeze.taeung@gmail.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@netronome.com> >> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.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@fb.com> >> Signed-off-by: Taeung Song <treeze.taeung@gmail.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); >> >
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); + + buf[0] = '\0'; + /* Forge string buf with all available names */ + for (i = 0; i < ARRAY_SIZE(section_names); i++) { + if (attach_type && !section_names[i].is_attachable) + continue; + + strcat(buf, " "); + strcat(buf, section_names[i].sec); + } + + return buf; +} + 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; 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; 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);
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@netronome.com> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com> Cc: Andrey Ignatov <rdna@fb.com> Signed-off-by: Taeung Song <treeze.taeung@gmail.com> --- 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(-)