| Message ID | 20240215215907.20121-2-osalvador@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | page_owner: print stacks and their outstanding allocations | expand |
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>
On Thu, Feb 15, 2024 at 3:37 PM Andrey Konovalov <andreyknvl@gmail.com> wrote: > > 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 */ 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
On Thu, Feb 15, 2024 at 4:25 PM Peter Collingbourne <pcc@google.com> wrote: > > On Thu, Feb 15, 2024 at 3:37 PM Andrey Konovalov <andreyknvl@gmail.com> wrote: > > > > 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 */ > > 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/20240402001500.53533-1-pcc@google.com/ Peter
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;