Re: [PATCH 1/2] debugobject: fix concurrency issues with is_static_object
From: Waiman Long
Date: Fri Mar 03 2023 - 12:01:00 EST
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.
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");