From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S939995AbdDSWDm (ORCPT ); Wed, 19 Apr 2017 18:03:42 -0400 Received: from mail-io0-f178.google.com ([209.85.223.178]:35505 "EHLO mail-io0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S935492AbdDSWCi (ORCPT ); Wed, 19 Apr 2017 18:02:38 -0400 MIME-Version: 1.0 In-Reply-To: <94ac6ddc-eaac-8548-f83f-826ddf05ac69@digikod.net> References: <20170328234650.19695-1-mic@digikod.net> <20170328234650.19695-10-mic@digikod.net> <94ac6ddc-eaac-8548-f83f-826ddf05ac69@digikod.net> From: Kees Cook Date: Wed, 19 Apr 2017 15:02:34 -0700 X-Google-Sender-Auth: unORm7x5-BMBIDohJYrJsi9HgG8 Message-ID: Subject: Re: [PATCH net-next v6 09/11] seccomp: Enhance test_harness with an assert step mechanism To: =?UTF-8?B?TWlja2HDq2wgU2FsYcO8bg==?= Cc: LKML , Alexei Starovoitov , Andy Lutomirski , Arnaldo Carvalho de Melo , Casey Schaufler , Daniel Borkmann , David Drysdale , "David S . Miller" , "Eric W . Biederman" , James Morris , Jann Horn , Jonathan Corbet , Matthew Garrett , Michael Kerrisk , Paul Moore , Sargun Dhillon , "Serge E . Hallyn" , Shuah Khan , Tejun Heo , Thomas Graf , Will Drewry , "kernel-hardening@lists.openwall.com" , Linux API , linux-security-module , Network Development Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v3JM3lpp013572 On Wed, Apr 19, 2017 at 2:51 PM, Mickaël Salaün wrote: > > On 19/04/2017 02:02, Kees Cook wrote: >> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün 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ël Salaün >>> Cc: Andy Lutomirski >>> Cc: Arnaldo Carvalho de Melo >>> Cc: Kees Cook >>> Cc: Shuah Khan >>> Cc: Will Drewry >>> --- >>> 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/testing/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 = 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 -- Kees Cook Pixel Security