netdev - Re: [PATCH] selftests/bpf: use !E instead of comparing with NULL

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: <CAADnVQKjVDLBCLMzXoAQhJ5a+rZ1EKqc3dyYqpeG9M2KzGREMA@mail.gmail.com>
Date:   Tue, 13 Apr 2021 09:26:49 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     "Bird, Tim" <Tim.Bird@...y.com>
Cc:     Yang Li <yang.lee@...ux.alibaba.com>,
        Shuah Khan <shuah@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] selftests/bpf: use !E instead of comparing with NULL

On Tue, Apr 13, 2021 at 9:19 AM <Tim.Bird@...y.com> wrote:
>
> > -----Original Message-----
> > From: Alexei Starovoitov <alexei.starovoitov@...il.com>
> >
> > On Tue, Apr 13, 2021 at 9:10 AM <Tim.Bird@...y.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Alexei Starovoitov <alexei.starovoitov@...il.com>
> > > >
> > > > On Tue, Apr 13, 2021 at 2:52 AM Yang Li <yang.lee@...ux.alibaba.com> wrote:
> > > > >
> > > > > Fix the following coccicheck warnings:
> > > > > ./tools/testing/selftests/bpf/progs/profiler.inc.h:189:7-11: WARNING
> > > > > comparing pointer to 0, suggest !E
> > > > > ./tools/testing/selftests/bpf/progs/profiler.inc.h:361:7-11: WARNING
> > > > > comparing pointer to 0, suggest !E
> > > > > ./tools/testing/selftests/bpf/progs/profiler.inc.h:386:14-18: WARNING
> > > > > comparing pointer to 0, suggest !E
> > > > > ./tools/testing/selftests/bpf/progs/profiler.inc.h:402:14-18: WARNING
> > > > > comparing pointer to 0, suggest !E
> > > > > ./tools/testing/selftests/bpf/progs/profiler.inc.h:433:7-11: WARNING
> > > > > comparing pointer to 0, suggest !E
> > > > > ./tools/testing/selftests/bpf/progs/profiler.inc.h:534:14-18: WARNING
> > > > > comparing pointer to 0, suggest !E
> > > > > ./tools/testing/selftests/bpf/progs/profiler.inc.h:625:7-11: WARNING
> > > > > comparing pointer to 0, suggest !E
> > > > > ./tools/testing/selftests/bpf/progs/profiler.inc.h:767:7-11: WARNING
> > > > > comparing pointer to 0, suggest !E
> > > > >
> > > > > Reported-by: Abaci Robot <abaci@...ux.alibaba.com>
> > > > > Signed-off-by: Yang Li <yang.lee@...ux.alibaba.com>
> > > > > ---
> > > > >  tools/testing/selftests/bpf/progs/profiler.inc.h | 22 +++++++++++-----------
> > > > >  1 file changed, 11 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/tools/testing/selftests/bpf/progs/profiler.inc.h b/tools/testing/selftests/bpf/progs/profiler.inc.h
> > > > > index 4896fdf8..a33066c 100644
> > > > > --- a/tools/testing/selftests/bpf/progs/profiler.inc.h
> > > > > +++ b/tools/testing/selftests/bpf/progs/profiler.inc.h
> > > > > @@ -189,7 +189,7 @@ static INLINE void populate_ancestors(struct task_struct* task,
> > > > >  #endif
> > > > >         for (num_ancestors = 0; num_ancestors < MAX_ANCESTORS; num_ancestors++) {
> > > > >                 parent = BPF_CORE_READ(parent, real_parent);
> > > > > -               if (parent == NULL)
> > > > > +               if (!parent)
> > > >
> > > > Sorry, but I'd like the progs to stay as close as possible to the way
> > > > they were written.
> > > Why?
> > >
> > > > They might not adhere to kernel coding style in some cases.
> > > > The code could be grossly inefficient and even buggy.
> > > There would have to be a really good reason to accept
> > > grossly inefficient and even buggy code into the kernel.
> > >
> > > Can you please explain what that reason is?
> >
> > It's not the kernel. It's a test of bpf program.
> That doesn't answer the question of why you don't want any changes.
>
> Why would we not use kernel coding style guidelines and quality thresholds for
> testing code?  This *is* going into the kernel source tree, where it will be
> maintained and used by other developers.

because the way the C code is written makes llvm generate a particular
code pattern that may not be seen otherwise.
Like removing 'if' because it's useless to humans, but not to the compiler
will change generated code which may or may not trigger the behavior
the prog intends to cover.
In particular this profiler.inc.h test is compiled three different ways to
maximize code generation differences.
It may not be checking error paths in some cases which can be considered
a bug, but that's the intended behavior of the C code as it was written.
So it has nothing to do with "quality of kernel code".
and it should not be used by developers. It's neither sample nor example.