* [PATCH v3 0/2] kunit: fail tests on UBSAN errors
@ 2021-02-09 22:14 Daniel Latypov
2021-02-09 22:14 ` [PATCH v3 1/2] kunit: support failure from dynamic analysis tools Daniel Latypov
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Daniel Latypov @ 2021-02-09 22:14 UTC (permalink / raw)
To: brendanhiggins
Cc: davidgow, alan.maguire, linux-kernel, kunit-dev, linux-kselftest,
skhan, Daniel Latypov
v1 by Uriel is here: [1].
Since it's been a while, I've dropped the Reviewed-By's.
It depended on commit 83c4e7a0363b ("KUnit: KASAN Integration") which
hadn't been merged yet, so that caused some kerfuffle with applying them
previously and the series was reverted.
This revives the series but makes the kunit_fail_current_test() function
take a format string and logs the file and line number of the failing
code, addressing Alan Maguire's comments on the previous version.
As a result, the patch that makes UBSAN errors was tweaked slightly to
include an error message.
v2 -> v3:
Fix kunit_fail_current_test() so it works w/ CONFIG_KUNIT=m
s/_/__ on the helper func to match others in test.c
[1] https://lore.kernel.org/linux-kselftest/20200806174326.3577537-1-urielguajardojr@gmail.com/
Uriel Guajardo (2):
kunit: support failure from dynamic analysis tools
kunit: ubsan integration
include/kunit/test-bug.h | 30 ++++++++++++++++++++++++++++++
lib/kunit/test.c | 37 +++++++++++++++++++++++++++++++++----
lib/ubsan.c | 3 +++
3 files changed, 66 insertions(+), 4 deletions(-)
create mode 100644 include/kunit/test-bug.h
base-commit: 1e0d27fce010b0a4a9e595506b6ede75934c31be
--
2.30.0.478.g8a0d178c01-goog
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v3 1/2] kunit: support failure from dynamic analysis tools 2021-02-09 22:14 [PATCH v3 0/2] kunit: fail tests on UBSAN errors Daniel Latypov @ 2021-02-09 22:14 ` Daniel Latypov 2021-02-11 8:54 ` David Gow 2021-02-09 22:14 ` [PATCH v3 2/2] kunit: ubsan integration Daniel Latypov 2021-02-10 10:34 ` [PATCH v3 0/2] kunit: fail tests on UBSAN errors Alan Maguire 2 siblings, 1 reply; 13+ messages in thread From: Daniel Latypov @ 2021-02-09 22:14 UTC (permalink / raw) To: brendanhiggins Cc: davidgow, alan.maguire, linux-kernel, kunit-dev, linux-kselftest, skhan, Uriel Guajardo, Daniel Latypov From: Uriel Guajardo <urielguajardo@google.com> Add a kunit_fail_current_test() function to fail the currently running test, if any, with an error message. This is largely intended for dynamic analysis tools like UBSAN and for fakes. E.g. say I had a fake ops struct for testing and I wanted my `free` function to complain if it was called with an invalid argument, or caught a double-free. Most return void and have no normal means of signalling failure (e.g. super_operations, iommu_ops, etc.). Key points: * Always update current->kunit_test so anyone can use it. * commit 83c4e7a0363b ("KUnit: KASAN Integration") only updated it for CONFIG_KASAN=y * Create a new header <kunit/test-bug.h> so non-test code doesn't have to include all of <kunit/test.h> (e.g. lib/ubsan.c) * Forward the file and line number to make it easier to track down failures * Declare the helper function for nice __printf() warnings about mismatched format strings even when KUnit is not enabled. Example output from kunit_fail_current_test("message"): [15:19:34] [FAILED] example_simple_test [15:19:34] # example_simple_test: initializing [15:19:34] # example_simple_test: lib/kunit/kunit-example-test.c:24: message [15:19:34] not ok 1 - example_simple_test Co-developed-by: Daniel Latypov <dlatypov@google.com> Signed-off-by: Uriel Guajardo <urielguajardo@google.com> Signed-off-by: Daniel Latypov <dlatypov@google.com> --- include/kunit/test-bug.h | 30 ++++++++++++++++++++++++++++++ lib/kunit/test.c | 37 +++++++++++++++++++++++++++++++++---- 2 files changed, 63 insertions(+), 4 deletions(-) create mode 100644 include/kunit/test-bug.h diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h new file mode 100644 index 000000000000..18b1034ec43a --- /dev/null +++ b/include/kunit/test-bug.h @@ -0,0 +1,30 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * KUnit API allowing dynamic analysis tools to interact with KUnit tests + * + * Copyright (C) 2020, Google LLC. + * Author: Uriel Guajardo <urielguajardo@google.com> + */ + +#ifndef _KUNIT_TEST_BUG_H +#define _KUNIT_TEST_BUG_H + +#define kunit_fail_current_test(fmt, ...) \ + __kunit_fail_current_test(__FILE__, __LINE__, fmt, ##__VA_ARGS__) + +#if IS_ENABLED(CONFIG_KUNIT) + +extern __printf(3, 4) void __kunit_fail_current_test(const char *file, int line, + const char *fmt, ...); + +#else + +static __printf(3, 4) void __kunit_fail_current_test(const char *file, int line, + const char *fmt, ...) +{ +} + +#endif + + +#endif /* _KUNIT_TEST_BUG_H */ diff --git a/lib/kunit/test.c b/lib/kunit/test.c index ec9494e914ef..5794059505cf 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -7,6 +7,7 @@ */ #include <kunit/test.h> +#include <kunit/test-bug.h> #include <linux/kernel.h> #include <linux/kref.h> #include <linux/sched/debug.h> @@ -16,6 +17,38 @@ #include "string-stream.h" #include "try-catch-impl.h" +/* + * Fail the current test and print an error message to the log. + */ +void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...) +{ + va_list args; + int len; + char *buffer; + + if (!current->kunit_test) + return; + + kunit_set_failure(current->kunit_test); + + /* kunit_err() only accepts literals, so evaluate the args first. */ + va_start(args, fmt); + len = vsnprintf(NULL, 0, fmt, args) + 1; + va_end(args); + + buffer = kunit_kmalloc(current->kunit_test, len, GFP_KERNEL); + if (!buffer) + return; + + va_start(args, fmt); + vsnprintf(buffer, len, fmt, args); + va_end(args); + + kunit_err(current->kunit_test, "%s:%d: %s", file, line, buffer); + kunit_kfree(current->kunit_test, buffer); +} +EXPORT_SYMBOL_GPL(__kunit_fail_current_test); + /* * Append formatted message to log, size of which is limited to * KUNIT_LOG_SIZE bytes (including null terminating byte). @@ -273,9 +306,7 @@ static void kunit_try_run_case(void *data) struct kunit_suite *suite = ctx->suite; struct kunit_case *test_case = ctx->test_case; -#if (IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT)) current->kunit_test = test; -#endif /* IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT) */ /* * kunit_run_case_internal may encounter a fatal error; if it does, @@ -624,9 +655,7 @@ void kunit_cleanup(struct kunit *test) spin_unlock(&test->lock); kunit_remove_resource(test, res); } -#if (IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT)) current->kunit_test = NULL; -#endif /* IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT)*/ } EXPORT_SYMBOL_GPL(kunit_cleanup); -- 2.30.0.478.g8a0d178c01-goog ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] kunit: support failure from dynamic analysis tools 2021-02-09 22:14 ` [PATCH v3 1/2] kunit: support failure from dynamic analysis tools Daniel Latypov @ 2021-02-11 8:54 ` David Gow 2021-02-11 15:40 ` Alan Maguire 0 siblings, 1 reply; 13+ messages in thread From: David Gow @ 2021-02-11 8:54 UTC (permalink / raw) To: Daniel Latypov Cc: Brendan Higgins, Alan Maguire, Linux Kernel Mailing List, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan, Uriel Guajardo On Wed, Feb 10, 2021 at 6:14 AM Daniel Latypov <dlatypov@google.com> wrote: > > From: Uriel Guajardo <urielguajardo@google.com> > > Add a kunit_fail_current_test() function to fail the currently running > test, if any, with an error message. > > This is largely intended for dynamic analysis tools like UBSAN and for > fakes. > E.g. say I had a fake ops struct for testing and I wanted my `free` > function to complain if it was called with an invalid argument, or > caught a double-free. Most return void and have no normal means of > signalling failure (e.g. super_operations, iommu_ops, etc.). > > Key points: > * Always update current->kunit_test so anyone can use it. > * commit 83c4e7a0363b ("KUnit: KASAN Integration") only updated it for > CONFIG_KASAN=y > > * Create a new header <kunit/test-bug.h> so non-test code doesn't have > to include all of <kunit/test.h> (e.g. lib/ubsan.c) > > * Forward the file and line number to make it easier to track down > failures > > * Declare the helper function for nice __printf() warnings about mismatched > format strings even when KUnit is not enabled. > > Example output from kunit_fail_current_test("message"): > [15:19:34] [FAILED] example_simple_test > [15:19:34] # example_simple_test: initializing > [15:19:34] # example_simple_test: lib/kunit/kunit-example-test.c:24: message > [15:19:34] not ok 1 - example_simple_test > > Co-developed-by: Daniel Latypov <dlatypov@google.com> > Signed-off-by: Uriel Guajardo <urielguajardo@google.com> > Signed-off-by: Daniel Latypov <dlatypov@google.com> > --- > include/kunit/test-bug.h | 30 ++++++++++++++++++++++++++++++ > lib/kunit/test.c | 37 +++++++++++++++++++++++++++++++++---- > 2 files changed, 63 insertions(+), 4 deletions(-) > create mode 100644 include/kunit/test-bug.h > > diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h > new file mode 100644 > index 000000000000..18b1034ec43a > --- /dev/null > +++ b/include/kunit/test-bug.h > @@ -0,0 +1,30 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * KUnit API allowing dynamic analysis tools to interact with KUnit tests > + * > + * Copyright (C) 2020, Google LLC. > + * Author: Uriel Guajardo <urielguajardo@google.com> > + */ > + > +#ifndef _KUNIT_TEST_BUG_H > +#define _KUNIT_TEST_BUG_H > + > +#define kunit_fail_current_test(fmt, ...) \ > + __kunit_fail_current_test(__FILE__, __LINE__, fmt, ##__VA_ARGS__) > + > +#if IS_ENABLED(CONFIG_KUNIT) As the kernel test robot has pointed out on the second patch, this probably should be IS_BUILTIN(), otherwise this won't build if KUnit is a module, and the code calling it isn't. This does mean that things like UBSAN integration won't work if KUnit is a module, which is a shame. (It's worth noting that the KASAN integration worked around this by only calling inline functions, which would therefore be built-in even if the rest of KUnit was built as a module. I don't think it's quite as convenient to do that here, though.) > + > +extern __printf(3, 4) void __kunit_fail_current_test(const char *file, int line, > + const char *fmt, ...); > + > +#else > + > +static __printf(3, 4) void __kunit_fail_current_test(const char *file, int line, > + const char *fmt, ...) > +{ > +} > + > +#endif > + > + > +#endif /* _KUNIT_TEST_BUG_H */ > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index ec9494e914ef..5794059505cf 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -7,6 +7,7 @@ > */ > > #include <kunit/test.h> > +#include <kunit/test-bug.h> > #include <linux/kernel.h> > #include <linux/kref.h> > #include <linux/sched/debug.h> > @@ -16,6 +17,38 @@ > #include "string-stream.h" > #include "try-catch-impl.h" > > +/* > + * Fail the current test and print an error message to the log. > + */ > +void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...) > +{ > + va_list args; > + int len; > + char *buffer; > + > + if (!current->kunit_test) > + return; > + > + kunit_set_failure(current->kunit_test); > + > + /* kunit_err() only accepts literals, so evaluate the args first. */ > + va_start(args, fmt); > + len = vsnprintf(NULL, 0, fmt, args) + 1; > + va_end(args); > + > + buffer = kunit_kmalloc(current->kunit_test, len, GFP_KERNEL); > + if (!buffer) > + return; > + > + va_start(args, fmt); > + vsnprintf(buffer, len, fmt, args); > + va_end(args); > + > + kunit_err(current->kunit_test, "%s:%d: %s", file, line, buffer); > + kunit_kfree(current->kunit_test, buffer); > +} > +EXPORT_SYMBOL_GPL(__kunit_fail_current_test); > + > /* > * Append formatted message to log, size of which is limited to > * KUNIT_LOG_SIZE bytes (including null terminating byte). > @@ -273,9 +306,7 @@ static void kunit_try_run_case(void *data) > struct kunit_suite *suite = ctx->suite; > struct kunit_case *test_case = ctx->test_case; > > -#if (IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT)) > current->kunit_test = test; > -#endif /* IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT) */ > > /* > * kunit_run_case_internal may encounter a fatal error; if it does, > @@ -624,9 +655,7 @@ void kunit_cleanup(struct kunit *test) > spin_unlock(&test->lock); > kunit_remove_resource(test, res); > } > -#if (IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT)) > current->kunit_test = NULL; > -#endif /* IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT)*/ > } > EXPORT_SYMBOL_GPL(kunit_cleanup); > > -- > 2.30.0.478.g8a0d178c01-goog > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] kunit: support failure from dynamic analysis tools 2021-02-11 8:54 ` David Gow @ 2021-02-11 15:40 ` Alan Maguire 2021-02-11 20:58 ` Daniel Latypov 0 siblings, 1 reply; 13+ messages in thread From: Alan Maguire @ 2021-02-11 15:40 UTC (permalink / raw) To: David Gow Cc: Daniel Latypov, Brendan Higgins, Alan Maguire, Linux Kernel Mailing List, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan, Uriel Guajardo On Thu, 11 Feb 2021, David Gow wrote: > On Wed, Feb 10, 2021 at 6:14 AM Daniel Latypov <dlatypov@google.com> wrote: > > > > From: Uriel Guajardo <urielguajardo@google.com> > > > > Add a kunit_fail_current_test() function to fail the currently running > > test, if any, with an error message. > > > > This is largely intended for dynamic analysis tools like UBSAN and for > > fakes. > > E.g. say I had a fake ops struct for testing and I wanted my `free` > > function to complain if it was called with an invalid argument, or > > caught a double-free. Most return void and have no normal means of > > signalling failure (e.g. super_operations, iommu_ops, etc.). > > > > Key points: > > * Always update current->kunit_test so anyone can use it. > > * commit 83c4e7a0363b ("KUnit: KASAN Integration") only updated it for > > CONFIG_KASAN=y > > > > * Create a new header <kunit/test-bug.h> so non-test code doesn't have > > to include all of <kunit/test.h> (e.g. lib/ubsan.c) > > > > * Forward the file and line number to make it easier to track down > > failures > > > > * Declare the helper function for nice __printf() warnings about mismatched > > format strings even when KUnit is not enabled. > > > > Example output from kunit_fail_current_test("message"): > > [15:19:34] [FAILED] example_simple_test > > [15:19:34] # example_simple_test: initializing > > [15:19:34] # example_simple_test: lib/kunit/kunit-example-test.c:24: message > > [15:19:34] not ok 1 - example_simple_test > > > > Co-developed-by: Daniel Latypov <dlatypov@google.com> > > Signed-off-by: Uriel Guajardo <urielguajardo@google.com> > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > > --- > > include/kunit/test-bug.h | 30 ++++++++++++++++++++++++++++++ > > lib/kunit/test.c | 37 +++++++++++++++++++++++++++++++++---- > > 2 files changed, 63 insertions(+), 4 deletions(-) > > create mode 100644 include/kunit/test-bug.h > > > > diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h > > new file mode 100644 > > index 000000000000..18b1034ec43a > > --- /dev/null > > +++ b/include/kunit/test-bug.h > > @@ -0,0 +1,30 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * KUnit API allowing dynamic analysis tools to interact with KUnit tests > > + * > > + * Copyright (C) 2020, Google LLC. > > + * Author: Uriel Guajardo <urielguajardo@google.com> > > + */ > > + > > +#ifndef _KUNIT_TEST_BUG_H > > +#define _KUNIT_TEST_BUG_H > > + > > +#define kunit_fail_current_test(fmt, ...) \ > > + __kunit_fail_current_test(__FILE__, __LINE__, fmt, ##__VA_ARGS__) > > + > > +#if IS_ENABLED(CONFIG_KUNIT) > > As the kernel test robot has pointed out on the second patch, this > probably should be IS_BUILTIN(), otherwise this won't build if KUnit > is a module, and the code calling it isn't. > > This does mean that things like UBSAN integration won't work if KUnit > is a module, which is a shame. > > (It's worth noting that the KASAN integration worked around this by > only calling inline functions, which would therefore be built-in even > if the rest of KUnit was built as a module. I don't think it's quite > as convenient to do that here, though.) > Right, static inline'ing __kunit_fail_current_test() seems problematic because it calls other exported functions; more below.... > > + > > +extern __printf(3, 4) void __kunit_fail_current_test(const char *file, int line, > > + const char *fmt, ...); > > + > > +#else > > + > > +static __printf(3, 4) void __kunit_fail_current_test(const char *file, int line, > > + const char *fmt, ...) > > +{ > > +} > > + > > +#endif > > + > > + > > +#endif /* _KUNIT_TEST_BUG_H */ > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > > index ec9494e914ef..5794059505cf 100644 > > --- a/lib/kunit/test.c > > +++ b/lib/kunit/test.c > > @@ -7,6 +7,7 @@ > > */ > > > > #include <kunit/test.h> > > +#include <kunit/test-bug.h> > > #include <linux/kernel.h> > > #include <linux/kref.h> > > #include <linux/sched/debug.h> > > @@ -16,6 +17,38 @@ > > #include "string-stream.h" > > #include "try-catch-impl.h" > > > > +/* > > + * Fail the current test and print an error message to the log. > > + */ > > +void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...) > > +{ > > + va_list args; > > + int len; > > + char *buffer; > > + > > + if (!current->kunit_test) > > + return; > > + > > + kunit_set_failure(current->kunit_test); > > + currently kunit_set_failure() is static, but it could be inlined I suspect. > > + /* kunit_err() only accepts literals, so evaluate the args first. */ > > + va_start(args, fmt); > > + len = vsnprintf(NULL, 0, fmt, args) + 1; > > + va_end(args); > > + > > + buffer = kunit_kmalloc(current->kunit_test, len, GFP_KERNEL); kunit_kmalloc()/kunit_kfree() are exported also, but we could probably dodge allocation with a static buffer. In fact since we end up using an on-stack buffer for logging in kunit_log_append(), it might make sense to #define __kunit_fail_current_test() instead, i.e. #define __kunit_fail_current_test(file, line, fmt, ...) \ do { \ kunit_set_failure(current->kunit_test); \ kunit_err(current->kunit_test, "%s:%d: " fmt, \ ##__VA_ARGS__); \ } while (0) > > + if (!buffer) > > + return; > > + > > + va_start(args, fmt); > > + vsnprintf(buffer, len, fmt, args); > > + va_end(args); > > + > > + kunit_err(current->kunit_test, "%s:%d: %s", file, line, buffer); To get kunit_err() to work, we'd need to "static inline" kunit_log_append(). It's not a trivial function, but on the plus side it doesn't call any other exported kunit functions AFAICT. So while any of the above suggestions aren't intended to block Daniel's work, does the above seem reasonable for a follow-up series to get UBSAN working with module-based KUnit? Thanks! Alan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] kunit: support failure from dynamic analysis tools 2021-02-11 15:40 ` Alan Maguire @ 2021-02-11 20:58 ` Daniel Latypov 2021-02-11 21:33 ` Brendan Higgins 0 siblings, 1 reply; 13+ messages in thread From: Daniel Latypov @ 2021-02-11 20:58 UTC (permalink / raw) To: Alan Maguire Cc: David Gow, Brendan Higgins, Linux Kernel Mailing List, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan, Uriel Guajardo On Thu, Feb 11, 2021 at 7:40 AM Alan Maguire <alan.maguire@oracle.com> wrote: > > On Thu, 11 Feb 2021, David Gow wrote: > > > On Wed, Feb 10, 2021 at 6:14 AM Daniel Latypov <dlatypov@google.com> wrote: > > > > > > From: Uriel Guajardo <urielguajardo@google.com> > > > > > > Add a kunit_fail_current_test() function to fail the currently running > > > test, if any, with an error message. > > > > > > This is largely intended for dynamic analysis tools like UBSAN and for > > > fakes. > > > E.g. say I had a fake ops struct for testing and I wanted my `free` > > > function to complain if it was called with an invalid argument, or > > > caught a double-free. Most return void and have no normal means of > > > signalling failure (e.g. super_operations, iommu_ops, etc.). > > > > > > Key points: > > > * Always update current->kunit_test so anyone can use it. > > > * commit 83c4e7a0363b ("KUnit: KASAN Integration") only updated it for > > > CONFIG_KASAN=y > > > > > > * Create a new header <kunit/test-bug.h> so non-test code doesn't have > > > to include all of <kunit/test.h> (e.g. lib/ubsan.c) > > > > > > * Forward the file and line number to make it easier to track down > > > failures > > > > > > * Declare the helper function for nice __printf() warnings about mismatched > > > format strings even when KUnit is not enabled. > > > > > > Example output from kunit_fail_current_test("message"): > > > [15:19:34] [FAILED] example_simple_test > > > [15:19:34] # example_simple_test: initializing > > > [15:19:34] # example_simple_test: lib/kunit/kunit-example-test.c:24: message > > > [15:19:34] not ok 1 - example_simple_test > > > > > > Co-developed-by: Daniel Latypov <dlatypov@google.com> > > > Signed-off-by: Uriel Guajardo <urielguajardo@google.com> > > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > > > --- > > > include/kunit/test-bug.h | 30 ++++++++++++++++++++++++++++++ > > > lib/kunit/test.c | 37 +++++++++++++++++++++++++++++++++---- > > > 2 files changed, 63 insertions(+), 4 deletions(-) > > > create mode 100644 include/kunit/test-bug.h > > > > > > diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h > > > new file mode 100644 > > > index 000000000000..18b1034ec43a > > > --- /dev/null > > > +++ b/include/kunit/test-bug.h > > > @@ -0,0 +1,30 @@ > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > +/* > > > + * KUnit API allowing dynamic analysis tools to interact with KUnit tests > > > + * > > > + * Copyright (C) 2020, Google LLC. > > > + * Author: Uriel Guajardo <urielguajardo@google.com> > > > + */ > > > + > > > +#ifndef _KUNIT_TEST_BUG_H > > > +#define _KUNIT_TEST_BUG_H > > > + > > > +#define kunit_fail_current_test(fmt, ...) \ > > > + __kunit_fail_current_test(__FILE__, __LINE__, fmt, ##__VA_ARGS__) > > > + > > > +#if IS_ENABLED(CONFIG_KUNIT) > > > > As the kernel test robot has pointed out on the second patch, this > > probably should be IS_BUILTIN(), otherwise this won't build if KUnit > > is a module, and the code calling it isn't. > > > > This does mean that things like UBSAN integration won't work if KUnit > > is a module, which is a shame. > > > > (It's worth noting that the KASAN integration worked around this by > > only calling inline functions, which would therefore be built-in even > > if the rest of KUnit was built as a module. I don't think it's quite > > as convenient to do that here, though.) > > > > Right, static inline'ing __kunit_fail_current_test() seems problematic > because it calls other exported functions; more below.... > > > > + > > > +extern __printf(3, 4) void __kunit_fail_current_test(const char *file, int line, > > > + const char *fmt, ...); > > > + > > > +#else > > > + > > > +static __printf(3, 4) void __kunit_fail_current_test(const char *file, int line, > > > + const char *fmt, ...) > > > +{ > > > +} > > > + > > > +#endif > > > + > > > + > > > +#endif /* _KUNIT_TEST_BUG_H */ > > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > > > index ec9494e914ef..5794059505cf 100644 > > > --- a/lib/kunit/test.c > > > +++ b/lib/kunit/test.c > > > @@ -7,6 +7,7 @@ > > > */ > > > > > > #include <kunit/test.h> > > > +#include <kunit/test-bug.h> > > > #include <linux/kernel.h> > > > #include <linux/kref.h> > > > #include <linux/sched/debug.h> > > > @@ -16,6 +17,38 @@ > > > #include "string-stream.h" > > > #include "try-catch-impl.h" > > > > > > +/* > > > + * Fail the current test and print an error message to the log. > > > + */ > > > +void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...) > > > +{ > > > + va_list args; > > > + int len; > > > + char *buffer; > > > + > > > + if (!current->kunit_test) > > > + return; > > > + > > > + kunit_set_failure(current->kunit_test); > > > + > > currently kunit_set_failure() is static, but it could be inlined I > suspect. > > > > + /* kunit_err() only accepts literals, so evaluate the args first. */ > > > + va_start(args, fmt); > > > + len = vsnprintf(NULL, 0, fmt, args) + 1; > > > + va_end(args); > > > + > > > + buffer = kunit_kmalloc(current->kunit_test, len, GFP_KERNEL); > > kunit_kmalloc()/kunit_kfree() are exported also, but we could probably > dodge allocation with a static buffer. In fact since we end up > using an on-stack buffer for logging in kunit_log_append(), it might make Ah, right there's those as well. I originally had it on the stack, but the fact we use an on-stack buffer is why I switched over. I originally had it as a macro as you do now but liked the __printf() annotation to be closer* to the user's code and now down through several layers of macros (kunit_fail_current_test => kunit_err => kunit_printk => kunit_log => printk). And then having it on the stack and then calling into kunit_log_append() would (naively) use up 2 *KUNIT_LOG_SIZE stack space. So only a minor concern, and so I like the simpler def using a macro given the messiness. (But we'd give up the __printf checking when compiling w/o KUnit, which is a bit sad) *E.g. if one misuses kunit_err(), we get this message which is understandable, but a bit more noisy than I'd prefer. ../include/linux/kern_levels.h:5:18: warning: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘char *’ [-Wformat=] 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */ | ^~~~~~ ../include/kunit/test.h:621:10: note: in definition of macro ‘kunit_log’ 621 | printk(lvl fmt, ##__VA_ARGS__); \ | ^~~ ../include/kunit/test.h:662:2: note: in expansion of macro ‘kunit_printk’ 662 | kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__) | ^~~~~~~~~~~~ ../include/linux/kern_levels.h:11:18: note: in expansion of macro ‘KERN_SOH’ 11 | #define KERN_ERR KERN_SOH "3" /* error conditions */ | ^~~~~~~~ ../include/kunit/test.h:662:15: note: in expansion of macro ‘KERN_ERR’ 662 | kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__) | ^~~~~~~~ ../lib/kunit/kunit-example-test.c:30:2: note: in expansion of macro ‘kunit_err’ 30 | kunit_err(test, "invalid format string: %d", "hi"); | ^~~~~~~~~ > sense to #define __kunit_fail_current_test() instead, i.e. > > #define __kunit_fail_current_test(file, line, fmt, ...) \ > do { \ > kunit_set_failure(current->kunit_test); \ > kunit_err(current->kunit_test, "%s:%d: " fmt, \ > ##__VA_ARGS__); \ > } while (0) > > > > + if (!buffer) > > > + return; > > > + > > > + va_start(args, fmt); > > > + vsnprintf(buffer, len, fmt, args); > > > + va_end(args); > > > + > > > + kunit_err(current->kunit_test, "%s:%d: %s", file, line, buffer); > > To get kunit_err() to work, we'd need to "static inline" > kunit_log_append(). It's not a trivial function, but on the plus > side it doesn't call any other exported kunit functions AFAICT. > > So while any of the above suggestions aren't intended to block > Daniel's work, does the above seem reasonable for a follow-up > series to get UBSAN working with module-based KUnit? Thanks! Ack, so sounds like we'd want to go ahead with making it only work w/ CONFIG_KUNIT=y? I can simplify it down into a macro since the __printf() bit isn't too big of a deal. And then it'd let us only depend on kunit_log_append(), making it easier to get CONFIG_KUNIT=m working. Thanks both for digging into this! I saw KTR's email and was dreading having to dig into what the smallest needed change would be. > > Alan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] kunit: support failure from dynamic analysis tools 2021-02-11 20:58 ` Daniel Latypov @ 2021-02-11 21:33 ` Brendan Higgins 2021-02-17 0:23 ` Daniel Latypov 0 siblings, 1 reply; 13+ messages in thread From: Brendan Higgins @ 2021-02-11 21:33 UTC (permalink / raw) To: Daniel Latypov Cc: Alan Maguire, David Gow, Linux Kernel Mailing List, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan, Uriel Guajardo On Thu, Feb 11, 2021 at 12:58 PM Daniel Latypov <dlatypov@google.com> wrote: > > On Thu, Feb 11, 2021 at 7:40 AM Alan Maguire <alan.maguire@oracle.com> wrote: > > > > On Thu, 11 Feb 2021, David Gow wrote: > > > > > On Wed, Feb 10, 2021 at 6:14 AM Daniel Latypov <dlatypov@google.com> wrote: > > > > > > > > From: Uriel Guajardo <urielguajardo@google.com> > > > > > > > > Add a kunit_fail_current_test() function to fail the currently running > > > > test, if any, with an error message. > > > > > > > > This is largely intended for dynamic analysis tools like UBSAN and for > > > > fakes. > > > > E.g. say I had a fake ops struct for testing and I wanted my `free` > > > > function to complain if it was called with an invalid argument, or > > > > caught a double-free. Most return void and have no normal means of > > > > signalling failure (e.g. super_operations, iommu_ops, etc.). > > > > > > > > Key points: > > > > * Always update current->kunit_test so anyone can use it. > > > > * commit 83c4e7a0363b ("KUnit: KASAN Integration") only updated it for > > > > CONFIG_KASAN=y > > > > > > > > * Create a new header <kunit/test-bug.h> so non-test code doesn't have > > > > to include all of <kunit/test.h> (e.g. lib/ubsan.c) > > > > > > > > * Forward the file and line number to make it easier to track down > > > > failures > > > > > > > > * Declare the helper function for nice __printf() warnings about mismatched > > > > format strings even when KUnit is not enabled. > > > > > > > > Example output from kunit_fail_current_test("message"): > > > > [15:19:34] [FAILED] example_simple_test > > > > [15:19:34] # example_simple_test: initializing > > > > [15:19:34] # example_simple_test: lib/kunit/kunit-example-test.c:24: message > > > > [15:19:34] not ok 1 - example_simple_test > > > > > > > > Co-developed-by: Daniel Latypov <dlatypov@google.com> > > > > Signed-off-by: Uriel Guajardo <urielguajardo@google.com> > > > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > > > > --- > > > > include/kunit/test-bug.h | 30 ++++++++++++++++++++++++++++++ > > > > lib/kunit/test.c | 37 +++++++++++++++++++++++++++++++++---- > > > > 2 files changed, 63 insertions(+), 4 deletions(-) > > > > create mode 100644 include/kunit/test-bug.h > > > > > > > > diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h > > > > new file mode 100644 > > > > index 000000000000..18b1034ec43a > > > > --- /dev/null > > > > +++ b/include/kunit/test-bug.h > > > > @@ -0,0 +1,30 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > > +/* > > > > + * KUnit API allowing dynamic analysis tools to interact with KUnit tests > > > > + * > > > > + * Copyright (C) 2020, Google LLC. > > > > + * Author: Uriel Guajardo <urielguajardo@google.com> > > > > + */ > > > > + > > > > +#ifndef _KUNIT_TEST_BUG_H > > > > +#define _KUNIT_TEST_BUG_H > > > > + > > > > +#define kunit_fail_current_test(fmt, ...) \ > > > > + __kunit_fail_current_test(__FILE__, __LINE__, fmt, ##__VA_ARGS__) > > > > + > > > > +#if IS_ENABLED(CONFIG_KUNIT) > > > > > > As the kernel test robot has pointed out on the second patch, this > > > probably should be IS_BUILTIN(), otherwise this won't build if KUnit > > > is a module, and the code calling it isn't. > > > > > > This does mean that things like UBSAN integration won't work if KUnit > > > is a module, which is a shame. > > > > > > (It's worth noting that the KASAN integration worked around this by > > > only calling inline functions, which would therefore be built-in even > > > if the rest of KUnit was built as a module. I don't think it's quite > > > as convenient to do that here, though.) > > > > > > > Right, static inline'ing __kunit_fail_current_test() seems problematic > > because it calls other exported functions; more below.... > > > > > > + > > > > +extern __printf(3, 4) void __kunit_fail_current_test(const char *file, int line, > > > > + const char *fmt, ...); > > > > + > > > > +#else > > > > + > > > > +static __printf(3, 4) void __kunit_fail_current_test(const char *file, int line, > > > > + const char *fmt, ...) > > > > +{ > > > > +} > > > > + > > > > +#endif > > > > + > > > > + > > > > +#endif /* _KUNIT_TEST_BUG_H */ > > > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > > > > index ec9494e914ef..5794059505cf 100644 > > > > --- a/lib/kunit/test.c > > > > +++ b/lib/kunit/test.c > > > > @@ -7,6 +7,7 @@ > > > > */ > > > > > > > > #include <kunit/test.h> > > > > +#include <kunit/test-bug.h> > > > > #include <linux/kernel.h> > > > > #include <linux/kref.h> > > > > #include <linux/sched/debug.h> > > > > @@ -16,6 +17,38 @@ > > > > #include "string-stream.h" > > > > #include "try-catch-impl.h" > > > > > > > > +/* > > > > + * Fail the current test and print an error message to the log. > > > > + */ > > > > +void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...) > > > > +{ > > > > + va_list args; > > > > + int len; > > > > + char *buffer; > > > > + > > > > + if (!current->kunit_test) > > > > + return; > > > > + > > > > + kunit_set_failure(current->kunit_test); > > > > + > > > > currently kunit_set_failure() is static, but it could be inlined I > > suspect. > > > > > > + /* kunit_err() only accepts literals, so evaluate the args first. */ > > > > + va_start(args, fmt); > > > > + len = vsnprintf(NULL, 0, fmt, args) + 1; > > > > + va_end(args); > > > > + > > > > + buffer = kunit_kmalloc(current->kunit_test, len, GFP_KERNEL); > > > > kunit_kmalloc()/kunit_kfree() are exported also, but we could probably > > dodge allocation with a static buffer. In fact since we end up > > using an on-stack buffer for logging in kunit_log_append(), it might make > > Ah, right there's those as well. > > I originally had it on the stack, but the fact we use an on-stack > buffer is why I switched over. > > I originally had it as a macro as you do now but liked the __printf() > annotation to be closer* to the user's code and now down through > several layers of macros (kunit_fail_current_test => kunit_err => > kunit_printk => kunit_log => printk). > And then having it on the stack and then calling into > kunit_log_append() would (naively) use up 2 *KUNIT_LOG_SIZE stack > space. > > So only a minor concern, and so I like the simpler def using a macro > given the messiness. > (But we'd give up the __printf checking when compiling w/o KUnit, > which is a bit sad) > > *E.g. if one misuses kunit_err(), we get this message which is > understandable, but a bit more noisy than I'd prefer. > ../include/linux/kern_levels.h:5:18: warning: format ‘%d’ expects > argument of type ‘int’, but argument 3 has type ‘char *’ [-Wformat=] > 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */ > | ^~~~~~ > ../include/kunit/test.h:621:10: note: in definition of macro ‘kunit_log’ > 621 | printk(lvl fmt, ##__VA_ARGS__); \ > | ^~~ > ../include/kunit/test.h:662:2: note: in expansion of macro ‘kunit_printk’ > 662 | kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__) > | ^~~~~~~~~~~~ > ../include/linux/kern_levels.h:11:18: note: in expansion of macro ‘KERN_SOH’ > 11 | #define KERN_ERR KERN_SOH "3" /* error conditions */ > | ^~~~~~~~ > ../include/kunit/test.h:662:15: note: in expansion of macro ‘KERN_ERR’ > 662 | kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__) > | ^~~~~~~~ > ../lib/kunit/kunit-example-test.c:30:2: note: in expansion of macro ‘kunit_err’ > 30 | kunit_err(test, "invalid format string: %d", "hi"); > | ^~~~~~~~~ > > > > sense to #define __kunit_fail_current_test() instead, i.e. > > > > #define __kunit_fail_current_test(file, line, fmt, ...) \ > > do { \ > > kunit_set_failure(current->kunit_test); \ > > kunit_err(current->kunit_test, "%s:%d: " fmt, \ > > ##__VA_ARGS__); \ > > } while (0) > > > > > > + if (!buffer) > > > > + return; > > > > + > > > > + va_start(args, fmt); > > > > + vsnprintf(buffer, len, fmt, args); > > > > + va_end(args); > > > > + > > > > + kunit_err(current->kunit_test, "%s:%d: %s", file, line, buffer); > > > > To get kunit_err() to work, we'd need to "static inline" > > kunit_log_append(). It's not a trivial function, but on the plus > > side it doesn't call any other exported kunit functions AFAICT. > > > > So while any of the above suggestions aren't intended to block > > Daniel's work, does the above seem reasonable for a follow-up > > series to get UBSAN working with module-based KUnit? Thanks! > > Ack, so sounds like we'd want to go ahead with making it only work w/ > CONFIG_KUNIT=y? I also think this is probably only useful when KUnit is built in. Although it would be certainly neat to be able to support utilizing `current->kunit_test` after the fact when KUnit is built as a module, the problem is that `kunit_test` is only a member of the task_struct when KUnit is enabled anyway: https://elixir.bootlin.com/linux/v5.10.15/source/include/linux/sched.h#L1234 I think that making `kunit_test` always present in task_struct is undesirable for many users, and so you then have to be sure that your kernel you want to load the KUnit module into was built with CONFIG_KUNIT=m. Overall, I think it is safer and easier to just require KUnit to be built in if you want to use `current->kunit_test`. In any case, that does not take away the ability to use KUnit test modules with it, just not KUnit itself as a module (CONFIG_KUNIT=y is compatible with tests built as modules). > I can simplify it down into a macro since the __printf() bit isn't too > big of a deal. > And then it'd let us only depend on kunit_log_append(), making it > easier to get CONFIG_KUNIT=m working. > > Thanks both for digging into this! > I saw KTR's email and was dreading having to dig into what the > smallest needed change would be. > > > > > Alan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] kunit: support failure from dynamic analysis tools 2021-02-11 21:33 ` Brendan Higgins @ 2021-02-17 0:23 ` Daniel Latypov 0 siblings, 0 replies; 13+ messages in thread From: Daniel Latypov @ 2021-02-17 0:23 UTC (permalink / raw) To: Brendan Higgins Cc: Alan Maguire, David Gow, Linux Kernel Mailing List, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan On Thu, Feb 11, 2021 at 1:33 PM 'Brendan Higgins' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > On Thu, Feb 11, 2021 at 12:58 PM Daniel Latypov <dlatypov@google.com> wrote: > > > > On Thu, Feb 11, 2021 at 7:40 AM Alan Maguire <alan.maguire@oracle.com> wrote: > > > > > > On Thu, 11 Feb 2021, David Gow wrote: > > > > > > > On Wed, Feb 10, 2021 at 6:14 AM Daniel Latypov <dlatypov@google.com> wrote: > > > > > > > > > > From: Uriel Guajardo <urielguajardo@google.com> > > > > > > > > > > Add a kunit_fail_current_test() function to fail the currently running > > > > > test, if any, with an error message. > > > > > > > > > > This is largely intended for dynamic analysis tools like UBSAN and for > > > > > fakes. > > > > > E.g. say I had a fake ops struct for testing and I wanted my `free` > > > > > function to complain if it was called with an invalid argument, or > > > > > caught a double-free. Most return void and have no normal means of > > > > > signalling failure (e.g. super_operations, iommu_ops, etc.). > > > > > > > > > > Key points: > > > > > * Always update current->kunit_test so anyone can use it. > > > > > * commit 83c4e7a0363b ("KUnit: KASAN Integration") only updated it for > > > > > CONFIG_KASAN=y > > > > > > > > > > * Create a new header <kunit/test-bug.h> so non-test code doesn't have > > > > > to include all of <kunit/test.h> (e.g. lib/ubsan.c) > > > > > > > > > > * Forward the file and line number to make it easier to track down > > > > > failures > > > > > > > > > > * Declare the helper function for nice __printf() warnings about mismatched > > > > > format strings even when KUnit is not enabled. > > > > > > > > > > Example output from kunit_fail_current_test("message"): > > > > > [15:19:34] [FAILED] example_simple_test > > > > > [15:19:34] # example_simple_test: initializing > > > > > [15:19:34] # example_simple_test: lib/kunit/kunit-example-test.c:24: message > > > > > [15:19:34] not ok 1 - example_simple_test > > > > > > > > > > Co-developed-by: Daniel Latypov <dlatypov@google.com> > > > > > Signed-off-by: Uriel Guajardo <urielguajardo@google.com> > > > > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > > > > > --- > > > > > include/kunit/test-bug.h | 30 ++++++++++++++++++++++++++++++ > > > > > lib/kunit/test.c | 37 +++++++++++++++++++++++++++++++++---- > > > > > 2 files changed, 63 insertions(+), 4 deletions(-) > > > > > create mode 100644 include/kunit/test-bug.h > > > > > > > > > > diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h > > > > > new file mode 100644 > > > > > index 000000000000..18b1034ec43a > > > > > --- /dev/null > > > > > +++ b/include/kunit/test-bug.h > > > > > @@ -0,0 +1,30 @@ > > > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > > > +/* > > > > > + * KUnit API allowing dynamic analysis tools to interact with KUnit tests > > > > > + * > > > > > + * Copyright (C) 2020, Google LLC. > > > > > + * Author: Uriel Guajardo <urielguajardo@google.com> > > > > > + */ > > > > > + > > > > > +#ifndef _KUNIT_TEST_BUG_H > > > > > +#define _KUNIT_TEST_BUG_H > > > > > + > > > > > +#define kunit_fail_current_test(fmt, ...) \ > > > > > + __kunit_fail_current_test(__FILE__, __LINE__, fmt, ##__VA_ARGS__) > > > > > + > > > > > +#if IS_ENABLED(CONFIG_KUNIT) > > > > > > > > As the kernel test robot has pointed out on the second patch, this > > > > probably should be IS_BUILTIN(), otherwise this won't build if KUnit > > > > is a module, and the code calling it isn't. > > > > > > > > This does mean that things like UBSAN integration won't work if KUnit > > > > is a module, which is a shame. > > > > > > > > (It's worth noting that the KASAN integration worked around this by > > > > only calling inline functions, which would therefore be built-in even > > > > if the rest of KUnit was built as a module. I don't think it's quite > > > > as convenient to do that here, though.) > > > > > > > > > > Right, static inline'ing __kunit_fail_current_test() seems problematic > > > because it calls other exported functions; more below.... > > > > > > > > + > > > > > +extern __printf(3, 4) void __kunit_fail_current_test(const char *file, int line, > > > > > + const char *fmt, ...); > > > > > + > > > > > +#else > > > > > + > > > > > +static __printf(3, 4) void __kunit_fail_current_test(const char *file, int line, > > > > > + const char *fmt, ...) > > > > > +{ > > > > > +} > > > > > + > > > > > +#endif > > > > > + > > > > > + > > > > > +#endif /* _KUNIT_TEST_BUG_H */ > > > > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > > > > > index ec9494e914ef..5794059505cf 100644 > > > > > --- a/lib/kunit/test.c > > > > > +++ b/lib/kunit/test.c > > > > > @@ -7,6 +7,7 @@ > > > > > */ > > > > > > > > > > #include <kunit/test.h> > > > > > +#include <kunit/test-bug.h> > > > > > #include <linux/kernel.h> > > > > > #include <linux/kref.h> > > > > > #include <linux/sched/debug.h> > > > > > @@ -16,6 +17,38 @@ > > > > > #include "string-stream.h" > > > > > #include "try-catch-impl.h" > > > > > > > > > > +/* > > > > > + * Fail the current test and print an error message to the log. > > > > > + */ > > > > > +void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...) > > > > > +{ > > > > > + va_list args; > > > > > + int len; > > > > > + char *buffer; > > > > > + > > > > > + if (!current->kunit_test) > > > > > + return; > > > > > + > > > > > + kunit_set_failure(current->kunit_test); > > > > > + > > > > > > currently kunit_set_failure() is static, but it could be inlined I > > > suspect. > > > > > > > > + /* kunit_err() only accepts literals, so evaluate the args first. */ > > > > > + va_start(args, fmt); > > > > > + len = vsnprintf(NULL, 0, fmt, args) + 1; > > > > > + va_end(args); > > > > > + > > > > > + buffer = kunit_kmalloc(current->kunit_test, len, GFP_KERNEL); > > > > > > kunit_kmalloc()/kunit_kfree() are exported also, but we could probably > > > dodge allocation with a static buffer. In fact since we end up > > > using an on-stack buffer for logging in kunit_log_append(), it might make > > > > Ah, right there's those as well. > > > > I originally had it on the stack, but the fact we use an on-stack > > buffer is why I switched over. > > > > I originally had it as a macro as you do now but liked the __printf() > > annotation to be closer* to the user's code and now down through > > several layers of macros (kunit_fail_current_test => kunit_err => > > kunit_printk => kunit_log => printk). > > And then having it on the stack and then calling into > > kunit_log_append() would (naively) use up 2 *KUNIT_LOG_SIZE stack > > space. > > > > So only a minor concern, and so I like the simpler def using a macro > > given the messiness. > > (But we'd give up the __printf checking when compiling w/o KUnit, > > which is a bit sad) > > > > *E.g. if one misuses kunit_err(), we get this message which is > > understandable, but a bit more noisy than I'd prefer. > > ../include/linux/kern_levels.h:5:18: warning: format ‘%d’ expects > > argument of type ‘int’, but argument 3 has type ‘char *’ [-Wformat=] > > 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */ > > | ^~~~~~ > > ../include/kunit/test.h:621:10: note: in definition of macro ‘kunit_log’ > > 621 | printk(lvl fmt, ##__VA_ARGS__); \ > > | ^~~ > > ../include/kunit/test.h:662:2: note: in expansion of macro ‘kunit_printk’ > > 662 | kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__) > > | ^~~~~~~~~~~~ > > ../include/linux/kern_levels.h:11:18: note: in expansion of macro ‘KERN_SOH’ > > 11 | #define KERN_ERR KERN_SOH "3" /* error conditions */ > > | ^~~~~~~~ > > ../include/kunit/test.h:662:15: note: in expansion of macro ‘KERN_ERR’ > > 662 | kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__) > > | ^~~~~~~~ > > ../lib/kunit/kunit-example-test.c:30:2: note: in expansion of macro ‘kunit_err’ > > 30 | kunit_err(test, "invalid format string: %d", "hi"); > > | ^~~~~~~~~ > > > > > > > sense to #define __kunit_fail_current_test() instead, i.e. > > > > > > #define __kunit_fail_current_test(file, line, fmt, ...) \ > > > do { \ > > > kunit_set_failure(current->kunit_test); \ > > > kunit_err(current->kunit_test, "%s:%d: " fmt, \ > > > ##__VA_ARGS__); \ > > > } while (0) > > > > > > > > + if (!buffer) > > > > > + return; > > > > > + > > > > > + va_start(args, fmt); > > > > > + vsnprintf(buffer, len, fmt, args); > > > > > + va_end(args); > > > > > + > > > > > + kunit_err(current->kunit_test, "%s:%d: %s", file, line, buffer); > > > > > > To get kunit_err() to work, we'd need to "static inline" > > > kunit_log_append(). It's not a trivial function, but on the plus > > > side it doesn't call any other exported kunit functions AFAICT. > > > > > > So while any of the above suggestions aren't intended to block > > > Daniel's work, does the above seem reasonable for a follow-up > > > series to get UBSAN working with module-based KUnit? Thanks! > > > > Ack, so sounds like we'd want to go ahead with making it only work w/ > > CONFIG_KUNIT=y? > > I also think this is probably only useful when KUnit is built in. > Although it would be certainly neat to be able to support utilizing > `current->kunit_test` after the fact when KUnit is built as a module, > the problem is that `kunit_test` is only a member of the task_struct > when KUnit is enabled anyway: > > https://elixir.bootlin.com/linux/v5.10.15/source/include/linux/sched.h#L1234 > > I think that making `kunit_test` always present in task_struct is > undesirable for many users, and so you then have to be sure that your > kernel you want to load the KUnit module into was built with > CONFIG_KUNIT=m. > > Overall, I think it is safer and easier to just require KUnit to be > built in if you want to use `current->kunit_test`. In any case, that > does not take away the ability to use KUnit test modules with it, just > not KUnit itself as a module (CONFIG_KUNIT=y is compatible with tests > built as modules). Good point, will change to IS_BUILTIN(), in that case. > > > I can simplify it down into a macro since the __printf() bit isn't too > > big of a deal. > > And then it'd let us only depend on kunit_log_append(), making it > > easier to get CONFIG_KUNIT=m working. Ugh, actually, I'll have to walk back making it a macro for now... The goal had been that <kunit/test-bug.h> wouldn't pull in kunit code. We can forward declare kunit_set_failure(), but not kunit_err(), which is a macro. So to make a static inline or macro version of kunit_fail_current_test() requires pulling in <kunit/test.h> > > > > Thanks both for digging into this! > > I saw KTR's email and was dreading having to dig into what the > > smallest needed change would be. > > > > > > > > Alan > > -- > 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@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/CAFd5g45kPdrKz-UnMagx1JdcRmLK0uG31m5OELvJKe%3DpTaND%2Bw%40mail.gmail.com. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 2/2] kunit: ubsan integration 2021-02-09 22:14 [PATCH v3 0/2] kunit: fail tests on UBSAN errors Daniel Latypov 2021-02-09 22:14 ` [PATCH v3 1/2] kunit: support failure from dynamic analysis tools Daniel Latypov @ 2021-02-09 22:14 ` Daniel Latypov 2021-02-10 22:00 ` kernel test robot 2021-02-11 7:15 ` kernel test robot 2021-02-10 10:34 ` [PATCH v3 0/2] kunit: fail tests on UBSAN errors Alan Maguire 2 siblings, 2 replies; 13+ messages in thread From: Daniel Latypov @ 2021-02-09 22:14 UTC (permalink / raw) To: brendanhiggins Cc: davidgow, alan.maguire, linux-kernel, kunit-dev, linux-kselftest, skhan, Uriel Guajardo, Daniel Latypov From: Uriel Guajardo <urielguajardo@google.com> Integrates UBSAN into the KUnit testing framework. It fails KUnit tests whenever it reports undefined behavior. When CONFIG_KUNIT=n, nothing is printed or even formatted, so this has no behavioral impact outside of tests. kunit_fail_current_test() effectively does a pr_err() as well, so there's some slight duplication, but it also ensures an error is recorded in the debugfs entry for the running KUnit test. Print a shorter version of the message to make it less spammy. Co-developed-by: Daniel Latypov <dlatypov@google.com> Signed-off-by: Uriel Guajardo <urielguajardo@google.com> Signed-off-by: Daniel Latypov <dlatypov@google.com> --- lib/ubsan.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/ubsan.c b/lib/ubsan.c index bec38c64d6a6..1ec7d6f1fe63 100644 --- a/lib/ubsan.c +++ b/lib/ubsan.c @@ -14,6 +14,7 @@ #include <linux/types.h> #include <linux/sched.h> #include <linux/uaccess.h> +#include <kunit/test-bug.h> #include "ubsan.h" @@ -141,6 +142,8 @@ static void ubsan_prologue(struct source_location *loc, const char *reason) "========================================\n"); pr_err("UBSAN: %s in %s:%d:%d\n", reason, loc->file_name, loc->line & LINE_MASK, loc->column & COLUMN_MASK); + + kunit_fail_current_test("%s in %s", reason, loc->file_name); } static void ubsan_epilogue(void) -- 2.30.0.478.g8a0d178c01-goog ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] kunit: ubsan integration 2021-02-09 22:14 ` [PATCH v3 2/2] kunit: ubsan integration Daniel Latypov @ 2021-02-10 22:00 ` kernel test robot 2021-02-11 7:15 ` kernel test robot 1 sibling, 0 replies; 13+ messages in thread From: kernel test robot @ 2021-02-10 22:00 UTC (permalink / raw) To: Daniel Latypov, brendanhiggins Cc: kbuild-all, davidgow, alan.maguire, linux-kernel, kunit-dev, linux-kselftest, skhan, Uriel Guajardo, Daniel Latypov [-- Attachment #1: Type: text/plain, Size: 1739 bytes --] Hi Daniel, Thank you for the patch! Yet something to improve: [auto build test ERROR on linus/master] [also build test ERROR on v5.11-rc7 next-20210125] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Daniel-Latypov/kunit-support-failure-from-dynamic-analysis-tools/20210210-074913 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git e0756cfc7d7cd08c98a53b6009c091a3f6a50be6 config: m68k-allmodconfig (attached as .config) compiler: m68k-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/e3ddf9fb46a603f7e8bf3918cb707a0da9681a17 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Daniel-Latypov/kunit-support-failure-from-dynamic-analysis-tools/20210210-074913 git checkout e3ddf9fb46a603f7e8bf3918cb707a0da9681a17 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): m68k-linux-ld: lib/ubsan.o: in function `ubsan_prologue': >> ubsan.c:(.text.unlikely+0x142): undefined reference to `__kunit_fail_current_test' --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 59610 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] kunit: ubsan integration @ 2021-02-10 22:00 ` kernel test robot 0 siblings, 0 replies; 13+ messages in thread From: kernel test robot @ 2021-02-10 22:00 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 1776 bytes --] Hi Daniel, Thank you for the patch! Yet something to improve: [auto build test ERROR on linus/master] [also build test ERROR on v5.11-rc7 next-20210125] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Daniel-Latypov/kunit-support-failure-from-dynamic-analysis-tools/20210210-074913 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git e0756cfc7d7cd08c98a53b6009c091a3f6a50be6 config: m68k-allmodconfig (attached as .config) compiler: m68k-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/e3ddf9fb46a603f7e8bf3918cb707a0da9681a17 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Daniel-Latypov/kunit-support-failure-from-dynamic-analysis-tools/20210210-074913 git checkout e3ddf9fb46a603f7e8bf3918cb707a0da9681a17 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): m68k-linux-ld: lib/ubsan.o: in function `ubsan_prologue': >> ubsan.c:(.text.unlikely+0x142): undefined reference to `__kunit_fail_current_test' --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 59610 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] kunit: ubsan integration 2021-02-09 22:14 ` [PATCH v3 2/2] kunit: ubsan integration Daniel Latypov @ 2021-02-11 7:15 ` kernel test robot 2021-02-11 7:15 ` kernel test robot 1 sibling, 0 replies; 13+ messages in thread From: kernel test robot @ 2021-02-11 7:15 UTC (permalink / raw) To: Daniel Latypov, brendanhiggins Cc: kbuild-all, davidgow, alan.maguire, linux-kernel, kunit-dev, linux-kselftest, skhan, Uriel Guajardo, Daniel Latypov [-- Attachment #1: Type: text/plain, Size: 1753 bytes --] Hi Daniel, Thank you for the patch! Yet something to improve: [auto build test ERROR on linus/master] [also build test ERROR on v5.11-rc7 next-20210125] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Daniel-Latypov/kunit-support-failure-from-dynamic-analysis-tools/20210210-074913 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git e0756cfc7d7cd08c98a53b6009c091a3f6a50be6 config: mips-allmodconfig (attached as .config) compiler: mips-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/e3ddf9fb46a603f7e8bf3918cb707a0da9681a17 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Daniel-Latypov/kunit-support-failure-from-dynamic-analysis-tools/20210210-074913 git checkout e3ddf9fb46a603f7e8bf3918cb707a0da9681a17 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): mips-linux-ld: lib/ubsan.o: in function `ubsan_prologue': >> ubsan.c:(.text.unlikely.ubsan_prologue+0x80): undefined reference to `__kunit_fail_current_test' --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 69844 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] kunit: ubsan integration @ 2021-02-11 7:15 ` kernel test robot 0 siblings, 0 replies; 13+ messages in thread From: kernel test robot @ 2021-02-11 7:15 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 1790 bytes --] Hi Daniel, Thank you for the patch! Yet something to improve: [auto build test ERROR on linus/master] [also build test ERROR on v5.11-rc7 next-20210125] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Daniel-Latypov/kunit-support-failure-from-dynamic-analysis-tools/20210210-074913 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git e0756cfc7d7cd08c98a53b6009c091a3f6a50be6 config: mips-allmodconfig (attached as .config) compiler: mips-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/e3ddf9fb46a603f7e8bf3918cb707a0da9681a17 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Daniel-Latypov/kunit-support-failure-from-dynamic-analysis-tools/20210210-074913 git checkout e3ddf9fb46a603f7e8bf3918cb707a0da9681a17 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): mips-linux-ld: lib/ubsan.o: in function `ubsan_prologue': >> ubsan.c:(.text.unlikely.ubsan_prologue+0x80): undefined reference to `__kunit_fail_current_test' --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 69844 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 0/2] kunit: fail tests on UBSAN errors 2021-02-09 22:14 [PATCH v3 0/2] kunit: fail tests on UBSAN errors Daniel Latypov 2021-02-09 22:14 ` [PATCH v3 1/2] kunit: support failure from dynamic analysis tools Daniel Latypov 2021-02-09 22:14 ` [PATCH v3 2/2] kunit: ubsan integration Daniel Latypov @ 2021-02-10 10:34 ` Alan Maguire 2 siblings, 0 replies; 13+ messages in thread From: Alan Maguire @ 2021-02-10 10:34 UTC (permalink / raw) To: Daniel Latypov Cc: brendanhiggins, davidgow, alan.maguire, linux-kernel, kunit-dev, linux-kselftest, skhan On Tue, 9 Feb 2021, Daniel Latypov wrote: > v1 by Uriel is here: [1]. > Since it's been a while, I've dropped the Reviewed-By's. > > It depended on commit 83c4e7a0363b ("KUnit: KASAN Integration") which > hadn't been merged yet, so that caused some kerfuffle with applying them > previously and the series was reverted. > > This revives the series but makes the kunit_fail_current_test() function > take a format string and logs the file and line number of the failing > code, addressing Alan Maguire's comments on the previous version. > > As a result, the patch that makes UBSAN errors was tweaked slightly to > include an error message. > > v2 -> v3: > Fix kunit_fail_current_test() so it works w/ CONFIG_KUNIT=m > s/_/__ on the helper func to match others in test.c > > [1] https://lore.kernel.org/linux-kselftest/20200806174326.3577537-1-urielguajardojr@gmail.com/ > For the series: Reviewed-by: Alan Maguire <alan.maguire@oracle.com> Thanks! ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-02-17 0:24 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-09 22:14 [PATCH v3 0/2] kunit: fail tests on UBSAN errors Daniel Latypov 2021-02-09 22:14 ` [PATCH v3 1/2] kunit: support failure from dynamic analysis tools Daniel Latypov 2021-02-11 8:54 ` David Gow 2021-02-11 15:40 ` Alan Maguire 2021-02-11 20:58 ` Daniel Latypov 2021-02-11 21:33 ` Brendan Higgins 2021-02-17 0:23 ` Daniel Latypov 2021-02-09 22:14 ` [PATCH v3 2/2] kunit: ubsan integration Daniel Latypov 2021-02-10 22:00 ` kernel test robot 2021-02-10 22:00 ` kernel test robot 2021-02-11 7:15 ` kernel test robot 2021-02-11 7:15 ` kernel test robot 2021-02-10 10:34 ` [PATCH v3 0/2] kunit: fail tests on UBSAN errors Alan Maguire
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.