[net-next,v7,01/10] selftest: Enhance kselftest_harness.h with a step mechanism

Commit Message

Mickaël Salaün Aug. 21, 2017, 12:09 a.m. UTC

This step mechanism may be useful to return an information about the
error without being able to write to TH_LOG_STREAM.

Set _metadata->no_print to true to print this counter.

Signed-off-by: Mickaël Salaün <[e-mail]>
Cc: Andy Lutomirski <[e-mail]>
Cc: Arnaldo Carvalho de Melo <[e-mail]>
Cc: Kees Cook <[e-mail]>
Cc: Shuah Khan <[e-mail]>
Cc: Will Drewry <[e-mail]>
Link: https://lkml.kernel.org/r/[e-mail]
---

This patch is intended to the kselftest tree:
https://lkml.kernel.org/r/[e-mail]

Changes since v6:
* add the step counter in assert/expect macros and use _metadata to
  enable the counter (suggested by Kees Cook)
---
 tools/testing/selftests/kselftest_harness.h   | 31 ++++++++++++++++++++++-----
 tools/testing/selftests/seccomp/seccomp_bpf.c |  2 +-
 2 files changed, 27 insertions(+), 6 deletions(-)

Comments

Alexei Starovoitov Aug. 24, 2017, 2:31 a.m. UTC | #1

On Mon, Aug 21, 2017 at 02:09:24AM +0200, Mickaël Salaün wrote:
> This step mechanism may be useful to return an information about the
> error without being able to write to TH_LOG_STREAM.
> 
> Set _metadata->no_print to true to print this counter.
> 
> Signed-off-by: Mickaël Salaün <[e-mail]>
> Cc: Andy Lutomirski <[e-mail]>
> Cc: Arnaldo Carvalho de Melo <[e-mail]>
> Cc: Kees Cook <[e-mail]>
> Cc: Shuah Khan <[e-mail]>
> Cc: Will Drewry <[e-mail]>
> Link: https://lkml.kernel.org/r/[e-mail]
> ---
> 
> This patch is intended to the kselftest tree:
> https://lkml.kernel.org/r/[e-mail]
> 
> Changes since v6:
> * add the step counter in assert/expect macros and use _metadata to
>   enable the counter (suggested by Kees Cook)
> ---
>  tools/testing/selftests/kselftest_harness.h   | 31 ++++++++++++++++++++++-----
>  tools/testing/selftests/seccomp/seccomp_bpf.c |  2 +-
>  2 files changed, 27 insertions(+), 6 deletions(-)

is there a dependency on this in patches 2+ ?
if not, I would send this patch to selftests right away.

Mickaël Salaün Aug. 25, 2017, 7:58 a.m. UTC | #2

On 24/08/2017 04:31, Alexei Starovoitov wrote:
> On Mon, Aug 21, 2017 at 02:09:24AM +0200, Mickaël Salaün wrote:
>> This step mechanism may be useful to return an information about the
>> error without being able to write to TH_LOG_STREAM.
>>
>> Set _metadata->no_print to true to print this counter.
>>
>> Signed-off-by: Mickaël Salaün <[e-mail]>
>> Cc: Andy Lutomirski <[e-mail]>
>> Cc: Arnaldo Carvalho de Melo <[e-mail]>
>> Cc: Kees Cook <[e-mail]>
>> Cc: Shuah Khan <[e-mail]>
>> Cc: Will Drewry <[e-mail]>
>> Link: https://lkml.kernel.org/r/[e-mail]
>> ---
>>
>> This patch is intended to the kselftest tree:
>> https://lkml.kernel.org/r/[e-mail]
>>
>> Changes since v6:
>> * add the step counter in assert/expect macros and use _metadata to
>>   enable the counter (suggested by Kees Cook)
>> ---
>>  tools/testing/selftests/kselftest_harness.h   | 31 ++++++++++++++++++++++-----
>>  tools/testing/selftests/seccomp/seccomp_bpf.c |  2 +-
>>  2 files changed, 27 insertions(+), 6 deletions(-)
> 
> is there a dependency on this in patches 2+ ?
> if not, I would send this patch to selftests right away.
> 
> 

The Landlock tests [patch 9/10] rely on it for now.

I sent it three weeks ago:
https://lkml.kernel.org/r/[e-mail]

