Message-ID: <CAGS_qxpwLbeYiFxL58aKbrU0sgjMq1HCnmoZMhFhXETRSrhV7g@mail.gmail.com>
Date:   Thu, 11 Feb 2021 12:58:31 -0800
From:   Daniel Latypov <dlatypov@...gle.com>
To:     Alan Maguire <alan.maguire@...cle.com>
Cc:     David Gow <davidgow@...gle.com>,
        Brendan Higgins <brendanhiggins@...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>,
        Uriel Guajardo <urielguajardo@...gle.com>
Subject: Re: [PATCH v3 1/2] kunit: support failure from dynamic analysis tools

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 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

Powered by blists - more mailing lists