From:   Schspa Shi <schspa@gmail.com>
To:     tglx@linutronix.de, longman@redhat.com, swboyd@chromium.org,
        linux@roeck-us.net, wuchi.zero@gmail.com
Cc:     linux-kernel@vger.kernel.org, Schspa Shi <schspa@gmail.com>,
        syzbot+5093ba19745994288b53@syzkaller.appspotmail.com
Subject: [PATCH v2 1/2] debugobject: fix concurrency issues with is_static_object
Date:   Sat,  4 Mar 2023 02:31:47 +0800
Message-Id: <20230303183147.934793-1-schspa@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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