* [PATCH] selftests/seccomp: Actually sleep for 1/10th second
@ 2019-01-27 9:43 Kees Cook
2019-01-27 10:25 ` Nick Desaulniers
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2019-01-27 9:43 UTC (permalink / raw)
To: Shuah Khan; +Cc: linux-kernel, linux-kselftest, ndesaulniers
Clang noticed that some none-zero sleep()s were actually using zero
anyway. This switches to nanosleep() to gain sub-second granularity.
seccomp_bpf.c:2625:9: warning: implicit conversion from 'double' to
'unsigned int' changes value from 0.1 to 0 [-Wliteral-conversion]
sleep(0.1);
~~~~~ ^~~
Signed-off-by: Kees Cook <keescook@chromium.org>
---
tools/testing/selftests/seccomp/seccomp_bpf.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 067cb4607d6c..a9f278c13f13 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -2569,6 +2569,7 @@ TEST_F(TSYNC, two_siblings_not_under_filter)
{
long ret, sib;
void *status;
+ struct timespec delay = { .tv_nsec = 100000000 };
ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
@@ -2622,7 +2623,7 @@ TEST_F(TSYNC, two_siblings_not_under_filter)
EXPECT_EQ(SIBLING_EXIT_UNKILLED, (long)status);
/* Poll for actual task death. pthread_join doesn't guarantee it. */
while (!kill(self->sibling[sib].system_tid, 0))
- sleep(0.1);
+ nanosleep(&delay, NULL);
/* Switch to the remaining sibling */
sib = !sib;
@@ -2647,7 +2648,7 @@ TEST_F(TSYNC, two_siblings_not_under_filter)
EXPECT_EQ(0, (long)status);
/* Poll for actual task death. pthread_join doesn't guarantee it. */
while (!kill(self->sibling[sib].system_tid, 0))
- sleep(0.1);
+ nanosleep(&delay, NULL);
ret = seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_TSYNC,
&self->apply_prog);
--
2.17.1
--
Kees Cook
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] selftests/seccomp: Actually sleep for 1/10th second
2019-01-27 9:43 [PATCH] selftests/seccomp: Actually sleep for 1/10th second Kees Cook
@ 2019-01-27 10:25 ` Nick Desaulniers
2019-01-27 19:13 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Nick Desaulniers @ 2019-01-27 10:25 UTC (permalink / raw)
To: Kees Cook; +Cc: Shuah Khan, LKML, linux-kselftest
On Sun, Jan 27, 2019 at 1:44 AM Kees Cook <keescook@chromium.org> wrote:
>
> Clang noticed that some none-zero sleep()s were actually using zero
> anyway. This switches to nanosleep() to gain sub-second granularity.
>
> seccomp_bpf.c:2625:9: warning: implicit conversion from 'double' to
> 'unsigned int' changes value from 0.1 to 0 [-Wliteral-conversion]
> sleep(0.1);
> ~~~~~ ^~~
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> tools/testing/selftests/seccomp/seccomp_bpf.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> index 067cb4607d6c..a9f278c13f13 100644
> --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> @@ -2569,6 +2569,7 @@ TEST_F(TSYNC, two_siblings_not_under_filter)
> {
> long ret, sib;
> void *status;
> + struct timespec delay = { .tv_nsec = 100000000 };
"Omitted fields are implicitly initialized the same as for objects
that have static storage duration."
https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
https://godbolt.org/z/cuGqxM
(So this wont sleep an arbitrary amount of seconds, phew)
>
> ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
> TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
> @@ -2622,7 +2623,7 @@ TEST_F(TSYNC, two_siblings_not_under_filter)
> EXPECT_EQ(SIBLING_EXIT_UNKILLED, (long)status);
> /* Poll for actual task death. pthread_join doesn't guarantee it. */
> while (!kill(self->sibling[sib].system_tid, 0))
> - sleep(0.1);
> + nanosleep(&delay, NULL);
> /* Switch to the remaining sibling */
> sib = !sib;
>
> @@ -2647,7 +2648,7 @@ TEST_F(TSYNC, two_siblings_not_under_filter)
> EXPECT_EQ(0, (long)status);
> /* Poll for actual task death. pthread_join doesn't guarantee it. */
> while (!kill(self->sibling[sib].system_tid, 0))
> - sleep(0.1);
> + nanosleep(&delay, NULL);
Interesting bug. If the sleeps weren't doing anything, are they even
needed? Does adding the tests cause them to continue to pass, or start
to fail? If they weren't doing anything, and the tests were passing,
maybe it's just better to remove them?
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] selftests/seccomp: Actually sleep for 1/10th second
2019-01-27 10:25 ` Nick Desaulniers
@ 2019-01-27 19:13 ` Kees Cook
2019-01-27 19:36 ` Nick Desaulniers
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2019-01-27 19:13 UTC (permalink / raw)
To: Nick Desaulniers; +Cc: Shuah Khan, LKML, open list:KERNEL SELFTEST FRAMEWORK
On Sun, Jan 27, 2019 at 11:25 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sun, Jan 27, 2019 at 1:44 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > Clang noticed that some none-zero sleep()s were actually using zero
> > anyway. This switches to nanosleep() to gain sub-second granularity.
> >
> > seccomp_bpf.c:2625:9: warning: implicit conversion from 'double' to
> > 'unsigned int' changes value from 0.1 to 0 [-Wliteral-conversion]
> > sleep(0.1);
> > ~~~~~ ^~~
> >
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > tools/testing/selftests/seccomp/seccomp_bpf.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> > index 067cb4607d6c..a9f278c13f13 100644
> > --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> > +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> > @@ -2569,6 +2569,7 @@ TEST_F(TSYNC, two_siblings_not_under_filter)
> > {
> > long ret, sib;
> > void *status;
> > + struct timespec delay = { .tv_nsec = 100000000 };
>
> "Omitted fields are implicitly initialized the same as for objects
> that have static storage duration."
> https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
> https://godbolt.org/z/cuGqxM
> (So this wont sleep an arbitrary amount of seconds, phew)
Yup. :) Even an empty initializer works ... = { };
(Except that padding bytes aren't always included in the zeroing...)
>
> >
> > ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
> > TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
> > @@ -2622,7 +2623,7 @@ TEST_F(TSYNC, two_siblings_not_under_filter)
> > EXPECT_EQ(SIBLING_EXIT_UNKILLED, (long)status);
> > /* Poll for actual task death. pthread_join doesn't guarantee it. */
> > while (!kill(self->sibling[sib].system_tid, 0))
> > - sleep(0.1);
> > + nanosleep(&delay, NULL);
> > /* Switch to the remaining sibling */
> > sib = !sib;
> >
> > @@ -2647,7 +2648,7 @@ TEST_F(TSYNC, two_siblings_not_under_filter)
> > EXPECT_EQ(0, (long)status);
> > /* Poll for actual task death. pthread_join doesn't guarantee it. */
> > while (!kill(self->sibling[sib].system_tid, 0))
> > - sleep(0.1);
> > + nanosleep(&delay, NULL);
>
> Interesting bug. If the sleeps weren't doing anything, are they even
> needed? Does adding the tests cause them to continue to pass, or start
> to fail? If they weren't doing anything, and the tests were passing,
> maybe it's just better to remove them?
It was just spinning. This restores the intention of not being so
aggressive in the wait loop. While the sleep could be removed, that
wasn't the intention.
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] selftests/seccomp: Actually sleep for 1/10th second
2019-01-27 19:13 ` Kees Cook
@ 2019-01-27 19:36 ` Nick Desaulniers
2019-01-27 23:53 ` Guenter Roeck
2019-01-28 7:58 ` Kees Cook
0 siblings, 2 replies; 6+ messages in thread
From: Nick Desaulniers @ 2019-01-27 19:36 UTC (permalink / raw)
To: Kees Cook
Cc: Shuah Khan, LKML, open list:KERNEL SELFTEST FRAMEWORK, Guenter Roeck
On Sun, Jan 27, 2019 at 11:13 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Sun, Jan 27, 2019 at 11:25 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > On Sun, Jan 27, 2019 at 1:44 AM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > Clang noticed that some none-zero sleep()s were actually using zero
> > > anyway. This switches to nanosleep() to gain sub-second granularity.
> > >
> > > seccomp_bpf.c:2625:9: warning: implicit conversion from 'double' to
> > > 'unsigned int' changes value from 0.1 to 0 [-Wliteral-conversion]
> > > sleep(0.1);
> > > ~~~~~ ^~~
> > >
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > ---
> > > tools/testing/selftests/seccomp/seccomp_bpf.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> > > index 067cb4607d6c..a9f278c13f13 100644
> > > --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> > > +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> > > @@ -2569,6 +2569,7 @@ TEST_F(TSYNC, two_siblings_not_under_filter)
> > > {
> > > long ret, sib;
> > > void *status;
> > > + struct timespec delay = { .tv_nsec = 100000000 };
> >
> > "Omitted fields are implicitly initialized the same as for objects
> > that have static storage duration."
> > https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
> > https://godbolt.org/z/cuGqxM
> > (So this wont sleep an arbitrary amount of seconds, phew)
>
> Yup. :) Even an empty initializer works ... = { };
> (Except that padding bytes aren't always included in the zeroing...)
>
> >
> > >
> > > ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
> > > TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
> > > @@ -2622,7 +2623,7 @@ TEST_F(TSYNC, two_siblings_not_under_filter)
> > > EXPECT_EQ(SIBLING_EXIT_UNKILLED, (long)status);
> > > /* Poll for actual task death. pthread_join doesn't guarantee it. */
> > > while (!kill(self->sibling[sib].system_tid, 0))
> > > - sleep(0.1);
> > > + nanosleep(&delay, NULL);
> > > /* Switch to the remaining sibling */
> > > sib = !sib;
> > >
> > > @@ -2647,7 +2648,7 @@ TEST_F(TSYNC, two_siblings_not_under_filter)
> > > EXPECT_EQ(0, (long)status);
> > > /* Poll for actual task death. pthread_join doesn't guarantee it. */
> > > while (!kill(self->sibling[sib].system_tid, 0))
> > > - sleep(0.1);
> > > + nanosleep(&delay, NULL);
> >
> > Interesting bug. If the sleeps weren't doing anything, are they even
> > needed? Does adding the tests cause them to continue to pass, or start
> > to fail? If they weren't doing anything, and the tests were passing,
> > maybe it's just better to remove them?
>
> It was just spinning.
So this test has been broken? If so, do you know for how long? Or
who's monitoring them? Either way, thanks for noticing and fixing.
+Guenter; did you notice if this test was failing? Are your boot tests
running kselftests?
> This restores the intention of not being so
> aggressive in the wait loop. While the sleep could be removed, that
> wasn't the intention.
Oh, yeah I guess the comment above it about pthread_join is relevant.
I just get highly highly suspicious whenever I see sleeps added to any
code.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] selftests/seccomp: Actually sleep for 1/10th second
2019-01-27 19:36 ` Nick Desaulniers
@ 2019-01-27 23:53 ` Guenter Roeck
2019-01-28 7:58 ` Kees Cook
1 sibling, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2019-01-27 23:53 UTC (permalink / raw)
To: Nick Desaulniers, Kees Cook
Cc: Shuah Khan, LKML, open list:KERNEL SELFTEST FRAMEWORK
On 1/27/19 11:36 AM, Nick Desaulniers wrote:
> On Sun, Jan 27, 2019 at 11:13 AM Kees Cook <keescook@chromium.org> wrote:
>>
>> On Sun, Jan 27, 2019 at 11:25 PM Nick Desaulniers
>> <ndesaulniers@google.com> wrote:
>>>
>>> On Sun, Jan 27, 2019 at 1:44 AM Kees Cook <keescook@chromium.org> wrote:
>>>>
>>>> Clang noticed that some none-zero sleep()s were actually using zero
>>>> anyway. This switches to nanosleep() to gain sub-second granularity.
>>>>
>>>> seccomp_bpf.c:2625:9: warning: implicit conversion from 'double' to
>>>> 'unsigned int' changes value from 0.1 to 0 [-Wliteral-conversion]
>>>> sleep(0.1);
>>>> ~~~~~ ^~~
>>>>
>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>> ---
>>>> tools/testing/selftests/seccomp/seccomp_bpf.c | 5 +++--
>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
>>>> index 067cb4607d6c..a9f278c13f13 100644
>>>> --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
>>>> +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
>>>> @@ -2569,6 +2569,7 @@ TEST_F(TSYNC, two_siblings_not_under_filter)
>>>> {
>>>> long ret, sib;
>>>> void *status;
>>>> + struct timespec delay = { .tv_nsec = 100000000 };
>>>
>>> "Omitted fields are implicitly initialized the same as for objects
>>> that have static storage duration."
>>> https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
>>> https://godbolt.org/z/cuGqxM
>>> (So this wont sleep an arbitrary amount of seconds, phew)
>>
>> Yup. :) Even an empty initializer works ... = { };
>> (Except that padding bytes aren't always included in the zeroing...)
>>
>>>
>>>>
>>>> ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
>>>> TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
>>>> @@ -2622,7 +2623,7 @@ TEST_F(TSYNC, two_siblings_not_under_filter)
>>>> EXPECT_EQ(SIBLING_EXIT_UNKILLED, (long)status);
>>>> /* Poll for actual task death. pthread_join doesn't guarantee it. */
>>>> while (!kill(self->sibling[sib].system_tid, 0))
>>>> - sleep(0.1);
>>>> + nanosleep(&delay, NULL);
>>>> /* Switch to the remaining sibling */
>>>> sib = !sib;
>>>>
>>>> @@ -2647,7 +2648,7 @@ TEST_F(TSYNC, two_siblings_not_under_filter)
>>>> EXPECT_EQ(0, (long)status);
>>>> /* Poll for actual task death. pthread_join doesn't guarantee it. */
>>>> while (!kill(self->sibling[sib].system_tid, 0))
>>>> - sleep(0.1);
>>>> + nanosleep(&delay, NULL);
>>>
>>> Interesting bug. If the sleeps weren't doing anything, are they even
>>> needed? Does adding the tests cause them to continue to pass, or start
>>> to fail? If they weren't doing anything, and the tests were passing,
>>> maybe it's just better to remove them?
>>
>> It was just spinning.
>
> So this test has been broken? If so, do you know for how long? Or
> who's monitoring them? Either way, thanks for noticing and fixing.
>
> +Guenter; did you notice if this test was failing? Are your boot tests
> running kselftests?
>
No, I don't run kselftests at this time.
Guenter
>> This restores the intention of not being so
>> aggressive in the wait loop. While the sleep could be removed, that
>> wasn't the intention.
>
> Oh, yeah I guess the comment above it about pthread_join is relevant.
> I just get highly highly suspicious whenever I see sleeps added to any
> code.
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] selftests/seccomp: Actually sleep for 1/10th second
2019-01-27 19:36 ` Nick Desaulniers
2019-01-27 23:53 ` Guenter Roeck
@ 2019-01-28 7:58 ` Kees Cook
1 sibling, 0 replies; 6+ messages in thread
From: Kees Cook @ 2019-01-28 7:58 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Shuah Khan, LKML, open list:KERNEL SELFTEST FRAMEWORK, Guenter Roeck
On Mon, Jan 28, 2019 at 8:37 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
> So this test has been broken? If so, do you know for how long? Or
> who's monitoring them? Either way, thanks for noticing and fixing.
No, it's been working fine. It just consumes 100% cpu during the
spin-wait. The intention was to sleep for a little bit to avoid a
tight loop. It was just resource-inefficient.
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-01-28 7:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-27 9:43 [PATCH] selftests/seccomp: Actually sleep for 1/10th second Kees Cook
2019-01-27 10:25 ` Nick Desaulniers
2019-01-27 19:13 ` Kees Cook
2019-01-27 19:36 ` Nick Desaulniers
2019-01-27 23:53 ` Guenter Roeck
2019-01-28 7:58 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).