Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

[Kees Cook <kees@kernel.org>]

On Thu, May 29, 2025 at 11:02:19AM +0200, Peter Zijlstra wrote:
> On Wed, May 28, 2025 at 03:47:42PM -0700, Kees Cook wrote:
> > On Mon, May 26, 2025 at 01:27:51PM +0000, Alessandro Carminati 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.
> > > 
> > > 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.
> > > 
> > > Implementation details:
> > > Check suppression directly in the `WARN()` Macros.
> > > This avoids the need for function symbol resolution or ELF section
> > > modification.
> > > Suppression is implemented directly in the `WARN*()` macros.
> > > 
> > > A helper function, `__kunit_is_suppressed_warning()`, is used to determine
> > > whether suppression applies. It is marked as `noinstr`, since some `WARN*()`
> > > sites reside in non-instrumentable sections. As it uses `strcmp`, a
> > > `noinstr` version of `strcmp` was introduced.
> > > The implementation is deliberately simple and avoids architecture-specific
> > > optimizations to preserve portability. Since this mechanism compares
> > > function names and is intended for test usage only, performance is not a
> > > primary concern.
> > > 
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > 
> > I like this -- it's very simple, it doesn't need to be fast-path, so
> > a linear list walker with strcmp is fine. Nice!
> 
> But it is on the fast path! This is still bloating every UD2 site
> instead of doing it on the other end.

Doing it on the other end doesn't look great (see the other reply).  I was
suggesting it's not on fast path because the added code is a dependant
conditional following an "unlikley" conditional. But if you wanted to
push it totally out of line, we'd likely need to pass __func__ into
warn_slowpath_fmt() and __warn_printk(), and then have __warn_printk()
return bool to make the call to __WARN_FLAGS() conditional. e.g.:

-		warn_slowpath_fmt(__FILE__, __LINE__, taint, arg); \
+		warn_slowpath_fmt(__FILE__, __LINE__, __func__, taint, arg); \

and:

-		__warn_printk(arg);					\
-		__WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\
+		if (__warn_printk(__func__, arg))			\
+			__WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\

But it still leaves bare __WARN unhandled...

-- 
Kees Cook