netdev - Re: [PATCH bpf] libbpf: fix missing _

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: <CAEf4BzburdiRTYSJUSpSFAxKmf6ELpvEeNW502eKskzyyMaUxQ@mail.gmail.com>
Date:   Thu, 18 Jul 2019 14:16:29 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...com>,
        Kernel Team <kernel-team@...com>, Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...il.com>
Subject: Re: [PATCH bpf] libbpf: fix missing __WORDSIZE definition

On Thu, Jul 18, 2019 at 12:14 PM Arnaldo Carvalho de Melo
<arnaldo.melo@...il.com> wrote:
>
> Em Thu, Jul 18, 2019 at 03:56:19PM -0300, Arnaldo Carvalho de Melo escreveu:
> > I'll stop and replace my patch with yours to see if it survives all the
> > test builds...
>
> So, Alpine:3.4, the first image for this distro I did when I started
> these builds, survives the 6 builds with gcc and clang with your patch:
>
>
> [perfbuilder@...co linux-perf-tools-build]$ export PERF_TARBALL=http://192.168.124.1/perf/perf-5.2.0.tar.xz
> [perfbuilder@...co linux-perf-tools-build]$ dm
>    1  alpine:3.4                    : Ok   gcc (Alpine 5.3.0) 5.3.0, clang version 3.8.0 (tags/RELEASE_380/final)
>
>
> [perfbuilder@...co linux-perf-tools-build]$ grep "+ make" dm.log/alpine\:3.4
> + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= -C /git/linux/tools/perf O=/tmp/build/perf
> + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf
> + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= -C /git/linux/tools/perf O=/tmp/build/perf CC=clang
> + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf CC=clang
> + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= LIBCLANGLLVM=1 -C /git/linux/tools/perf O=/tmp/build/perf CC=clang
> + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= LIBCLANGLLVM=1 -C /git/linux/tools/perf O=/tmp/build/perf
> [perfbuilder@...co linux-perf-tools-build]$
>
> Probably all the rest will go well, will let you know.
>
> Daniel, do you mind if I carry this one in my perf/core branch? Its
> small and shouldn't clash with other patches, I think. It should go
> upstream soon:
>
> Andrii, there are these others:

I took a look at them, but I think it would be better, if you could
post them as proper patches to
bpf@...r.kernel.org/netdev@...r.kernel.org, so that others can check
and comment, if necessary.

One nit for all three of them: we typically prefix subject with just
"libbpf: " instead of "tools lib libbpf".

>
> 8dfb6ed300bf tools lib bpf: Avoid designated initializers for unnamed union members

+ attr.sample_period = attr.wakeup_events = 1;

let's instead

+ attr.sample_period = 1;
+ attr.wakeup_events = 1;

I don't like multi-assignments.

Also, if we are doing explicit assignment, let's do it for all the
fields, not split initialization like that.


> 80f7f8f21441 tools lib bpf: Avoid using 'link' as it shadows a global definition in some systems

For this one I'm confused. What compiler/system you are getting it on?

I tried to reproduce it with this example (trying both global
variable, as well as function):

#include <stdio.h>

//int link = 1;
void link() {}

int f(int link) {
        return link;
}
int main() {
        printf("%d\n", f(123));
        return 0;
}

I haven't gotten any errors nor warnings. I'm certainly liking
existing naming better, but my main concern is that we'll probably add
more code like this, and we'll forget about this problem and will
re-introduce.

So I'd rather figure out why it's happening and some rare system and
see if we can mitigate that without all the renames.


> d93fe741e291 tools lib bpf: Fix endianness macro usage for some compilers

This one looks good to me, thanks!

Acked-by: Andrii Nakryiko <andriin@...com>


>
> If you could take a look at them at my tmp.perf/core branch at:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/log/?h=tmp.perf/core
>
> I'm force pushing it now to replace my __WORDSIZE patch with yours, the
> SHAs should be the 3 above and the one below.
>
> And to build cleanly I had to cherry pick this one:
>
> 3091dafc1884 (HEAD -> perf/core) libbpf: fix ptr to u64 conversion warning on 32-bit platforms
>
> Alpine:3.5 just finished:
>
>    2 alpine:3.5                    : Ok   gcc (Alpine 6.2.1) 6.2.1 20160822, clang version 3.8.1 (tags/RELEASE_381/final)
>
> - Arnaldo