Received-SPF: pass (google.com: domain of linux-kernel+bounces-67836-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240215215907.20121-1-osalvador@suse.de> <20240215215907.20121-2-osalvador@suse.de>
In-Reply-To: <20240215215907.20121-2-osalvador@suse.de>
From: Andrey Konovalov <andreyknvl@gmail.com>
Date: Fri, 16 Feb 2024 00:36:57 +0100
Message-ID: <CA+fCnZddd82=Gp2j4sdks+NGpn-GSvZq8isYOwXDO=Y3TyBG1g@mail.gmail.com>
Subject: Re: [PATCH v10 1/7] lib/stackdepot: Fix first entry having a 0-handle
To: Oscar Salvador <osalvador@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>, linux-kernel@vger.kernel.org, 
	linux-mm@kvack.org, Michal Hocko <mhocko@suse.com>, Vlastimil Babka <vbabka@suse.cz>, 
	Marco Elver <elver@google.com>, Alexander Potapenko <glider@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 15, 2024 at 10:58=E2=80=AFPM Oscar Salvador <osalvador@suse.de>=
 wrote:
>
> The very first entry of stack_record gets a handle of 0, but this is wron=
g
> 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 h=
andle. */
>  #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 =3D current_pool + pool_offset;
>
>         /* Pre-initialize handle once. */
> -       stack->handle.pool_index =3D pool_index;
> +       stack->handle.pool_index =3D pool_index + 1;
>         stack->handle.offset =3D pool_offset >> DEPOT_STACK_ALIGN;
>         stack->handle.extra =3D 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 =3D READ_ONCE(pools_num);
>         union handle_parts parts =3D { .handle =3D handle };
>         void *pool;
> +       u32 pool_index =3D parts.pool_index - 1;
>         size_t offset =3D 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 %0=
8x\n",
> -                    parts.pool_index, pools_num_cached, handle);
> +                    pool_index, pools_num_cached, handle);
>                 return NULL;
>         }
>
> -       pool =3D stack_pools[parts.pool_index];
> +       pool =3D stack_pools[pool_index];
>         if (WARN_ON(!pool))
>                 return NULL;
>
> --
> 2.43.0
>

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