Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220319055600.3471875-1-davidgow@google.com>
In-Reply-To: <20220319055600.3471875-1-davidgow@google.com>
From:   Daniel Latypov <dlatypov@google.com>
Date:   Mon, 21 Mar 2022 20:57:12 -0500
Message-ID: <CAGS_qxpqcc8O2HpmF3qB-uzXZrDNg9=h3nE_f7si=aOxXkRA+Q@mail.gmail.com>
Subject: Re: [PATCH] kunit: Rework kunit_resource allocation policy
To:     David Gow <davidgow@google.com>
Cc:     Brendan Higgins <brendanhiggins@google.com>,
        Shuah Khan <skhan@linuxfoundation.org>,
        kunit-dev@googlegroups.com, linux-kselftest@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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

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

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

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.

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

?