Date:   Sat, 27 Aug 2022 03:13:41 +0200
From:   "Jose E. Marchesi" <jose.marchesi@...cle.com>
To:     Martin KaFai Lau <kafai@...com>
Cc:     James Hilliard <james.hilliard1@...il.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
Subject: Re: [PATCH] selftests/bpf: Fix bind{4,6} tcp/socket header type
 conflict


> On Fri, Aug 26, 2022 at 11:42:10AM -0600, James Hilliard wrote:
>> On Fri, Aug 26, 2022 at 11:17 AM Martin KaFai Lau <kafai@...com> wrote:
>> >
>> > On Fri, Aug 26, 2022 at 12:13:54AM -0600, James Hilliard 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.
>> > The users have been using these headers in the bpf progs.
>> 
>> Users can migrate away from libc headers over time, migrating away
> imo, not without a good reason.

Something that may be a good reason is that there is no BPF port of
glibc (nor of musl, nor of newlib.)  And given BPF's restrictions as an
architecture (no more than 5 arguments supported in function calls, etc)
it is very unlikely that there will ever be one.

Including certain libc headers may work well enough, but in general when
including libc headers you risk dragging in x86, or aarch64, or whatever
architecture-specific headers as well, directly or indirectly.  C
libraries (and system libraries in general) are targetted at particular
architectures/ABIs/OSes.

This means that the same BPF program may be using different data
structures depending on the system where you build it.  In the worst
case, it may choke on assembler snippets.

Thats why the GCC port offers certain headers to provide standard C,
like stdint.h.  That's the usual way to go for bare-metal targets where
no libc is available.

Again, we will be happy to change that if that's what you want.  In that
case, it will be up to the user to provide the standard definitions, be
it by including glibc headers targetted at some other architecture, or
by some other mean.  Not that I would personally recommend that, but it
is up to you.

>
>> shouldn't cause regressions and should improve reliability.
> May be I am missing something.  I also don't understand the reliability
> part.
>
> In this sys/socket.h as an example, what is wrong in using
> "'int8_t' {aka 'signed char'}" from libc and the one from
> gcc "'int8_t'; have 'char'" must be used instead.
>
> Why LLVM bpf does not have issue ?
>
>> 
>> > The solution should be on the GCC-BPF side instead of changing
>> > all bpf progs.
>> 
>> I mean, GCC doesn't really control which libc is available, it seems to
>> be a bad idea to use libc headers in general as they are developed
>> separately from GCC and the kernel/libbpf.
>> 
>> I'm not really sure how one would fix this on the GCC-BPF side without
>> introducing more potential header conflicts.

Powered by blists - more mailing lists