| 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: <CABVgOSk_vea-LrPwJet6hQ4D3PBQOLVg32nZ_gE4c9kgGDEEnQ@mail.gmail.com>
Date: Tue, 12 Mar 2024 12:54:48 +0800
From: David Gow <davidgow@...gle.com>
To: Mickaël Salaün <mic@...ikod.net>
Cc: Brendan Higgins <brendanhiggins@...gle.com>, Kees Cook <keescook@...omium.org>,
Rae Moar <rmoar@...gle.com>, Shuah Khan <skhan@...uxfoundation.org>,
Alan Maguire <alan.maguire@...cle.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>, "H . Peter Anvin" <hpa@...or.com>,
Ingo Molnar <mingo@...hat.com>, James Morris <jamorris@...ux.microsoft.com>,
Luis Chamberlain <mcgrof@...nel.org>,
"Madhavan T . Venkataraman" <madvenka@...ux.microsoft.com>, Marco Pagani <marpagan@...hat.com>,
Paolo Bonzini <pbonzini@...hat.com>, Sean Christopherson <seanjc@...gle.com>, Stephen Boyd <sboyd@...nel.org>,
Thara Gopinath <tgopinath@...rosoft.com>, Thomas Gleixner <tglx@...utronix.de>,
Vitaly Kuznetsov <vkuznets@...hat.com>, Wanpeng Li <wanpengli@...cent.com>,
Zahra Tarkhani <ztarkhani@...rosoft.com>, kvm@...r.kernel.org,
linux-hardening@...r.kernel.org, linux-hyperv@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org,
linux-um@...ts.infradead.org, x86@...nel.org
Subject: Re: [PATCH v2 6/7] kunit: Print last test location on fault
On Sat, 2 Mar 2024 at 03:40, Mickaël Salaün <mic@...ikod.net> wrote:
>
> This helps identify the location of test faults.
>
> Cc: Brendan Higgins <brendanhiggins@...gle.com>
> Cc: David Gow <davidgow@...gle.com>
> Cc: Rae Moar <rmoar@...gle.com>
> Cc: Shuah Khan <skhan@...uxfoundation.org>
> Reviewed-by: Kees Cook <keescook@...omium.org>
> Signed-off-by: Mickaël Salaün <mic@...ikod.net>
> Link: https://lore.kernel.org/r/20240301194037.532117-7-mic@digikod.net
> ---
I like the idea of this, but am a little bit worried about how
confusing it might be, given that the location only updates on those
particular macros.
Maybe the answer is to make the __KUNIT_SAVE_LOC() macro, or something
equivalent, a supported API.
One possibility would be to have a KUNIT_MARKER() macro. If we really
wanted to, we could expand it to take a string so we can have a more
user-friendly KUNIT_MARKER(test, "parsing packet") description of
where things went wrong. Another could be to extend this to use the
code tagging framework[1], if that lands.
That being said, I think this is still an improvement without any of
those features. I've left a few comments below. Let me know what you
think.
Cheers,
-- David
[1]: https://lwn.net/Articles/906660/
>
> Changes since v1:
> * Added Kees's Reviewed-by.
> ---
> include/kunit/test.h | 24 +++++++++++++++++++++---
> lib/kunit/try-catch.c | 10 +++++++---
> 2 files changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index fcb4a4940ace..f3aa66eb0087 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -301,6 +301,8 @@ struct kunit {
> struct list_head resources; /* Protected by lock. */
>
> char status_comment[KUNIT_STATUS_COMMENT_SIZE];
> + /* Saves the last seen test. Useful to help with faults. */
> + struct kunit_loc last_seen;
> };
>
> static inline void kunit_set_failure(struct kunit *test)
> @@ -567,6 +569,15 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt,
> #define kunit_err(test, fmt, ...) \
> kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
>
> +/*
> + * Must be called at the beginning of each KUNIT_*_ASSERTION().
> + * Cf. KUNIT_CURRENT_LOC.
> + */
> +#define _KUNIT_SAVE_LOC(test) do { \
> + WRITE_ONCE(test->last_seen.file, __FILE__); \
> + WRITE_ONCE(test->last_seen.line, __LINE__); \
> +} while (0)
Can we get rid of the leading '_', make this public, and document it?
If we want to rename it to KUNIT_MARKER() or similar, that might work
better, too.
> +
> /**
> * KUNIT_SUCCEED() - A no-op expectation. Only exists for code clarity.
> * @test: The test context object.
> @@ -575,7 +586,7 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt,
> * words, it does nothing and only exists for code clarity. See
> * KUNIT_EXPECT_TRUE() for more information.
> */
> -#define KUNIT_SUCCEED(test) do {} while (0)
> +#define KUNIT_SUCCEED(test) _KUNIT_SAVE_LOC(test)
>
> void __noreturn __kunit_abort(struct kunit *test);
>
> @@ -601,14 +612,16 @@ void __kunit_do_failed_assertion(struct kunit *test,
> } while (0)
>
>
> -#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) \
> +#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) do { \
> + _KUNIT_SAVE_LOC(test); \
> _KUNIT_FAILED(test, \
> assert_type, \
> kunit_fail_assert, \
> kunit_fail_assert_format, \
> {}, \
> fmt, \
> - ##__VA_ARGS__)
> + ##__VA_ARGS__); \
> +} while (0)
>
> /**
> * KUNIT_FAIL() - Always causes a test to fail when evaluated.
> @@ -637,6 +650,7 @@ void __kunit_do_failed_assertion(struct kunit *test,
> fmt, \
> ...) \
> do { \
> + _KUNIT_SAVE_LOC(test); \
> if (likely(!!(condition_) == !!expected_true_)) \
> break; \
> \
> @@ -698,6 +712,7 @@ do { \
> .right_text = #right, \
> }; \
> \
> + _KUNIT_SAVE_LOC(test); \
> if (likely(__left op __right)) \
> break; \
> \
> @@ -758,6 +773,7 @@ do { \
> .right_text = #right, \
> }; \
> \
> + _KUNIT_SAVE_LOC(test); \
> if (likely((__left) && (__right) && (strcmp(__left, __right) op 0))) \
> break; \
> \
> @@ -791,6 +807,7 @@ do { \
> .right_text = #right, \
> }; \
> \
> + _KUNIT_SAVE_LOC(test); \
> if (likely(__left && __right)) \
> if (likely(memcmp(__left, __right, __size) op 0)) \
> break; \
> @@ -815,6 +832,7 @@ do { \
> do { \
> const typeof(ptr) __ptr = (ptr); \
> \
> + _KUNIT_SAVE_LOC(test); \
> if (!IS_ERR_OR_NULL(__ptr)) \
> break; \
> \
> diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> index c6ee4db0b3bd..2ec21c6918f3 100644
> --- a/lib/kunit/try-catch.c
> +++ b/lib/kunit/try-catch.c
> @@ -91,9 +91,13 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context)
>
> if (exit_code == -EFAULT)
> try_catch->try_result = 0;
> - else if (exit_code == -EINTR)
> - kunit_err(test, "try faulted\n");
> - else if (exit_code == -ETIMEDOUT)
> + else if (exit_code == -EINTR) {
> + if (test->last_seen.file)
> + kunit_err(test, "try faulted after %s:%d\n",
> + test->last_seen.file, test->last_seen.line);
It's possibly a bit confusing to just say "after file:line",
particularly if we then loop or call a function "higher up" in the
file. Maybe something like "try faulted: last line seen %s:%d" is
clearer.
> + else
> + kunit_err(test, "try faulted before the first test\n");
I don't like using "test" here, as it introduces ambiguity between
"kunit tests" and "assertions/expectations" if we call them both
tests. Maybe just "try faulted" here, or "try faulted (no markers
seen)" or similar?
> + } else if (exit_code == -ETIMEDOUT)
> kunit_err(test, "try timed out\n");
> else if (exit_code)
> kunit_err(test, "Unknown error: %d\n", exit_code);
> --
> 2.44.0
>
Download attachment "smime.p7s" of type "application/pkcs7-signature" (4014 bytes)
Powered by blists - more mailing lists