On 2/2/22 1:11 PM, Sherry Yang wrote:
seccomp_bpf failed on tests 47 global.user_notification_filter_empty and 48 global.user_notification_filter_empty_threaded when it's tested on updated kernel but with old kernel headers. Because old kernel headers don't have definition of macro __NR_clone3 which is required for these two tests. Since under selftests/, we can install headers once for all tests (the default INSTALL_HDR_PATH is usr/include), fix it by adding usr/include to the list of directories to be searched. Use "-isystem" to indicate it's a system directory as the real kernel headers directories are.
Signed-off-by: Sherry Yang sherry.yang@oracle.com Tested-by: Sherry Yang sherry.yang@oracle.com
tools/testing/selftests/seccomp/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/seccomp/Makefile b/tools/testing/selftests/seccomp/Makefile index 0ebfe8b0e147..585f7a0c10cb 100644 --- a/tools/testing/selftests/seccomp/Makefile +++ b/tools/testing/selftests/seccomp/Makefile @@ -1,5 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 -CFLAGS += -Wl,-no-as-needed -Wall +CFLAGS += -Wl,-no-as-needed -Wall -isystem ../../../../usr/include/ LDFLAGS += -lpthread TEST_GEN_PROGS := seccomp_bpf seccomp_benchmark
Looks good to me. Adding Kees Cook for his review of this change.
thanks, -- Shuah
On February 2, 2022 1:17:25 PM PST, Shuah Khan skhan@linuxfoundation.org wrote:
On 2/2/22 1:11 PM, Sherry Yang wrote:
seccomp_bpf failed on tests 47 global.user_notification_filter_empty and 48 global.user_notification_filter_empty_threaded when it's tested on updated kernel but with old kernel headers. Because old kernel headers don't have definition of macro __NR_clone3 which is required for these two tests. Since under selftests/, we can install headers once for all tests (the default INSTALL_HDR_PATH is usr/include), fix it by adding usr/include to the list of directories to be searched. Use "-isystem" to indicate it's a system directory as the real kernel headers directories are.
Signed-off-by: Sherry Yang sherry.yang@oracle.com Tested-by: Sherry Yang sherry.yang@oracle.com
tools/testing/selftests/seccomp/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/seccomp/Makefile b/tools/testing/selftests/seccomp/Makefile index 0ebfe8b0e147..585f7a0c10cb 100644 --- a/tools/testing/selftests/seccomp/Makefile +++ b/tools/testing/selftests/seccomp/Makefile @@ -1,5 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 -CFLAGS += -Wl,-no-as-needed -Wall +CFLAGS += -Wl,-no-as-needed -Wall -isystem ../../../../usr/include/
This didn't look right to me. That's outside the build tree, yes?
I thought missing headers had been globally solved already for the selftests? Going looking, though, I think I'm thinking of the KBUILD_OUTPIT work. Hm.
I thought there was a proper "updated kernel headers" dependency that should be used for this?
-Kees
LDFLAGS += -lpthread TEST_GEN_PROGS := seccomp_bpf seccomp_benchmark
Looks good to me. Adding Kees Cook for his review of this change.
thanks, -- Shuah
This didn't look right to me. That's outside the build tree, yes?
It’s inside the build tree. “../../../../usr/include“ may look a little confusing, it’s actually linux/usr/include (linux/ is top directory of the repo we git clone), i.e. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/usr/... The file tools/testing/sefltests/Makefile can install kernel headers in default path “usr/include”. “../../../../usr/include“ is also used in other Makefile of selftests, like https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tool...
I thought there was a proper "updated kernel headers" dependency that should be used for this?
Updating kernel headers could be one solution. Adding “../../../../usr/include“ in the header file search path could build the tests inside the sandbox.
Sherry
On Thu, Feb 03, 2022 at 07:40:46PM +0000, Sherry Yang wrote:
This didn't look right to me. That's outside the build tree, yes?
It’s inside the build tree. “../../../../usr/include“ may look a little confusing, it’s actually linux/usr/include (linux/ is top directory of the repo we git clone), i.e. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/usr/... The file tools/testing/sefltests/Makefile can install kernel headers in default path “usr/include”. “../../../../usr/include“ is also used in other Makefile of selftests, like https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tool...
Ah-ha, thanks. Following the other example, should it just be -I instead of -isystem?
I thought there was a proper "updated kernel headers" dependency that should be used for this?
Updating kernel headers could be one solution. Adding “../../../../usr/include“ in the header file search path could build the tests inside the sandbox.
Yeah, this probably needs separate attention. For this patch, if -I is sufficient, let's do that.
Thanks!
On Feb 3, 2022, at 12:20 PM, Kees Cook keescook@chromium.org wrote:
On Thu, Feb 03, 2022 at 07:40:46PM +0000, Sherry Yang wrote:
This didn't look right to me. That's outside the build tree, yes?
It’s inside the build tree. “../../../../usr/include“ may look a little confusing, it’s actually linux/usr/include (linux/ is top directory of the repo we git clone), i.e. https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/... The file tools/testing/sefltests/Makefile can install kernel headers in default path “usr/include”. “../../../../usr/include“ is also used in other Makefile of selftests, like https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/...
Ah-ha, thanks. Following the other example, should it just be -I instead of -isystem?
In this case, “-I” works but gcc gives warnings, shown below
———Warning Begin——— gcc -Wl,-no-as-needed -Wall -I../../../../usr/include/ -lpthread seccomp_bpf.c -o /home/opc/linux/tools/testing/selftests/seccomp/seccomp_bpf In file included from seccomp_bpf.c:29: ../../../../usr/include/linux/ptrace.h:50: warning: "PTRACE_GETREGSET" redefined #define PTRACE_GETREGSET 0x4204
In file included from seccomp_bpf.c:26: /usr/include/sys/ptrace.h:153: note: this is the location of the previous definition #define PTRACE_GETREGSET PTRACE_GETREGSET
In file included from seccomp_bpf.c:29: ../../../../usr/include/linux/ptrace.h:51: warning: "PTRACE_SETREGSET" redefined #define PTRACE_SETREGSET 0x4205
In file included from seccomp_bpf.c:26: /usr/include/sys/ptrace.h:157: note: this is the location of the previous definition #define PTRACE_SETREGSET PTRACE_SETREGSET
In file included from seccomp_bpf.c:29: ../../../../usr/include/linux/ptrace.h:53: warning: "PTRACE_SEIZE" redefined #define PTRACE_SEIZE 0x4206
In file included from seccomp_bpf.c:26: /usr/include/sys/ptrace.h:162: note: this is the location of the previous definition #define PTRACE_SEIZE PTRACE_SEIZE
In file included from seccomp_bpf.c:29: ../../../../usr/include/linux/ptrace.h:54: warning: "PTRACE_INTERRUPT" redefined #define PTRACE_INTERRUPT 0x4207
In file included from seccomp_bpf.c:26: /usr/include/sys/ptrace.h:166: note: this is the location of the previous definition #define PTRACE_INTERRUPT PTRACE_INTERRUPT
In file included from seccomp_bpf.c:29: ../../../../usr/include/linux/ptrace.h:55: warning: "PTRACE_LISTEN" redefined #define PTRACE_LISTEN 0x4208
In file included from seccomp_bpf.c:26: /usr/include/sys/ptrace.h:170: note: this is the location of the previous definition #define PTRACE_LISTEN PTRACE_LISTEN
In file included from seccomp_bpf.c:29: ../../../../usr/include/linux/ptrace.h:57: warning: "PTRACE_PEEKSIGINFO" redefined #define PTRACE_PEEKSIGINFO 0x4209
In file included from seccomp_bpf.c:26: /usr/include/sys/ptrace.h:174: note: this is the location of the previous definition #define PTRACE_PEEKSIGINFO PTRACE_PEEKSIGINFO
In file included from seccomp_bpf.c:29: ../../../../usr/include/linux/ptrace.h:65: warning: "PTRACE_GETSIGMASK" redefined #define PTRACE_GETSIGMASK 0x420a
In file included from seccomp_bpf.c:26: /usr/include/sys/ptrace.h:178: note: this is the location of the previous definition #define PTRACE_GETSIGMASK PTRACE_GETSIGMASK
In file included from seccomp_bpf.c:29: ../../../../usr/include/linux/ptrace.h:66: warning: "PTRACE_SETSIGMASK" redefined #define PTRACE_SETSIGMASK 0x420b
In file included from seccomp_bpf.c:26: /usr/include/sys/ptrace.h:182: note: this is the location of the previous definition #define PTRACE_SETSIGMASK PTRACE_SETSIGMASK
In file included from seccomp_bpf.c:29: ../../../../usr/include/linux/ptrace.h:68: warning: "PTRACE_SECCOMP_GET_FILTER" redefined #define PTRACE_SECCOMP_GET_FILTER 0x420c
In file included from seccomp_bpf.c:26: /usr/include/sys/ptrace.h:186: note: this is the location of the previous definition #define PTRACE_SECCOMP_GET_FILTER PTRACE_SECCOMP_GET_FILTER
In file included from seccomp_bpf.c:29: ../../../../usr/include/linux/ptrace.h:69: warning: "PTRACE_SECCOMP_GET_METADATA" redefined #define PTRACE_SECCOMP_GET_METADATA 0x420d
In file included from seccomp_bpf.c:26: /usr/include/sys/ptrace.h:190: note: this is the location of the previous definition #define PTRACE_SECCOMP_GET_METADATA PTRACE_SECCOMP_GET_METADATA ———Warning End———
So there is redefinition problem between glibc and kernel headers. I tried updating kernel headers, the ptrace.h installed in /usr/include/linux/ptrace.h is the same as we installed in the sandbox ../../../../usr/include/linux/ptrace.h, however, gcc doesn’t throw these warnings if we compile seccomp_bpf.c using /usr/include/linux/ptrace.h. This is because system headers will automatically suppress these warnings (refer to https://gcc.gnu.org/onlinedocs/cpp/System-Headers.html). In my opinion, it’s fair to use “-isystem”, since they're actually generated kernel headers.
Sherry
On 2/3/22 1:46 PM, Sherry Yang wrote:
On Feb 3, 2022, at 12:20 PM, Kees Cook keescook@chromium.org wrote:
On Thu, Feb 03, 2022 at 07:40:46PM +0000, Sherry Yang wrote:
This didn't look right to me. That's outside the build tree, yes?
It’s inside the build tree. “../../../../usr/include“ may look a little confusing, it’s actually linux/usr/include (linux/ is top directory of the repo we git clone), i.e. https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/... The file tools/testing/sefltests/Makefile can install kernel headers in default path “usr/include”. “../../../../usr/include“ is also used in other Makefile of selftests, like https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/...
Ah-ha, thanks. Following the other example, should it just be -I instead of -isystem?
In this case, “-I” works but gcc gives warnings, shown below
———Warning Begin——— gcc -Wl,-no-as-needed -Wall -I../../../../usr/include/ -lpthread seccomp_bpf.c -o /home/opc/linux/tools/testing/selftests/seccomp/seccomp_bpf In file included from seccomp_bpf.c:29: ../../../../usr/include/linux/ptrace.h:50: warning: "PTRACE_GETREGSET" redefined #define PTRACE_GETREGSET 0x4204 In file included from seccomp_bpf.c:26: /usr/include/sys/ptrace.h:153: note: this is the location of the previous definition #define PTRACE_GETREGSET PTRACE_GETREGSET
———Warning End———
So there is redefinition problem between glibc and kernel headers. I tried updating kernel headers, the ptrace.h installed in /usr/include/linux/ptrace.h is the same as we installed in the sandbox ../../../../usr/include/linux/ptrace.h, however, gcc doesn’t throw these warnings if we compile seccomp_bpf.c using /usr/include/linux/ptrace.h. This is because system headers will automatically suppress these warnings (refer to https://gcc.gnu.org/onlinedocs/cpp/System-Headers.html). In my opinion, it’s fair to use “-isystem”, since they're actually generated kernel headers.
Sherry
Sounds like -i works - I will queue this up for Linux 5.17-rc5.
thanks, -- Shuah
linux-kselftest-mirror@lists.linaro.org
-
Kees Cook -
Sherry Yang -
Shuah Khan