| Message ID | 20200220230546.769250-1-andriin@fb.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] selftests/bpf: fix trampoline_count clean up logic | expand |
On Thu, Feb 20, 2020 at 03:05:46PM -0800, Andrii Nakryiko wrote: > Libbpf's Travis CI tests caught this issue. Ensure bpf_link and bpf_object > clean up is performed correctly. > > Fixes: d633d57902a5 ("selftest/bpf: Add test for allowed trampolines count") > Cc: Jiri Olsa <jolsa@kernel.org> > Signed-off-by: Andrii Nakryiko <andriin@fb.com> Applied, Thanks
On Thu, Feb 20, 2020 at 3:07 PM Andrii Nakryiko <andriin@fb.com> wrote: > > Libbpf's Travis CI tests caught this issue. Ensure bpf_link and bpf_object > clean up is performed correctly. > > Fixes: d633d57902a5 ("selftest/bpf: Add test for allowed trampolines count") > Cc: Jiri Olsa <jolsa@kernel.org> > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > --- > .../bpf/prog_tests/trampoline_count.c | 25 +++++++++++++------ > 1 file changed, 18 insertions(+), 7 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c > index 1f6ccdaed1ac..781c8d11604b 100644 > --- a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c > +++ b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c > @@ -55,31 +55,40 @@ void test_trampoline_count(void) > /* attach 'allowed' 40 trampoline programs */ > for (i = 0; i < MAX_TRAMP_PROGS; i++) { > obj = bpf_object__open_file(object, NULL); > - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) > + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) { > + obj = NULL; I think we don't need obj and link in cleanup? Did I miss anything?
On 2/20/20 6:06 PM, Song Liu wrote: > On Thu, Feb 20, 2020 at 3:07 PM Andrii Nakryiko <andriin@fb.com> wrote: >> >> Libbpf's Travis CI tests caught this issue. Ensure bpf_link and bpf_object >> clean up is performed correctly. >> >> Fixes: d633d57902a5 ("selftest/bpf: Add test for allowed trampolines count") >> Cc: Jiri Olsa <jolsa@kernel.org> >> Signed-off-by: Andrii Nakryiko <andriin@fb.com> >> --- >> .../bpf/prog_tests/trampoline_count.c | 25 +++++++++++++------ >> 1 file changed, 18 insertions(+), 7 deletions(-) >> >> diff --git a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c >> index 1f6ccdaed1ac..781c8d11604b 100644 >> --- a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c >> +++ b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c >> @@ -55,31 +55,40 @@ void test_trampoline_count(void) >> /* attach 'allowed' 40 trampoline programs */ >> for (i = 0; i < MAX_TRAMP_PROGS; i++) { >> obj = bpf_object__open_file(object, NULL); >> - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) >> + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) { >> + obj = NULL; > > I think we don't need obj and link in cleanup? Did I miss anything? > We do set obj below (line 87) after this loop, so need to clean it up as well. As for link, yeah, technically link doesn't have to be set to NULL, but I kind of did it for completeness without thinking too much.
> On Feb 20, 2020, at 8:20 PM, Andrii Nakryiko <andriin@fb.com> wrote: > > On 2/20/20 6:06 PM, Song Liu wrote: >> On Thu, Feb 20, 2020 at 3:07 PM Andrii Nakryiko <andriin@fb.com> wrote: >>> >>> Libbpf's Travis CI tests caught this issue. Ensure bpf_link and bpf_object >>> clean up is performed correctly. >>> >>> Fixes: d633d57902a5 ("selftest/bpf: Add test for allowed trampolines count") >>> Cc: Jiri Olsa <jolsa@kernel.org> >>> Signed-off-by: Andrii Nakryiko <andriin@fb.com> >>> --- >>> .../bpf/prog_tests/trampoline_count.c | 25 +++++++++++++------ >>> 1 file changed, 18 insertions(+), 7 deletions(-) >>> >>> diff --git a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c >>> index 1f6ccdaed1ac..781c8d11604b 100644 >>> --- a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c >>> +++ b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c >>> @@ -55,31 +55,40 @@ void test_trampoline_count(void) >>> /* attach 'allowed' 40 trampoline programs */ >>> for (i = 0; i < MAX_TRAMP_PROGS; i++) { >>> obj = bpf_object__open_file(object, NULL); >>> - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) >>> + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) { >>> + obj = NULL; >> I think we don't need obj and link in cleanup? Did I miss anything? > > We do set obj below (line 87) after this loop, so need to clean it up as well. As for link, yeah, technically link doesn't have to be set to NULL, but I kind of did it for completeness without thinking too much. I meant "obj = NULL;" before "goto cleanup;", as we don't use obj in the cleanup path. Anyway, this is not a real issue. Thanks, Song
On Thu, Feb 20, 2020 at 8:31 PM Song Liu <songliubraving@fb.com> wrote: > > > > > On Feb 20, 2020, at 8:20 PM, Andrii Nakryiko <andriin@fb.com> wrote: > > > > On 2/20/20 6:06 PM, Song Liu wrote: > >> On Thu, Feb 20, 2020 at 3:07 PM Andrii Nakryiko <andriin@fb.com> wrote: > >>> > >>> Libbpf's Travis CI tests caught this issue. Ensure bpf_link and bpf_object > >>> clean up is performed correctly. > >>> > >>> Fixes: d633d57902a5 ("selftest/bpf: Add test for allowed trampolines count") > >>> Cc: Jiri Olsa <jolsa@kernel.org> > >>> Signed-off-by: Andrii Nakryiko <andriin@fb.com> > >>> --- > >>> .../bpf/prog_tests/trampoline_count.c | 25 +++++++++++++------ > >>> 1 file changed, 18 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c > >>> index 1f6ccdaed1ac..781c8d11604b 100644 > >>> --- a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c > >>> +++ b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c > >>> @@ -55,31 +55,40 @@ void test_trampoline_count(void) > >>> /* attach 'allowed' 40 trampoline programs */ > >>> for (i = 0; i < MAX_TRAMP_PROGS; i++) { > >>> obj = bpf_object__open_file(object, NULL); > >>> - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) > >>> + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) { > >>> + obj = NULL; > >> I think we don't need obj and link in cleanup? Did I miss anything? > > > > We do set obj below (line 87) after this loop, so need to clean it up as well. As for link, yeah, technically link doesn't have to be set to NULL, but I kind of did it for completeness without thinking too much. > > I meant "obj = NULL;" before "goto cleanup;", as we don't use obj in the > cleanup path. > > Anyway, this is not a real issue. Ah, I see what you are saying, we skip over that bpf_object__close() call, right. > > Thanks, > Song >
On Thu, Feb 20, 2020 at 11:05 PM GMT, Andrii Nakryiko wrote: > Libbpf's Travis CI tests caught this issue. Ensure bpf_link and bpf_object > clean up is performed correctly. > > Fixes: d633d57902a5 ("selftest/bpf: Add test for allowed trampolines count") > Cc: Jiri Olsa <jolsa@kernel.org> > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > --- > .../bpf/prog_tests/trampoline_count.c | 25 +++++++++++++------ > 1 file changed, 18 insertions(+), 7 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c > index 1f6ccdaed1ac..781c8d11604b 100644 > --- a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c > +++ b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c > @@ -55,31 +55,40 @@ void test_trampoline_count(void) > /* attach 'allowed' 40 trampoline programs */ > for (i = 0; i < MAX_TRAMP_PROGS; i++) { > obj = bpf_object__open_file(object, NULL); > - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) > + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) { > + obj = NULL; > goto cleanup; > + } > > err = bpf_object__load(obj); > if (CHECK(err, "obj_load", "err %d\n", err)) > goto cleanup; > inst[i].obj = obj; > + obj = NULL; > > if (rand() % 2) { > - link = load(obj, fentry_name); > - if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) > + link = load(inst[i].obj, fentry_name); > + if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) { > + link = NULL; > goto cleanup; > + } > inst[i].link_fentry = link; > } else { > - link = load(obj, fexit_name); > - if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) > + link = load(inst[i].obj, fexit_name); > + if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) { > + link = NULL; > goto cleanup; > + } > inst[i].link_fexit = link; > } > } > > /* and try 1 extra.. */ > obj = bpf_object__open_file(object, NULL); > - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) > + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) { > + obj = NULL; > goto cleanup; > + } > > err = bpf_object__load(obj); > if (CHECK(err, "obj_load", "err %d\n", err)) > @@ -104,7 +113,9 @@ void test_trampoline_count(void) > cleanup_extra: > bpf_object__close(obj); > cleanup: > - while (--i) { > + if (i >= MAX_TRAMP_PROGS) > + i = MAX_TRAMP_PROGS - 1; > + for (; i >= 0; i--) { > bpf_link__destroy(inst[i].link_fentry); > bpf_link__destroy(inst[i].link_fexit); > bpf_object__close(inst[i].obj); I'm not sure I'm grasping what the fix is about. We don't access obj or link from cleanup, so what is the point of setting them to NULL before jumping there? Or is it all just an ancillary change to clamping the loop index variable to (MAX_TRAMP_PROGS - 1)?
On Thu, Feb 20, 2020 at 03:05:46PM -0800, Andrii Nakryiko wrote: > Libbpf's Travis CI tests caught this issue. Ensure bpf_link and bpf_object > clean up is performed correctly. > > Fixes: d633d57902a5 ("selftest/bpf: Add test for allowed trampolines count") > Cc: Jiri Olsa <jolsa@kernel.org> > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > --- > .../bpf/prog_tests/trampoline_count.c | 25 +++++++++++++------ > 1 file changed, 18 insertions(+), 7 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c > index 1f6ccdaed1ac..781c8d11604b 100644 > --- a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c > +++ b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c > @@ -55,31 +55,40 @@ void test_trampoline_count(void) > /* attach 'allowed' 40 trampoline programs */ > for (i = 0; i < MAX_TRAMP_PROGS; i++) { > obj = bpf_object__open_file(object, NULL); > - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) > + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) { > + obj = NULL; > goto cleanup; > + } > > err = bpf_object__load(obj); > if (CHECK(err, "obj_load", "err %d\n", err)) > goto cleanup; > inst[i].obj = obj; > + obj = NULL; > > if (rand() % 2) { > - link = load(obj, fentry_name); > - if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) > + link = load(inst[i].obj, fentry_name); > + if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) { > + link = NULL; > goto cleanup; > + } > inst[i].link_fentry = link; > } else { > - link = load(obj, fexit_name); > - if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) > + link = load(inst[i].obj, fexit_name); > + if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) { > + link = NULL; > goto cleanup; > + } > inst[i].link_fexit = link; > } > } > > /* and try 1 extra.. */ > obj = bpf_object__open_file(object, NULL); > - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) > + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) { > + obj = NULL; > goto cleanup; > + } > > err = bpf_object__load(obj); > if (CHECK(err, "obj_load", "err %d\n", err)) > @@ -104,7 +113,9 @@ void test_trampoline_count(void) > cleanup_extra: > bpf_object__close(obj); > cleanup: > - while (--i) { > + if (i >= MAX_TRAMP_PROGS) > + i = MAX_TRAMP_PROGS - 1; > + for (; i >= 0; i--) { ugh right, if we fail in first iteration, 'i' would get get -1 in here thanks, jirka > bpf_link__destroy(inst[i].link_fentry); > bpf_link__destroy(inst[i].link_fexit); > bpf_object__close(inst[i].obj); > -- > 2.17.1 >
On Fri, Feb 21, 2020 at 2:21 AM Jakub Sitnicki <jakub@cloudflare.com> wrote: > > On Thu, Feb 20, 2020 at 11:05 PM GMT, Andrii Nakryiko wrote: > > Libbpf's Travis CI tests caught this issue. Ensure bpf_link and bpf_object > > clean up is performed correctly. > > > > Fixes: d633d57902a5 ("selftest/bpf: Add test for allowed trampolines count") > > Cc: Jiri Olsa <jolsa@kernel.org> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > > --- > > .../bpf/prog_tests/trampoline_count.c | 25 +++++++++++++------ > > 1 file changed, 18 insertions(+), 7 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c > > index 1f6ccdaed1ac..781c8d11604b 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c > > +++ b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c > > @@ -55,31 +55,40 @@ void test_trampoline_count(void) > > /* attach 'allowed' 40 trampoline programs */ > > for (i = 0; i < MAX_TRAMP_PROGS; i++) { > > obj = bpf_object__open_file(object, NULL); > > - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) > > + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) { > > + obj = NULL; > > goto cleanup; > > + } > > > > err = bpf_object__load(obj); > > if (CHECK(err, "obj_load", "err %d\n", err)) > > goto cleanup; > > inst[i].obj = obj; > > + obj = NULL; > > > > if (rand() % 2) { > > - link = load(obj, fentry_name); > > - if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) > > + link = load(inst[i].obj, fentry_name); > > + if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) { > > + link = NULL; > > goto cleanup; > > + } > > inst[i].link_fentry = link; > > } else { > > - link = load(obj, fexit_name); > > - if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) > > + link = load(inst[i].obj, fexit_name); > > + if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) { > > + link = NULL; > > goto cleanup; > > + } > > inst[i].link_fexit = link; > > } > > } > > > > /* and try 1 extra.. */ > > obj = bpf_object__open_file(object, NULL); > > - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) > > + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) { > > + obj = NULL; > > goto cleanup; > > + } > > > > err = bpf_object__load(obj); > > if (CHECK(err, "obj_load", "err %d\n", err)) > > @@ -104,7 +113,9 @@ void test_trampoline_count(void) > > cleanup_extra: > > bpf_object__close(obj); > > cleanup: > > - while (--i) { > > + if (i >= MAX_TRAMP_PROGS) > > + i = MAX_TRAMP_PROGS - 1; > > + for (; i >= 0; i--) { > > bpf_link__destroy(inst[i].link_fentry); > > bpf_link__destroy(inst[i].link_fexit); > > bpf_object__close(inst[i].obj); > > I'm not sure I'm grasping what the fix is about. > > We don't access obj or link from cleanup, so what is the point of > setting them to NULL before jumping there? > > Or is it all just an ancillary change to clamping the loop index > variable to (MAX_TRAMP_PROGS - 1)? Yeah, mostly ancillary. But this is not just clamping to MAX_TRAMP_PROGS-1. As Jiri pointed out, it's also handling a case of i == 0.
diff --git a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c index 1f6ccdaed1ac..781c8d11604b 100644 --- a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c +++ b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c @@ -55,31 +55,40 @@ void test_trampoline_count(void) /* attach 'allowed' 40 trampoline programs */ for (i = 0; i < MAX_TRAMP_PROGS; i++) { obj = bpf_object__open_file(object, NULL); - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) { + obj = NULL; goto cleanup; + } err = bpf_object__load(obj); if (CHECK(err, "obj_load", "err %d\n", err)) goto cleanup; inst[i].obj = obj; + obj = NULL; if (rand() % 2) { - link = load(obj, fentry_name); - if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) + link = load(inst[i].obj, fentry_name); + if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) { + link = NULL; goto cleanup; + } inst[i].link_fentry = link; } else { - link = load(obj, fexit_name); - if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) + link = load(inst[i].obj, fexit_name); + if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) { + link = NULL; goto cleanup; + } inst[i].link_fexit = link; } } /* and try 1 extra.. */ obj = bpf_object__open_file(object, NULL); - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) { + obj = NULL; goto cleanup; + } err = bpf_object__load(obj); if (CHECK(err, "obj_load", "err %d\n", err)) @@ -104,7 +113,9 @@ void test_trampoline_count(void) cleanup_extra: bpf_object__close(obj); cleanup: - while (--i) { + if (i >= MAX_TRAMP_PROGS) + i = MAX_TRAMP_PROGS - 1; + for (; i >= 0; i--) { bpf_link__destroy(inst[i].link_fentry); bpf_link__destroy(inst[i].link_fexit); bpf_object__close(inst[i].obj);
Libbpf's Travis CI tests caught this issue. Ensure bpf_link and bpf_object clean up is performed correctly. Fixes: d633d57902a5 ("selftest/bpf: Add test for allowed trampolines count") Cc: Jiri Olsa <jolsa@kernel.org> Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- .../bpf/prog_tests/trampoline_count.c | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-)