Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
MIME-Version: 1.0
References: <e237a31ef7ca6213c46f87e4609bd7d3eb48fedf.1698351974.git.andreyknvl@google.com>
In-Reply-To: <e237a31ef7ca6213c46f87e4609bd7d3eb48fedf.1698351974.git.andreyknvl@google.com>
From:   Marco Elver <elver@google.com>
Date:   Fri, 27 Oct 2023 14:53:57 +0200
Message-ID: <CANpmjNOrKpkV3aEPsTZSuL6Nb7R5NyiBh84xkbxM-802nzDtBg@mail.gmail.com>
Subject: Re: [PATCH 1/1] lib/stackdepot: print disabled message only if truly disabled
To:     andrey.konovalov@linux.dev
Cc:     Alexander Potapenko <glider@google.com>,
        Andrey Konovalov <andreyknvl@gmail.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        Vlastimil Babka <vbabka@suse.cz>, kasan-dev@googlegroups.com,
        Evgenii Stepanov <eugenis@google.com>,
        Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Andrey Konovalov <andreyknvl@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, 26 Oct 2023 at 22:28, <andrey.konovalov@linux.dev> wrote:
>
> From: Andrey Konovalov <andreyknvl@google.com>
>
> Currently, if stack_depot_disable=off is passed to the kernel
> command-line after stack_depot_disable=on, stack depot prints a message
> that it is disabled, while it is actually enabled.
>
> Fix this by moving printing the disabled message to
> stack_depot_early_init. Place it before the
> __stack_depot_early_init_requested check, so that the message is printed
> even if early stack depot init has not been requested.
>
> Also drop the stack_table = NULL assignment from disable_stack_depot,
> as stack_table is NULL by default.
>
> Fixes: e1fdc403349c ("lib: stackdepot: add support to disable stack depot")
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>

Reviewed-by: Marco Elver <elver@google.com>

for the change here, but there's a way to make it simpler (see below).

> ---
>  lib/stackdepot.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index 2f5aa851834e..0eeaef4f2523 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -101,14 +101,7 @@ static int next_pool_required = 1;
>
>  static int __init disable_stack_depot(char *str)
>  {
> -       int ret;
> -
> -       ret = kstrtobool(str, &stack_depot_disabled);
> -       if (!ret && stack_depot_disabled) {
> -               pr_info("disabled\n");
> -               stack_table = NULL;
> -       }
> -       return 0;
> +       return kstrtobool(str, &stack_depot_disabled);
>  }
>  early_param("stack_depot_disable", disable_stack_depot);
>
> @@ -130,6 +123,15 @@ int __init stack_depot_early_init(void)
>                 return 0;
>         __stack_depot_early_init_passed = true;
>
> +       /*
> +        * Print disabled message even if early init has not been requested:
> +        * stack_depot_init() will not print one.
> +        */
> +       if (stack_depot_disabled) {
> +               pr_info("disabled\n");
> +               return 0;
> +       }
> +
>         /*
>          * If KASAN is enabled, use the maximum order: KASAN is frequently used
>          * in fuzzing scenarios, which leads to a large number of different
> @@ -138,7 +140,11 @@ int __init stack_depot_early_init(void)
>         if (kasan_enabled() && !stack_bucket_number_order)
>                 stack_bucket_number_order = STACK_BUCKET_NUMBER_ORDER_MAX;

stack_bucket_number_order seems like a redundant variable, that should
at least be __ro_after_init. All code that does "if
(stack_bucket_number_order) ..." could just do "if (kasan_enabled())
..." and use STACK_BUCKET_NUMBER_ORDER_MAX constant directly instead.

The code here could be simplified if it was removed. No idea why it
was introduced in the first place. I think f9987921cb541 introduced it
and there it said "complemented with a boot-time kernel parameter",
but that never happened.

So I'd be in favor of removing that variable, which will also simplify
this code.