netdev - Re: [PATCH bpf-next 8/8] selftests/bpf: Add tests for attaching bpf

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAEf4BzZ0qhdABJrG5mkeUM9je3FoJG3jTQ8oeFwT8JV7xS5+Qg@mail.gmail.com>
Date:   Thu, 28 May 2020 11:31:29 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Jakub Sitnicki <jakub@...udflare.com>
Cc:     bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        kernel-team@...udflare.com
Subject: Re: [PATCH bpf-next 8/8] selftests/bpf: Add tests for attaching
 bpf_link to netns

On Thu, May 28, 2020 at 6:29 AM Jakub Sitnicki <jakub@...udflare.com> wrote:
>
> On Thu, May 28, 2020 at 08:08 AM CEST, Andrii Nakryiko wrote:
> > On Wed, May 27, 2020 at 12:16 PM Jakub Sitnicki <jakub@...udflare.com> wrote:
> >>
> >> Extend the existing test case for flow dissector attaching to cover:
> >>
> >>  - link creation,
> >>  - link updates,
> >>  - link info querying,
> >>  - mixing links with direct prog attachment.
> >>
> >> Signed-off-by: Jakub Sitnicki <jakub@...udflare.com>
> >> ---
> >
> > You are not using bpf_program__attach_netns() at all. Would be nice to
> > actually use higher-level API here...
>
> That's true. I didn't exercise the high-level API. I can cover that.
>
> >
> > Also... what's up with people using CHECK_FAIL + perror instead of
> > CHECK? Is CHECK being avoided for some reason or people are just not
> > aware of it (which is strange, because CHECK was there before
> > CHECK_FAIL)?
>
> I can only speak for myself. Funnily enough I think I've switched from
> CHECK to CHECK_FAIL when I touched on BPF flow dissector last time [0].
>
> CHECK needs and "external" duration variable to be in scope, and so it
> was suggested to me that if I'm not measuring run-time with
> bpf_prog_test_run, CHECK_FAIL might be a better choice.

duration is unfortunate and historical, we can eventually fix that,
but it simply didn't feel worthwhile to me. I just do `static int
duration;` at the top of the test and forget about it.

>
> CHECK is also perhaps too verbose because it emits a log message on
> success (to report duration, I assume).

I actually find that CHECK emitting message regardless of success or
failure (in verbose mode or when test fails) helps a lot to narrow
down where exactly the failure happens (it's not always obvious from
error message). So unless we are talking about 100+ element loops, I'm
all for CHECK vebosity. Again, you'll see it only when something fails
or you run tests in verbose mode.

CHECK_FAIL is the worst in that case, because it makes me blind in
terms of tracking progress of test.

>
> You have a better overview of all the tests than me, but if I had the
> cycles I'd see if renaming CHECK to something more specific, for those
> test that actually track prog run time, can work.

I'd make CHECK not depend on duration instead, and convert existing
cases that do rely on duration to something like CHECK_TIMED.

>
> -jkbs
>
>
> [0] https://lore.kernel.org/bpf/87imov1y5m.fsf@cloudflare.com/
>
>
>
> >
> >>  .../bpf/prog_tests/flow_dissector_reattach.c  | 500 +++++++++++++++++-
> >>  1 file changed, 471 insertions(+), 29 deletions(-)
> >>
> >
> > [...]