Message-ID: <alpine.LRH.2.20.1910111105350.21459@dhcp-10-175-191-48.vpn.oracle.com>
Date:   Fri, 11 Oct 2019 11:25:33 +0100 (BST)
From:   Alan Maguire <alan.maguire@...cle.com>
To:     Brendan Higgins <brendanhiggins@...gle.com>
cc:     Alan Maguire <alan.maguire@...cle.com>,
        Kees Cook <keescook@...omium.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Shuah Khan <skhan@...uxfoundation.org>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Iurii Zaikin <yzaikin@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        catalin.marinas@....com, joe.lawrence@...hat.com,
        penguin-kernel@...ove.sakura.ne.jp, schowdary@...dia.com,
        urezki@...il.com, andriy.shevchenko@...ux.intel.com,
        changbin.du@...el.com, kunit-dev@...glegroups.com,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Knut Omang <knut.omang@...cle.com>
Subject: Re: [PATCH v2 linux-kselftest-test 1/3] kunit: allow kunit tests to
 be loaded as a module

On Fri, 11 Oct 2019, Brendan Higgins wrote:

> Sorry for the delayed reply. I will be on vacation until Wednesday,
> October 16th.
> 
> On Wed, Oct 9, 2019 at 9:36 AM Alan Maguire <alan.maguire@...cle.com> wrote:
> >
> > On Tue, 8 Oct 2019, Brendan Higgins wrote:
> >
> > > On Tue, Oct 08, 2019 at 03:55:44PM +0100, Alan Maguire wrote:
> [...]
> > > > diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> > > > index e6d17aa..e4f3a97 100644
> > > > --- a/lib/kunit/string-stream.c
> > > > +++ b/lib/kunit/string-stream.c
> > > > @@ -100,6 +100,7 @@ int string_stream_vadd(struct string_stream *stream,
> > > >
> > > >     return 0;
> > > >  }
> > > > +EXPORT_SYMBOL_GPL(string_stream_vadd);
> > >
> > > Is this actually needed by anything other than lib/kunit/test.c right
> > > now? Maybe we should move the include file into the kunit/ directory to
> > > hide these so no one else can use them.
> > >
> >
> > I tried this, and it's the right answer I think but it exposes
> > a problem with symbol visibility when kunit is compiled as a module.
> > More on this below...
> >
> > > >  int string_stream_add(struct string_stream *stream, const char *fmt, ...)
> > > >  {
> > > > @@ -112,6 +113,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...)
> > > >
> > > >     return result;
> > > >  }
> > > > +EXPORT_SYMBOL_GPL(string_stream_add);
> > > [...]
> > > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> > > > index c83c0fa..e7896f1 100644
> > > > --- a/lib/kunit/test.c
> > > > +++ b/lib/kunit/test.c
> > > [...]
> > > > @@ -50,6 +51,7 @@ static unsigned long kunit_test_timeout(void)
> > > >      * For more background on this topic, see:
> > > >      * https://mike-bland.com/2011/11/01/small-medium-large.html
> > > >      */
> > > > +#ifndef MODULE
> > >
> > > Why is this block of code "ifndef MODULE"?
> > >
> >
> > Symbol visibility is the problem again; sysctl_hung_task_timeout_secs
> > isn't exported so when kunit is a module it can't find the symbol.
> >
> > I think I saw Kees mentioned something about symbol lookup too; in KTF
> > Knut solved this by defining ktf_find_symbol(). I'd suggest we may need a
> > kunit_find_symbol() with a function signature
> 
> I thought we were just talking about exposing symbols for linking
> outside of a compilation unit (static vs. not static); nevertheless, I
> think you are right that it is relevant here. Kees, thoughts?
> 
> > void *kunit_find_symbol(const char *modname, const char *symbol_name);
> >
> > ...which does a [module_]kallsyms_lookup_sym().
> >
> > If the above makes sense I can look at adding it as a patch (and adding
> > a test of it of course!). What do you think?
> 
> So that won't work if you are trying to link against a symbol not in a
> module, right? Also, it won't work against a static symbol, right?
> 

Nope, works in both cases with the proviso that we need to use an 
alternative name for symbols when compiling built-in.  For example
in the case of the string-stream tests, we'd use a test init callback
to initialize used symbols:

static int string_stream_test_init(struct kunit *test)
{
        _alloc_string_stream = kunit_find_symbol("alloc_string_stream");
        _string_stream_add = kunit_find_symbol("string_stream_add");
        _string_stream_get_string = kunit_find_symbol("string_stream_get_string");
        _string_stream_is_empty = kunit_find_symbol("string_stream_is_empty");
        if (IS_ERR(_alloc_string_stream) ||
            IS_ERR(_string_stream_add) ||
            IS_ERR(_string_stream_get_string) ||
            IS_ERR(_string_stream_is_empty))
                return EINVAL;
        return 0;
} 

I've tested this when string-stream-test is compiled built-in and as a 
module.  We can of course create a wrapper macro to handle these 
assignments.

To illustrate further here's the test cases I'd propose adding to 
test-test.c with the changes. 

In the first case we're grabbing the "modules" variable from the kernel,
and in the second we're grabbing a static symbol from the test-test.ko
module (when it is compiled as a module):

/*
 * Find non-exported kernel symbol; we use the modules list as a safe
 * choice that should always be present.
 */
static void kunit_find_symbol_kernel(struct kunit *test)
{
        KUNIT_ASSERT_NOT_ERR_OR_NULL(test, kunit_find_symbol("modules"));
}

#ifdef MODULE
/*
 * If we are compiled as a module, use this module for lookup.
 */
static void kunit_find_symbol_module(struct kunit *test)
{
        KUNIT_ASSERT_NOT_ERR_OR_NULL(test,
                                     kunit_find_symbol("kunit_find_symbol_kernel"));
}
#endif

 
> Even so, I think it is pretty wonky to expect users to either a)
> export any symbol name to be tested,

Absolutely not, I'd never advocate that.  Nothing should need to change in 
the component under test simply to facilitate testing, especially if 
there's a way the test framework can work around it.

> or b) have to access them via
> kunit_find_symbol.  I think it is fine to have some tests that cannot
> be compiled as modules, if there is no other user friendly way to make
> this work in those cases.

That's fine, and I agree in some cases it's unworkable, but there are 
going to be a lot of tristate componenets we'd like to test, and 
restricting testing of those by requiring CONFIG_FOO=y seems like a 
limitation too.  In practice I've found symbol lookup isn't needed 
extensively for test development.  For cases where the weight of symbol 
lookup is too heavy the tests can simply stay built-in - the non-exported 
nature of the symbols is probably suggesting something about the nature of 
the interface that makes that a more natural choice anyway.  However for 
other cases I think there's value to having something like this feature.
Of course there may be better ways to realize the functionality than what 
I'm proposing.

Thanks!

Alan 

Powered by blists - more mailing lists