netdev - Re: [PATCH bpf-next] selftests/bpf: fix trampoline

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAEf4BzYuMr56P3EVyvdr4mvV3qbx496GMgfAU1Gd4aJo5RUR2A@mail.gmail.com>
Date:   Fri, 21 Feb 2020 16:21:36 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Jakub Sitnicki <jakub@...udflare.com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>, Jiri Olsa <jolsa@...nel.org>
Subject: Re: [PATCH bpf-next] selftests/bpf: fix trampoline_count clean up logic

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