linux-kernel - Re: [PATCH] kunit: Rework 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] [day] [month] [year] [list]

Date:   Wed, 23 Mar 2022 14:20:37 -0500
From:   Daniel Latypov <dlatypov@...gle.com>
To:     Brendan Higgins <brendanhiggins@...gle.com>
Cc:     David Gow <davidgow@...gle.com>,
        Shuah Khan <skhan@...uxfoundation.org>,
        KUnit Development <kunit-dev@...glegroups.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] kunit: Rework kunit_resource allocation policy

On Wed, Mar 23, 2022 at 2:22 AM Brendan Higgins
<brendanhiggins@...gle.com> wrote:
>

<snip>

>
> > The latter ~never need to get "found" (e.g. kunit_kmalloc() users).
> > The one exception: when people use kunit_kfree() to free things early,
> > which requires us to "find" these resources we otherwise wouldn't care
> > about.
> >
> > So I don't know how we can split the API unless we get rid of kunit_kfree().
> > Its presence means kunit_kmalloc() and friends need refcounting.
>
> Do we need to choose between dropping kunit_kfree() and refcounting? I
> think this is semantically different from other findable resources,
> and I think it fairly obviously entails the complexity of using it.

Yes, they're different.
We could do something different and just have a atomic bool "is_freed"
for the kunit_kmalloc() style resources.

But is it worth it?

Currently kunit_kfree() is defined as
697:void kunit_kfree(struct kunit *test, const void *ptr)
698-{
699-    struct kunit_resource *res;
700-
701-    res = kunit_find_resource(test, kunit_resource_instance_match,
702-                              (void *)ptr);
703-
704-    /*
705-     * Removing the resource from the list of resources drops the
706-     * reference count to 1; the final put will trigger the free.
707-     */
708-    kunit_remove_resource(test, res);
709-
710-    kunit_put_resource(res);
711-
712-}

i.e. the overhead of using a refcount is that we need to call
kunit_put_resource() bc we called kunit_find_resource().
IMO, this less semantic overhead than adding a different mechanism
specifically for kunit_kfree().

Tangent:
Huh, it segfaults if you call kunit_kfree() on a non-kunit allocated ptr.
res == NULL on 701 in that case, but kunit_remove_resource() doesn't
guard against that.
It also happens if you call kunit_free() twice.

That's analogous to how kfree() works, so I guess that's fine.
A difference though is
  kfree(NULL); // is fine
  kunit_free(test, NULL); // segfaults, res == NULL above

But thinking on it more, someone could register a resource w/ data == NULL.
I.e. a named resource which just acts as a flag via presence/absence.
kunit_kfree(test, NULL) would the most recent such resource though.

Should we do the trick/hack where we check the free function first in
kunit_kfree() to avoid such confusion?