Re: [PATCH v3 1/2] kunit: support failure from dynamic analysis tools

[Alan Maguire]

On Thu, 11 Feb 2021, David Gow wrote:

> On Wed, Feb 10, 2021 at 6:14 AM Daniel Latypov <dlatypov@xxxxxxxxxx> wrote:
> >
> > From: Uriel Guajardo <urielguajardo@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Uriel Guajardo <urielguajardo@xxxxxxxxxx>
> > Signed-off-by: Daniel Latypov <dlatypov@xxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxx>
> > + */
> > +
> > +#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