From: Andrey Konovalov <[email protected]>
A few fixes for the stack depot eviction series.
Changes v1->v2:
- Add Fixes tags.
- Use per-object spinlock for protecting aux stack handles instead of a
global one.
Andrey Konovalov (4):
lib/stackdepot: add printk_deferred_enter/exit guards
kasan: handle concurrent kasan_record_aux_stack calls
kasan: memset free track in qlink_free
lib/stackdepot: fix comment in include/linux/stackdepot.h
include/linux/stackdepot.h | 2 --
lib/stackdepot.c | 9 +++++++++
mm/kasan/generic.c | 32 +++++++++++++++++++++++++++++---
mm/kasan/kasan.h | 2 ++
mm/kasan/quarantine.c | 2 +-
5 files changed, 41 insertions(+), 6 deletions(-)
--
2.25.1
From: Andrey Konovalov <[email protected]>
As stack traces can now be evicted from the stack depot, remove the
comment saying that they are never removed.
Fixes: 108be8def46e ("lib/stackdepot: allow users to evict stack traces")
Reviewed-by: Marco Elver <[email protected]>
Signed-off-by: Andrey Konovalov <[email protected]>
---
include/linux/stackdepot.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
index a6796f178913..adcbb8f23600 100644
--- a/include/linux/stackdepot.h
+++ b/include/linux/stackdepot.h
@@ -11,8 +11,6 @@
* SLUB_DEBUG needs 256 bytes per object for that). Since allocation and free
* stack traces often repeat, using stack depot allows to save about 100x space.
*
- * Stack traces are never removed from the stack depot.
- *
* Author: Alexander Potapenko <[email protected]>
* Copyright (C) 2016 Google, Inc.
*
--
2.25.1
From: Andrey Konovalov <[email protected]>
Stack depot functions can be called from various contexts that do
allocations, including with console locks taken. At the same time, stack
depot functions might print WARNING's or refcount-related failures.
This can cause a deadlock on console locks.
Add printk_deferred_enter/exit guards to stack depot to avoid this.
Reported-by: Tetsuo Handa <[email protected]>
Closes: https://lore.kernel.org/all/[email protected]/
Fixes: 108be8def46e ("lib/stackdepot: allow users to evict stack traces")
Fixes: cd11016e5f52 ("mm, kasan: stackdepot implementation. Enable stackdepot for SLAB")
Reviewed-by: Marco Elver <[email protected]>
Signed-off-by: Andrey Konovalov <[email protected]>
---
lib/stackdepot.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/lib/stackdepot.c b/lib/stackdepot.c
index 870cce2f4cbd..a0be5d05c7f0 100644
--- a/lib/stackdepot.c
+++ b/lib/stackdepot.c
@@ -506,12 +506,14 @@ depot_stack_handle_t stack_depot_save_flags(unsigned long *entries,
bucket = &stack_table[hash & stack_hash_mask];
read_lock_irqsave(&pool_rwlock, flags);
+ printk_deferred_enter();
/* Fast path: look the stack trace up without full locking. */
found = find_stack(bucket, entries, nr_entries, hash);
if (found) {
if (depot_flags & STACK_DEPOT_FLAG_GET)
refcount_inc(&found->count);
+ printk_deferred_exit();
read_unlock_irqrestore(&pool_rwlock, flags);
goto exit;
}
@@ -520,6 +522,7 @@ depot_stack_handle_t stack_depot_save_flags(unsigned long *entries,
if (new_pool_required)
need_alloc = true;
+ printk_deferred_exit();
read_unlock_irqrestore(&pool_rwlock, flags);
/*
@@ -541,6 +544,7 @@ depot_stack_handle_t stack_depot_save_flags(unsigned long *entries,
}
write_lock_irqsave(&pool_rwlock, flags);
+ printk_deferred_enter();
found = find_stack(bucket, entries, nr_entries, hash);
if (!found) {
@@ -562,6 +566,7 @@ depot_stack_handle_t stack_depot_save_flags(unsigned long *entries,
depot_keep_new_pool(&prealloc);
}
+ printk_deferred_exit();
write_unlock_irqrestore(&pool_rwlock, flags);
exit:
if (prealloc) {
@@ -600,9 +605,11 @@ unsigned int stack_depot_fetch(depot_stack_handle_t handle,
return 0;
read_lock_irqsave(&pool_rwlock, flags);
+ printk_deferred_enter();
stack = depot_fetch_stack(handle);
+ printk_deferred_exit();
read_unlock_irqrestore(&pool_rwlock, flags);
*entries = stack->entries;
@@ -619,6 +626,7 @@ void stack_depot_put(depot_stack_handle_t handle)
return;
write_lock_irqsave(&pool_rwlock, flags);
+ printk_deferred_enter();
stack = depot_fetch_stack(handle);
if (WARN_ON(!stack))
@@ -633,6 +641,7 @@ void stack_depot_put(depot_stack_handle_t handle)
}
out:
+ printk_deferred_exit();
write_unlock_irqrestore(&pool_rwlock, flags);
}
EXPORT_SYMBOL_GPL(stack_depot_put);
--
2.25.1
From: Andrey Konovalov <[email protected]>
kasan_record_aux_stack can be called concurrently on the same object.
This might lead to a race condition when rotating the saved aux stack
trace handles, which in turns leads to incorrect accounting of stack
depot handles and refcount underflows in the stack depot code.
Fix by introducing a spinlock to protect the aux stack trace handles
in kasan_record_aux_stack.
Reported-by: Tetsuo Handa <[email protected]>
Reported-by: [email protected]
Closes: https://lore.kernel.org/all/[email protected]/
Fixes: 773688a6cb24 ("kasan: use stack_depot_put for Generic mode")
Signed-off-by: Andrey Konovalov <[email protected]>
---
Changes v1->v2:
- Use per-object spinlock instead of a global one.
---
mm/kasan/generic.c | 32 +++++++++++++++++++++++++++++---
mm/kasan/kasan.h | 2 ++
2 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
index 54e20b2bc3e1..b9d41d6c70fd 100644
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -25,6 +25,7 @@
#include <linux/sched.h>
#include <linux/sched/task_stack.h>
#include <linux/slab.h>
+#include <linux/spinlock.h>
#include <linux/stackdepot.h>
#include <linux/stacktrace.h>
#include <linux/string.h>
@@ -471,8 +472,18 @@ void kasan_init_object_meta(struct kmem_cache *cache, const void *object)
struct kasan_free_meta *free_meta;
alloc_meta = kasan_get_alloc_meta(cache, object);
- if (alloc_meta)
+ if (alloc_meta) {
__memset(alloc_meta, 0, sizeof(*alloc_meta));
+
+ /*
+ * Temporarily disable KASAN bug reporting to allow instrumented
+ * spin_lock_init to access aux_lock, which resides inside of a
+ * redzone.
+ */
+ kasan_disable_current();
+ spin_lock_init(&alloc_meta->aux_lock);
+ kasan_enable_current();
+ }
free_meta = kasan_get_free_meta(cache, object);
if (free_meta)
__memset(free_meta, 0, sizeof(*free_meta));
@@ -502,6 +513,8 @@ static void __kasan_record_aux_stack(void *addr, depot_flags_t depot_flags)
struct kmem_cache *cache;
struct kasan_alloc_meta *alloc_meta;
void *object;
+ depot_stack_handle_t new_handle, old_handle;
+ unsigned long flags;
if (is_kfence_address(addr) || !slab)
return;
@@ -512,9 +525,22 @@ static void __kasan_record_aux_stack(void *addr, depot_flags_t depot_flags)
if (!alloc_meta)
return;
- stack_depot_put(alloc_meta->aux_stack[1]);
+ new_handle = kasan_save_stack(0, depot_flags);
+
+ /*
+ * Temporarily disable KASAN bug reporting to allow instrumented
+ * spinlock functions to access aux_lock, which resides inside of a
+ * redzone.
+ */
+ kasan_disable_current();
+ spin_lock_irqsave(&alloc_meta->aux_lock, flags);
+ old_handle = alloc_meta->aux_stack[1];
alloc_meta->aux_stack[1] = alloc_meta->aux_stack[0];
- alloc_meta->aux_stack[0] = kasan_save_stack(0, depot_flags);
+ alloc_meta->aux_stack[0] = new_handle;
+ spin_unlock_irqrestore(&alloc_meta->aux_lock, flags);
+ kasan_enable_current();
+
+ stack_depot_put(old_handle);
}
void kasan_record_aux_stack(void *addr)
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 5e298e3ac909..8b4125fecdc7 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -6,6 +6,7 @@
#include <linux/kasan.h>
#include <linux/kasan-tags.h>
#include <linux/kfence.h>
+#include <linux/spinlock.h>
#include <linux/stackdepot.h>
#if defined(CONFIG_KASAN_SW_TAGS) || defined(CONFIG_KASAN_HW_TAGS)
@@ -249,6 +250,7 @@ struct kasan_global {
struct kasan_alloc_meta {
struct kasan_track alloc_track;
/* Free track is stored in kasan_free_meta. */
+ spinlock_t aux_lock;
depot_stack_handle_t aux_stack[2];
};
--
2.25.1
From: Andrey Konovalov <[email protected]>
Instead of only zeroing out the stack depot handle when evicting the
free stack trace in qlink_free, zero out the whole track.
Do this just to produce a similar effect for alloc and free meta. The
other fields of the free track besides the stack trace handle are
considered invalid at this point anyway, so no harm in zeroing them out.
Fixes: 773688a6cb24 ("kasan: use stack_depot_put for Generic mode")
Reviewed-by: Marco Elver <[email protected]>
Signed-off-by: Andrey Konovalov <[email protected]>
---
mm/kasan/quarantine.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 265ca2bbe2dd..782e045da911 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -157,7 +157,7 @@ static void qlink_free(struct qlist_node *qlink, struct kmem_cache *cache)
if (free_meta &&
*(u8 *)kasan_mem_to_shadow(object) == KASAN_SLAB_FREETRACK) {
stack_depot_put(free_meta->free_track.stack);
- free_meta->free_track.stack = 0;
+ __memset(&free_meta->free_track, 0, sizeof(free_meta->free_track));
}
/*
--
2.25.1
On Thu, 14 Dec 2023 at 01:48, <[email protected]> wrote:
>
> From: Andrey Konovalov <[email protected]>
>
> kasan_record_aux_stack can be called concurrently on the same object.
> This might lead to a race condition when rotating the saved aux stack
> trace handles, which in turns leads to incorrect accounting of stack
> depot handles and refcount underflows in the stack depot code.
>
> Fix by introducing a spinlock to protect the aux stack trace handles
> in kasan_record_aux_stack.
>
> Reported-by: Tetsuo Handa <[email protected]>
> Reported-by: [email protected]
> Closes: https://lore.kernel.org/all/[email protected]/
> Fixes: 773688a6cb24 ("kasan: use stack_depot_put for Generic mode")
> Signed-off-by: Andrey Konovalov <[email protected]>
>
> ---
>
> Changes v1->v2:
> - Use per-object spinlock instead of a global one.
> ---
> mm/kasan/generic.c | 32 +++++++++++++++++++++++++++++---
> mm/kasan/kasan.h | 2 ++
> 2 files changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> index 54e20b2bc3e1..b9d41d6c70fd 100644
> --- a/mm/kasan/generic.c
> +++ b/mm/kasan/generic.c
> @@ -25,6 +25,7 @@
> #include <linux/sched.h>
> #include <linux/sched/task_stack.h>
> #include <linux/slab.h>
> +#include <linux/spinlock.h>
> #include <linux/stackdepot.h>
> #include <linux/stacktrace.h>
> #include <linux/string.h>
> @@ -471,8 +472,18 @@ void kasan_init_object_meta(struct kmem_cache *cache, const void *object)
> struct kasan_free_meta *free_meta;
>
> alloc_meta = kasan_get_alloc_meta(cache, object);
> - if (alloc_meta)
> + if (alloc_meta) {
> __memset(alloc_meta, 0, sizeof(*alloc_meta));
> +
> + /*
> + * Temporarily disable KASAN bug reporting to allow instrumented
> + * spin_lock_init to access aux_lock, which resides inside of a
> + * redzone.
> + */
> + kasan_disable_current();
> + spin_lock_init(&alloc_meta->aux_lock);
> + kasan_enable_current();
> + }
> free_meta = kasan_get_free_meta(cache, object);
> if (free_meta)
> __memset(free_meta, 0, sizeof(*free_meta));
> @@ -502,6 +513,8 @@ static void __kasan_record_aux_stack(void *addr, depot_flags_t depot_flags)
> struct kmem_cache *cache;
> struct kasan_alloc_meta *alloc_meta;
> void *object;
> + depot_stack_handle_t new_handle, old_handle;
> + unsigned long flags;
>
> if (is_kfence_address(addr) || !slab)
> return;
> @@ -512,9 +525,22 @@ static void __kasan_record_aux_stack(void *addr, depot_flags_t depot_flags)
> if (!alloc_meta)
> return;
>
> - stack_depot_put(alloc_meta->aux_stack[1]);
> + new_handle = kasan_save_stack(0, depot_flags);
> +
> + /*
> + * Temporarily disable KASAN bug reporting to allow instrumented
> + * spinlock functions to access aux_lock, which resides inside of a
> + * redzone.
> + */
> + kasan_disable_current();
> + spin_lock_irqsave(&alloc_meta->aux_lock, flags);
> + old_handle = alloc_meta->aux_stack[1];
> alloc_meta->aux_stack[1] = alloc_meta->aux_stack[0];
> - alloc_meta->aux_stack[0] = kasan_save_stack(0, depot_flags);
> + alloc_meta->aux_stack[0] = new_handle;
> + spin_unlock_irqrestore(&alloc_meta->aux_lock, flags);
> + kasan_enable_current();
> +
> + stack_depot_put(old_handle);
> }
>
> void kasan_record_aux_stack(void *addr)
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 5e298e3ac909..8b4125fecdc7 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -6,6 +6,7 @@
> #include <linux/kasan.h>
> #include <linux/kasan-tags.h>
> #include <linux/kfence.h>
> +#include <linux/spinlock.h>
> #include <linux/stackdepot.h>
>
> #if defined(CONFIG_KASAN_SW_TAGS) || defined(CONFIG_KASAN_HW_TAGS)
> @@ -249,6 +250,7 @@ struct kasan_global {
> struct kasan_alloc_meta {
> struct kasan_track alloc_track;
> /* Free track is stored in kasan_free_meta. */
> + spinlock_t aux_lock;
This needs to be raw_spinlock, because
kasan_record_aux_stack_noalloc() can be called from non-sleepable
contexts (otherwise lockdep will complain for RT kernels).
On Thu, Dec 14, 2023 at 9:35 AM Marco Elver <[email protected]> wrote:
>
> > #if defined(CONFIG_KASAN_SW_TAGS) || defined(CONFIG_KASAN_HW_TAGS)
> > @@ -249,6 +250,7 @@ struct kasan_global {
> > struct kasan_alloc_meta {
> > struct kasan_track alloc_track;
> > /* Free track is stored in kasan_free_meta. */
> > + spinlock_t aux_lock;
>
> This needs to be raw_spinlock, because
> kasan_record_aux_stack_noalloc() can be called from non-sleepable
> contexts (otherwise lockdep will complain for RT kernels).
Right, will fix in v3. Thank you!