Anyway, until this patch is merged in the kselftest tree and then
available to net-next, I'll have to keep it here.

Alexei Starovoitov Aug. 26, 2017, 1:07 a.m. UTC | #3

On Fri, Aug 25, 2017 at 09:58:33AM +0200, Mickaël Salaün wrote:
> 
> 
> On 24/08/2017 04:31, Alexei Starovoitov wrote:
> > On Mon, Aug 21, 2017 at 02:09:24AM +0200, Mickaël Salaün wrote:
> >> This step mechanism may be useful to return an information about the
> >> error without being able to write to TH_LOG_STREAM.
> >>
> >> Set _metadata->no_print to true to print this counter.
> >>
> >> Signed-off-by: Mickaël Salaün <[e-mail]>
> >> Cc: Andy Lutomirski <[e-mail]>
> >> Cc: Arnaldo Carvalho de Melo <[e-mail]>
> >> Cc: Kees Cook <[e-mail]>
> >> Cc: Shuah Khan <[e-mail]>
> >> Cc: Will Drewry <[e-mail]>
> >> Link: https://lkml.kernel.org/r/[e-mail]
> >> ---
> >>
> >> This patch is intended to the kselftest tree:
> >> https://lkml.kernel.org/r/[e-mail]
> >>
> >> Changes since v6:
> >> * add the step counter in assert/expect macros and use _metadata to
> >>   enable the counter (suggested by Kees Cook)
> >> ---
> >>  tools/testing/selftests/kselftest_harness.h   | 31 ++++++++++++++++++++++-----
> >>  tools/testing/selftests/seccomp/seccomp_bpf.c |  2 +-
> >>  2 files changed, 27 insertions(+), 6 deletions(-)
> > 
> > is there a dependency on this in patches 2+ ?
> > if not, I would send this patch to selftests right away.
> > 
> > 
> 
> The Landlock tests [patch 9/10] rely on it for now.
> 
> I sent it three weeks ago:
> https://lkml.kernel.org/r/[e-mail]
> 
> Anyway, until this patch is merged in the kselftest tree and then
> available to net-next, I'll have to keep it here.

Shuah,
could you please pick up this patch into your tree?

Shuah Aug. 28, 2017, 6:01 p.m. UTC | #4

On 08/25/2017 07:07 PM, Alexei Starovoitov wrote:
> On Fri, Aug 25, 2017 at 09:58:33AM +0200, Mickaël Salaün wrote:
>>
>>
>> On 24/08/2017 04:31, Alexei Starovoitov wrote:
>>> On Mon, Aug 21, 2017 at 02:09:24AM +0200, Mickaël Salaün wrote:
>>>> This step mechanism may be useful to return an information about the
>>>> error without being able to write to TH_LOG_STREAM.
>>>>
>>>> Set _metadata->no_print to true to print this counter.
>>>>
>>>> Signed-off-by: Mickaël Salaün <[e-mail]>
>>>> Cc: Andy Lutomirski <[e-mail]>
>>>> Cc: Arnaldo Carvalho de Melo <[e-mail]>
>>>> Cc: Kees Cook <[e-mail]>
>>>> Cc: Shuah Khan <[e-mail]>
>>>> Cc: Will Drewry <[e-mail]>
>>>> Link: https://lkml.kernel.org/r/[e-mail]
>>>> ---
>>>>
>>>> This patch is intended to the kselftest tree:
>>>> https://lkml.kernel.org/r/[e-mail]
>>>>
>>>> Changes since v6:
>>>> * add the step counter in assert/expect macros and use _metadata to
>>>>   enable the counter (suggested by Kees Cook)
>>>> ---
>>>>  tools/testing/selftests/kselftest_harness.h   | 31 ++++++++++++++++++++++-----
>>>>  tools/testing/selftests/seccomp/seccomp_bpf.c |  2 +-
>>>>  2 files changed, 27 insertions(+), 6 deletions(-)
>>>
>>> is there a dependency on this in patches 2+ ?
>>> if not, I would send this patch to selftests right away.
>>>
>>>
>>
>> The Landlock tests [patch 9/10] rely on it for now.
>>
>> I sent it three weeks ago:
>> https://lkml.kernel.org/r/[e-mail]
>>
>> Anyway, until this patch is merged in the kselftest tree and then
>> available to net-next, I'll have to keep it here.
> 
> Shuah,
> could you please pick up this patch into your tree?
> 

