From: Andrey Konovalov <[email protected]>
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 <[email protected]>
---
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;
- if (!__stack_depot_early_init_requested || stack_depot_disabled)
+ /*
+ * Check if early init has been requested after setting
+ * stack_bucket_number_order: stack_depot_init() uses its value.
+ */
+ if (!__stack_depot_early_init_requested)
return 0;
/*
--
2.25.1
On Thu, 26 Oct 2023 at 22:28, <[email protected]> wrote:
>
> From: Andrey Konovalov <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Marco Elver <[email protected]>
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.
On Fri, Oct 27, 2023 at 2:54 PM Marco Elver <[email protected]> wrote:
>
> 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.
On the first thought, this seems reasonable with the current code.
On the second though, I think I will soon add a command-line parameter
to allow controlling how much memory is used for the stack depot hash
table.
I propose we keep the code as is for now, but I've taken a note to
either drop this variable or mark it as __ro_after_init when
implementing memory bounding controls for stack depot.
Thanks!