From mboxrd@z Thu Jan  1 00:00:00 1970
From: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Subject: Re: [PATCH net-next v6 09/11] seccomp: Enhance test_harness with an
 assert step mechanism
Date: Wed, 19 Apr 2017 15:02:34 -0700
Message-ID: <CAGXu5jJfnA=XeUNc4eAsUvXCja725riA7Vaod=BmM2ZbXdfQ4A@mail.gmail.com>
References: <20170328234650.19695-1-mic@digikod.net> <20170328234650.19695-10-mic@digikod.net>
 <CAGXu5j+D-FP8Kt9unNOqKrQJP4DYTpmgkJxWykZyrYiVPz3Y3Q@mail.gmail.com> <94ac6ddc-eaac-8548-f83f-826ddf05ac69@digikod.net>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Return-path: <linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
In-Reply-To: <94ac6ddc-eaac-8548-f83f-826ddf05ac69-WFhQfpSGs3bR7s880joybQ@public.gmane.org>
Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
To: =?UTF-8?B?TWlja2HDq2wgU2FsYcO8bg==?= <mic-WFhQfpSGs3bR7s880joybQ@public.gmane.org>
Cc: LKML <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Alexei Starovoitov <ast-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>, Arnaldo Carvalho de Melo <acme-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Casey Schaufler <casey-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org>, Daniel Borkmann <daniel-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org>, David Drysdale <drysdale-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>, "David S . Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>, "Eric W . Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>, James Morris <james.l.morris-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>, Jann Horn <jann-XZ1E9jl8jIdeoWH0uzbU5w@public.gmane.org>, Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>, Matthew Garrett <mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org>, Michael Kerrisk <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Paul Moore <paul-r2n+y4ga6xFZroRs9YW3xA@public.gmane.org>, Sargun Dhillon <sargun-GaZTRHToo+CzQB+pC5nmwQ@public.gmane.org>, "Serge E . Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>, Shuah Khan <shuah-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Thomas Graf <tgraf-G/eBtMaohhA@public.gmane.org>, Will Drewry <wa>
List-Id: linux-api@vger.kernel.org

On Wed, Apr 19, 2017 at 2:51 PM, Micka=C3=ABl Sala=C3=BCn <mic-WFhQfpSGs3bR7s880joybQ@public.gmane.org>=
 wrote:
>
> On 19/04/2017 02:02, Kees Cook wrote:
>> On Tue, Mar 28, 2017 at 4:46 PM, Micka=C3=ABl Sala=C3=BCn <mic@digikod.n=
et> wrote:
>>> This is useful to return an information about the error without being
>>> able to write to TH_LOG_STREAM.
>>>
>>> Helpers from test_harness.h may be useful outside of the seccomp
>>> directory.
>>>
>>> Signed-off-by: Micka=C3=ABl Sala=C3=BCn <mic-WFhQfpSGs3bR7s880joybQ@public.gmane.org>
>>> Cc: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
>>> Cc: Arnaldo Carvalho de Melo <acme-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>> Cc: Shuah Khan <shuah-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> Cc: Will Drewry <wad-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>> ---
>>>  tools/testing/selftests/seccomp/test_harness.h | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/seccomp/test_harness.h b/tools/tes=
ting/selftests/seccomp/test_harness.h
>>> index a786c69c7584..77e407663e06 100644
>>> --- a/tools/testing/selftests/seccomp/test_harness.h
>>> +++ b/tools/testing/selftests/seccomp/test_harness.h
>>> @@ -397,7 +397,7 @@ struct __test_metadata {
>>>         const char *name;
>>>         void (*fn)(struct __test_metadata *);
>>>         int termsig;
>>> -       int passed;
>>> +       __s8 passed;
>>
>> Why the reduction here? int is signed too?
>
> Because the return code of a process is capped to 8 bits and I use a
> negative value to not mess with the current interpretation of 0 (error)
> and 1 (OK) for the "passed" variable.
>
>>
>>>         int trigger; /* extra handler after the evaluation */
>>>         struct __test_metadata *prev, *next;
>>>  };
>>> @@ -476,6 +476,12 @@ void __run_test(struct __test_metadata *t)
>>>                                         "instead of by signal (code: %d=
)\n",
>>>                                         t->name,
>>>                                         WEXITSTATUS(status));
>>> +                       } else if (t->passed < 0) {
>>> +                               fprintf(TH_LOG_STREAM,
>>> +                                       "%s: Failed at step #%d\n",
>>> +                                       t->name,
>>> +                                       t->passed * -1);
>>> +                               t->passed =3D 0;
>>>                         }
>>
>> Instead of creating an overloaded mechanism here, perhaps have an
>> option reporting mechanism that can be enabled. Like adding to
>> __test_metadata "bool no_stream; int test_number;" and adding
>> test_number++ to each ASSERT/EXCEPT call, and doing something like:
>>
>> if (t->no_stream) {
>>                               fprintf(TH_LOG_STREAM,
>>                                       "%s: Failed at step #%d\n",
>>                                       t->name,
>>                                        t->test_number);
>> }
>>
>> It'd be a cleaner approach, maybe?
>
> Good idea, we will then be able to use 255 steps!
>
> Do you want me to send this as a separate patch?
>
> Can we move test_harness.h outside of the seccomp directory to be
> available to other subsystems as well?

Yeah, I would do two patches, and send them out separately (to shuah
with lkml and me in cc at least), one to move test_hardness.h into
some include/ directory, and then to add the new logic for streamless
reporting.

Thanks!

-Kees


--=20
Kees Cook
Pixel Security