Re: [PATCH] selftests/seccomp: Fix seccomp failure by adding missing headers

Re: [PATCH] selftests/seccomp: Fix seccomp failure by adding missing headers

[Shuah Khan]

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

Re: [PATCH] selftests/seccomp: Fix seccomp failure by adding missing headers

[Kees Cook]

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

-- 
Kees Cook

Re: [PATCH] selftests/seccomp: Fix seccomp failure by adding missing headers

[Sherry Yang]

> 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/include?h=v5.17-rc2
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/tools/testing/selftests/clone3/Makefile?h=v5.17-rc2

> 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

Re: [PATCH] selftests/seccomp: Fix seccomp failure by adding missing headers

[Kees Cook]

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/include?h=v5.17-rc2
> 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/tools/testing/selftests/clone3/Makefile?h=v5.17-rc2

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!

-- 
Kees Cook

Re: [PATCH] selftests/seccomp: Fix seccomp failure by adding missing headers

[Sherry Yang]

> 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/torvalds/linux.git/tree/usr/include?h=v5.17-rc2__;!!ACWV5N9M2RV99hQ!cP8-SXVNX-k1LuWYjfUYvCYlrOJsInLi9l7hNsqLoXiFULd7xqRS9HRF9WnTno3nBg$ 
>> 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/torvalds/linux.git/tree/tools/testing/selftests/clone3/Makefile?h=v5.17-rc2__;!!ACWV5N9M2RV99hQ!cP8-SXVNX-k1LuWYjfUYvCYlrOJsInLi9l7hNsqLoXiFULd7xqRS9HRF9WmhyH6mcQ$ 
> 
> 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

Re: [PATCH] selftests/seccomp: Fix seccomp failure by adding missing headers

[Shuah Khan]

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/torvalds/linux.git/tree/usr/include?h=v5.17-rc2__;!!ACWV5N9M2RV99hQ!cP8-SXVNX-k1LuWYjfUYvCYlrOJsInLi9l7hNsqLoXiFULd7xqRS9HRF9WnTno3nBg$
>>> 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/torvalds/linux.git/tree/tools/testing/selftests/clone3/Makefile?h=v5.17-rc2__;!!ACWV5N9M2RV99hQ!cP8-SXVNX-k1LuWYjfUYvCYlrOJsInLi9l7hNsqLoXiFULd7xqRS9HRF9WmhyH6mcQ$
>>
>> 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

Re: [PATCH] selftests/seccomp: Fix seccomp failure by adding missing headers

[Sherry Yang]

> Sounds like -i works - I will queue this up for Linux 5.17-rc5.

Yeah, -I works, but just with some compiler warnings.

If this is acceptable, do I need to send V2 patch with -I?

Sherry

Re: [PATCH] selftests/seccomp: Fix seccomp failure by adding missing headers

[Shuah Khan]

On 2/10/22 11:08 AM, Sherry Yang wrote:
> Yeah, -I works, but just with some compiler warnings.
> 

warnings aren't good. Kees, hope you are okay with this.

> If warnings are acceptable, do I need to send V2 patch with -I?
> 
> Sherry
> 

No top posting please. No need to fix it to -I - please send v2
I can't find your patch on lore - not sure why.

Please cc linux-kselftest and linux-kernel@vger.kernel.org

Also run scripts/get_maintainer.pl for a complete list of recipients
for this patch.

thanks,
-- Shuah

Re: [PATCH] selftests/seccomp: Fix seccomp failure by adding missing headers

[Sherry Yang]

> No top posting please. No need to fix it to -I - please send v2
> I can't find your patch on lore - not sure why.
> 
> Please cc linux-kselftest and linux-kernel@vger.kernel.org
> 
> Also run scripts/get_maintainer.pl for a complete list of recipients
> for this patch.

Just sent v2, it’s on lore now: https://lore.kernel.org/all/20220210203049.67249-1-sherry.yang@oracle.com/T/#u

Sherry