Date:   Tue, 16 Feb 2021 16:23:44 -0800
From:   Daniel Latypov <dlatypov@...gle.com>
To:     Brendan Higgins <brendanhiggins@...gle.com>
Cc:     Alan Maguire <alan.maguire@...cle.com>,
        David Gow <davidgow@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        KUnit Development <kunit-dev@...glegroups.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH v3 1/2] kunit: support failure from dynamic analysis tools

On Thu, Feb 11, 2021 at 1:33 PM 'Brendan Higgins' via KUnit
Development <kunit-dev@...glegroups.com> wrote:
>
> On Thu, Feb 11, 2021 at 12:58 PM Daniel Latypov <dlatypov@...gle.com> wrote:
> >
> > On Thu, Feb 11, 2021 at 7:40 AM Alan Maguire <alan.maguire@...cle.com> wrote:
> > >
> > > On Thu, 11 Feb 2021, David Gow wrote:
> > >
> > > > On Wed, Feb 10, 2021 at 6:14 AM Daniel Latypov <dlatypov@...gle.com> wrote:
> > > > >
> > > > > From: Uriel Guajardo <urielguajardo@...gle.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@...gle.com>
> > > > > Signed-off-by: Uriel Guajardo <urielguajardo@...gle.com>
> > > > > Signed-off-by: Daniel Latypov <dlatypov@...gle.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@...gle.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@...glegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/CAFd5g45kPdrKz-UnMagx1JdcRmLK0uG31m5OELvJKe%3DpTaND%2Bw%40mail.gmail.com.

Powered by blists - more mailing lists