Message-ID: <CAFd5g45kPdrKz-UnMagx1JdcRmLK0uG31m5OELvJKe=pTaND+w@mail.gmail.com>
Date:   Thu, 11 Feb 2021 13:33:03 -0800
From:   Brendan Higgins <brendanhiggins@...gle.com>
To:     Daniel Latypov <dlatypov@...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>,
        Uriel Guajardo <urielguajardo@...gle.com>
Subject: Re: [PATCH v3 1/2] kunit: support failure from dynamic analysis tools

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

> 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