Message-ID: <CABVgOS=bEq3K+0SU+1E8e6ZguBb0WV9UYNBaT5O+hydAe8p2WQ@mail.gmail.com>
Date:   Fri, 22 Jul 2022 15:16:49 +0800
From:   David Gow <davidgow@...gle.com>
To:     Daniel Latypov <dlatypov@...gle.com>
Cc:     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>
Subject: Re: [PATCH 3/4] kunit: make kunit_kfree() only work on pointers from
 kunit_malloc() and friends

On Fri, Jul 22, 2022 at 2:02 AM 'Daniel Latypov' via KUnit Development
<kunit-dev@...glegroups.com> wrote:
>
> kunit_kfree() exists to clean up allocations from kunit_kmalloc() and
> friends early instead of waiting for this to happen automatically at the
> end of the test.
>
> But it can be used on *anything* registered with the kunit resource API.
>
> E.g. the last 2 statements are equivalent:
>   struct kunit_resource *res = something();
>   kfree(res->data);
>   kunit_put_resource(res);
>
> The problem is that there could be multiple resources that point to the
> same `data`.
>
> E.g. you can have a named resource acting as a pseudo-global variable in
> a test. If you point it to data allocated with kunit_kmalloc(), then
> calling `kunit_kfree(ptr)` has the chance to delete either the named
> resource or to kfree `ptr`.
> Which one it does depends on the order the resources are registered as
> kunit_kfree() will delete resources in LIFO order.
>
> So this patch restricts kunit_kfree() to only working on resources
> created by kunit_kmalloc(). Calling it is therefore guaranteed to free
> the memory, not do anything else.
>
> Note: kunit_resource_instance_match() wasn't used outside of KUnit, so
> it should be safe to remove from the public interface. It's also
> generally dangerous, as shown above, and shouldn't be used.
>
> Signed-off-by: Daniel Latypov <dlatypov@...gle.com>
> ---

This is basically part of a sneaky, but sensible trend to make
resources more obviously "typed". Given how many issues that can
cause, this is definitely a worthwhile change.

I have some plans to further refactor some of the resources stuff down
the line (and to improve the documentation somewhat), so something not
dissimilar to this was going to happen eventually.

In any case,
Reviewed-by: David Gow <davidgow@...gle.com>

Cheers,
-- David

>  include/kunit/resource.h | 16 ----------------
>  lib/kunit/kunit-test.c   |  7 +++++++
>  lib/kunit/test.c         | 10 ++++++++--
>  3 files changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/include/kunit/resource.h b/include/kunit/resource.h
> index 09c2b34d1c61..cf6fb8f2ac1b 100644
> --- a/include/kunit/resource.h
> +++ b/include/kunit/resource.h
> @@ -300,22 +300,6 @@ typedef bool (*kunit_resource_match_t)(struct kunit *test,
>                                        struct kunit_resource *res,
>                                        void *match_data);
>
> -/**
> - * kunit_resource_instance_match() - Match a resource with the same instance.
> - * @test: Test case to which the resource belongs.
> - * @res: The resource.
> - * @match_data: The resource pointer to match against.
> - *
> - * An instance of kunit_resource_match_t that matches a resource whose
> - * allocation matches @match_data.
> - */
> -static inline bool kunit_resource_instance_match(struct kunit *test,
> -                                                struct kunit_resource *res,
> -                                                void *match_data)
> -{
> -       return res->data == match_data;
> -}
> -
>  /**
>   * kunit_resource_name_match() - Match a resource with the same name.
>   * @test: Test case to which the resource belongs.
> diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
> index 13d0bd8b07a9..4df0335d0d06 100644
> --- a/lib/kunit/kunit-test.c
> +++ b/lib/kunit/kunit-test.c
> @@ -161,6 +161,13 @@ static void kunit_resource_test_alloc_resource(struct kunit *test)
>         kunit_put_resource(res);
>  }
>
> +static inline bool kunit_resource_instance_match(struct kunit *test,
> +                                                struct kunit_resource *res,
> +                                                void *match_data)
> +{
> +       return res->data == match_data;
> +}
> +
>  /*
>   * Note: tests below use kunit_alloc_and_get_resource(), so as a consequence
>   * they have a reference to the associated resource that they must release
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 0fb2771ca03e..82019a78462e 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -689,12 +689,18 @@ void *kunit_kmalloc_array(struct kunit *test, size_t n, size_t size, gfp_t gfp)
>  }
>  EXPORT_SYMBOL_GPL(kunit_kmalloc_array);
>
> +static inline bool kunit_kfree_match(struct kunit *test,
> +                                    struct kunit_resource *res, void *match_data)
> +{
> +       /* Only match resources allocated with kunit_kmalloc() and friends. */
> +       return res->free == kunit_kmalloc_array_free && res->data == match_data;
> +}
> +
>  void kunit_kfree(struct kunit *test, const void *ptr)
>  {
>         struct kunit_resource *res;
>
> -       res = kunit_find_resource(test, kunit_resource_instance_match,
> -                                 (void *)ptr);
> +       res = kunit_find_resource(test, kunit_kfree_match, (void *)ptr);
>
>         /*
>          * Removing the resource from the list of resources drops the
> --
> 2.37.1.359.gd136c6c3e2-goog
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@...glegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20220721180214.3223778-3-dlatypov%40google.com.

Powered by blists - more mailing lists