debug_objects_mem_init() is invoked from mm_core_init() before work queues
are available. If debug_objects_mem_init() destroys the kmem cache in the
error path it causes an Oops in __queue_work():
Oops: Oops: 0000 [#1] PREEMPT SMP PTI
RIP: 0010:__queue_work+0x35/0x6a0
queue_work_on+0x66/0x70
flush_all_cpus_locked+0xdf/0x1a0
__kmem_cache_shutdown+0x2f/0x340
kmem_cache_destroy+0x4e/0x150
mm_core_init+0x9e/0x120
start_kernel+0x298/0x800
x86_64_start_reservations+0x18/0x30
x86_64_start_kernel+0xc5/0xe0
common_startup_64+0x12c/0x138
Further the object cache pointer is used in various places to check for
early boot operation. It is exposed before the replacments for the static
boot time objects are allocated and the self test operates on it.
This can be avoided by:
1) Running the self test with the static boot objects
2) Exposing it only after the replacement objects have been added to
the pool.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
lib/debugobjects.c | 68 +++++++++++++++++++++++++++--------------------------
1 file changed, 35 insertions(+), 33 deletions(-)
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -1211,7 +1211,7 @@ static __initconst const struct debug_ob
static __initdata struct self_test obj = { .static_init = 0 };
-static void __init debug_objects_selftest(void)
+static bool __init debug_objects_selftest(void)
{
int fixups, oldfixups, warnings, oldwarnings;
unsigned long flags;
@@ -1280,9 +1280,10 @@ static void __init debug_objects_selftes
descr_test = NULL;
local_irq_restore(flags);
+ return !!debug_objects_enabled;
}
#else
-static inline void debug_objects_selftest(void) { }
+static inline bool debug_objects_selftest(void) { return true; }
#endif
/*
@@ -1302,18 +1303,21 @@ void __init debug_objects_early_init(voi
}
/*
- * Convert the statically allocated objects to dynamic ones:
+ * Convert the statically allocated objects to dynamic ones.
+ * debug_objects_mem_init() is called early so only one CPU is up and
+ * interrupts are disabled, which means it is safe to replace the active
+ * object references.
*/
-static int __init debug_objects_replace_static_objects(void)
+static bool __init debug_objects_replace_static_objects(struct kmem_cache *cache)
{
struct debug_bucket *db = obj_hash;
- struct hlist_node *tmp;
struct debug_obj *obj, *new;
+ struct hlist_node *tmp;
HLIST_HEAD(objects);
int i, cnt = 0;
for (i = 0; i < ODEBUG_POOL_SIZE; i++) {
- obj = kmem_cache_zalloc(obj_cache, GFP_KERNEL);
+ obj = kmem_cache_zalloc(cache, GFP_KERNEL);
if (!obj)
goto free;
hlist_add_head(&obj->node, &objects);
@@ -1322,12 +1326,6 @@ static int __init debug_objects_replace_
debug_objects_allocated += i;
/*
- * debug_objects_mem_init() is now called early that only one CPU is up
- * and interrupts have been disabled, so it is safe to replace the
- * active object references.
- */
-
- /*
* Replace the statically allocated objects list with the allocated
* objects list.
*/
@@ -1347,15 +1345,14 @@ static int __init debug_objects_replace_
}
}
- pr_debug("%d of %d active objects replaced\n",
- cnt, obj_pool_used);
- return 0;
+ pr_debug("%d of %d active objects replaced\n", cnt, obj_pool_used);
+ return true;
free:
hlist_for_each_entry_safe(obj, tmp, &objects, node) {
hlist_del(&obj->node);
- kmem_cache_free(obj_cache, obj);
+ kmem_cache_free(cache, obj);
}
- return -ENOMEM;
+ return false;
}
/*
@@ -1366,6 +1363,7 @@ static int __init debug_objects_replace_
*/
void __init debug_objects_mem_init(void)
{
+ struct kmem_cache *cache;
int cpu, extras;
if (!debug_objects_enabled)
@@ -1380,29 +1378,33 @@ void __init debug_objects_mem_init(void)
for_each_possible_cpu(cpu)
INIT_HLIST_HEAD(&per_cpu(percpu_obj_pool.free_objs, cpu));
- obj_cache = kmem_cache_create("debug_objects_cache",
- sizeof (struct debug_obj), 0,
- SLAB_DEBUG_OBJECTS | SLAB_NOLEAKTRACE,
- NULL);
+ if (!debug_objects_selftest())
+ return;
+
+ cache = kmem_cache_create("debug_objects_cache", sizeof (struct debug_obj), 0,
+ SLAB_DEBUG_OBJECTS | SLAB_NOLEAKTRACE, NULL);
- if (!obj_cache || debug_objects_replace_static_objects()) {
+ if (!cache || !debug_objects_replace_static_objects(cache)) {
debug_objects_enabled = 0;
- kmem_cache_destroy(obj_cache);
- pr_warn("out of memory.\n");
+ pr_warn("Out of memory.\n");
return;
- } else
- debug_objects_selftest();
-
-#ifdef CONFIG_HOTPLUG_CPU
- cpuhp_setup_state_nocalls(CPUHP_DEBUG_OBJ_DEAD, "object:offline", NULL,
- object_cpu_offline);
-#endif
+ }
/*
- * Increase the thresholds for allocating and freeing objects
- * according to the number of possible CPUs available in the system.
+ * Adjust the thresholds for allocating and freeing objects
+ * according to the number of possible CPUs available in the
+ * system.
*/
extras = num_possible_cpus() * ODEBUG_BATCH_SIZE;
debug_objects_pool_size += extras;
debug_objects_pool_min_level += extras;
+
+ /* Everything worked. Expose the cache */
+ obj_cache = cache;
+
+#ifdef CONFIG_HOTPLUG_CPU
+ cpuhp_setup_state_nocalls(CPUHP_DEBUG_OBJ_DEAD, "object:offline", NULL,
+ object_cpu_offline);
+#endif
+ return;
}
On 2024/10/8 0:49, Thomas Gleixner wrote:
> debug_objects_mem_init() is invoked from mm_core_init() before work queues
> are available. If debug_objects_mem_init() destroys the kmem cache in the
> error path it causes an Oops in __queue_work():
>
> Oops: Oops: 0000 [#1] PREEMPT SMP PTI
> RIP: 0010:__queue_work+0x35/0x6a0
> queue_work_on+0x66/0x70
> flush_all_cpus_locked+0xdf/0x1a0
> __kmem_cache_shutdown+0x2f/0x340
> kmem_cache_destroy+0x4e/0x150
> mm_core_init+0x9e/0x120
> start_kernel+0x298/0x800
> x86_64_start_reservations+0x18/0x30
> x86_64_start_kernel+0xc5/0xe0
> common_startup_64+0x12c/0x138
>
> Further the object cache pointer is used in various places to check for
> early boot operation. It is exposed before the replacments for the static
> boot time objects are allocated and the self test operates on it.
>
> This can be avoided by:
>
> 1) Running the self test with the static boot objects
>
> 2) Exposing it only after the replacement objects have been added to
> the pool.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> lib/debugobjects.c | 68 +++++++++++++++++++++++++++--------------------------
> 1 file changed, 35 insertions(+), 33 deletions(-)
>
> --- a/lib/debugobjects.c
> +++ b/lib/debugobjects.c
> @@ -1211,7 +1211,7 @@ static __initconst const struct debug_ob
>
> static __initdata struct self_test obj = { .static_init = 0 };
>
> -static void __init debug_objects_selftest(void)
> +static bool __init debug_objects_selftest(void)
> {
> int fixups, oldfixups, warnings, oldwarnings;
> unsigned long flags;
> @@ -1280,9 +1280,10 @@ static void __init debug_objects_selftes
> descr_test = NULL;
>
> local_irq_restore(flags);
> + return !!debug_objects_enabled;
> }
> #else
> -static inline void debug_objects_selftest(void) { }
> +static inline bool debug_objects_selftest(void) { return true; }
> #endif
>
> /*
> @@ -1302,18 +1303,21 @@ void __init debug_objects_early_init(voi
> }
>
> /*
> - * Convert the statically allocated objects to dynamic ones:
> + * Convert the statically allocated objects to dynamic ones.
> + * debug_objects_mem_init() is called early so only one CPU is up and
> + * interrupts are disabled, which means it is safe to replace the active
> + * object references.
> */
> -static int __init debug_objects_replace_static_objects(void)
> +static bool __init debug_objects_replace_static_objects(struct kmem_cache *cache)
> {
> struct debug_bucket *db = obj_hash;
> - struct hlist_node *tmp;
> struct debug_obj *obj, *new;
> + struct hlist_node *tmp;
> HLIST_HEAD(objects);
> int i, cnt = 0;
>
> for (i = 0; i < ODEBUG_POOL_SIZE; i++) {
> - obj = kmem_cache_zalloc(obj_cache, GFP_KERNEL);
> + obj = kmem_cache_zalloc(cache, GFP_KERNEL);
> if (!obj)
> goto free;
> hlist_add_head(&obj->node, &objects);
> @@ -1322,12 +1326,6 @@ static int __init debug_objects_replace_
> debug_objects_allocated += i;
>
> /*
> - * debug_objects_mem_init() is now called early that only one CPU is up
> - * and interrupts have been disabled, so it is safe to replace the
> - * active object references.
> - */
> -
> - /*
> * Replace the statically allocated objects list with the allocated
> * objects list.
> */
> @@ -1347,15 +1345,14 @@ static int __init debug_objects_replace_
> }
> }
>
> - pr_debug("%d of %d active objects replaced\n",
> - cnt, obj_pool_used);
> - return 0;
> + pr_debug("%d of %d active objects replaced\n", cnt, obj_pool_used);
> + return true;
> free:
> hlist_for_each_entry_safe(obj, tmp, &objects, node) {
> hlist_del(&obj->node);
> - kmem_cache_free(obj_cache, obj);
> + kmem_cache_free(cache, obj);
> }
> - return -ENOMEM;
> + return false;
> }
>
> /*
> @@ -1366,6 +1363,7 @@ static int __init debug_objects_replace_
> */
> void __init debug_objects_mem_init(void)
> {
> + struct kmem_cache *cache;
> int cpu, extras;
>
> if (!debug_objects_enabled)
> @@ -1380,29 +1378,33 @@ void __init debug_objects_mem_init(void)
> for_each_possible_cpu(cpu)
> INIT_HLIST_HEAD(&per_cpu(percpu_obj_pool.free_objs, cpu));
>
> - obj_cache = kmem_cache_create("debug_objects_cache",
> - sizeof (struct debug_obj), 0,
> - SLAB_DEBUG_OBJECTS | SLAB_NOLEAKTRACE,
> - NULL);
> + if (!debug_objects_selftest())
> + return;
> +
> + cache = kmem_cache_create("debug_objects_cache", sizeof (struct debug_obj), 0,
> + SLAB_DEBUG_OBJECTS | SLAB_NOLEAKTRACE, NULL);
>
> - if (!obj_cache || debug_objects_replace_static_objects()) {
> + if (!cache || !debug_objects_replace_static_objects(cache)) {
> debug_objects_enabled = 0;
> - kmem_cache_destroy(obj_cache);
kmem_cache_destroy(cache) should be kept, or move it into debug_objects_replace_static_objects()
and place it above 'return false'.
> - pr_warn("out of memory.\n");
> + pr_warn("Out of memory.\n");
> return;
> - } else
> - debug_objects_selftest();
> -
> -#ifdef CONFIG_HOTPLUG_CPU
> - cpuhp_setup_state_nocalls(CPUHP_DEBUG_OBJ_DEAD, "object:offline", NULL,
> - object_cpu_offline);
> -#endif
> + }
>
> /*
> - * Increase the thresholds for allocating and freeing objects
> - * according to the number of possible CPUs available in the system.
> + * Adjust the thresholds for allocating and freeing objects
> + * according to the number of possible CPUs available in the
> + * system.
> */
> extras = num_possible_cpus() * ODEBUG_BATCH_SIZE;
> debug_objects_pool_size += extras;
> debug_objects_pool_min_level += extras;
> +
> + /* Everything worked. Expose the cache */
> + obj_cache = cache;
> +
> +#ifdef CONFIG_HOTPLUG_CPU
> + cpuhp_setup_state_nocalls(CPUHP_DEBUG_OBJ_DEAD, "object:offline", NULL,
> + object_cpu_offline);
> +#endif
> + return;
> }
>
> .
>
--
Regards,
Zhen Lei
On Thu, Oct 10 2024 at 10:14, Leizhen wrote:
> On 2024/10/8 0:49, Thomas Gleixner wrote:
>> - if (!obj_cache || debug_objects_replace_static_objects()) {
>> + if (!cache || !debug_objects_replace_static_objects(cache)) {
>> debug_objects_enabled = 0;
>> - kmem_cache_destroy(obj_cache);
>
> kmem_cache_destroy(cache) should be kept, or move it into debug_objects_replace_static_objects()
> and place it above 'return false'.
At that point it can't be destroyed. See the backtrace.
So we just give the objects back and leak the kmem_cache.
Thanks,
tglx
On 2024/10/10 19:46, Thomas Gleixner wrote:
> On Thu, Oct 10 2024 at 10:14, Leizhen wrote:
>> On 2024/10/8 0:49, Thomas Gleixner wrote:
>>> - if (!obj_cache || debug_objects_replace_static_objects()) {
>>> + if (!cache || !debug_objects_replace_static_objects(cache)) {
>>> debug_objects_enabled = 0;
>>> - kmem_cache_destroy(obj_cache);
>>
>> kmem_cache_destroy(cache) should be kept, or move it into debug_objects_replace_static_objects()
>> and place it above 'return false'.
>
> At that point it can't be destroyed. See the backtrace.
>
> So we just give the objects back and leak the kmem_cache.
Oh, sorry, I didn't figure it out before, but now I do.
But shouldn't we add kmemleak_ignore(cache) ?
>
> Thanks,
>
> tglx
>
> .
>
--
Regards,
Zhen Lei
On Thu, Oct 10 2024 at 21:31, Leizhen wrote:
> On 2024/10/10 19:46, Thomas Gleixner wrote:
>> On Thu, Oct 10 2024 at 10:14, Leizhen wrote:
>>> On 2024/10/8 0:49, Thomas Gleixner wrote:
>>>> - if (!obj_cache || debug_objects_replace_static_objects()) {
>>>> + if (!cache || !debug_objects_replace_static_objects(cache)) {
>>>> debug_objects_enabled = 0;
>>>> - kmem_cache_destroy(obj_cache);
>>>
>>> kmem_cache_destroy(cache) should be kept, or move it into debug_objects_replace_static_objects()
>>> and place it above 'return false'.
>>
>> At that point it can't be destroyed. See the backtrace.
>>
>> So we just give the objects back and leak the kmem_cache.
>
> Oh, sorry, I didn't figure it out before, but now I do.
> But shouldn't we add kmemleak_ignore(cache) ?
I don't know if it's worth the trouble. With the follow up changes the
only reason why this can happen is that the static object conversion
cannot allocate memory. Leaking the kmemcache in that case is the least
of the worries.
I just tripped over that back trace because I intentionally triggered
the error path.
Thanks,
tglx
On 2024/10/12 4:37, Thomas Gleixner wrote:
> On Thu, Oct 10 2024 at 21:31, Leizhen wrote:
>> On 2024/10/10 19:46, Thomas Gleixner wrote:
>>> On Thu, Oct 10 2024 at 10:14, Leizhen wrote:
>>>> On 2024/10/8 0:49, Thomas Gleixner wrote:
>>>>> - if (!obj_cache || debug_objects_replace_static_objects()) {
>>>>> + if (!cache || !debug_objects_replace_static_objects(cache)) {
>>>>> debug_objects_enabled = 0;
>>>>> - kmem_cache_destroy(obj_cache);
>>>>
>>>> kmem_cache_destroy(cache) should be kept, or move it into debug_objects_replace_static_objects()
>>>> and place it above 'return false'.
>>>
>>> At that point it can't be destroyed. See the backtrace.
>>>
>>> So we just give the objects back and leak the kmem_cache.
>>
>> Oh, sorry, I didn't figure it out before, but now I do.
>> But shouldn't we add kmemleak_ignore(cache) ?
>
> I don't know if it's worth the trouble. With the follow up changes the
> only reason why this can happen is that the static object conversion
> cannot allocate memory. Leaking the kmemcache in that case is the least
> of the worries.
>
> I just tripped over that back trace because I intentionally triggered
> the error path.
Yes, that's an error path that almost never goes to.
OK, so keep it still.
>
> Thanks,
>
> tglx
>
>
> .
>
--
Regards,
Zhen Lei
The following commit has been merged into the core/debugobjects branch of tip:
Commit-ID: 55fb412ef7d0c33226fcac4ebc68c60282e5f150
Gitweb: https://git.kernel.org/tip/55fb412ef7d0c33226fcac4ebc68c60282e5f150
Author: Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Mon, 07 Oct 2024 18:49:54 +02:00
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 15 Oct 2024 17:30:30 +02:00
debugobjects: Dont destroy kmem cache in init()
debug_objects_mem_init() is invoked from mm_core_init() before work queues
are available. If debug_objects_mem_init() destroys the kmem cache in the
error path it causes an Oops in __queue_work():
Oops: Oops: 0000 [#1] PREEMPT SMP PTI
RIP: 0010:__queue_work+0x35/0x6a0
queue_work_on+0x66/0x70
flush_all_cpus_locked+0xdf/0x1a0
__kmem_cache_shutdown+0x2f/0x340
kmem_cache_destroy+0x4e/0x150
mm_core_init+0x9e/0x120
start_kernel+0x298/0x800
x86_64_start_reservations+0x18/0x30
x86_64_start_kernel+0xc5/0xe0
common_startup_64+0x12c/0x138
Further the object cache pointer is used in various places to check for
early boot operation. It is exposed before the replacments for the static
boot time objects are allocated and the self test operates on it.
This can be avoided by:
1) Running the self test with the static boot objects
2) Exposing it only after the replacement objects have been added to
the pool.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/all/20241007164913.137021337@linutronix.de
---
lib/debugobjects.c | 68 +++++++++++++++++++++++----------------------
1 file changed, 35 insertions(+), 33 deletions(-)
diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index 798ce5a..1f6bf0f 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -1211,7 +1211,7 @@ static __initconst const struct debug_obj_descr descr_type_test = {
static __initdata struct self_test obj = { .static_init = 0 };
-static void __init debug_objects_selftest(void)
+static bool __init debug_objects_selftest(void)
{
int fixups, oldfixups, warnings, oldwarnings;
unsigned long flags;
@@ -1280,9 +1280,10 @@ out:
descr_test = NULL;
local_irq_restore(flags);
+ return !!debug_objects_enabled;
}
#else
-static inline void debug_objects_selftest(void) { }
+static inline bool debug_objects_selftest(void) { return true; }
#endif
/*
@@ -1302,18 +1303,21 @@ void __init debug_objects_early_init(void)
}
/*
- * Convert the statically allocated objects to dynamic ones:
+ * Convert the statically allocated objects to dynamic ones.
+ * debug_objects_mem_init() is called early so only one CPU is up and
+ * interrupts are disabled, which means it is safe to replace the active
+ * object references.
*/
-static int __init debug_objects_replace_static_objects(void)
+static bool __init debug_objects_replace_static_objects(struct kmem_cache *cache)
{
struct debug_bucket *db = obj_hash;
- struct hlist_node *tmp;
struct debug_obj *obj, *new;
+ struct hlist_node *tmp;
HLIST_HEAD(objects);
int i, cnt = 0;
for (i = 0; i < ODEBUG_POOL_SIZE; i++) {
- obj = kmem_cache_zalloc(obj_cache, GFP_KERNEL);
+ obj = kmem_cache_zalloc(cache, GFP_KERNEL);
if (!obj)
goto free;
hlist_add_head(&obj->node, &objects);
@@ -1322,12 +1326,6 @@ static int __init debug_objects_replace_static_objects(void)
debug_objects_allocated += i;
/*
- * debug_objects_mem_init() is now called early that only one CPU is up
- * and interrupts have been disabled, so it is safe to replace the
- * active object references.
- */
-
- /*
* Replace the statically allocated objects list with the allocated
* objects list.
*/
@@ -1347,15 +1345,14 @@ static int __init debug_objects_replace_static_objects(void)
}
}
- pr_debug("%d of %d active objects replaced\n",
- cnt, obj_pool_used);
- return 0;
+ pr_debug("%d of %d active objects replaced\n", cnt, obj_pool_used);
+ return true;
free:
hlist_for_each_entry_safe(obj, tmp, &objects, node) {
hlist_del(&obj->node);
- kmem_cache_free(obj_cache, obj);
+ kmem_cache_free(cache, obj);
}
- return -ENOMEM;
+ return false;
}
/*
@@ -1366,6 +1363,7 @@ free:
*/
void __init debug_objects_mem_init(void)
{
+ struct kmem_cache *cache;
int cpu, extras;
if (!debug_objects_enabled)
@@ -1380,29 +1378,33 @@ void __init debug_objects_mem_init(void)
for_each_possible_cpu(cpu)
INIT_HLIST_HEAD(&per_cpu(percpu_obj_pool.free_objs, cpu));
- obj_cache = kmem_cache_create("debug_objects_cache",
- sizeof (struct debug_obj), 0,
- SLAB_DEBUG_OBJECTS | SLAB_NOLEAKTRACE,
- NULL);
+ if (!debug_objects_selftest())
+ return;
- if (!obj_cache || debug_objects_replace_static_objects()) {
+ cache = kmem_cache_create("debug_objects_cache", sizeof (struct debug_obj), 0,
+ SLAB_DEBUG_OBJECTS | SLAB_NOLEAKTRACE, NULL);
+
+ if (!cache || !debug_objects_replace_static_objects(cache)) {
debug_objects_enabled = 0;
- kmem_cache_destroy(obj_cache);
- pr_warn("out of memory.\n");
+ pr_warn("Out of memory.\n");
return;
- } else
- debug_objects_selftest();
-
-#ifdef CONFIG_HOTPLUG_CPU
- cpuhp_setup_state_nocalls(CPUHP_DEBUG_OBJ_DEAD, "object:offline", NULL,
- object_cpu_offline);
-#endif
+ }
/*
- * Increase the thresholds for allocating and freeing objects
- * according to the number of possible CPUs available in the system.
+ * Adjust the thresholds for allocating and freeing objects
+ * according to the number of possible CPUs available in the
+ * system.
*/
extras = num_possible_cpus() * ODEBUG_BATCH_SIZE;
debug_objects_pool_size += extras;
debug_objects_pool_min_level += extras;
+
+ /* Everything worked. Expose the cache */
+ obj_cache = cache;
+
+#ifdef CONFIG_HOTPLUG_CPU
+ cpuhp_setup_state_nocalls(CPUHP_DEBUG_OBJ_DEAD, "object:offline", NULL,
+ object_cpu_offline);
+#endif
+ return;
}© 2016 - 2024 Red Hat, Inc.