diff mbox series

[01/18] lib/stackdepot: fix setting next_slab_inited in init_stack_slab

Message ID 9fbb4d2bf9b2676a29b120980b5ffbda8e2304ee.1675111415.git.andreyknvl@google.com (mailing list archive)
State New
Headers show
Series lib/stackdepot: fixes and clean-ups | expand

Commit Message

andrey.konovalov@linux.dev Jan. 30, 2023, 8:49 p.m. UTC 
From: Andrey Konovalov <andreyknvl@google.com>

In commit 305e519ce48e ("lib/stackdepot.c: fix global out-of-bounds in
stack_slabs"), init_stack_slab was changed to only use preallocated
memory for the next slab if the slab number limit is not reached.
However, setting next_slab_inited was not moved together with updating
stack_slabs.

Set next_slab_inited only if the preallocated memory was used for the
next slab.

Fixes: 305e519ce48e ("lib/stackdepot.c: fix global out-of-bounds in stack_slabs")
Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 lib/stackdepot.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Andrew Morton Jan. 31, 2023, 12:18 a.m. UTC | #1 
On Mon, 30 Jan 2023 21:49:25 +0100 andrey.konovalov@linux.dev wrote:

> In commit 305e519ce48e ("lib/stackdepot.c: fix global out-of-bounds in
> stack_slabs"), init_stack_slab was changed to only use preallocated
> memory for the next slab if the slab number limit is not reached.
> However, setting next_slab_inited was not moved together with updating
> stack_slabs.
> 
> Set next_slab_inited only if the preallocated memory was used for the
> next slab.

Please provide a full description of the user-visible runtime effects
of the bug (always always).

I'll add the cc:stable (per your comments in the [0/N] cover letter),
but it's more reliable to add it to the changelog yourself.

As to when I upstream this: don't know - that depends on the
user-visible-effects thing.
Alexander Potapenko Jan. 31, 2023, 9:07 a.m. UTC | #2 
On Mon, Jan 30, 2023 at 9:49 PM <andrey.konovalov@linux.dev> wrote:
>
> From: Andrey Konovalov <andreyknvl@google.com>
>
> In commit 305e519ce48e ("lib/stackdepot.c: fix global out-of-bounds in
> stack_slabs"), init_stack_slab was changed to only use preallocated
> memory for the next slab if the slab number limit is not reached.
> However, setting next_slab_inited was not moved together with updating
> stack_slabs.
>
> Set next_slab_inited only if the preallocated memory was used for the
> next slab.
>
> Fixes: 305e519ce48e ("lib/stackdepot.c: fix global out-of-bounds in stack_slabs")
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
Reviewed-by: Alexander Potapenko <glider@google.com>
Alexander Potapenko Jan. 31, 2023, 9:29 a.m. UTC | #3 
On Mon, Jan 30, 2023 at 9:49 PM <andrey.konovalov@linux.dev> wrote:
>
> From: Andrey Konovalov <andreyknvl@google.com>
>
> In commit 305e519ce48e ("lib/stackdepot.c: fix global out-of-bounds in
> stack_slabs"), init_stack_slab was changed to only use preallocated
> memory for the next slab if the slab number limit is not reached.
> However, setting next_slab_inited was not moved together with updating
> stack_slabs.
>
> Set next_slab_inited only if the preallocated memory was used for the
> next slab.
>
> Fixes: 305e519ce48e ("lib/stackdepot.c: fix global out-of-bounds in stack_slabs")
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>

Wait, I think there's a problem here.

> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index 79e894cf8406..0eed9bbcf23e 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -105,12 +105,13 @@ static bool init_stack_slab(void **prealloc)
>                 if (depot_index + 1 < STACK_ALLOC_MAX_SLABS) {
If we get to this branch, but the condition is false, this means that:
 - next_slab_inited == 0
 - depot_index == STACK_ALLOC_MAX_SLABS+1
 - stack_slabs[depot_index] != NULL.

So stack_slabs[] is at full capacity, but upon leaving
init_stack_slab() we'll always keep next_slab_inited==0.

Now every time __stack_depot_save() is called for a known stack trace,
it will preallocate 1<<STACK_ALLOC_ORDER pages (because
next_slab_inited==0), then find the stack trace id in the hash, then
pass the preallocated pages to init_stack_slab(), which will not
change the value of next_slab_inited.
Then the preallocated pages will be freed, and next time
__stack_depot_save() is called they'll be allocated again.
Andrey Konovalov Jan. 31, 2023, 6:59 p.m. UTC | #4 
On Tue, Jan 31, 2023 at 10:30 AM Alexander Potapenko <glider@google.com> wrote:
>
> Wait, I think there's a problem here.
>
> > diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> > index 79e894cf8406..0eed9bbcf23e 100644
> > --- a/lib/stackdepot.c
> > +++ b/lib/stackdepot.c
> > @@ -105,12 +105,13 @@ static bool init_stack_slab(void **prealloc)
> >                 if (depot_index + 1 < STACK_ALLOC_MAX_SLABS) {
> If we get to this branch, but the condition is false, this means that:
>  - next_slab_inited == 0
>  - depot_index == STACK_ALLOC_MAX_SLABS+1
>  - stack_slabs[depot_index] != NULL.
>
> So stack_slabs[] is at full capacity, but upon leaving
> init_stack_slab() we'll always keep next_slab_inited==0.
>
> Now every time __stack_depot_save() is called for a known stack trace,
> it will preallocate 1<<STACK_ALLOC_ORDER pages (because
> next_slab_inited==0), then find the stack trace id in the hash, then
> pass the preallocated pages to init_stack_slab(), which will not
> change the value of next_slab_inited.
> Then the preallocated pages will be freed, and next time
> __stack_depot_save() is called they'll be allocated again.

Ah, right, missed that.

What do you think about renaming next_slab_inited to
next_slab_required and inverting the used values (0/1 -> 1/0)? This
would make this part of code less confusing.
Andrey Konovalov Jan. 31, 2023, 7 p.m. UTC | #5 
On Tue, Jan 31, 2023 at 1:18 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Mon, 30 Jan 2023 21:49:25 +0100 andrey.konovalov@linux.dev wrote:
>
> > In commit 305e519ce48e ("lib/stackdepot.c: fix global out-of-bounds in
> > stack_slabs"), init_stack_slab was changed to only use preallocated
> > memory for the next slab if the slab number limit is not reached.
> > However, setting next_slab_inited was not moved together with updating
> > stack_slabs.
> >
> > Set next_slab_inited only if the preallocated memory was used for the
> > next slab.
>
> Please provide a full description of the user-visible runtime effects
> of the bug (always always).
>
> I'll add the cc:stable (per your comments in the [0/N] cover letter),
> but it's more reliable to add it to the changelog yourself.

Right, will do this next time.

> As to when I upstream this: don't know - that depends on the
> user-visible-effects thing.

Looks like there's no bug to fix after all as per comments by Alexander.

Thanks!
Alexander Potapenko Feb. 1, 2023, 11:51 a.m. UTC | #6 
On Tue, Jan 31, 2023 at 8:00 PM Andrey Konovalov <andreyknvl@gmail.com> wrote:
>
> On Tue, Jan 31, 2023 at 10:30 AM Alexander Potapenko <glider@google.com> wrote:
> >
> > Wait, I think there's a problem here.
> >
> > > diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> > > index 79e894cf8406..0eed9bbcf23e 100644
> > > --- a/lib/stackdepot.c
> > > +++ b/lib/stackdepot.c
> > > @@ -105,12 +105,13 @@ static bool init_stack_slab(void **prealloc)
> > >                 if (depot_index + 1 < STACK_ALLOC_MAX_SLABS) {
> > If we get to this branch, but the condition is false, this means that:
> >  - next_slab_inited == 0
> >  - depot_index == STACK_ALLOC_MAX_SLABS+1
> >  - stack_slabs[depot_index] != NULL.
> >
> > So stack_slabs[] is at full capacity, but upon leaving
> > init_stack_slab() we'll always keep next_slab_inited==0.
> >
> > Now every time __stack_depot_save() is called for a known stack trace,
> > it will preallocate 1<<STACK_ALLOC_ORDER pages (because
> > next_slab_inited==0), then find the stack trace id in the hash, then
> > pass the preallocated pages to init_stack_slab(), which will not
> > change the value of next_slab_inited.
> > Then the preallocated pages will be freed, and next time
> > __stack_depot_save() is called they'll be allocated again.
>
> Ah, right, missed that.
>
> What do you think about renaming next_slab_inited to
> next_slab_required and inverting the used values (0/1 -> 1/0)? This
> would make this part of code less confusing.

"Required" as in "requires a preallocated buffer, but does not have one yet"?
Yes, that's probably better.
(In any case we'll need to add a comment to that variable explaining
the circumstances under which one or another value is possible).
diff mbox series

Patch

diff --git a/lib/stackdepot.c b/lib/stackdepot.c
index 79e894cf8406..0eed9bbcf23e 100644
--- a/lib/stackdepot.c
+++ b/lib/stackdepot.c
@@ -105,12 +105,13 @@  static bool init_stack_slab(void **prealloc)
 		if (depot_index + 1 < STACK_ALLOC_MAX_SLABS) {
 			stack_slabs[depot_index + 1] = *prealloc;
 			*prealloc = NULL;
+			/*
+			 * This smp_store_release pairs with smp_load_acquire()
+			 * from |next_slab_inited| above and in
+			 * stack_depot_save().
+			 */
+			smp_store_release(&next_slab_inited, 1);
 		}
-		/*
-		 * This smp_store_release pairs with smp_load_acquire() from
-		 * |next_slab_inited| above and in stack_depot_save().
-		 */
-		smp_store_release(&next_slab_inited, 1);
 	}
 	return true;
 }