Date:   Fri, 26 Aug 2022 09:25:39 -0600
From:   James Hilliard <james.hilliard1@...il.com>
To:     "Jose E. Marchesi" <jose.marchesi@...cle.com>
Cc:     Martin KaFai Lau <kafai@...com>, bpf@...r.kernel.org,
        Andrii Nakryiko <andrii@...nel.org>,
        Mykola Lysenko <mykolal@...com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
        Shuah Khan <shuah@...nel.org>, linux-kselftest@...r.kernel.org,
        linux-kernel@...r.kernel.org, david.faust@...cle.com
Subject: Re: [PATCH] selftests/bpf: Fix bind{4,6} tcp/socket header type conflict

On Fri, Aug 26, 2022 at 7:19 AM Jose E. Marchesi
<jose.marchesi@...cle.com> wrote:
>
>
> > On Thu, Aug 25, 2022 at 11:49 PM Martin KaFai Lau <kafai@...com> wrote:
> >>
> >> On Thu, Aug 25, 2022 at 11:31:15PM -0600, James Hilliard wrote:
> >> > On Thu, Aug 25, 2022 at 11:16 PM Martin KaFai Lau <kafai@...com> wrote:
> >> > >
> >> > > On Thu, Aug 25, 2022 at 04:17:49PM -0600, James Hilliard wrote:
> >> > > > There is a potential for us to hit a type conflict when including
> >> > > > netinet/tcp.h with sys/socket.h, we can replace both of these includes
> >> > > > with linux/tcp.h to avoid this conflict.
> >> > > >
> >> > > > Fixes errors like:
> >> > > > In file included from /usr/include/netinet/tcp.h:91,
> >> > > >                  from progs/bind4_prog.c:10:
> >> > > > /home/buildroot/opt/cross/lib/gcc/bpf/13.0.0/include/stdint.h:34:23: error: conflicting types for 'int8_t'; have 'char'
> >> > > >    34 | typedef __INT8_TYPE__ int8_t;
> >> > > >       |                       ^~~~~~
> >> > > > In file included from /usr/include/x86_64-linux-gnu/sys/types.h:155,
> >> > > >                  from /usr/include/x86_64-linux-gnu/bits/socket.h:29,
> >> > > >                  from /usr/include/x86_64-linux-gnu/sys/socket.h:33,
> >> > > >                  from progs/bind4_prog.c:9:
> >> > > > /usr/include/x86_64-linux-gnu/bits/stdint-intn.h:24:18: note: previous declaration of 'int8_t' with type 'int8_t' {aka 'signed char'}
> >> > > >    24 | typedef __int8_t int8_t;
> >> > > >       |                  ^~~~~~
> >> > > > /home/buildroot/opt/cross/lib/gcc/bpf/13.0.0/include/stdint.h:43:24:
> >> > > > error: conflicting types for 'int64_t'; have 'long int'
> >> > > >    43 | typedef __INT64_TYPE__ int64_t;
> >> > > >       |                        ^~~~~~~
> >> > > > /usr/include/x86_64-linux-gnu/bits/stdint-intn.h:27:19: note:
> >> > > > previous declaration of 'int64_t' with type 'int64_t' {aka
> >> > > > 'long long int'}
> >> > > >    27 | typedef __int64_t int64_t;
> >> > > >       |                   ^~~~~~~
> >> > > > make: *** [Makefile:537:
> >> > > > /home/buildroot/bpf-next/tools/testing/selftests/bpf/bpf_gcc/bind4_prog.o]
> >> > > > Error 1
> >> > > >
> >> > > > Signed-off-by: James Hilliard <james.hilliard1@...il.com>
> >> > > > ---
> >> > > >  tools/testing/selftests/bpf/progs/bind4_prog.c | 3 +--
> >> > > >  tools/testing/selftests/bpf/progs/bind6_prog.c | 3 +--
> >> > > >  2 files changed, 2 insertions(+), 4 deletions(-)
> >> > > >
> >> > > > diff --git a/tools/testing/selftests/bpf/progs/bind4_prog.c
> >> > > > b/tools/testing/selftests/bpf/progs/bind4_prog.c
> >> > > > index 474c6a62078a..6bd20042fd53 100644
> >> > > > --- a/tools/testing/selftests/bpf/progs/bind4_prog.c
> >> > > > +++ b/tools/testing/selftests/bpf/progs/bind4_prog.c
> >> > > > @@ -6,8 +6,7 @@
> >> > > >  #include <linux/bpf.h>
> >> > > >  #include <linux/in.h>
> >> > > >  #include <linux/in6.h>
> >> > > > -#include <sys/socket.h>
> >> > > > -#include <netinet/tcp.h>
> >> > > These includes look normal to me.  What environment is hitting this.
> >> >
> >> > I was hitting this error with GCC 13(GCC master branch).
> >> These two includes (<sys/socket.h> and <netinet/tcp.h>) are normal,
> >> so does it mean all existing programs need to change to use gcc 13 ?
> >
> > Well I think it's mostly just an issue getting hit with GCC-BPF as it
> > looks to me like a cross compilation host/target header conflict.
>
> This is an interesting issue.
>
> Right now the BPF GCC target is a sort of a bare-metal target.  As such,
> it provides a set of header files that implement ISO C types and other
> machinery (i.e. it doesn't rely on a C library to provide them):
>
>   iso646.h
>   stdalign.h
>   stdarg.h
>   stdatomic.h
>   stdbool.h
>   stddef.h
>   stdfix.h
>   stdint.h
>   stdnoreturn.h
>   tgmath.h
>   unwind.h
>   varargs.h
>
> This is because we were expecting this to be used like:
>
>        <compiler-provided std C headers>
>                   |        |
>                   v        |
>         <kernel headers>   |
>                   |        |
>                   v        v
>                <BPF C program>
>
> However, if it is expected/intended for C BPF programs to include libc
> headers, such as sys/socket.h, this can quickly go sour as you have
> found with that conflict.
>
> So this leads to the question: should we turn the BPF target into a
> target that assumes a libc?  This basically means we will be assuming
> BPF programs are always compiled in an environment that provides a
> standard stdint.h, stdbool.h and friends.

Well for a normal GCC BPF setup we're basically cross compiling for the
BPF bare metal target while sharing headers with the build host(for libbpf
and any other libc headers that get included).

On the other hand when using GCC BPF as part of a full cross toolchain
we actually end up sharing headers with our real cross target architecture
sysroot(which would provide a libc), essentially in that case BPF is a bare
metal cross target which shares headers with the real cross target(which
is not a bare metal target). For this libbpf is installed to the real
cross target
sysroot which is used by both GCC BPF(for bpf progs) and the real cross
target GCC compiler(for userspace side). From my understanding with this
setup GCC BPF will pick up the real cross target libc headers as a fallback
which may sometimes have conflict/compatibility issues with the kernel
headers.

I think it's probably best to avoid depending on libc headers as things may
otherwise get even more complex. You would essentially have 2 libc's
in a normal GCC BPF setup and 3 libc's in a full cross toolchain setup(you'd
have one for the build host, one for the real cross target arch and one for
the BPF target arch).

