linux-kernel - Re: [PATCH v5 05/10] kunit: Don't use a managed alloc in is

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: <CABVgOSmgKOssPH7482hm5TECo5CqnUmCzLGq1aHrdKUQkC+tMQ@mail.gmail.com>
Date:   Fri, 25 Aug 2023 14:49:33 +0800
From:   David Gow <davidgow@...gle.com>
To:     Richard Fitzgerald <rf@...nsource.cirrus.com>
Cc:     brendan.higgins@...ux.dev, rmoar@...gle.com,
        linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com,
        linux-kernel@...r.kernel.org, patches@...nsource.cirrus.com
Subject: Re: [PATCH v5 05/10] kunit: Don't use a managed alloc in is_literal()

On Thu, 24 Aug 2023 at 22:31, Richard Fitzgerald
<rf@...nsource.cirrus.com> wrote:
>
> There is no need to use a test-managed alloc in is_literal().
> The function frees the temporary buffer before returning.
>
> This removes the only use of the test and gfp members of
> struct string_stream outside of the string_stream implementation.
>
> Signed-off-by: Richard Fitzgerald <rf@...nsource.cirrus.com>
> ---

This makes sense to me, particularly given how independent
string-stream otherwise is from the KUnit resource management bits.

The only possible downside is that the memory won't be cleaned up if
strncmp() crashes due to 'text' being somehow invalid. But given this
is really only even used with static data (generated by the assert
macros), and to fail on the strncmp and not the strlen() would require
some horrible race-condition-y madness, I don't think it's ever
reasonably possible to hit that case.

So, looks good.

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

Cheers,
-- David


>  lib/kunit/assert.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
> index 05a09652f5a1..dd1d633d0fe2 100644
> --- a/lib/kunit/assert.c
> +++ b/lib/kunit/assert.c
> @@ -89,8 +89,7 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
>  EXPORT_SYMBOL_GPL(kunit_ptr_not_err_assert_format);
>
>  /* Checks if `text` is a literal representing `value`, e.g. "5" and 5 */
> -static bool is_literal(struct kunit *test, const char *text, long long value,
> -                      gfp_t gfp)
> +static bool is_literal(const char *text, long long value)
>  {
>         char *buffer;
>         int len;
> @@ -100,14 +99,15 @@ static bool is_literal(struct kunit *test, const char *text, long long value,
>         if (strlen(text) != len)
>                 return false;
>
> -       buffer = kunit_kmalloc(test, len+1, gfp);
> +       buffer = kmalloc(len+1, GFP_KERNEL);
>         if (!buffer)
>                 return false;
>
>         snprintf(buffer, len+1, "%lld", value);
>         ret = strncmp(buffer, text, len) == 0;
>
> -       kunit_kfree(test, buffer);
> +       kfree(buffer);
> +
>         return ret;
>  }
>
> @@ -125,14 +125,12 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
>                           binary_assert->text->left_text,
>                           binary_assert->text->operation,
>                           binary_assert->text->right_text);
> -       if (!is_literal(stream->test, binary_assert->text->left_text,
> -                       binary_assert->left_value, stream->gfp))
> +       if (!is_literal(binary_assert->text->left_text, binary_assert->left_value))
>                 string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld (0x%llx)\n",
>                                   binary_assert->text->left_text,
>                                   binary_assert->left_value,
>                                   binary_assert->left_value);
> -       if (!is_literal(stream->test, binary_assert->text->right_text,
> -                       binary_assert->right_value, stream->gfp))
> +       if (!is_literal(binary_assert->text->right_text, binary_assert->right_value))
>                 string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld (0x%llx)",
>                                   binary_assert->text->right_text,
>                                   binary_assert->right_value,
> --
> 2.30.2
>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4003 bytes)