Messages in this thread Patch in this message | | | From | Schspa Shi <> | Subject | [PATCH v2 1/2] debugobject: fix concurrency issues with is_static_object | Date | Sat, 4 Mar 2023 02:31:47 +0800 |
| |
The is_static_object implementation relay on the initial state of the object. If multiple places are accessed concurrently, there is a probability that the debug object has been registered in the system, which will invalidate the judgment of is_static_object.
The following is the scenario where the problem occurs:
T0 T1 ========================================================================= mod_timer(); debug_object_assert_init db = get_bucket((unsigned long) addr); raw_spin_lock_irqsave(&db->lock, flags); obj = lookup_object(addr, db); if (!obj) { raw_spin_unlock_irqrestore(&db->lock, flags); << Context switch >> mod_timer(); debug_object_assert_init ... enqueue_timer(); /* * The initial state changed a static timer object, and * is_static_object will return false */
if (descr->is_static_object && descr->is_static_object(addr)) { debug_object_init(); } else { << Hit here for a static object >> debug_print_object(&o, "assert_init"); debug_object_fixup(descr->fixup_assert_init, addr, ODEBUG_STATE_NOTAVAILABLE); } }
To fix it, we got the is_static_object called within db->lock, and save it's state to struct debug_obj. This will ensure we won't hit the code branch not belong to the static object.
For the same static object, debug_object_init may enter multiple times, but there is a lock in debug_object_init to ensure that there is no problem.
Reported-by: syzbot+5093ba19745994288b53@syzkaller.appspotmail.com Link: https://syzkaller.appspot.com/bug?id=22c8a5938eab640d1c6bcc0e3dc7be519d878462 Signed-off-by: Schspa Shi <schspa@gmail.com> --- Changes from v1: - Reduce debug object size as Waiman Long suggested. - Add description for new members.
--- include/linux/debugobjects.h | 6 ++- lib/debugobjects.c | 71 ++++++++++++++++++++++++++++-------- 2 files changed, 60 insertions(+), 17 deletions(-)
diff --git a/include/linux/debugobjects.h b/include/linux/debugobjects.h index 32444686b6ff4..19f9fb80557f6 100644 --- a/include/linux/debugobjects.h +++ b/include/linux/debugobjects.h @@ -21,13 +21,17 @@ struct debug_obj_descr; * struct debug_obj - representation of an tracked object * @node: hlist node to link the object into the tracker list * @state: tracked object state + * @is_state: flag used to indicate that it is a static object * @astate: current active state * @object: pointer to the real object * @descr: pointer to an object type specific debug description structure */ struct debug_obj { struct hlist_node node; - enum debug_obj_state state; + struct { + enum debug_obj_state state : 31; + bool is_static : 1; + }; unsigned int astate; void *object; const struct debug_obj_descr *descr; diff --git a/lib/debugobjects.c b/lib/debugobjects.c index df86e649d8be0..d1be18158a1f7 100644 --- a/lib/debugobjects.c +++ b/lib/debugobjects.c @@ -275,6 +275,8 @@ alloc_object(void *addr, struct debug_bucket *b, const struct debug_obj_descr *d obj->descr = descr; obj->state = ODEBUG_STATE_NONE; obj->astate = 0; + obj->is_static = descr->is_static_object && + descr->is_static_object(addr); hlist_add_head(&obj->node, &b->list); } return obj; @@ -581,7 +583,16 @@ __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack debug_objects_oom(); return; } + check_stack = true; + } else { + /* + * The debug object is inited, and we should check this again + */ + if (obj->is_static) { + raw_spin_unlock_irqrestore(&db->lock, flags); + return; + } } switch (obj->state) { @@ -640,6 +651,29 @@ void debug_object_init_on_stack(void *addr, const struct debug_obj_descr *descr) } EXPORT_SYMBOL_GPL(debug_object_init_on_stack); +/* + * Check static object. + */ +static bool debug_object_check_static(struct debug_bucket *db, void *addr, + const struct debug_obj_descr *descr) +{ + struct debug_obj *obj; + + /* + * The is_static_object implementation relay on the initial state of the + * object. If multiple places are accessed concurrently, there is a + * probability that the debug object has been registered in the system, + * which will invalidate the judgment of is_static_object. + */ + lockdep_assert_held(&db->lock); + + obj = lookup_object(addr, db); + if (likely(obj)) + return obj->is_static; + + return descr->is_static_object && descr->is_static_object(addr); +} + /** * debug_object_activate - debug checks when an object is activated * @addr: address of the object @@ -656,6 +690,7 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr) struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr }; + bool is_static; if (!debug_objects_enabled) return 0; @@ -696,6 +731,7 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr) return ret; } + is_static = debug_object_check_static(db, addr, descr); raw_spin_unlock_irqrestore(&db->lock, flags); /* @@ -705,7 +741,7 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr) * static object is tracked in the object tracker. If * not, this must be a bug, so we try to fix it up. */ - if (descr->is_static_object && descr->is_static_object(addr)) { + if (is_static) { /* track this static object */ debug_object_init(addr, descr); debug_object_activate(addr, descr); @@ -872,6 +908,7 @@ void debug_object_assert_init(void *addr, const struct debug_obj_descr *descr) struct debug_bucket *db; struct debug_obj *obj; unsigned long flags; + bool is_static; if (!debug_objects_enabled) return; @@ -886,13 +923,14 @@ void debug_object_assert_init(void *addr, const struct debug_obj_descr *descr) .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr }; + is_static = debug_object_check_static(db, addr, descr); raw_spin_unlock_irqrestore(&db->lock, flags); /* * Maybe the object is static, and we let the type specific * code confirm. Track this static object if true, else invoke * fixup. */ - if (descr->is_static_object && descr->is_static_object(addr)) { + if (is_static) { /* Track this static object */ debug_object_init(addr, descr); } else { @@ -1215,7 +1253,8 @@ static __initconst const struct debug_obj_descr descr_type_test = { .fixup_free = fixup_free, }; -static __initdata struct self_test obj = { .static_init = 0 }; +static struct self_test obj __initdata = { .static_init = 0 }; +static struct self_test sobj __initdata = { .static_init = 1 }; static void __init debug_objects_selftest(void) { @@ -1256,26 +1295,26 @@ static void __init debug_objects_selftest(void) if (check_results(&obj, ODEBUG_STATE_NONE, fixups, warnings)) goto out; - obj.static_init = 1; - debug_object_activate(&obj, &descr_type_test); - if (check_results(&obj, ODEBUG_STATE_ACTIVE, fixups, warnings)) + debug_object_init(&sobj, &descr_type_test); + debug_object_activate(&sobj, &descr_type_test); + if (check_results(&sobj, ODEBUG_STATE_ACTIVE, fixups, warnings)) goto out; - debug_object_init(&obj, &descr_type_test); - if (check_results(&obj, ODEBUG_STATE_INIT, ++fixups, ++warnings)) + debug_object_init(&sobj, &descr_type_test); + if (check_results(&sobj, ODEBUG_STATE_INIT, ++fixups, ++warnings)) goto out; - debug_object_free(&obj, &descr_type_test); - if (check_results(&obj, ODEBUG_STATE_NONE, fixups, warnings)) + debug_object_free(&sobj, &descr_type_test); + if (check_results(&sobj, ODEBUG_STATE_NONE, fixups, warnings)) goto out; #ifdef CONFIG_DEBUG_OBJECTS_FREE - debug_object_init(&obj, &descr_type_test); - if (check_results(&obj, ODEBUG_STATE_INIT, fixups, warnings)) + debug_object_init(&sobj, &descr_type_test); + if (check_results(&sobj, ODEBUG_STATE_INIT, fixups, warnings)) goto out; - debug_object_activate(&obj, &descr_type_test); - if (check_results(&obj, ODEBUG_STATE_ACTIVE, fixups, warnings)) + debug_object_activate(&sobj, &descr_type_test); + if (check_results(&sobj, ODEBUG_STATE_ACTIVE, fixups, warnings)) goto out; - __debug_check_no_obj_freed(&obj, sizeof(obj)); - if (check_results(&obj, ODEBUG_STATE_NONE, ++fixups, ++warnings)) + __debug_check_no_obj_freed(&sobj, sizeof(sobj)); + if (check_results(&sobj, ODEBUG_STATE_NONE, ++fixups, ++warnings)) goto out; #endif pr_info("selftest passed\n"); -- 2.39.2
| |