Some unit tests intentionally trigger warning backtraces by passing bad parameters to kernel API functions. Such unit tests typically check the return value from such calls, not the existence of the warning backtrace.
Such intentionally generated warning backtraces are neither desirable nor useful for a number of reasons. - They can result in overlooked real problems. - A warning that suddenly starts to show up in unit tests needs to be investigated and has to be marked to be ignored, for example by adjusting filter scripts. Such filters are ad-hoc because there is no real standard format for warnings. On top of that, such filter scripts would require constant maintenance.
One option to address problem would be to add messages such as "expected warning backtraces start / end here" to the kernel log. However, that would again require filter scripts, it might result in missing real problematic warning backtraces triggered while the test is running, and the irrelevant backtrace(s) would still clog the kernel log.
Solve the problem by providing a means to identify and suppress specific warning backtraces while executing test code. Support suppressing multiple backtraces while at the same time limiting changes to generic code to the absolute minimum. Architecture specific changes are kept at minimum by retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and CONFIG_KUNIT are enabled.
The first patch of the series introduces the necessary infrastructure. The second patch marks the warning message in drm_calc_scale() in the DRM subsystem as intentional where warranted. This patch is intended to serve as an example for the use of the functionality introduced with this series. The last three patches in the series introduce the necessary architecture specific changes for x86, arm64, and loongarch.
This series is based on the RFC patch and subsequent discussion at https://patchwork.kernel.org/project/linux-kselftest/patch/02546e59-1afe-4b0... and offers a more comprehensive solution of the problem discussed there.
Checkpatch note: Remaining checkpatch errors and warnings were deliberately ignored. Some are triggered by matching coding style or by comments interpreted as code, others by assembler macros which are disliked by checkpatch. Suggestions for improvements are welcome.
Some questions:
- Is the general approach promising ? If not, are there other possible solutions ? - Function pointers are only added to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled. This avoids image size increases if CONFIG_KUNIT=n. Downside is slightly more complex architecture specific assembler code. If function pointers were always added to the __bug_table section, vmlinux image size would increase by approximately 0.6-0.7%. Is the increased complexity in assembler code worth the reduced image size ? I think so, but I would like to hear other opinions. - There are additional possibilities associated with storing the bug function name in the __bug_table section. It could be independent of KUNIT, it could be a configuration flag, and/or it could be used to display the name of the offending function in BUG/WARN messages. Is any of those of interest ?
---------------------------------------------------------------- Guenter Roeck (5): bug: Core support for suppressing warning backtraces drm: Suppress intentional warning backtraces in scaling unit tests x86: Add support for suppressing warning tracebacks arm64: Add support for suppressing warning tracebacks loongarch: Add support for suppressing warning tracebacks
arch/arm64/include/asm/asm-bug.h | 29 +++++++++++++------- arch/arm64/include/asm/bug.h | 8 +++++- arch/loongarch/include/asm/bug.h | 38 ++++++++++++++++++-------- arch/x86/include/asm/bug.h | 21 +++++++++++---- drivers/gpu/drm/tests/drm_rect_test.c | 6 +++++ include/asm-generic/bug.h | 16 ++++++++--- include/kunit/bug.h | 51 +++++++++++++++++++++++++++++++++++ include/linux/bug.h | 13 +++++++++ lib/bug.c | 51 ++++++++++++++++++++++++++++++++--- lib/kunit/Makefile | 6 +++-- lib/kunit/bug.c | 40 +++++++++++++++++++++++++++ 11 files changed, 243 insertions(+), 36 deletions(-) create mode 100644 include/kunit/bug.h create mode 100644 lib/kunit/bug.c
Some unit tests intentionally trigger warning backtraces by passing bad parameters to API functions. Such unit tests typically check the return value from those calls, not the existence of the warning backtrace.
Such intentionally generated warning backtraces are neither desirable nor useful for a number of reasons. - They can result in overlooked real problems. - A warning that suddenly starts to show up in unit tests needs to be investigated and has to be marked to be ignored, for example by adjusting filter scripts. Such filters are ad-hoc because there is no real standard format for warnings. On top of that, such filter scripts would require constant maintenance.
One option to address problem would be to add messages such as "expected warning backtraces start / end here" to the kernel log. However, that would again require filter scripts, it might result in missing real problematic warning backtraces triggered while the test is running, and the irrelevant backtrace(s) would still clog the kernel log.
Solve the problem by providing a means to identify and suppress specific warning backtraces while executing test code.
Cc: Dan Carpenter dan.carpenter@linaro.org Cc: Daniel Diaz daniel.diaz@linaro.org Cc: Naresh Kamboju naresh.kamboju@linaro.org Cc: Kees Cook keescook@chromium.org Signed-off-by: Guenter Roeck linux@roeck-us.net --- include/asm-generic/bug.h | 16 +++++++++--- include/kunit/bug.h | 51 +++++++++++++++++++++++++++++++++++++++ include/linux/bug.h | 13 ++++++++++ lib/bug.c | 51 ++++++++++++++++++++++++++++++++++++--- lib/kunit/Makefile | 6 +++-- lib/kunit/bug.c | 40 ++++++++++++++++++++++++++++++ 6 files changed, 168 insertions(+), 9 deletions(-) create mode 100644 include/kunit/bug.h create mode 100644 lib/kunit/bug.c
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h index 6e794420bd39..b0069564eb8f 100644 --- a/include/asm-generic/bug.h +++ b/include/asm-generic/bug.h @@ -2,6 +2,7 @@ #ifndef _ASM_GENERIC_BUG_H #define _ASM_GENERIC_BUG_H
+#include <kunit/bug.h> #include <linux/compiler.h> #include <linux/instrumentation.h> #include <linux/once_lite.h> @@ -39,8 +40,14 @@ struct bug_entry { #ifdef CONFIG_DEBUG_BUGVERBOSE #ifndef CONFIG_GENERIC_BUG_RELATIVE_POINTERS const char *file; +#ifdef HAVE_BUG_FUNCTION + const char *function; +#endif #else signed int file_disp; +#ifdef HAVE_BUG_FUNCTION + signed int function_disp; +#endif #endif unsigned short line; #endif @@ -96,15 +103,18 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); #define __WARN() __WARN_printf(TAINT_WARN, NULL) #define __WARN_printf(taint, arg...) do { \ instrumentation_begin(); \ - warn_slowpath_fmt(__FILE__, __LINE__, taint, arg); \ + if (!IS_SUPPRESSED_WARNING(__func__)) \ + warn_slowpath_fmt(__FILE__, __LINE__, taint, arg);\ instrumentation_end(); \ } while (0) #else #define __WARN() __WARN_FLAGS(BUGFLAG_TAINT(TAINT_WARN)) #define __WARN_printf(taint, arg...) do { \ instrumentation_begin(); \ - __warn_printk(arg); \ - __WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\ + if (!IS_SUPPRESSED_WARNING(__func__)) { \ + __warn_printk(arg); \ + __WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\ + } \ instrumentation_end(); \ } while (0) #define WARN_ON_ONCE(condition) ({ \ diff --git a/include/kunit/bug.h b/include/kunit/bug.h new file mode 100644 index 000000000000..11b8ae795320 --- /dev/null +++ b/include/kunit/bug.h @@ -0,0 +1,51 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * KUnit helpers for backtrace suppression + * + * Copyright (c) 2024 Guenter Roeck linux@roeck-us.net + */ + +#ifndef _KUNIT_BUG_H +#define _KUNIT_BUG_H + +#ifndef __ASSEMBLY__ + +#include <linux/kconfig.h> + +#if IS_ENABLED(CONFIG_KUNIT) + +#include <linux/types.h> +#include <linux/stringify.h> + +struct __suppressed_warning { + struct list_head node; + const char *function; +}; + +void __start_suppress_warning(struct __suppressed_warning *warning); +void __end_suppress_warning(struct __suppressed_warning *warning); +bool __is_suppressed_warning(const char *function); + +#define DEFINE_SUPPRESSED_WARNING(func) \ + struct __suppressed_warning __kunit_suppress_##func = \ + { .function = __stringify(func) } + +#define START_SUPPRESSED_WARNING(func) \ + __start_suppress_warning(&__kunit_suppress_##func) + +#define END_SUPPRESSED_WARNING(func) \ + __end_suppress_warning(&__kunit_suppress_##func) + +#define IS_SUPPRESSED_WARNING(func) \ + __is_suppressed_warning(func) + +#else /* CONFIG_KUNIT */ + +#define DEFINE_SUPPRESSED_WARNING(func) +#define START_SUPPRESSED_WARNING(func) +#define END_SUPPRESSED_WARNING(func) +#define IS_SUPPRESSED_WARNING(func) (false) + +#endif /* CONFIG_KUNIT */ +#endif /* __ASSEMBLY__ */ +#endif /* _KUNIT_BUG_H */ diff --git a/include/linux/bug.h b/include/linux/bug.h index 348acf2558f3..c668762dc76a 100644 --- a/include/linux/bug.h +++ b/include/linux/bug.h @@ -36,6 +36,9 @@ static inline int is_warning_bug(const struct bug_entry *bug) return bug->flags & BUGFLAG_WARNING; }
+void bug_get_file_function_line(struct bug_entry *bug, const char **file, + const char **function, unsigned int *line); + void bug_get_file_line(struct bug_entry *bug, const char **file, unsigned int *line);
@@ -62,6 +65,16 @@ static inline enum bug_trap_type report_bug(unsigned long bug_addr, }
struct bug_entry; +static inline void bug_get_file_function_line(struct bug_entry *bug, + const char **file, + const char **function, + unsigned int *line) +{ + *file = NULL; + *function = NULL; + *line = 0; +} + static inline void bug_get_file_line(struct bug_entry *bug, const char **file, unsigned int *line) { diff --git a/lib/bug.c b/lib/bug.c index e0ff21989990..6b85d4042e07 100644 --- a/lib/bug.c +++ b/lib/bug.c @@ -26,6 +26,14 @@ when CONFIG_DEBUG_BUGVERBOSE is not enabled, so you must generate the values accordingly.
+ 2a.Optionally implement support for the "function" entry in struct + bug_entry. This entry must point to the name of the function triggering + the warning or bug trap (normally __func__). This is only needed if + both CONFIG_DEBUG_BUGVERBOSE and CONFIG_KUNIT are enabled and if + the architecture wants to implement support for suppressing warning + backtraces. The architecture must define HAVE_BUG_FUNCTION if it adds + pointers to function names to struct bug_entry. + 3. Implement the trap - In the illegal instruction trap handler (typically), verify that the fault was in kernel mode, and call report_bug() @@ -127,14 +135,21 @@ static inline struct bug_entry *module_find_bug(unsigned long bugaddr) } #endif
-void bug_get_file_line(struct bug_entry *bug, const char **file, - unsigned int *line) +void bug_get_file_function_line(struct bug_entry *bug, const char **file, + const char **function, unsigned int *line) { + *function = NULL; #ifdef CONFIG_DEBUG_BUGVERBOSE #ifdef CONFIG_GENERIC_BUG_RELATIVE_POINTERS *file = (const char *)&bug->file_disp + bug->file_disp; +#ifdef HAVE_BUG_FUNCTION + *function = (const char *)&bug->function_disp + bug->function_disp; +#endif #else *file = bug->file; +#ifdef HAVE_BUG_FUNCTION + *function = bug->function; +#endif #endif *line = bug->line; #else @@ -143,6 +158,13 @@ void bug_get_file_line(struct bug_entry *bug, const char **file, #endif }
+void bug_get_file_line(struct bug_entry *bug, const char **file, unsigned int *line) +{ + const char *function; + + bug_get_file_function_line(bug, file, &function, line); +} + struct bug_entry *find_bug(unsigned long bugaddr) { struct bug_entry *bug; @@ -157,8 +179,9 @@ struct bug_entry *find_bug(unsigned long bugaddr) static enum bug_trap_type __report_bug(unsigned long bugaddr, struct pt_regs *regs) { struct bug_entry *bug; - const char *file; + const char *file, *function; unsigned line, warning, once, done; + char __maybe_unused sym[KSYM_SYMBOL_LEN];
if (!is_valid_bugaddr(bugaddr)) return BUG_TRAP_TYPE_NONE; @@ -169,12 +192,32 @@ static enum bug_trap_type __report_bug(unsigned long bugaddr, struct pt_regs *re
disable_trace_on_warning();
- bug_get_file_line(bug, &file, &line); + bug_get_file_function_line(bug, &file, &function, &line); +#if IS_ENABLED(CONFIG_KUNIT) && defined(CONFIG_KALLSYMS) + if (!function) { + /* + * This will be seen if report_bug is called on an architecture + * with no architecture-specific support for suppressing warning + * backtraces, if CONFIG_DEBUG_BUGVERBOSE is not enabled, or if + * the calling code is from assembler which does not record a + * function name. Extracting the function name from the bug + * address is less than perfect since compiler optimization may + * result in 'bugaddr' pointing to a function which does not + * actually trigger the warning, but it is better than no + * suppression at all. + */ + sprint_symbol_no_offset(sym, bugaddr); + function = sym; + } +#endif /* IS_ENABLED(CONFIG_KUNIT) && defined(CONFIG_KALLSYMS) */
warning = (bug->flags & BUGFLAG_WARNING) != 0; once = (bug->flags & BUGFLAG_ONCE) != 0; done = (bug->flags & BUGFLAG_DONE) != 0;
+ if (warning && IS_SUPPRESSED_WARNING(function)) + return BUG_TRAP_TYPE_WARN; + if (warning && once) { if (done) return BUG_TRAP_TYPE_WARN; diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile index 309659a32a78..545b57c3be48 100644 --- a/lib/kunit/Makefile +++ b/lib/kunit/Makefile @@ -14,8 +14,10 @@ ifeq ($(CONFIG_KUNIT_DEBUGFS),y) kunit-objs += debugfs.o endif
-# KUnit 'hooks' are built-in even when KUnit is built as a module. -obj-y += hooks.o +# KUnit 'hooks' and bug handling are built-in even when KUnit is built +# as a module. +obj-y += hooks.o \ + bug.o
obj-$(CONFIG_KUNIT_TEST) += kunit-test.o
diff --git a/lib/kunit/bug.c b/lib/kunit/bug.c new file mode 100644 index 000000000000..f93544d7a9d1 --- /dev/null +++ b/lib/kunit/bug.c @@ -0,0 +1,40 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * KUnit helpers for backtrace suppression + * + * Copyright (c) 2024 Guenter Roeck linux@roeck-us.net + */ + +#include <kunit/bug.h> +#include <linux/export.h> +#include <linux/list.h> +#include <linux/string.h> + +static LIST_HEAD(suppressed_warnings); + +void __start_suppress_warning(struct __suppressed_warning *warning) +{ + list_add(&warning->node, &suppressed_warnings); +} +EXPORT_SYMBOL_GPL(__start_suppress_warning); + +void __end_suppress_warning(struct __suppressed_warning *warning) +{ + list_del(&warning->node); +} +EXPORT_SYMBOL_GPL(__end_suppress_warning); + +bool __is_suppressed_warning(const char *function) +{ + struct __suppressed_warning *warning; + + if (!function) + return false; + + list_for_each_entry(warning, &suppressed_warnings, node) { + if (!strcmp(function, warning->function)) + return true; + } + return false; +} +EXPORT_SYMBOL_GPL(__is_suppressed_warning);
On Tue, Mar 05, 2024 at 10:40:29AM -0800, Guenter Roeck wrote:
[...] warning = (bug->flags & BUGFLAG_WARNING) != 0; once = (bug->flags & BUGFLAG_ONCE) != 0; done = (bug->flags & BUGFLAG_DONE) != 0;
- if (warning && IS_SUPPRESSED_WARNING(function))
return BUG_TRAP_TYPE_WARN;
I had to re-read __report_bug() more carefully, but yes, this works -- it's basically leaving early, like "once" does.
This looks like a reasonable approach!
Something very similar to this is checking that a warning happens. i.e. you talk about drm selftests checking function return values, but I've got a bunch of tests (LKDTM) that live outside of KUnit because I haven't had a clean way to check for specific warnings/bugs. I feel like future changes built on top of this series could add counters or something that KUnit could examine. E.g. I did this manually for some fortify tests:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for...
-Kees
On 3/5/24 11:54, Kees Cook wrote:
On Tue, Mar 05, 2024 at 10:40:29AM -0800, Guenter Roeck wrote:
[...] warning = (bug->flags & BUGFLAG_WARNING) != 0; once = (bug->flags & BUGFLAG_ONCE) != 0; done = (bug->flags & BUGFLAG_DONE) != 0;
- if (warning && IS_SUPPRESSED_WARNING(function))
return BUG_TRAP_TYPE_WARN;I had to re-read __report_bug() more carefully, but yes, this works -- it's basically leaving early, like "once" does.
This looks like a reasonable approach!
Something very similar to this is checking that a warning happens. i.e. you talk about drm selftests checking function return values, but I've got a bunch of tests (LKDTM) that live outside of KUnit because I haven't had a clean way to check for specific warnings/bugs. I feel like future changes built on top of this series could add counters or something that KUnit could examine. E.g. I did this manually for some fortify tests:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for...
Sounds like a good idea. It should be straightforward to add a counter to struct __suppressed_warning. This way the calling code could easily check if an expected warning backtrace actually happened.
Thanks, Guenter
The drm_test_rect_calc_hscale and drm_test_rect_calc_vscale unit tests intentionally trigger warning backtraces by providing bad parameters to the tested functions. What is tested is the return value, not the existence of a warning backtrace. Suppress the backtraces to avoid clogging the kernel log.
Signed-off-by: Guenter Roeck linux@roeck-us.net --- drivers/gpu/drm/tests/drm_rect_test.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/tests/drm_rect_test.c b/drivers/gpu/drm/tests/drm_rect_test.c index 76332cd2ead8..75614cb4deb5 100644 --- a/drivers/gpu/drm/tests/drm_rect_test.c +++ b/drivers/gpu/drm/tests/drm_rect_test.c @@ -406,22 +406,28 @@ KUNIT_ARRAY_PARAM(drm_rect_scale, drm_rect_scale_cases, drm_rect_scale_case_desc
static void drm_test_rect_calc_hscale(struct kunit *test) { + DEFINE_SUPPRESSED_WARNING(drm_calc_scale); const struct drm_rect_scale_case *params = test->param_value; int scaling_factor;
+ START_SUPPRESSED_WARNING(drm_calc_scale); scaling_factor = drm_rect_calc_hscale(¶ms->src, ¶ms->dst, params->min_range, params->max_range); + END_SUPPRESSED_WARNING(drm_calc_scale);
KUNIT_EXPECT_EQ(test, scaling_factor, params->expected_scaling_factor); }
static void drm_test_rect_calc_vscale(struct kunit *test) { + DEFINE_SUPPRESSED_WARNING(drm_calc_scale); const struct drm_rect_scale_case *params = test->param_value; int scaling_factor;
+ START_SUPPRESSED_WARNING(drm_calc_scale); scaling_factor = drm_rect_calc_vscale(¶ms->src, ¶ms->dst, params->min_range, params->max_range); + END_SUPPRESSED_WARNING(drm_calc_scale);
KUNIT_EXPECT_EQ(test, scaling_factor, params->expected_scaling_factor); }
Add support for selectively suppressing WARNING tracebacks to x86. This requires adding the function triggering tracebacks to the __bug_table object section.
To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable).
Signed-off-by: Guenter Roeck linux@roeck-us.net --- arch/x86/include/asm/bug.h | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h index a3ec87d198ac..8e45143fa676 100644 --- a/arch/x86/include/asm/bug.h +++ b/arch/x86/include/asm/bug.h @@ -21,6 +21,15 @@ # define __BUG_REL(val) ".long " __stringify(val) " - ." #endif
+#if IS_ENABLED(CONFIG_KUNIT) +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR __BUG_REL(%c1) +# define __BUG_FUNC __func__ +#else +# define __BUG_FUNC_PTR +# define __BUG_FUNC NULL +#endif /* IS_ENABLED(CONFIG_KUNIT) */ + #ifdef CONFIG_DEBUG_BUGVERBOSE
#define _BUG_FLAGS(ins, flags, extra) \ @@ -29,12 +38,13 @@ do { \ ".pushsection __bug_table,"aw"\n" \ "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \ "\t" __BUG_REL(%c0) "\t# bug_entry::file\n" \ - "\t.word %c1" "\t# bug_entry::line\n" \ - "\t.word %c2" "\t# bug_entry::flags\n" \ - "\t.org 2b+%c3\n" \ + "\t" __BUG_FUNC_PTR "\t# bug_entry::function\n" \ + "\t.word %c2" "\t# bug_entry::line\n" \ + "\t.word %c3" "\t# bug_entry::flags\n" \ + "\t.org 2b+%c4\n" \ ".popsection\n" \ extra \ - : : "i" (__FILE__), "i" (__LINE__), \ + : : "i" (__FILE__), "i" (__BUG_FUNC), "i" (__LINE__),\ "i" (flags), \ "i" (sizeof(struct bug_entry))); \ } while (0) @@ -80,7 +90,8 @@ do { \ do { \ __auto_type __flags = BUGFLAG_WARNING|(flags); \ instrumentation_begin(); \ - _BUG_FLAGS(ASM_UD2, __flags, ASM_REACHABLE); \ + if (!IS_SUPPRESSED_WARNING(__func__)) \ + _BUG_FLAGS(ASM_UD2, __flags, ASM_REACHABLE); \ instrumentation_end(); \ } while (0)
Add support for selectively suppressing WARNING tracebacks to arm64. This requires adding the function triggering tracebacks to the __bug_table object section.
To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable).
Signed-off-by: Guenter Roeck linux@roeck-us.net --- arch/arm64/include/asm/asm-bug.h | 29 +++++++++++++++++++---------- arch/arm64/include/asm/bug.h | 8 +++++++- 2 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/arch/arm64/include/asm/asm-bug.h b/arch/arm64/include/asm/asm-bug.h index c762038ba400..6884089a7191 100644 --- a/arch/arm64/include/asm/asm-bug.h +++ b/arch/arm64/include/asm/asm-bug.h @@ -8,36 +8,45 @@ #include <asm/brk-imm.h>
#ifdef CONFIG_DEBUG_BUGVERBOSE -#define _BUGVERBOSE_LOCATION(file, line) __BUGVERBOSE_LOCATION(file, line) -#define __BUGVERBOSE_LOCATION(file, line) \ + +#if IS_ENABLED(CONFIG_KUNIT) +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR(func) .long func - .; +#else +# define __BUG_FUNC_PTR(func) +#endif + +#define _BUGVERBOSE_LOCATION(file, func, line) __BUGVERBOSE_LOCATION(file, func, line) +#define __BUGVERBOSE_LOCATION(file, func, line) \ .pushsection .rodata.str,"aMS",@progbits,1; \ 14472: .string file; \ .popsection; \ \ .long 14472b - .; \ + __BUG_FUNC_PTR(func) \ .short line; #else -#define _BUGVERBOSE_LOCATION(file, line) +#define _BUGVERBOSE_LOCATION(file, func, line) #endif
#ifdef CONFIG_GENERIC_BUG
-#define __BUG_ENTRY(flags) \ +#define __BUG_ENTRY(flags, func) \ .pushsection __bug_table,"aw"; \ .align 2; \ 14470: .long 14471f - .; \ -_BUGVERBOSE_LOCATION(__FILE__, __LINE__) \ - .short flags; \ +_BUGVERBOSE_LOCATION(__FILE__, func, __LINE__) \ + .short flags; \ .popsection; \ 14471: #else -#define __BUG_ENTRY(flags) +#define __BUG_ENTRY(flags, func) #endif
-#define ASM_BUG_FLAGS(flags) \ - __BUG_ENTRY(flags) \ +#define ASM_BUG_FLAGS(flags, func) \ + __BUG_ENTRY(flags, func) \ brk BUG_BRK_IMM
-#define ASM_BUG() ASM_BUG_FLAGS(0) +#define ASM_BUG() ASM_BUG_FLAGS(0, .)
#endif /* __ASM_ASM_BUG_H */ diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h index 28be048db3f6..044c5e24a17d 100644 --- a/arch/arm64/include/asm/bug.h +++ b/arch/arm64/include/asm/bug.h @@ -11,8 +11,14 @@
#include <asm/asm-bug.h>
+#ifdef HAVE_BUG_FUNCTION +# define __BUG_FUNC __func__ +#else +# define __BUG_FUNC NULL +#endif + #define __BUG_FLAGS(flags) \ - asm volatile (__stringify(ASM_BUG_FLAGS(flags))); + asm volatile (__stringify(ASM_BUG_FLAGS(flags, %c0)) : : "i" (__BUG_FUNC));
#define BUG() do { \ __BUG_FLAGS(0); \
Add support for selectively suppressing WARNING tracebacks to loongarch. This requires adding the function triggering tracebacks to the __bug_table object section.
To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable).
Signed-off-by: Guenter Roeck linux@roeck-us.net --- arch/loongarch/include/asm/bug.h | 38 +++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 11 deletions(-)
diff --git a/arch/loongarch/include/asm/bug.h b/arch/loongarch/include/asm/bug.h index d4ca3ba25418..25f2b5ae7702 100644 --- a/arch/loongarch/include/asm/bug.h +++ b/arch/loongarch/include/asm/bug.h @@ -3,47 +3,63 @@ #define __ASM_BUG_H
#include <asm/break.h> +#include <kunit/bug.h> #include <linux/stringify.h>
#ifndef CONFIG_DEBUG_BUGVERBOSE -#define _BUGVERBOSE_LOCATION(file, line) +#define _BUGVERBOSE_LOCATION(file, func, line) #else -#define __BUGVERBOSE_LOCATION(file, line) \ +#if IS_ENABLED(CONFIG_KUNIT) +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR(func) .long func - .; +#else +# define __BUG_FUNC_PTR(func) +#endif + +#define __BUGVERBOSE_LOCATION(file, func, line) \ .pushsection .rodata.str, "aMS", @progbits, 1; \ 10002: .string file; \ .popsection; \ \ .long 10002b - .; \ + __BUG_FUNC_PTR(func) \ .short line; -#define _BUGVERBOSE_LOCATION(file, line) __BUGVERBOSE_LOCATION(file, line) +#define _BUGVERBOSE_LOCATION(file, func, line) __BUGVERBOSE_LOCATION(file, func, line) #endif
#ifndef CONFIG_GENERIC_BUG -#define __BUG_ENTRY(flags) +#define __BUG_ENTRY(flags, func) #else -#define __BUG_ENTRY(flags) \ +#define __BUG_ENTRY(flags, func) \ .pushsection __bug_table, "aw"; \ .align 2; \ 10000: .long 10001f - .; \ - _BUGVERBOSE_LOCATION(__FILE__, __LINE__) \ + _BUGVERBOSE_LOCATION(__FILE__, func, __LINE__) \ .short flags; \ .popsection; \ 10001: #endif
-#define ASM_BUG_FLAGS(flags) \ - __BUG_ENTRY(flags) \ +#define ASM_BUG_FLAGS(flags, func) \ + __BUG_ENTRY(flags, func) \ break BRK_BUG
-#define ASM_BUG() ASM_BUG_FLAGS(0) +#define ASM_BUG() ASM_BUG_FLAGS(0, .) + +#ifdef HAVE_BUG_FUNCTION +# define __BUG_FUNC __func__ +#else +# define __BUG_FUNC NULL +#endif
#define __BUG_FLAGS(flags) \ - asm_inline volatile (__stringify(ASM_BUG_FLAGS(flags))); + asm_inline volatile (__stringify(ASM_BUG_FLAGS(flags, %0)) : : "i" (__BUG_FUNC));
#define __WARN_FLAGS(flags) \ do { \ instrumentation_begin(); \ - __BUG_FLAGS(BUGFLAG_WARNING|(flags)); \ + if (!IS_SUPPRESSED_WARNING(__func__)) \ + __BUG_FLAGS(BUGFLAG_WARNING|(flags)); \ instrumentation_end(); \ } while (0)
Hello!
On Tue, 5 Mar 2024 at 12:40, Guenter Roeck linux@roeck-us.net wrote:
Some unit tests intentionally trigger warning backtraces by passing bad parameters to kernel API functions. Such unit tests typically check the return value from such calls, not the existence of the warning backtrace.
Such intentionally generated warning backtraces are neither desirable nor useful for a number of reasons.
- They can result in overlooked real problems.
- A warning that suddenly starts to show up in unit tests needs to be investigated and has to be marked to be ignored, for example by adjusting filter scripts. Such filters are ad-hoc because there is no real standard format for warnings. On top of that, such filter scripts would require constant maintenance.
One option to address problem would be to add messages such as "expected warning backtraces start / end here" to the kernel log. However, that would again require filter scripts, it might result in missing real problematic warning backtraces triggered while the test is running, and the irrelevant backtrace(s) would still clog the kernel log.
Solve the problem by providing a means to identify and suppress specific warning backtraces while executing test code. Support suppressing multiple backtraces while at the same time limiting changes to generic code to the absolute minimum. Architecture specific changes are kept at minimum by retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and CONFIG_KUNIT are enabled.
The first patch of the series introduces the necessary infrastructure. The second patch marks the warning message in drm_calc_scale() in the DRM subsystem as intentional where warranted. This patch is intended to serve as an example for the use of the functionality introduced with this series. The last three patches in the series introduce the necessary architecture specific changes for x86, arm64, and loongarch.
This series is based on the RFC patch and subsequent discussion at https://patchwork.kernel.org/project/linux-kselftest/patch/02546e59-1afe-4b0... and offers a more comprehensive solution of the problem discussed there.
Checkpatch note: Remaining checkpatch errors and warnings were deliberately ignored. Some are triggered by matching coding style or by comments interpreted as code, others by assembler macros which are disliked by checkpatch. Suggestions for improvements are welcome.
Some questions:
- Is the general approach promising ? If not, are there other possible solutions ?
- Function pointers are only added to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled. This avoids image size increases if CONFIG_KUNIT=n. Downside is slightly more complex architecture specific assembler code. If function pointers were always added to the __bug_table section, vmlinux image size would increase by approximately 0.6-0.7%. Is the increased complexity in assembler code worth the reduced image size ? I think so, but I would like to hear other opinions.
- There are additional possibilities associated with storing the bug function name in the __bug_table section. It could be independent of KUNIT, it could be a configuration flag, and/or it could be used to display the name of the offending function in BUG/WARN messages. Is any of those of interest ?
Thank you SO very much for this work! This is very much appreciated! We run into these warnings at LKFT all the time, and making sure that the noise doesn't drown the relevant signal is very important.
Greetings!
Daniel Díaz daniel.diaz@linaro.org
Hi Daniel,
On 3/6/24 10:24, Daniel Díaz wrote: [ ... ]
Thank you SO very much for this work! This is very much appreciated!
Thanks a lot for the feedback.
We run into these warnings at LKFT all the time, and making sure that the noise doesn't drown the relevant signal is very important.
Can you send me a list of all the warnings you are seeing ? I do see lots of warnings when running drm tests in qemu, but I am not sure if those are caused by emulation problems or if they are expected. A list of warnings seen on real hardware would help me prepare additional patches to address (or, rather, suppress) those.
Thanks, Guenter
On Tue, Mar 05, 2024 at 10:40:28AM -0800, Guenter Roeck wrote:
Some unit tests intentionally trigger warning backtraces by passing bad parameters to kernel API functions. Such unit tests typically check the return value from such calls, not the existence of the warning backtrace.
Such intentionally generated warning backtraces are neither desirable nor useful for a number of reasons.
- They can result in overlooked real problems.
- A warning that suddenly starts to show up in unit tests needs to be investigated and has to be marked to be ignored, for example by adjusting filter scripts. Such filters are ad-hoc because there is no real standard format for warnings. On top of that, such filter scripts would require constant maintenance.
One option to address problem would be to add messages such as "expected warning backtraces start / end here" to the kernel log. However, that would again require filter scripts, it might result in missing real problematic warning backtraces triggered while the test is running, and the irrelevant backtrace(s) would still clog the kernel log.
Solve the problem by providing a means to identify and suppress specific warning backtraces while executing test code. Support suppressing multiple backtraces while at the same time limiting changes to generic code to the absolute minimum. Architecture specific changes are kept at minimum by retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and CONFIG_KUNIT are enabled.
The first patch of the series introduces the necessary infrastructure. The second patch marks the warning message in drm_calc_scale() in the DRM subsystem as intentional where warranted. This patch is intended to serve as an example for the use of the functionality introduced with this series. The last three patches in the series introduce the necessary architecture specific changes for x86, arm64, and loongarch.
This series is based on the RFC patch and subsequent discussion at https://patchwork.kernel.org/project/linux-kselftest/patch/02546e59-1afe-4b0... and offers a more comprehensive solution of the problem discussed there.
Checkpatch note: Remaining checkpatch errors and warnings were deliberately ignored. Some are triggered by matching coding style or by comments interpreted as code, others by assembler macros which are disliked by checkpatch. Suggestions for improvements are welcome.
Some questions:
- Is the general approach promising ? If not, are there other possible solutions ?
- Function pointers are only added to the __bug_table section if both CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled. This avoids image size increases if CONFIG_KUNIT=n. Downside is slightly more complex architecture specific assembler code. If function pointers were always added to the __bug_table section, vmlinux image size would increase by approximately 0.6-0.7%. Is the increased complexity in assembler code worth the reduced image size ? I think so, but I would like to hear other opinions.
- There are additional possibilities associated with storing the bug function name in the __bug_table section. It could be independent of KUNIT, it could be a configuration flag, and/or it could be used to display the name of the offending function in BUG/WARN messages. Is any of those of interest ?
I am ready to send a full version of this series with support for all affected architectures. I am undecided if I should send it now, based on v6.8, and send v2 after rebasing it to v6.9-rc1, or if I should just wait for v6.9-rc1.
I understand that some maintainers dislike getting new patch series while the commit window is is open. On the ther side, I tested the series thoroughly on top of v6.8-rc7, and initial v6.9 release candidates may have their own problems. Given that, I tend to send the series now.
Any thoughts ? Unless there is strong negative feedback, I'll likely do that in a day or two.
Thanks, Guenter
linux-kselftest-mirror@lists.linaro.org
-
Daniel Díaz -
Guenter Roeck -
Kees Cook