| Message ID | 20220826052925.980431-1-james.hilliard1@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3721359d3907c313833a2fd6e40c36a30179ea89 |
| Headers | show |
| Series | [v2] selftests/bpf: Fix bind{4,6} tcp/socket header type conflict | expand |
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@gmail.com> > --- > 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 ? 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 ?
On Fri, Aug 26, 2022 at 5:05 PM Martin KaFai Lau <kafai@fb.com> 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@gmail.com> > > --- > > 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. > > 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@gmail.com/
Hello: This patch was applied to bpf/bpf-next.git (master) by Daniel Borkmann <daniel@iogearbox.net>: On Thu, 25 Aug 2022 23:29:22 -0600 you 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 > > [...] Here is the summary with links: - [v2] selftests/bpf: Fix bind{4,6} tcp/socket header type conflict https://git.kernel.org/bpf/bpf-next/c/3721359d3907 You are awesome, thank you!
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@fb.com> 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@gmail.com> > > > --- > > > 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. > > > > > 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@gmail.com/
On Mon, Aug 29, 2022 at 12:03 PM Martin KaFai Lau <kafai@fb.com> 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@fb.com> 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@gmail.com> > > > > --- > > > > 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@gmail.com/
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> #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..d62cd9e9cf0e 100644 --- a/tools/testing/selftests/bpf/progs/bind6_prog.c +++ b/tools/testing/selftests/bpf/progs/bind6_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> #include <errno.h>
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@gmail.com> --- 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(-)