Cross build systems will typically allow a libc choice as
well(glibc/musl/uclibc)
and we don't really want the bpf programs to have to care about the specific
libc being used as they are bare metal programs which shouldn't depend on
a libc.

>
> Thoughts?
>
> >> > > I don't prefer the selftest writers need to remember this rule.
> >> > >
> >> > > Beside, afaict, tcp.h should be removed because
> >> > > I don't see this test needs it.  I tried removing it
> >> > > and it works fine.  It should be removed instead of replacing it
> >> > > with another unnecessary tcp.h.
> >> >
> >> > Oh, that does also appear to work, thought I had tried that already but I guess
> >> > I hadn't, sent a v2 with them removed:
> >> > https://lore.kernel.org/bpf/20220826052925.980431-1-james.hilliard1@gmail.com/T/#u
> >> >
> >> > >
> >> > > > +#include <linux/tcp.h>
> >> > > >  #include <linux/if.h>
> >> > > >  #include <errno.h>
> >> > > >
> >> > > > diff --git a/tools/testing/selftests/bpf/progs/bind6_prog.c
> >> > > > b/tools/testing/selftests/bpf/progs/bind6_prog.c
> >> > > > index c19cfa869f30..f37617b35a55 100644
> >> > > > --- a/tools/testing/selftests/bpf/progs/bind6_prog.c
> >> > > > +++ b/tools/testing/selftests/bpf/progs/bind6_prog.c
> >> > > > @@ -6,8 +6,7 @@
> >> > > >  #include <linux/bpf.h>
> >> > > >  #include <linux/in.h>
> >> > > >  #include <linux/in6.h>
> >> > > > -#include <sys/socket.h>
> >> > > > -#include <netinet/tcp.h>
> >> > > > +#include <linux/tcp.h>
> >> > > >  #include <linux/if.h>
> >> > > >  #include <errno.h>
> >> > > >
> >> > > > --
> >> > > > 2.34.1
> >> > > >

Powered by blists - more mailing lists