linux-kernel - Re: [PATCH] kunit: kunit_skip() should not overwrite KUNIT

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CABVgOSnLq2=2dZ1424Z1jpws-boAwOCf=mmpbLvP1nnPvcYj_w@mail.gmail.com>
Date:   Sat, 14 Jan 2023 13:51:45 +0800
From:   David Gow <davidgow@...gle.com>
To:     Daniel Latypov <dlatypov@...gle.com>
Cc:     brendanhiggins@...gle.com, rmoar@...gle.com,
        linux-kernel@...r.kernel.org, kunit-dev@...glegroups.com,
        linux-kselftest@...r.kernel.org, skhan@...uxfoundation.org
Subject: Re: [PATCH] kunit: kunit_skip() should not overwrite KUNIT_FAIL()

On Sat, 14 Jan 2023 at 06:07, 'Daniel Latypov' via KUnit Development
<kunit-dev@...glegroups.com> wrote:
>
> Currently, kunit_skip() and kunit_mark_skipped() will overwrite the
> current test's status even if it was already marked FAILED.
>
> E.g. a test that just contains this
>  KUNIT_FAIL(test, "FAIL REASON");
>  kunit_skip(test, "SKIP REASON");
> will be marked "SKIPPED" in the end.
>
> Now, tests like the above don't and shouldn't exist.
> But what happens if non-test code (e.g. KASAN) calls kunit_fail_current_test()?
>
> E.g. if we have
>   if (do_some_invalid_memory_accesses())
>     kunit_skip(");
> then the KASAN failures will get masked!
>
> This patch: make it so kunit_mark_skipped() does not modify the status
> if it's already set to something (either already to SKIPPED or FAILURE).
>
> Before this change, the KTAP output would look like
>     # example_simple_test: EXPECTATION FAILED at lib/kunit/kunit-example-test.c:23
> FAIL REASON
>     ok 1 example_simple_test # SKIP SKIP REASON
>
> After this change:
>     # example_simple_test: EXPECTATION FAILED at lib/kunit/kunit-example-test.c:23
> FAIL REASON
>     # example_simple_test: status already changed, not marking skipped: SKIP REASON
>     not ok 1 example_simple_test
>
> Signed-off-by: Daniel Latypov <dlatypov@...gle.com>
> ---

Thanks very much: this makes much more sense than the old behaviour.

My only suggestion is that we add a test to verify this behaviour to
the kunit_status suite, such as:
diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
index 4df0335d0d06..fa114785b01e 100644
--- a/lib/kunit/kunit-test.c
+++ b/lib/kunit/kunit-test.c
@@ -510,9 +510,33 @@ static void kunit_status_mark_skipped_test(struct
kunit *test)
       KUNIT_EXPECT_STREQ(test, fake.status_comment, "Accepts format
string: YES");
}

+static void kunit_status_skip_after_fail_test(struct kunit *test)
+{
+       struct kunit fake;
+
+       kunit_init_test(&fake, "fake test", NULL);
+
+       /* Test starts off SUCCESS. */
+       KUNIT_EXPECT_EQ(test, fake.status, KUNIT_SUCCESS);
+
+       /* Fail the test. */
+       kunit_set_failure(&fake);
+       KUNIT_EXPECT_EQ(test, fake.status, KUNIT_FAILURE);
+
+       /* Now mark it as skipped. */
+       kunit_mark_skipped(&fake, "Skip message");
+
+       /* The test has still failed. */
+       KUNIT_EXPECT_EQ(test, fake.status, KUNIT_FAILURE);
+
+       /* We shouldn't use the skip reason as a status comment. */
+       KUNIT_EXPECT_STREQ(test, fake.status_comment, "");
+}
+
static struct kunit_case kunit_status_test_cases[] = {
       KUNIT_CASE(kunit_status_set_failure_test),
       KUNIT_CASE(kunit_status_mark_skipped_test),
+       KUNIT_CASE(kunit_status_skip_after_fail_test),
       {}
};


--

Otherwise, this looks great!

Reviewed-by: David Gow <davidgow@...gle.com>

Cheers,
-- David
>  include/kunit/test.h | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 87ea90576b50..39936463dde5 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -386,11 +386,18 @@ void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...);
>   *
>   * Marks the test as skipped. @fmt is given output as the test status
>   * comment, typically the reason the test was skipped.
> + * This has no effect if the test has already been marked skipped or failed.
>   *
>   * Test execution continues after kunit_mark_skipped() is called.
>   */
>  #define kunit_mark_skipped(test_or_suite, fmt, ...)                    \
>         do {                                                            \
> +               if (READ_ONCE((test_or_suite)->status) != KUNIT_SUCCESS) {\
> +                       kunit_warn(test_or_suite, "status already "     \
> +                                  "changed, not marking skipped: " fmt,\
> +                                  ##__VA_ARGS__);                      \
> +                       break;                                          \
> +               }                                                       \
>                 WRITE_ONCE((test_or_suite)->status, KUNIT_SKIPPED);     \
>                 scnprintf((test_or_suite)->status_comment,              \
>                           KUNIT_STATUS_COMMENT_SIZE,                    \
>
> base-commit: 7dd4b804e08041ff56c88bdd8da742d14b17ed25
> --
> 2.39.0.314.g84b9a713c41-goog
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@...glegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230113220718.2901010-1-dlatypov%40google.com.

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4003 bytes)