Re: [PATCH 1/2] debugobject: fix concurrency issues with is_static_object
From: Schspa Shi
Date: Fri Mar 03 2023 - 12:56:36 EST
Waiman Long <longman@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
>> Link: https://syzkaller.appspot.com/bug?id=22c8a5938eab640d1c6bcc0e3dc7be519d878462
>> Signed-off-by: Schspa Shi <schspa@xxxxxxxxx>
>> ---
>> 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