Message ID | 20191007212237.1704211-1-andriin@fb.com |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | [bpf-next] bpftool: fix bpftool build by switching to bpf_object__open_file() | expand |
On 10/07, Andrii Nakryiko wrote: > As part of libbpf in 5e61f2707029 ("libbpf: stop enforcing kern_version, > populate it for users") non-LIBBPF_API __bpf_object__open_xattr() API > was removed from libbpf.h header. This broke bpftool, which relied on > that function. This patch fixes the build by switching to newly added > bpf_object__open_file() which provides the same capabilities, but is > official and future-proof API. > > Fixes: 5e61f2707029 ("libbpf: stop enforcing kern_version, populate it for users") > Reported-by: Stanislav Fomichev <sdf@google.com> > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > --- > tools/bpf/bpftool/main.c | 4 ++-- > tools/bpf/bpftool/main.h | 2 +- > tools/bpf/bpftool/prog.c | 22 ++++++++++++---------- > 3 files changed, 15 insertions(+), 13 deletions(-) > > diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c > index 93d008687020..4764581ff9ea 100644 > --- a/tools/bpf/bpftool/main.c > +++ b/tools/bpf/bpftool/main.c > @@ -27,7 +27,7 @@ bool json_output; > bool show_pinned; > bool block_mount; > bool verifier_logs; > -int bpf_flags; > +bool relaxed_maps; > struct pinned_obj_table prog_table; > struct pinned_obj_table map_table; > > @@ -396,7 +396,7 @@ int main(int argc, char **argv) > show_pinned = true; > break; > case 'm': > - bpf_flags = MAPS_RELAX_COMPAT; > + relaxed_maps = true; > break; > case 'n': > block_mount = true; > diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h > index af9ad56c303a..2899095f8254 100644 > --- a/tools/bpf/bpftool/main.h > +++ b/tools/bpf/bpftool/main.h > @@ -94,7 +94,7 @@ extern bool json_output; > extern bool show_pinned; > extern bool block_mount; > extern bool verifier_logs; > -extern int bpf_flags; > +extern bool relaxed_maps; > extern struct pinned_obj_table prog_table; > extern struct pinned_obj_table map_table; > > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > index 43fdbbfe41bb..8191cd595963 100644 > --- a/tools/bpf/bpftool/prog.c > +++ b/tools/bpf/bpftool/prog.c > @@ -1092,9 +1092,7 @@ static int do_run(int argc, char **argv) > static int load_with_options(int argc, char **argv, bool first_prog_only) > { > struct bpf_object_load_attr load_attr = { 0 }; > - struct bpf_object_open_attr open_attr = { > - .prog_type = BPF_PROG_TYPE_UNSPEC, > - }; > + enum bpf_prog_type prog_type = BPF_PROG_TYPE_UNSPEC; > enum bpf_attach_type expected_attach_type; > struct map_replace *map_replace = NULL; > struct bpf_program *prog = NULL, *pos; > @@ -1105,11 +1103,16 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) > const char *pinfile; > unsigned int i, j; > __u32 ifindex = 0; > + const char *file; > int idx, err; > > + LIBBPF_OPTS(bpf_object_open_opts, open_opts, > + .relaxed_maps = relaxed_maps, > + ); > + > if (!REQ_ARGS(2)) > return -1; > - open_attr.file = GET_ARG(); > + file = GET_ARG(); > pinfile = GET_ARG(); > > while (argc) { > @@ -1118,7 +1121,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) > > NEXT_ARG(); > > - if (open_attr.prog_type != BPF_PROG_TYPE_UNSPEC) { > + if (prog_type != BPF_PROG_TYPE_UNSPEC) { > p_err("program type already specified"); > goto err_free_reuse_maps; > } > @@ -1135,8 +1138,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) > strcat(type, *argv); > strcat(type, "/"); > > - err = libbpf_prog_type_by_name(type, > - &open_attr.prog_type, > + err = libbpf_prog_type_by_name(type, &prog_type, > &expected_attach_type); > free(type); > if (err < 0) > @@ -1224,16 +1226,16 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) > > set_max_rlimit(); > > - obj = __bpf_object__open_xattr(&open_attr, bpf_flags); > + obj = bpf_object__open_file(file, &open_opts); > if (IS_ERR_OR_NULL(obj)) { > p_err("failed to open object file"); > goto err_free_reuse_maps; > } > > bpf_object__for_each_program(pos, obj) { > - enum bpf_prog_type prog_type = open_attr.prog_type; > + enum bpf_prog_type prog_type = prog_type; Are you sure it works that way? $ cat tmp.c #include <stdio.h> int main() { int x = 1; printf("outer x=%d\n", x); { int x = x; printf("inner x=%d\n", x); } return 0; } $ gcc tmp.c && ./a.out outer x=1 inner x=0 Other than that: Reviewed-by: Stanislav Fomichev <sdf@google.com> > > - if (open_attr.prog_type == BPF_PROG_TYPE_UNSPEC) { > + if (prog_type == BPF_PROG_TYPE_UNSPEC) { > const char *sec_name = bpf_program__title(pos, false); > > err = libbpf_prog_type_by_name(sec_name, &prog_type, > -- > 2.17.1 >
On Mon, Oct 7, 2019 at 2:46 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > On 10/07, Andrii Nakryiko wrote: > > As part of libbpf in 5e61f2707029 ("libbpf: stop enforcing kern_version, > > populate it for users") non-LIBBPF_API __bpf_object__open_xattr() API > > was removed from libbpf.h header. This broke bpftool, which relied on > > that function. This patch fixes the build by switching to newly added > > bpf_object__open_file() which provides the same capabilities, but is > > official and future-proof API. > > > > Fixes: 5e61f2707029 ("libbpf: stop enforcing kern_version, populate it for users") > > Reported-by: Stanislav Fomichev <sdf@google.com> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > > --- > > tools/bpf/bpftool/main.c | 4 ++-- > > tools/bpf/bpftool/main.h | 2 +- > > tools/bpf/bpftool/prog.c | 22 ++++++++++++---------- > > 3 files changed, 15 insertions(+), 13 deletions(-) > > > > diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c > > index 93d008687020..4764581ff9ea 100644 > > --- a/tools/bpf/bpftool/main.c > > +++ b/tools/bpf/bpftool/main.c > > @@ -27,7 +27,7 @@ bool json_output; > > bool show_pinned; > > bool block_mount; > > bool verifier_logs; > > -int bpf_flags; > > +bool relaxed_maps; > > struct pinned_obj_table prog_table; > > struct pinned_obj_table map_table; > > > > @@ -396,7 +396,7 @@ int main(int argc, char **argv) > > show_pinned = true; > > break; > > case 'm': > > - bpf_flags = MAPS_RELAX_COMPAT; > > + relaxed_maps = true; > > break; > > case 'n': > > block_mount = true; > > diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h > > index af9ad56c303a..2899095f8254 100644 > > --- a/tools/bpf/bpftool/main.h > > +++ b/tools/bpf/bpftool/main.h > > @@ -94,7 +94,7 @@ extern bool json_output; > > extern bool show_pinned; > > extern bool block_mount; > > extern bool verifier_logs; > > -extern int bpf_flags; > > +extern bool relaxed_maps; > > extern struct pinned_obj_table prog_table; > > extern struct pinned_obj_table map_table; > > > > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > > index 43fdbbfe41bb..8191cd595963 100644 > > --- a/tools/bpf/bpftool/prog.c > > +++ b/tools/bpf/bpftool/prog.c > > @@ -1092,9 +1092,7 @@ static int do_run(int argc, char **argv) > > static int load_with_options(int argc, char **argv, bool first_prog_only) > > { > > struct bpf_object_load_attr load_attr = { 0 }; > > - struct bpf_object_open_attr open_attr = { > > - .prog_type = BPF_PROG_TYPE_UNSPEC, > > - }; > > + enum bpf_prog_type prog_type = BPF_PROG_TYPE_UNSPEC; > > enum bpf_attach_type expected_attach_type; > > struct map_replace *map_replace = NULL; > > struct bpf_program *prog = NULL, *pos; > > @@ -1105,11 +1103,16 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) > > const char *pinfile; > > unsigned int i, j; > > __u32 ifindex = 0; > > + const char *file; > > int idx, err; > > > > + LIBBPF_OPTS(bpf_object_open_opts, open_opts, > > + .relaxed_maps = relaxed_maps, > > + ); > > + > > if (!REQ_ARGS(2)) > > return -1; > > - open_attr.file = GET_ARG(); > > + file = GET_ARG(); > > pinfile = GET_ARG(); > > > > while (argc) { > > @@ -1118,7 +1121,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) > > > > NEXT_ARG(); > > > > - if (open_attr.prog_type != BPF_PROG_TYPE_UNSPEC) { > > + if (prog_type != BPF_PROG_TYPE_UNSPEC) { > > p_err("program type already specified"); > > goto err_free_reuse_maps; > > } > > @@ -1135,8 +1138,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) > > strcat(type, *argv); > > strcat(type, "/"); > > > > - err = libbpf_prog_type_by_name(type, > > - &open_attr.prog_type, > > + err = libbpf_prog_type_by_name(type, &prog_type, > > &expected_attach_type); > > free(type); > > if (err < 0) > > @@ -1224,16 +1226,16 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) > > > > set_max_rlimit(); > > > > - obj = __bpf_object__open_xattr(&open_attr, bpf_flags); > > + obj = bpf_object__open_file(file, &open_opts); > > if (IS_ERR_OR_NULL(obj)) { > > p_err("failed to open object file"); > > goto err_free_reuse_maps; > > } > > > > bpf_object__for_each_program(pos, obj) { > > - enum bpf_prog_type prog_type = open_attr.prog_type; > > + enum bpf_prog_type prog_type = prog_type; > Are you sure it works that way? Oh, I did this pretty mechanically, didn't notice I'm shadowing. In either case I'd like to avoid shadowing, so I'll rename one of them, good catch! > > $ cat tmp.c > #include <stdio.h> > > int main() > { > int x = 1; > printf("outer x=%d\n", x); > > { > int x = x; > printf("inner x=%d\n", x); > } > > return 0; > } > > $ gcc tmp.c && ./a.out > outer x=1 > inner x=0 > > Other than that: > Reviewed-by: Stanislav Fomichev <sdf@google.com> > > > > > - if (open_attr.prog_type == BPF_PROG_TYPE_UNSPEC) { > > + if (prog_type == BPF_PROG_TYPE_UNSPEC) { > > const char *sec_name = bpf_program__title(pos, false); > > > > err = libbpf_prog_type_by_name(sec_name, &prog_type, > > -- > > 2.17.1 > >
-- Andrii On Mon, Oct 7, 2019 at 2:50 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Mon, Oct 7, 2019 at 2:46 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > > On 10/07, Andrii Nakryiko wrote: > > > As part of libbpf in 5e61f2707029 ("libbpf: stop enforcing kern_version, > > > populate it for users") non-LIBBPF_API __bpf_object__open_xattr() API > > > was removed from libbpf.h header. This broke bpftool, which relied on > > > that function. This patch fixes the build by switching to newly added > > > bpf_object__open_file() which provides the same capabilities, but is > > > official and future-proof API. > > > > > > Fixes: 5e61f2707029 ("libbpf: stop enforcing kern_version, populate it for users") > > > Reported-by: Stanislav Fomichev <sdf@google.com> > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > > > --- > > > tools/bpf/bpftool/main.c | 4 ++-- > > > tools/bpf/bpftool/main.h | 2 +- > > > tools/bpf/bpftool/prog.c | 22 ++++++++++++---------- > > > 3 files changed, 15 insertions(+), 13 deletions(-) > > > [...] > > > --- a/tools/bpf/bpftool/prog.c > > > +++ b/tools/bpf/bpftool/prog.c > > > @@ -1092,9 +1092,7 @@ static int do_run(int argc, char **argv) > > > static int load_with_options(int argc, char **argv, bool first_prog_only) > > > { > > > struct bpf_object_load_attr load_attr = { 0 }; > > > - struct bpf_object_open_attr open_attr = { > > > - .prog_type = BPF_PROG_TYPE_UNSPEC, > > > - }; > > > + enum bpf_prog_type prog_type = BPF_PROG_TYPE_UNSPEC; > > > enum bpf_attach_type expected_attach_type; > > > struct map_replace *map_replace = NULL; [...] > > > > > > bpf_object__for_each_program(pos, obj) { > > > - enum bpf_prog_type prog_type = open_attr.prog_type; > > > + enum bpf_prog_type prog_type = prog_type; > > Are you sure it works that way? > > Oh, I did this pretty mechanically, didn't notice I'm shadowing. In > either case I'd like to avoid shadowing, so I'll rename one of them, > good catch! > > > > > $ cat tmp.c > > #include <stdio.h> > > > > int main() > > { > > int x = 1; > > printf("outer x=%d\n", x); > > > > { > > int x = x; It's amazing `int x = x;` is compiled successfully when there is no x in outer scope. And it's also amazing that it's doing the wrong thing when there is a shadowed variable in outer scope. I can't imagine the case where this will be a meaningful behavior... > > printf("inner x=%d\n", x); > > } > > > > return 0; > > } > > > > $ gcc tmp.c && ./a.out > > outer x=1 > > inner x=0 > > > > Other than that: > > Reviewed-by: Stanislav Fomichev <sdf@google.com> > > > > > > > > - if (open_attr.prog_type == BPF_PROG_TYPE_UNSPEC) { > > > + if (prog_type == BPF_PROG_TYPE_UNSPEC) { > > > const char *sec_name = bpf_program__title(pos, false); > > > > > > err = libbpf_prog_type_by_name(sec_name, &prog_type, > > > -- > > > 2.17.1 > > >
On 10/07, Andrii Nakryiko wrote: > -- Andrii > > On Mon, Oct 7, 2019 at 2:50 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Mon, Oct 7, 2019 at 2:46 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > > > > On 10/07, Andrii Nakryiko wrote: > > > > As part of libbpf in 5e61f2707029 ("libbpf: stop enforcing kern_version, > > > > populate it for users") non-LIBBPF_API __bpf_object__open_xattr() API > > > > was removed from libbpf.h header. This broke bpftool, which relied on > > > > that function. This patch fixes the build by switching to newly added > > > > bpf_object__open_file() which provides the same capabilities, but is > > > > official and future-proof API. > > > > > > > > Fixes: 5e61f2707029 ("libbpf: stop enforcing kern_version, populate it for users") > > > > Reported-by: Stanislav Fomichev <sdf@google.com> > > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > > > > --- > > > > tools/bpf/bpftool/main.c | 4 ++-- > > > > tools/bpf/bpftool/main.h | 2 +- > > > > tools/bpf/bpftool/prog.c | 22 ++++++++++++---------- > > > > 3 files changed, 15 insertions(+), 13 deletions(-) > > > > > > [...] > > > > > --- a/tools/bpf/bpftool/prog.c > > > > +++ b/tools/bpf/bpftool/prog.c > > > > @@ -1092,9 +1092,7 @@ static int do_run(int argc, char **argv) > > > > static int load_with_options(int argc, char **argv, bool first_prog_only) > > > > { > > > > struct bpf_object_load_attr load_attr = { 0 }; > > > > - struct bpf_object_open_attr open_attr = { > > > > - .prog_type = BPF_PROG_TYPE_UNSPEC, > > > > - }; > > > > + enum bpf_prog_type prog_type = BPF_PROG_TYPE_UNSPEC; > > > > enum bpf_attach_type expected_attach_type; > > > > struct map_replace *map_replace = NULL; > > [...] > > > > > > > > > bpf_object__for_each_program(pos, obj) { > > > > - enum bpf_prog_type prog_type = open_attr.prog_type; > > > > + enum bpf_prog_type prog_type = prog_type; > > > Are you sure it works that way? > > > > Oh, I did this pretty mechanically, didn't notice I'm shadowing. In > > either case I'd like to avoid shadowing, so I'll rename one of them, > > good catch! > > > > > > > > $ cat tmp.c > > > #include <stdio.h> > > > > > > int main() > > > { > > > int x = 1; > > > printf("outer x=%d\n", x); > > > > > > { > > > int x = x; > > It's amazing `int x = x;` is compiled successfully when there is no x > in outer scope. And it's also amazing that it's doing the wrong thing > when there is a shadowed variable in outer scope. I can't imagine the > case where this will be a meaningful behavior... Enjoy your daily dose of undefined behavior :-D > > > printf("inner x=%d\n", x); > > > } > > > > > > return 0; > > > } > > > > > > $ gcc tmp.c && ./a.out > > > outer x=1 > > > inner x=0 > > > > > > Other than that: > > > Reviewed-by: Stanislav Fomichev <sdf@google.com> > > > > > > > > > > > - if (open_attr.prog_type == BPF_PROG_TYPE_UNSPEC) { > > > > + if (prog_type == BPF_PROG_TYPE_UNSPEC) { > > > > const char *sec_name = bpf_program__title(pos, false); > > > > > > > > err = libbpf_prog_type_by_name(sec_name, &prog_type, > > > > -- > > > > 2.17.1 > > > >
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c index 93d008687020..4764581ff9ea 100644 --- a/tools/bpf/bpftool/main.c +++ b/tools/bpf/bpftool/main.c @@ -27,7 +27,7 @@ bool json_output; bool show_pinned; bool block_mount; bool verifier_logs; -int bpf_flags; +bool relaxed_maps; struct pinned_obj_table prog_table; struct pinned_obj_table map_table; @@ -396,7 +396,7 @@ int main(int argc, char **argv) show_pinned = true; break; case 'm': - bpf_flags = MAPS_RELAX_COMPAT; + relaxed_maps = true; break; case 'n': block_mount = true; diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h index af9ad56c303a..2899095f8254 100644 --- a/tools/bpf/bpftool/main.h +++ b/tools/bpf/bpftool/main.h @@ -94,7 +94,7 @@ extern bool json_output; extern bool show_pinned; extern bool block_mount; extern bool verifier_logs; -extern int bpf_flags; +extern bool relaxed_maps; extern struct pinned_obj_table prog_table; extern struct pinned_obj_table map_table; diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c index 43fdbbfe41bb..8191cd595963 100644 --- a/tools/bpf/bpftool/prog.c +++ b/tools/bpf/bpftool/prog.c @@ -1092,9 +1092,7 @@ static int do_run(int argc, char **argv) static int load_with_options(int argc, char **argv, bool first_prog_only) { struct bpf_object_load_attr load_attr = { 0 }; - struct bpf_object_open_attr open_attr = { - .prog_type = BPF_PROG_TYPE_UNSPEC, - }; + enum bpf_prog_type prog_type = BPF_PROG_TYPE_UNSPEC; enum bpf_attach_type expected_attach_type; struct map_replace *map_replace = NULL; struct bpf_program *prog = NULL, *pos; @@ -1105,11 +1103,16 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) const char *pinfile; unsigned int i, j; __u32 ifindex = 0; + const char *file; int idx, err; + LIBBPF_OPTS(bpf_object_open_opts, open_opts, + .relaxed_maps = relaxed_maps, + ); + if (!REQ_ARGS(2)) return -1; - open_attr.file = GET_ARG(); + file = GET_ARG(); pinfile = GET_ARG(); while (argc) { @@ -1118,7 +1121,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) NEXT_ARG(); - if (open_attr.prog_type != BPF_PROG_TYPE_UNSPEC) { + if (prog_type != BPF_PROG_TYPE_UNSPEC) { p_err("program type already specified"); goto err_free_reuse_maps; } @@ -1135,8 +1138,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) strcat(type, *argv); strcat(type, "/"); - err = libbpf_prog_type_by_name(type, - &open_attr.prog_type, + err = libbpf_prog_type_by_name(type, &prog_type, &expected_attach_type); free(type); if (err < 0) @@ -1224,16 +1226,16 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) set_max_rlimit(); - obj = __bpf_object__open_xattr(&open_attr, bpf_flags); + obj = bpf_object__open_file(file, &open_opts); if (IS_ERR_OR_NULL(obj)) { p_err("failed to open object file"); goto err_free_reuse_maps; } bpf_object__for_each_program(pos, obj) { - enum bpf_prog_type prog_type = open_attr.prog_type; + enum bpf_prog_type prog_type = prog_type; - if (open_attr.prog_type == BPF_PROG_TYPE_UNSPEC) { + if (prog_type == BPF_PROG_TYPE_UNSPEC) { const char *sec_name = bpf_program__title(pos, false); err = libbpf_prog_type_by_name(sec_name, &prog_type,
As part of libbpf in 5e61f2707029 ("libbpf: stop enforcing kern_version, populate it for users") non-LIBBPF_API __bpf_object__open_xattr() API was removed from libbpf.h header. This broke bpftool, which relied on that function. This patch fixes the build by switching to newly added bpf_object__open_file() which provides the same capabilities, but is official and future-proof API. Fixes: 5e61f2707029 ("libbpf: stop enforcing kern_version, populate it for users") Reported-by: Stanislav Fomichev <sdf@google.com> Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- tools/bpf/bpftool/main.c | 4 ++-- tools/bpf/bpftool/main.h | 2 +- tools/bpf/bpftool/prog.c | 22 ++++++++++++---------- 3 files changed, 15 insertions(+), 13 deletions(-)