| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9a3ff6a1-de73-08fc-49ad-b68f3b1776b8@nvidia.com>
Date: Mon, 13 Jul 2020 17:50:34 -0700
From: Ralph Campbell <rcampbell@...dia.com>
To: Kees Cook <keescook@...omium.org>
CC: Shuah Khan <shuah@...nel.org>,
Christian Brauner <christian@...uner.io>,
David Gow <davidgow@...gle.com>,
Frank Rowand <frowand.list@...il.com>,
Paolo Bonzini <pbonzini@...hat.com>,
"Bird, Tim" <Tim.Bird@...y.com>,
Brendan Higgins <brendanhiggins@...gle.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Andy Lutomirski <luto@...capital.net>,
Will Drewry <wad@...omium.org>,
<linux-kselftest@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 6/8] selftests/harness: Refactor XFAIL into SKIP
On 7/13/20 5:13 PM, Kees Cook wrote:
> On Mon, Jul 13, 2020 at 12:08:08PM -0700, Ralph Campbell wrote:
>>
>> On 6/22/20 11:16 AM, Kees Cook wrote:
>>> Plumb the old XFAIL result into a TAP SKIP.
>>>
>>> Signed-off-by: Kees Cook <keescook@...omium.org>
>>> ---
>>> tools/testing/selftests/kselftest_harness.h | 64 ++++++++++++++-----
>>> tools/testing/selftests/seccomp/seccomp_bpf.c | 8 +--
>>> 2 files changed, 52 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
>>> index f8f7e47c739a..b519765904a6 100644
>>> --- a/tools/testing/selftests/kselftest_harness.h
>>> +++ b/tools/testing/selftests/kselftest_harness.h
>>> @@ -112,22 +112,22 @@
>>> __FILE__, __LINE__, _metadata->name, ##__VA_ARGS__)
>>> /**
>>> - * XFAIL(statement, fmt, ...)
>>> + * SKIP(statement, fmt, ...)
>>> *
>>> - * @statement: statement to run after reporting XFAIL
>>> + * @statement: statement to run after reporting SKIP
>>> * @fmt: format string
>>> * @...: optional arguments
>>> *
>>> - * This forces a "pass" after reporting a failure with an XFAIL prefix,
>>> + * This forces a "pass" after reporting why something is being skipped
>>> * and runs "statement", which is usually "return" or "goto skip".
>>> */
>>> -#define XFAIL(statement, fmt, ...) do { \
>>> +#define SKIP(statement, fmt, ...) do { \
>>> if (TH_LOG_ENABLED) { \
>>> - fprintf(TH_LOG_STREAM, "# XFAIL " fmt "\n", \
>>> + fprintf(TH_LOG_STREAM, "# SKIP " fmt "\n", \
>>> ##__VA_ARGS__); \
>>> } \
>>> - /* TODO: find a way to pass xfail to test runner process. */ \
>>> _metadata->passed = 1; \
>>> + _metadata->skip = 1; \
>>> _metadata->trigger = 0; \
>>> statement; \
>>> } while (0)
>>> @@ -777,6 +777,7 @@ struct __test_metadata {
>>> struct __fixture_metadata *fixture;
>>> int termsig;
>>> int passed;
>>> + int skip; /* did SKIP get used? */
>>> int trigger; /* extra handler after the evaluation */
>>> int timeout; /* seconds to wait for test timeout */
>>> bool timed_out; /* did this test timeout instead of exiting? */
>>> @@ -866,17 +867,31 @@ void __wait_for_test(struct __test_metadata *t)
>>> fprintf(TH_LOG_STREAM,
>>> "# %s: Test terminated by timeout\n", t->name);
>>> } else if (WIFEXITED(status)) {
>>> - t->passed = t->termsig == -1 ? !WEXITSTATUS(status) : 0;
>>> if (t->termsig != -1) {
>>> + t->passed = 0;
>>> 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 {
>>> + switch (WEXITSTATUS(status)) {
>>> + /* Success */
>>> + case 0:
>>> + t->passed = 1;
>>> + break;
>>> + /* SKIP */
>>> + case 255:
>>> + t->passed = 1;
>>> + t->skip = 1;
>>> + break;
>>> + /* Other failure, assume step report. */
>>> + default:
>>> + t->passed = 0;
>>> + fprintf(TH_LOG_STREAM,
>>> + "# %s: Test failed at step #%d\n",
>>> + t->name,
>>> + WEXITSTATUS(status));
>>> + }
>>> }
>>> } else if (WIFSIGNALED(status)) {
>>> t->passed = 0;
>>> @@ -906,6 +921,7 @@ void __run_test(struct __fixture_metadata *f,
>>> {
>>> /* reset test struct */
>>> t->passed = 1;
>>> + t->skip = 0;
>>> t->trigger = 0;
>>> t->step = 0;
>>> t->no_print = 0;
>>> @@ -918,15 +934,31 @@ void __run_test(struct __fixture_metadata *f,
>>> t->passed = 0;
>>> } else if (t->pid == 0) {
>>> t->fn(t, variant);
>>> - /* return the step that failed or 0 */
>>> - _exit(t->passed ? 0 : t->step);
>>> + /* Make sure step doesn't get lost in reporting */
>>> + if (t->step >= 255) {
>>> + ksft_print_msg("Too many test steps (%u)!?\n", t->step);
>>> + t->step = 254;
>>> + }
>>
>> I noticed that this message is now appearing in the HMM self tests.
>> I haven't quite tracked down why ->steps should be 255 after running
>> the first test. I did notice that ASSERT*() calls __INC_STEP() but
>> that doesn't explain it.
>> Separately, maybe __INC_STEP() should check for < 254 instead of < 255?
>>
>> Set CONFIG_HMM_TESTS=m, build and install kernel and modules.
>> cd tools/testing/selftests/vm
>> make
>> ./test_hmm.sh smoke
>> Running smoke test. Note, this test provides basic coverage.
>> [ 106.803476] memmap_init_zone_device initialised 65536 pages in 7ms
>> [ 106.810141] added new 256 MB chunk (total 1 chunks, 256 MB) PFNs [0x3ffff0000 0x400000000)
>> [ 106.823703] memmap_init_zone_device initialised 65536 pages in 4ms
>> [ 106.829968] added new 256 MB chunk (total 1 chunks, 256 MB) PFNs [0x3fffe0000 0x3ffff0000)
>> [ 106.838655] HMM test module loaded. This is only for testing HMM.
>> TAP version 13
>> 1..20
>> # Starting 20 tests from 3 test cases.
>> # RUN hmm.open_close ...
>> # OK hmm.open_close
>> ok 1 hmm.open_close
>> # RUN hmm.anon_read ...
>> # Too many test steps (255)!?
>> # OK hmm.anon_read
>
> Oooh:
>
> #define NTIMES 256
>
> Yes, that's a lot of steps. :)
>
> I agree,__ INC_STEP() needs adjustment, though it should be 253. Does
> this work for you?
>
> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index 935029d4fb21..4f78e4805633 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -680,7 +680,8 @@
> __bail(_assert, _metadata->no_print, _metadata->step))
>
> #define __INC_STEP(_metadata) \
> - if (_metadata->passed && _metadata->step < 255) \
> + /* Keep "step" below 255 (which is used for "SKIP" reporting). */ \
> + if (_metadata->passed && _metadata->step < 253) \
> _metadata->step++;
>
> #define is_signed_type(var) (!!(((__typeof__(var))(-1)) < (__typeof__(var))1))
> @@ -976,12 +977,6 @@ void __run_test(struct __fixture_metadata *f,
> t->passed = 0;
> } else if (t->pid == 0) {
> t->fn(t, variant);
> - /* Make sure step doesn't get lost in reporting */
> - if (t->step >= 255) {
> - ksft_print_msg("Too many test steps (%u)!?\n", t->step);
> - t->step = 254;
> - }
> - /* Use 255 for SKIP */
> if (t->skip)
> _exit(255);
> /* Pass is exit 0 */
>
Yes, this works for me.
Thanks!
Powered by blists - more mailing lists