Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220721180214.3223778-1-dlatypov@google.com> <20220721180214.3223778-4-dlatypov@google.com>
In-Reply-To: <20220721180214.3223778-4-dlatypov@google.com>
From:   David Gow <davidgow@google.com>
Date:   Fri, 22 Jul 2022 15:35:14 +0800
Message-ID: <CABVgOSkHCDt4DPRTCpSgWUSa5_5adEoqwW1fNnXyev9fK1eiuQ@mail.gmail.com>
Subject: Re: [PATCH 4/4] kunit: make knuit_kfree() not segfault on invalid inputs
To:     Daniel Latypov <dlatypov@google.com>
Cc:     Brendan Higgins <brendanhiggins@google.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        KUnit Development <kunit-dev@googlegroups.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        Shuah Khan <skhan@linuxfoundation.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

(Nit: typo in the subject line "knuit_free" --> "kunit_free"
On Fri, Jul 22, 2022 at 2:02 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> kunit_kfree() can only work on data ("resources") allocated by KUnit.
>
> Currently for code like this,
> > void *ptr = kmalloc(4, GFP_KERNEL);
> > kunit_kfree(test, ptr);
> kunit_kfree() will segfault.
>
> It'll try and look up the kunit_resource associated with `ptr` and get a
> NULL back, but it won't check for this. This means we also segfault if
> you double-free.

Personally, I don't think the case of people calling kunit_kfree() on
pointers allocated with kmalloc() is too worrying, but I do think we
should error more gracefully in cases like double-frees (and maybe
handle kfree(NULL) situations).
>
> Change kunit_kfree() so it'll notice these invalid pointers and respond
> by failing the test.
>
> Implementation: kunit_destroy_resource() does what kunit_kfree() does,
> but is more generic and returns -ENOENT when it can't find the resource.
> Sadly, unlike just letting it crash, this means we don't get a stack
> trace. But kunit_kfree() is so infrequently used it shouldn't be hard to
> track down the bad callsite anyways.

One day we should look into printing stacktraces on failed
expectations... It could be spammy in some cases, but it'd be nice to
have the option for things like this.
>
> After this change, the above code gives:
> > # example_simple_test: EXPECTATION FAILED at lib/kunit/test.c:702
> > kunit_kfree: 00000000626ec200 already freed or not allocated by kunit
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---

Looks good to me: this is both more correct and so much simpler as a
function. I can live without the nitpicks fixed.

Reviewed-by: David Gow <davidgow@google.com>

Thanks!
-- David

>  lib/kunit/test.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 82019a78462e..c7ca87484968 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -698,18 +698,8 @@ static inline bool kunit_kfree_match(struct kunit *test,
>
>  void kunit_kfree(struct kunit *test, const void *ptr)
>  {
> -       struct kunit_resource *res;
> -
> -       res = kunit_find_resource(test, kunit_kfree_match, (void *)ptr);
> -
> -       /*
> -        * Removing the resource from the list of resources drops the
> -        * reference count to 1; the final put will trigger the free.
> -        */
> -       kunit_remove_resource(test, res);
> -
> -       kunit_put_resource(res);
> -
> +       if (kunit_destroy_resource(test, kunit_kfree_match, (void *)ptr))
> +               KUNIT_FAIL(test, "kunit_kfree: %px already freed or not allocated by kunit", ptr);

_Maybe_ we should no-op if ptr is NULL. I think it's legal for
free()/kfree(), and while I don't see much use of it for kunit tests,
maybe it'll save someone confusion down the road.

But I could live with it either way...

>  }
>  EXPORT_SYMBOL_GPL(kunit_kfree);
>
> --
> 2.37.1.359.gd136c6c3e2-goog
>