2024-02-15 21:58:17

by Oscar Salvador

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

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 <[email protected]>
Signed-off-by: Marco Elver <[email protected]>
Signed-off-by: Oscar Salvador <[email protected]>
Acked-by: Vlastimil Babka <[email protected]>
---
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

2024-02-15 23:37:20

by Andrey Konovalov

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

On Thu, Feb 15, 2024 at 10:58 PM Oscar Salvador <[email protected]> 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 <[email protected]>
> Signed-off-by: Marco Elver <[email protected]>
> Signed-off-by: Oscar Salvador <[email protected]>
> Acked-by: Vlastimil Babka <[email protected]>
> ---
> 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 <[email protected]>

2024-02-16 00:26:51

by Peter Collingbourne

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

On Thu, Feb 15, 2024 at 3:37 PM Andrey Konovalov <[email protected]> wrote:
>
> On Thu, Feb 15, 2024 at 10:58 PM Oscar Salvador <[email protected]> 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 <[email protected]>
> > Signed-off-by: Marco Elver <[email protected]>
> > Signed-off-by: Oscar Salvador <[email protected]>
> > Acked-by: Vlastimil Babka <[email protected]>
> > ---
> > 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 */

Can we rename this, say to pool_index_plus_1? This will make the code
a bit clearer, as well as make it possible for debugging tools such as
drgn [1] to be able to tell when the off-by-one was introduced and
adapt accordingly.

Peter

[1] https://github.com/osandov/drgn/pull/376

2024-04-02 00:18:12

by Peter Collingbourne

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

On Thu, Feb 15, 2024 at 4:25 PM Peter Collingbourne <[email protected]> wrote:
>
> On Thu, Feb 15, 2024 at 3:37 PM Andrey Konovalov <[email protected]> wrote:
> >
> > On Thu, Feb 15, 2024 at 10:58 PM Oscar Salvador <osalvador@susede> 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 <[email protected]>
> > > Signed-off-by: Marco Elver <[email protected]>
> > > Signed-off-by: Oscar Salvador <[email protected]>
> > > Acked-by: Vlastimil Babka <[email protected]>
> > > ---
> > > 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 */
>
> Can we rename this, say to pool_index_plus_1? This will make the code
> a bit clearer, as well as make it possible for debugging tools such as
> drgn [1] to be able to tell when the off-by-one was introduced and
> adapt accordingly.
>
> Peter
>
> [1] https://github.com/osandov/drgn/pull/376

Unfortunately this message was not acted upon, and it looks like akpm
picked up the patch and it made its way into Linus's tree. So I sent a
followup to fix this here:
https://lore.kernel.org/all/[email protected]/

Peter