Re: [PATCH] kunit: Rework kunit_resource allocation policy

[David Gow <davidgow@google.com>]

[-- Attachment #1: Type: text/plain, Size: 7123 bytes --]

On Tue, Mar 22, 2022 at 9:57 AM 'Daniel Latypov' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> On Sat, Mar 19, 2022 at 12:56 AM David Gow <davidgow@google.com> wrote:
> >
> > KUnit's test-managed resources can be created in two ways:
> > - Using the kunit_add_resource() family of functions, which accept a
> >   struct kunit_resource pointer, typically allocated statically or on
> >   the stack during the test.
> > - Using the kunit_alloc_resource() family of functions, which allocate a
> >   struct kunit_resource using kzalloc() behind the scenes.
> >
> > Both of these families of functions accept a 'free' function to be
> > called when the resource is finally disposed of.
> >
> > At present, KUnit will kfree() the resource if this 'free' function is
> > specified, and will not if it is NULL. However, this can lead
> > kunit_alloc_resource() to leak memory (if no 'free' function is passed
> > in), or kunit_add_resource() to incorrectly kfree() memory which was
> > allocated by some other means (on the stack, as part of a larger
> > allocation, etc), if a 'free' function is provided.
>
> Trying it with this:
>
> static void noop_free_resource(struct kunit_resource *) {}
>
> struct kunit_resource global_res;
>
> static void example_simple_test(struct kunit *test)
> {
>         kunit_add_resource(test, NULL, noop_free_resource, &global_res, test);
> }
>
> Running then with
> $ run_kunit --kunitconfig=lib/kunit --arch=x86_64
> --build_dir=kunit_x86/ --kconfig_add=CONFIG_KASAN=y
>
> Before:
> BUG: KASAN: double-free or invalid-free in kunit_cleanup+0x51/0xb0
>
> After:
> Passes
>

Phew! :-)
I'm glad it works.

> >
> > Instead, always kfree() if the resource was allocated with
> > kunit_alloc_resource(), and never kfree() if it was passed into
> > kunit_add_resource() by the user. (If the user of kunit_add_resource()
> > wishes the resource be kfree()ed, they can call kfree() on the resource
> > from within the 'free' function.
> >
> > This is implemented by adding a 'should_free' member to
>
> nit: would `should_kfree` be a bit better?
> `should_free` almost sounds like "should we invoke res->free" (as
> nonsensical as that might be)
>

I think I had it as should_kfree at some point. I agree it's a little
clearer. I'll rename it back.

The other option I considered was to have a "flags" member, of which
SHOULD_KFREE could be one. Though I eventually decided to leave that
until we needed another flag.

> > struct kunit_resource and setting it appropriately. To facilitate this,
> > the various resource add/alloc functions have been refactored somewhat,
> > making them all call a __kunit_add_resource() helper after setting the
> > 'should_free' member appropriately. In the process, all other functions
> > have been made static inline functions.
> >
> > Signed-off-by: David Gow <davidgow@google.com>
>
> Tested-by: Daniel Latypov <dlatypov@google.com>
>
>
> > ---
> >  include/kunit/test.h | 135 +++++++++++++++++++++++++++++++++++--------
> >  lib/kunit/test.c     |  65 +++------------------
> >  2 files changed, 120 insertions(+), 80 deletions(-)
> >
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index 00b9ff7783ab..5a3aacbadda2 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -36,11 +36,14 @@ typedef void (*kunit_resource_free_t)(struct kunit_resource *);
> >   * struct kunit_resource - represents a *test managed resource*
> >   * @data: for the user to store arbitrary data.
> >   * @name: optional name
> > - * @free: a user supplied function to free the resource. Populated by
> > - * kunit_resource_alloc().
> > + * @free: a user supplied function to free the resource.
> >   *
> >   * Represents a *test managed resource*, a resource which will automatically be
> > - * cleaned up at the end of a test case.
> > + * cleaned up at the end of a test case. This cleanup is performed by the 'free'
> > + * function. The resource itself is allocated with kmalloc() and freed with
> > + * kfree() if created with kunit_alloc_{,and_get_}resource(), otherwise it must
> > + * be freed by the user, typically with the 'free' function, or automatically if
> > + * it's allocated on the stack.
>
> I'm not a fan of this complexity, but I'm not sure if we have a way
> around it, esp. w/ stack-allocated data.
>
The other option is to make all resources allocated with
kunit_alloc_resource() require a non-NULL 'free' function which calls
kfree() itself. This is much simpler on the KUnit side, but does put
some of that burden on the user (and may prevent a free() function
from being shared between allocated and non-allocated resources).

> Perhaps this would be a bit easier to read if we tweaked it a bit like:
> "freed with kfree() if allocated by KUnit (via kunit_alloc..."
>
> Maybe we can drop the "or automatically, if it's allocated on the
> stack" as well.

Yeah: I'm not 100% happy with that wording. I wanted to make it clear
that there are cases where no automatic freeing is needed, but I agree
it's really just making things more confusing.
>
> A bigger way to simplify: perhaps we should get rid of
> kunit_alloc_and_get_resource() first?
> It's only used in KUnit's tests for itself.
> They could instead use kunit_alloc_resource() +
> kunit_find_resource(test, kunit_resource_instance_match, data).
> We could even define the helper with the same name in kunit-test.c
> (the only place it's used).
>
> Alternatively, we could make it an internal helper and define
> kunit_alloc_resource() as
>
> void *kunit_alloc_resource(...)
> {
>    struct kunit_resource *res = _kunit_alloc_and_get_resource(...)
>    if (res) return res->data;
>    return NULL;
> }
>
> ?
>

I was thinking about this a bit this morning, and I think we should do
the opposite: get rid of kunit_alloc_resource() and leave only
kunit_alloc_and_get_resource().
Then, split the resource system basically in two:
- The system for managing "findable" resources, whose main purpose is
for cases like the KASAN integration and the stub stuff where main
goal is tying some named bit of data to a test, and reference counting
it so it can safely be retrieved and used throughout the kernel if
need be.
- The simpler "free this on test exit" system, which could be as
simple as a kunit_defer(func, context) function built on top of the
former. This wouldn't need detailed tracking of reference counts, etc,

(tl;dr: I think that kunit_alloc_resource() is broken, refcount-wise,
if we're trying to implement the first kind of system, but useful for
the second, and this is quite confusing. So kunit_alloc_resource()
probably shouldn't be used alongside kunit_find_resource(), as there
could be a potential race condition. Now, this shouldn't happen in
practice, as most tests are single threaded and none are doing fancy
things with kunit_remove_resource(), but
kunit_alloc_and_get_resource() should be safer, as you're not playing
with a resource you don't have a reference to according to the
refcount.)

That's a more complicated refactor and redesign of the resources
system, though, so I'd rather fix this first.

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]