Thanks. Applied to linux-kselftest next for 4.14-rc1.

-- Shuah

Patch

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index c56f72e07cd7..850ff6946027 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -51,6 +51,9 @@ 
 #define __KSELFTEST_HARNESS_H
 
 #define _GNU_SOURCE
+#include <asm/types.h>
+#include <errno.h>
+#include <stdbool.h>
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -555,12 +558,18 @@ 
  * return while still providing an optional block to the API consumer.
  */
 #define OPTIONAL_HANDLER(_assert) \
-	for (; _metadata->trigger;  _metadata->trigger = __bail(_assert))
+	for (; _metadata->trigger; _metadata->trigger = \
+			__bail(_assert, _metadata->no_print, _metadata->step))
+
+#define __INC_STEP(_metadata) \
+	if (_metadata->passed && _metadata->step < 255) \
+		_metadata->step++;
 
 #define __EXPECT(_expected, _seen, _t, _assert) do { \
 	/* Avoid multiple evaluation of the cases */ \
 	__typeof__(_expected) __exp = (_expected); \
 	__typeof__(_seen) __seen = (_seen); \
+	__INC_STEP(_metadata); \
 	if (!(__exp _t __seen)) { \
 		unsigned long long __exp_print = (uintptr_t)__exp; \
 		unsigned long long __seen_print = (uintptr_t)__seen; \
@@ -576,6 +585,7 @@ 
 #define __EXPECT_STR(_expected, _seen, _t, _assert) do { \
 	const char *__exp = (_expected); \
 	const char *__seen = (_seen); \
+	__INC_STEP(_metadata); \
 	if (!(strcmp(__exp, __seen) _t 0))  { \
 		__TH_LOG("Expected '%s' %s '%s'.", __exp, #_t, __seen); \
 		_metadata->passed = 0; \
@@ -590,6 +600,8 @@  struct __test_metadata {
 	int termsig;
 	int passed;
 	int trigger; /* extra handler after the evaluation */
+	__u8 step;
+	bool no_print; /* manual trigger when TH_LOG_STREAM is not available */
 	struct __test_metadata *prev, *next;
 };
 
@@ -634,10 +646,13 @@  static inline void __register_test(struct __test_metadata *t)
 	}
 }
 
-static inline int __bail(int for_realz)
+static inline int __bail(int for_realz, bool no_print, __u8 step)
 {
-	if (for_realz)
+	if (for_realz) {
+		if (no_print)
+			_exit(step);
 		abort();
+	}
 	return 0;
 }
 
@@ -655,18 +670,24 @@  void __run_test(struct __test_metadata *t)
 		t->passed = 0;
 	} else if (child_pid == 0) {
 		t->fn(t);
-		_exit(t->passed);
+		/* return the step that failed or 0 */
+		_exit(t->passed ? 0 : t->step);
 	} else {
 		/* TODO(wad) add timeout support. */
 		waitpid(child_pid, &status, 0);
 		if (WIFEXITED(status)) {
-			t->passed = t->termsig == -1 ? WEXITSTATUS(status) : 0;
+			t->passed = t->termsig == -1 ? !WEXITSTATUS(status) : 0;
 			if (t->termsig != -1) {
 				fprintf(TH_LOG_STREAM,
 					"%s: Test exited normally "
 					"instead of by signal (code: %d)\n",
 					t->name,
 					WEXITSTATUS(status));
+			} else if (!t->passed) {
+				fprintf(TH_LOG_STREAM,
+					"%s: Test failed at step #%d\n",
+					t->name,
+					WEXITSTATUS(status));
 			}
 		} else if (WIFSIGNALED(status)) {
 			t->passed = 0;
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 73f5ea6778ce..4d6f92a9df6b 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -107,7 +107,7 @@  TEST(mode_strict_support)
 	ASSERT_EQ(0, ret) {
 		TH_LOG("Kernel does not support CONFIG_SECCOMP");
 	}
-	syscall(__NR_exit, 1);
+	syscall(__NR_exit, 0);
 }
 
 TEST_SIGNAL(mode_strict_cannot_call_prctl, SIGKILL)