2020-03-27 09:37:15

by Andrea Righi

[permalink] [raw]
Subject: [PATCH] kselftest/runner: avoid using timeout when timeout is disabled

Avoid using /usr/bin/timeout unnecessarily if timeout is set to 0
(disabled) in the "settings" file for a specific test.

NOTE: without this change (and adding timeout=0 in the corresponding
settings file - tools/testing/selftests/seccomp/settings) the
seccomp_bpf selftest is always failing with a timeout event during the
syscall_restart step.

Signed-off-by: Andrea Righi <[email protected]>
---
tools/testing/selftests/kselftest/runner.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
index e84d901f8567..2cd3c8def0f6 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -32,7 +32,7 @@ tap_prefix()
tap_timeout()
{
# Make sure tests will time out if utility is available.
- if [ -x /usr/bin/timeout ] ; then
+ if [ -x /usr/bin/timeout ] && [ $kselftest_timeout -gt 0 ] ; then
/usr/bin/timeout "$kselftest_timeout" "$1"
else
"$1"
--
2.25.1


2020-03-27 19:29:47

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH] kselftest/runner: avoid using timeout when timeout is disabled

On Fri, Mar 27, 2020 at 10:36:20AM +0100, Andrea Righi wrote:
> Avoid using /usr/bin/timeout unnecessarily if timeout is set to 0
> (disabled) in the "settings" file for a specific test.

That seems to be a reasonable optimization, sure.

> NOTE: without this change (and adding timeout=0 in the corresponding
> settings file - tools/testing/selftests/seccomp/settings) the
> seccomp_bpf selftest is always failing with a timeout event during the
> syscall_restart step.

This, however, is worrisome. I think there is something else wrong here.
I will investigate why the output of seccomp_bpf is weird when running
under the runner scripts. Hmmm. The output looks corrupted...

-Kees

> Signed-off-by: Andrea Righi <[email protected]>
> ---
> tools/testing/selftests/kselftest/runner.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
> index e84d901f8567..2cd3c8def0f6 100644
> --- a/tools/testing/selftests/kselftest/runner.sh
> +++ b/tools/testing/selftests/kselftest/runner.sh
> @@ -32,7 +32,7 @@ tap_prefix()
> tap_timeout()
> {
> # Make sure tests will time out if utility is available.
> - if [ -x /usr/bin/timeout ] ; then
> + if [ -x /usr/bin/timeout ] && [ $kselftest_timeout -gt 0 ] ; then
> /usr/bin/timeout "$kselftest_timeout" "$1"
> else
> "$1"
> --
> 2.25.1
>

--
Kees Cook

2020-03-28 11:32:24

by Andrea Righi

[permalink] [raw]
Subject: Re: [PATCH] kselftest/runner: avoid using timeout when timeout is disabled

On Fri, Mar 27, 2020 at 12:28:05PM -0700, Kees Cook wrote:
> On Fri, Mar 27, 2020 at 10:36:20AM +0100, Andrea Righi wrote:
> > Avoid using /usr/bin/timeout unnecessarily if timeout is set to 0
> > (disabled) in the "settings" file for a specific test.
>
> That seems to be a reasonable optimization, sure.
>
> > NOTE: without this change (and adding timeout=0 in the corresponding
> > settings file - tools/testing/selftests/seccomp/settings) the
> > seccomp_bpf selftest is always failing with a timeout event during the
> > syscall_restart step.
>
> This, however, is worrisome. I think there is something else wrong here.
> I will investigate why the output of seccomp_bpf is weird when running
> under the runner scripts. Hmmm. The output looks corrupted...

Running seccomp_bpf directly (without using runner.sh) shows this error:

$ sudo ./tools/testing/selftests/seccomp/seccomp_bpf
...
seccomp_bpf.c:2839:global.syscall_restart:Expected true (1) == WIFSTOPPED(status) (0)
global.syscall_restart: Test terminated by assertion

Instead, running it via /usr/bin/timeout (with timeout disabled):

$ sudo /usr/bin/timeout 0 ./tools/testing/selftests/seccomp/seccomp_bpf
...
[ RUN ] TSYNC.siblings_fail_prctl

It gets stuck here forever, basically it's during the execution of
syscall_restart().

I'll investigate more later.

Thanks,
-Andrea

2020-04-09 06:30:40

by Andrea Righi

[permalink] [raw]
Subject: Re: [PATCH] kselftest/runner: avoid using timeout when timeout is disabled

On Fri, Mar 27, 2020 at 12:28:05PM -0700, Kees Cook wrote:
> On Fri, Mar 27, 2020 at 10:36:20AM +0100, Andrea Righi wrote:
> > Avoid using /usr/bin/timeout unnecessarily if timeout is set to 0
> > (disabled) in the "settings" file for a specific test.
>
> That seems to be a reasonable optimization, sure.
>
> > NOTE: without this change (and adding timeout=0 in the corresponding
> > settings file - tools/testing/selftests/seccomp/settings) the
> > seccomp_bpf selftest is always failing with a timeout event during the
> > syscall_restart step.
>
> This, however, is worrisome. I think there is something else wrong here.
> I will investigate why the output of seccomp_bpf is weird when running
> under the runner scripts. Hmmm. The output looks corrupted...
>
> -Kees

Hi Kees,

a quick update on this.

After further investigation Cascardo (added in cc) found that the
culprit of this issue was the usage of nanosleep() vs clock_nanosleep()
in glibc. He already sent a fix for this:

https://lkml.org/lkml/2020/4/8/968

Without this we are getting the following error:

seccomp_bpf.c:2839:global.syscall_restart:Expected true (1) == WIFSTOPPED(status) (0)
# global.syscall_restart: Test terminated by assertion

I still think my timeout optimization patch can be useful, but for this
particular problem we should definitely apply Cascardo's fix.

Thanks,
-Andrea

>
> > Signed-off-by: Andrea Righi <[email protected]>
> > ---
> > tools/testing/selftests/kselftest/runner.sh | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
> > index e84d901f8567..2cd3c8def0f6 100644
> > --- a/tools/testing/selftests/kselftest/runner.sh
> > +++ b/tools/testing/selftests/kselftest/runner.sh
> > @@ -32,7 +32,7 @@ tap_prefix()
> > tap_timeout()
> > {
> > # Make sure tests will time out if utility is available.
> > - if [ -x /usr/bin/timeout ] ; then
> > + if [ -x /usr/bin/timeout ] && [ $kselftest_timeout -gt 0 ] ; then
> > /usr/bin/timeout "$kselftest_timeout" "$1"
> > else
> > "$1"
> > --
> > 2.25.1
> >
>
> --
> Kees Cook