On Fri, Feb 21, 2020 at 2:21 AM Jakub Sitnicki <jakub@xxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Andrii Nakryiko <andriin@xxxxxx>
> > ---
> > .../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.
