linux-kernel - Re: [PATCH v3 kunit-next 1/2] kunit: generalize kunit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFd5g44-eDSDQqt+ZR-CibEjgZ3x6RzYf6WFQ7t088Bf2c8qRg@mail.gmail.com>
Date:   Thu, 28 May 2020 12:40:01 -0700
From:   Brendan Higgins <brendanhiggins@...gle.com>
To:     Alan Maguire <alan.maguire@...cle.com>
Cc:     shuah <shuah@...nel.org>,
        Patricia Alfonso <trishalfonso@...gle.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        KUnit Development <kunit-dev@...glegroups.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 kunit-next 1/2] kunit: generalize kunit_resource API
 beyond allocated resources

On Fri, Mar 27, 2020 at 5:45 AM Alan Maguire <alan.maguire@...cle.com> wrote:
>
> In its original form, the kunit resources API - consisting the
> struct kunit_resource and associated functions - was focused on
> adding allocated resources during test operation that would be
> automatically cleaned up on test completion.
>
> The recent RFC patch proposing converting KASAN tests to KUnit [1]
> showed another potential model - where outside of test context,
> but with a pointer to the test state, we wish to access/update
> test-related data, but expressly want to avoid allocations.
>
> It turns out we can generalize the kunit_resource to support
> static resources where the struct kunit_resource * is passed
> in and initialized for us. As part of this work, we also
> change the "allocation" field to the more general "data" name,
> as instead of associating an allocation, we can associate a
> pointer to static data.  Static data is distinguished by a NULL
> free functions.  A test is added to cover using kunit_add_resource()
> with a static resource and data.
>
> Finally we also make use of the kernel's krefcount interfaces
> to manage reference counting of KUnit resources.  The motivation
> for this is simple; if we have kernel threads accessing and
> using resources (say via kunit_find_resource()) we need to
> ensure we do not remove said resources (or indeed free them
> if they were dynamically allocated) until the reference count
> reaches zero.  A new function - kunit_put_resource() - is
> added to handle this, and it should be called after a
> thread using kunit_find_resource() is finished with the
> retrieved resource.
>
> We ensure that the functions needed to look up, use and
> drop reference count are "static inline"-defined so that
> they can be used by builtin code as well as modules in
> the case that KUnit is built as a module.
>
> A cosmetic change here also; I've tried moving to
> kunit_[action]_resource() as the format of function names
> for consistency and readability.
>
> [1] https://lkml.org/lkml/2020/2/26/1286
>
> Signed-off-by: Alan Maguire <alan.maguire@...cle.com>

One comment below, other than that:

Reviewed-by: Brendan Higgins <brendanhiggins@...gle.com>

Sorry for not keeping up with this. I forgot that I didn't give this a
reviewed-by.

> ---
>  include/kunit/test.h      | 157 +++++++++++++++++++++++++++++++++++++---------
>  lib/kunit/kunit-test.c    |  74 ++++++++++++++++------
>  lib/kunit/string-stream.c |  14 ++---
>  lib/kunit/test.c          | 154 ++++++++++++++++++++++++---------------------
>  4 files changed, 270 insertions(+), 129 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 9b0c46a..8c7f3ff 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -52,30 +59,27 @@
>   *
>   *     static void kunit_kmalloc_free(struct kunit_resource *res)
>   *     {
> - *             kfree(res->allocation);
> + *             kfree(res->data);
>   *     }
>   *
>   *     void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp)
>   *     {
>   *             struct kunit_kmalloc_params params;
> - *             struct kunit_resource *res;
>   *
>   *             params.size = size;
>   *             params.gfp = gfp;
>   *
> - *             res = kunit_alloc_resource(test, kunit_kmalloc_init,
> + *             return kunit_alloc_resource(test, kunit_kmalloc_init,
>   *                     kunit_kmalloc_free, &params);
> - *             if (res)
> - *                     return res->allocation;
> - *
> - *             return NULL;
>   *     }
>   */
>  struct kunit_resource {
> -       void *allocation;
> -       kunit_resource_free_t free;
> +       void *data;
>
>         /* private: internal use only. */
> +       kunit_resource_init_t init;

Apologies for bringing this up so late, but it looks like you never
addressed my comment from v1:

I don't think you use this `init` field anywhere; I only see it passed
as a parameter. Is there something obvious that I am missing?

> +       kunit_resource_free_t free;
> +       struct kref refcount;
>         struct list_head node;
>  };