From: Andrey Konovalov <[email protected]>
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 <[email protected]>
---
lib/stackdepot.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
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;
}
--
2.25.1
On Mon, 30 Jan 2023 21:49:25 +0100 [email protected] 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.
On Mon, Jan 30, 2023 at 9:49 PM <[email protected]> wrote:
>
> From: Andrey Konovalov <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Alexander Potapenko <[email protected]>
On Mon, Jan 30, 2023 at 9:49 PM <[email protected]> wrote:
>
> From: Andrey Konovalov <[email protected]>
>
> 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 <[email protected]>
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.
On Tue, Jan 31, 2023 at 10:30 AM Alexander Potapenko <[email protected]> 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.
On Tue, Jan 31, 2023 at 1:18 AM Andrew Morton <[email protected]> wrote:
>
> On Mon, 30 Jan 2023 21:49:25 +0100 [email protected] 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!
On Tue, Jan 31, 2023 at 8:00 PM Andrey Konovalov <[email protected]> wrote:
>
> On Tue, Jan 31, 2023 at 10:30 AM Alexander Potapenko <[email protected]> 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).