Re: [PATCH v2] selftests/bpf: Fix bind{4,6} tcp/socket header type conflict

[James Hilliard]

On Mon, Aug 29, 2022 at 12:03 PM Martin KaFai Lau <kafai@xxxxxx> wrote:
>
> On Sat, Aug 27, 2022 at 05:38:29AM -0600, James Hilliard wrote:
> > On Fri, Aug 26, 2022 at 5:05 PM Martin KaFai Lau <kafai@xxxxxx> wrote:
> > >
> > > On Thu, Aug 25, 2022 at 11:29:22PM -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 remove these as they are not
> > > > actually needed.
> > > >
> > > > 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@xxxxxxxxx>
> > > > ---
> > > > Changes v1 -> v2:
> > > >   - just remove netinet/tcp.h and sys/socket.h
> > > > ---
> > > >  tools/testing/selftests/bpf/progs/bind4_prog.c | 2 --
> > > >  tools/testing/selftests/bpf/progs/bind6_prog.c | 2 --
> > > >  2 files changed, 4 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/progs/bind4_prog.c b/tools/testing/selftests/bpf/progs/bind4_prog.c
> > > > index 474c6a62078a..a487f60b73ac 100644
> > > > --- a/tools/testing/selftests/bpf/progs/bind4_prog.c
> > > > +++ b/tools/testing/selftests/bpf/progs/bind4_prog.c
> > > > @@ -6,8 +6,6 @@
> > > >  #include <linux/bpf.h>
> > > >  #include <linux/in.h>
> > > >  #include <linux/in6.h>
> > > > -#include <sys/socket.h>
> > > > -#include <netinet/tcp.h>
> > > >  #include <linux/if.h>
> > > Are the AF_INET and SOCK_STREAM coming from linux/if.h somehow
> > > and they are not from indirectly including sys/socket.h ?
> >
> > Hmm, seems they are both coming from sys/socket.h:
> >
> > Tests with my v2 patch applied:
> > progs/bind4_prog.c:15: error: "AF_INET" redefined [-Werror]
> >    15 | #define AF_INET nonsense
> >       |
> > In file included from /usr/include/x86_64-linux-gnu/sys/socket.h:33,
> >                  from /usr/include/linux/if.h:28,
> >                  from progs/bind4_prog.c:9:
> > /usr/include/x86_64-linux-gnu/bits/socket.h:97: note: this is the
> > location of the previous definition
> >    97 | #define AF_INET         PF_INET
> >       |
> >
> > progs/bind4_prog.c:15: error: "SOCK_STREAM" redefined [-Werror]
> >    15 | #define SOCK_STREAM nonsense
> >       |
> > In file included from /usr/include/x86_64-linux-gnu/bits/socket.h:38,
> >                  from /usr/include/x86_64-linux-gnu/sys/socket.h:33,
> >                  from /usr/include/linux/if.h:28,
> >                  from progs/bind4_prog.c:9:
> > /usr/include/x86_64-linux-gnu/bits/socket_type.h:28: note: this is the
> > location of the previous definition
> >    28 | #define SOCK_STREAM SOCK_STREAM
> >       |
> >
> > So I guess the problematic header is netinet/tcp.h and sys/socket.h is
> > just a redundant include?
> >
> > Removing just netinet/tcp.h does appear sufficient to fix the issue.
> Yeah, it is what I am puzzled and getting at.
> <sys/socket.h> is fine and <netinet/tcp.h> is not ok.
> They are both from glibc ?  This kind of header changes
> is hard to reason without doing the kind of experiment
> that you just did.

I think so, it kind of looks to me like most of the tests were avoiding this
issue in various ways already, the percentage with this issue seems to
be fairly low.

>
> >
> > >
> > > If the program does not need if.h, what should it use ?
> > > There are other progs in selftest/bpf that include sys/socket.h
> > > and they have no issue ?
> >
> > I'm still working through gcc issues with the test suite so there's
> > probably some cases I haven't identified yet but this is the only one
> > that seemed to need any code changes when removing those 2
> > headers that I've found so far:
> > https://lore.kernel.org/bpf/20220826055025.1018491-1-james.hilliard1@xxxxxxxxx/