Re: [PATCH v10 1/7] lib/stackdepot: Fix first entry having a 0-handle

[Andrey Konovalov <andreyknvl@gmail.com>]

On Thu, Feb 15, 2024 at 10:58 PM Oscar Salvador <osalvador@suse.de> wrote:
>
> The very first entry of stack_record gets a handle of 0, but this is wrong
> because stackdepot treats a 0-handle as a non-valid one.
> E.g: See the check in stack_depot_fetch()
>
> Fix this by adding and offset of 1.
>
> This bug has been lurking since the very beginning of stackdepot,
> but no one really cared as it seems.
> Because of that I am not adding a Fixes tag.
>
> Co-developed-by: Marco Elver <elver@google.com>
> Signed-off-by: Marco Elver <elver@google.com>
> Signed-off-by: Oscar Salvador <osalvador@suse.de>
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  lib/stackdepot.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index 4a7055a63d9f..c043a4186bc5 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -45,15 +45,16 @@
>  #define DEPOT_POOL_INDEX_BITS (DEPOT_HANDLE_BITS - DEPOT_OFFSET_BITS - \
>                                STACK_DEPOT_EXTRA_BITS)
>  #define DEPOT_POOLS_CAP 8192
> +/* The pool_index is offset by 1 so the first record does not have a 0 handle. */
>  #define DEPOT_MAX_POOLS \
> -       (((1LL << (DEPOT_POOL_INDEX_BITS)) < DEPOT_POOLS_CAP) ? \
> -        (1LL << (DEPOT_POOL_INDEX_BITS)) : DEPOT_POOLS_CAP)
> +       (((1LL << (DEPOT_POOL_INDEX_BITS)) - 1 < DEPOT_POOLS_CAP) ? \
> +        (1LL << (DEPOT_POOL_INDEX_BITS)) - 1 : DEPOT_POOLS_CAP)
>
>  /* Compact structure that stores a reference to a stack. */
>  union handle_parts {
>         depot_stack_handle_t handle;
>         struct {
> -               u32 pool_index  : DEPOT_POOL_INDEX_BITS;
> +               u32 pool_index  : DEPOT_POOL_INDEX_BITS; /* pool_index is offset by 1 */
>                 u32 offset      : DEPOT_OFFSET_BITS;
>                 u32 extra       : STACK_DEPOT_EXTRA_BITS;
>         };
> @@ -372,7 +373,7 @@ static struct stack_record *depot_pop_free_pool(void **prealloc, size_t size)
>         stack = current_pool + pool_offset;
>
>         /* Pre-initialize handle once. */
> -       stack->handle.pool_index = pool_index;
> +       stack->handle.pool_index = pool_index + 1;
>         stack->handle.offset = pool_offset >> DEPOT_STACK_ALIGN;
>         stack->handle.extra = 0;
>         INIT_LIST_HEAD(&stack->hash_list);
> @@ -483,18 +484,19 @@ static struct stack_record *depot_fetch_stack(depot_stack_handle_t handle)
>         const int pools_num_cached = READ_ONCE(pools_num);
>         union handle_parts parts = { .handle = handle };
>         void *pool;
> +       u32 pool_index = parts.pool_index - 1;
>         size_t offset = parts.offset << DEPOT_STACK_ALIGN;
>         struct stack_record *stack;
>
>         lockdep_assert_not_held(&pool_lock);
>
> -       if (parts.pool_index > pools_num_cached) {
> +       if (pool_index > pools_num_cached) {
>                 WARN(1, "pool index %d out of bounds (%d) for stack id %08x\n",
> -                    parts.pool_index, pools_num_cached, handle);
> +                    pool_index, pools_num_cached, handle);
>                 return NULL;
>         }
>
> -       pool = stack_pools[parts.pool_index];
> +       pool = stack_pools[pool_index];
>         if (WARN_ON(!pool))
>                 return NULL;
>
> --
> 2.43.0
>

Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>