Messages in this thread Patch in this message | | | From | Schspa Shi <> | Subject | Re: [PATCH 1/2] debugobject: fix concurrency issues with is_static_object | Date | Sat, 04 Mar 2023 01:53:22 +0800 |
| |
Waiman Long <longman@redhat.com> writes:
> On 3/3/23 11:19, Schspa Shi wrote: >> 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> >> --- >> include/linux/debugobjects.h | 1 + >> lib/debugobjects.c | 71 ++++++++++++++++++++++++++++-------- >> 2 files changed, 56 insertions(+), 16 deletions(-) >> >> diff --git a/include/linux/debugobjects.h b/include/linux/debugobjects.h >> index 32444686b6ff4..544a6111b97f6 100644 >> --- a/include/linux/debugobjects.h >> +++ b/include/linux/debugobjects.h >> @@ -30,6 +30,7 @@ struct debug_obj { >> enum debug_obj_state state; >> unsigned int astate; >> void *object; >> + bool is_static; >> const struct debug_obj_descr *descr; >> }; > > The patch looks reasonable. My main concern is the increase in size of the > debug_obj structure. It is an additional 8 bytes on 64-bit arches. How much will > we save performance-wise by caching it in the debug_obj. Alternatively, you may > pack it within the current size by, maybe, reducing the size of state. >
Yes, we can change this to:
struct debug_obj { struct hlist_node node; struct { enum debug_obj_state state : 31; bool is_static : 1; }; unsigned int astate; void *object; const struct debug_obj_descr *descr; };
Which won't increase the object size.
(gdb) ptype /o struct debug_obj /* offset | size */ type = struct debug_obj { /* 0 | 0 */ struct hlist_node { <incomplete type>
/* total size (bytes): 0 */ } node; /* 16 | 4 */ struct { /* 16: 0 | 4 */ enum debug_obj_state state : 31; /* 19: 7 | 1 */ bool is_static : 1;
/* total size (bytes): 4 */ }; /* 20 | 4 */ unsigned int astate; /* 24 | 8 */ void *object; /* 32 | 8 */ const struct debug_obj_descr *descr;
/* total size (bytes): 40 */ }
> Cheers, > Longman > >> 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");
-- BRs Schspa Shi
| |