debugobjects: insulate non-fixup logic related to static obj from fixup callbacks
authorDu, Changbin <changbin.du@intel.com>
Fri, 20 May 2016 00:09:41 +0000 (17:09 -0700)
committerLinus Torvalds <torvalds@linux-foundation.org>
Fri, 20 May 2016 02:12:14 +0000 (19:12 -0700)
When activating a static object we need make sure that the object is
tracked in the object tracker.  If it is a non-static object then the
activation is illegal.

In previous implementation, each subsystem need take care of this in
their fixup callbacks.  Actually we can put it into debugobjects core.
Thus we can save duplicated code, and have *pure* fixup callbacks.

To achieve this, a new callback "is_static_object" is introduced to let
the type specific code decide whether a object is static or not.  If
yes, we take it into object tracker, otherwise give warning and invoke
fixup callback.

This change has paassed debugobjects selftest, and I also do some test
with all debugobjects supports enabled.

At last, I have a concern about the fixups that can it change the object
which is in incorrect state on fixup? Because the 'addr' may not point
to any valid object if a non-static object is not tracked.  Then Change
such object can overwrite someone's memory and cause unexpected
behaviour.  For example, the timer_fixup_activate bind timer to function
stub_timer.

Link: http://lkml.kernel.org/r/1462576157-14539-1-git-send-email-changbin.du@intel.com
[changbin.du@intel.com: improve code comments where invoke the new is_static_object callback]
Link: http://lkml.kernel.org/r/1462777431-8171-1-git-send-email-changbin.du@intel.com
Signed-off-by: Du, Changbin <changbin.du@intel.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Josh Triplett <josh@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tejun Heo <tj@kernel.org>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
include/linux/debugobjects.h
kernel/rcu/update.c
kernel/time/hrtimer.c
kernel/time/timer.c
kernel/workqueue.c
lib/debugobjects.c

index a899f10c93654c2cbe605d38cd163f309091ebe4..46056cb161fce0e1f3de707d887abd2ffcb1d02b 100644 (file)
@@ -38,6 +38,7 @@ struct debug_obj {
  * @name:              name of the object typee
  * @debug_hint:                function returning address, which have associated
  *                     kernel symbol, to allow identify the object
+ * @is_static_object   return true if the obj is static, otherwise return false
  * @fixup_init:                fixup function, which is called when the init check
  *                     fails. All fixup functions must return true if fixup
  *                     was successful, otherwise return false
@@ -53,6 +54,7 @@ struct debug_obj {
 struct debug_obj_descr {
        const char              *name;
        void *(*debug_hint)(void *addr);
+       bool (*is_static_object)(void *addr);
        bool (*fixup_init)(void *addr, enum debug_obj_state state);
        bool (*fixup_activate)(void *addr, enum debug_obj_state state);
        bool (*fixup_destroy)(void *addr, enum debug_obj_state state);
index a9df198eb22d1a22d9ff720f89180075d86d4cd0..3e888cd5a5941c43dd05f52209f7f5e884a9bf43 100644 (file)
@@ -380,29 +380,9 @@ void destroy_rcu_head(struct rcu_head *head)
        debug_object_free(head, &rcuhead_debug_descr);
 }
 
-/*
- * fixup_activate is called when:
- * - an active object is activated
- * - an unknown object is activated (might be a statically initialized object)
- * Activation is performed internally by call_rcu().
- */
-static bool rcuhead_fixup_activate(void *addr, enum debug_obj_state state)
+static bool rcuhead_is_static_object(void *addr)
 {
-       struct rcu_head *head = addr;
-
-       switch (state) {
-
-       case ODEBUG_STATE_NOTAVAILABLE:
-               /*
-                * This is not really a fixup. We just make sure that it is
-                * tracked in the object tracker.
-                */
-               debug_object_init(head, &rcuhead_debug_descr);
-               debug_object_activate(head, &rcuhead_debug_descr);
-               return false;
-       default:
-               return true;
-       }
+       return true;
 }
 
 /**
@@ -440,7 +420,7 @@ EXPORT_SYMBOL_GPL(destroy_rcu_head_on_stack);
 
 struct debug_obj_descr rcuhead_debug_descr = {
        .name = "rcu_head",
-       .fixup_activate = rcuhead_fixup_activate,
+       .is_static_object = rcuhead_is_static_object,
 };
 EXPORT_SYMBOL_GPL(rcuhead_debug_descr);
 #endif /* #ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD */
index f962a58c095788b7e1310079d8d891c5fe53b198..8c7392c4fdbd9ace42b096a2710257f1ea70e5ca 100644 (file)
@@ -351,16 +351,11 @@ static bool hrtimer_fixup_init(void *addr, enum debug_obj_state state)
 /*
  * fixup_activate is called when:
  * - an active object is activated
- * - an unknown object is activated (might be a statically initialized object)
+ * - an unknown non-static object is activated
  */
 static bool hrtimer_fixup_activate(void *addr, enum debug_obj_state state)
 {
        switch (state) {
-
-       case ODEBUG_STATE_NOTAVAILABLE:
-               WARN_ON_ONCE(1);
-               return false;
-
        case ODEBUG_STATE_ACTIVE:
                WARN_ON(1);
 
index be33481a4da182dde2c0d693306a7bfbb90aca3b..3a95f9728778c587b316274fd510b79ed5574494 100644 (file)
@@ -489,6 +489,14 @@ static void *timer_debug_hint(void *addr)
        return ((struct timer_list *) addr)->function;
 }
 
+static bool timer_is_static_object(void *addr)
+{
+       struct timer_list *timer = addr;
+
+       return (timer->entry.pprev == NULL &&
+               timer->entry.next == TIMER_ENTRY_STATIC);
+}
+
 /*
  * fixup_init is called when:
  * - an active object is initialized
@@ -516,30 +524,16 @@ static void stub_timer(unsigned long data)
 /*
  * fixup_activate is called when:
  * - an active object is activated
- * - an unknown object is activated (might be a statically initialized object)
+ * - an unknown non-static object is activated
  */
 static bool timer_fixup_activate(void *addr, enum debug_obj_state state)
 {
        struct timer_list *timer = addr;
 
        switch (state) {
-
        case ODEBUG_STATE_NOTAVAILABLE:
-               /*
-                * This is not really a fixup. The timer was
-                * statically initialized. We just make sure that it
-                * is tracked in the object tracker.
-                */
-               if (timer->entry.pprev == NULL &&
-                   timer->entry.next == TIMER_ENTRY_STATIC) {
-                       debug_object_init(timer, &timer_debug_descr);
-                       debug_object_activate(timer, &timer_debug_descr);
-                       return false;
-               } else {
-                       setup_timer(timer, stub_timer, 0);
-                       return true;
-               }
-               return false;
+               setup_timer(timer, stub_timer, 0);
+               return true;
 
        case ODEBUG_STATE_ACTIVE:
                WARN_ON(1);
@@ -577,18 +571,8 @@ static bool timer_fixup_assert_init(void *addr, enum debug_obj_state state)
 
        switch (state) {
        case ODEBUG_STATE_NOTAVAILABLE:
-               if (timer->entry.next == TIMER_ENTRY_STATIC) {
-                       /*
-                        * This is not really a fixup. The timer was
-                        * statically initialized. We just make sure that it
-                        * is tracked in the object tracker.
-                        */
-                       debug_object_init(timer, &timer_debug_descr);
-                       return false;
-               } else {
-                       setup_timer(timer, stub_timer, 0);
-                       return true;
-               }
+               setup_timer(timer, stub_timer, 0);
+               return true;
        default:
                return false;
        }
@@ -597,6 +581,7 @@ static bool timer_fixup_assert_init(void *addr, enum debug_obj_state state)
 static struct debug_obj_descr timer_debug_descr = {
        .name                   = "timer_list",
        .debug_hint             = timer_debug_hint,
+       .is_static_object       = timer_is_static_object,
        .fixup_init             = timer_fixup_init,
        .fixup_activate         = timer_fixup_activate,
        .fixup_free             = timer_fixup_free,
index 6751b18fd9acade416807846bd232815a1e4bc7e..e1c0e996b5ae63175feb24d5d6a6c8b541b63287 100644 (file)
@@ -433,6 +433,13 @@ static void *work_debug_hint(void *addr)
        return ((struct work_struct *) addr)->func;
 }
 
+static bool work_is_static_object(void *addr)
+{
+       struct work_struct *work = addr;
+
+       return test_bit(WORK_STRUCT_STATIC_BIT, work_data_bits(work));
+}
+
 /*
  * fixup_init is called when:
  * - an active object is initialized
@@ -451,39 +458,6 @@ static bool work_fixup_init(void *addr, enum debug_obj_state state)
        }
 }
 
-/*
- * fixup_activate is called when:
- * - an active object is activated
- * - an unknown object is activated (might be a statically initialized object)
- */
-static bool work_fixup_activate(void *addr, enum debug_obj_state state)
-{
-       struct work_struct *work = addr;
-
-       switch (state) {
-
-       case ODEBUG_STATE_NOTAVAILABLE:
-               /*
-                * This is not really a fixup. The work struct was
-                * statically initialized. We just make sure that it
-                * is tracked in the object tracker.
-                */
-               if (test_bit(WORK_STRUCT_STATIC_BIT, work_data_bits(work))) {
-                       debug_object_init(work, &work_debug_descr);
-                       debug_object_activate(work, &work_debug_descr);
-                       return false;
-               }
-               WARN_ON_ONCE(1);
-               return false;
-
-       case ODEBUG_STATE_ACTIVE:
-               WARN_ON(1);
-
-       default:
-               return false;
-       }
-}
-
 /*
  * fixup_free is called when:
  * - an active object is freed
@@ -505,8 +479,8 @@ static bool work_fixup_free(void *addr, enum debug_obj_state state)
 static struct debug_obj_descr work_debug_descr = {
        .name           = "work_struct",
        .debug_hint     = work_debug_hint,
+       .is_static_object = work_is_static_object,
        .fixup_init     = work_fixup_init,
-       .fixup_activate = work_fixup_activate,
        .fixup_free     = work_fixup_free,
 };
 
index 2f07c8c697b8ce5a0b672256f13d53ea21e475a6..a8e12601eb37dca5694faa1a5bfc9cb5e45497f1 100644 (file)
@@ -431,14 +431,21 @@ int debug_object_activate(void *addr, struct debug_obj_descr *descr)
 
        raw_spin_unlock_irqrestore(&db->lock, flags);
        /*
-        * This happens when a static object is activated. We
-        * let the type specific code decide whether this is
-        * true or not.
+        * We are here when a static object is activated. We
+        * let the type specific code confirm whether this is
+        * true or not. if true, we just make sure that the
+        * static object is tracked in the object tracker. If
+        * not, this must be a bug, so we try to fix it up.
         */
-       if (debug_object_fixup(descr->fixup_activate, addr,
-                          ODEBUG_STATE_NOTAVAILABLE)) {
+       if (descr->is_static_object && descr->is_static_object(addr)) {
+               /* track this static object */
+               debug_object_init(addr, descr);
+               debug_object_activate(addr, descr);
+       } else {
                debug_print_object(&o, "activate");
-               return -EINVAL;
+               ret = debug_object_fixup(descr->fixup_activate, addr,
+                                       ODEBUG_STATE_NOTAVAILABLE);
+               return ret ? 0 : -EINVAL;
        }
        return 0;
 }
@@ -602,12 +609,18 @@ void debug_object_assert_init(void *addr, struct debug_obj_descr *descr)
 
                raw_spin_unlock_irqrestore(&db->lock, flags);
                /*
-                * Maybe the object is static.  Let the type specific
-                * code decide what to do.
+                * Maybe the object is static, and we let the type specific
+                * code confirm. Track this static object if true, else invoke
+                * fixup.
                 */
-               if (debug_object_fixup(descr->fixup_assert_init, addr,
-                                      ODEBUG_STATE_NOTAVAILABLE))
+               if (descr->is_static_object && descr->is_static_object(addr)) {
+                       /* Track this static object */
+                       debug_object_init(addr, descr);
+               } else {
                        debug_print_object(&o, "assert_init");
+                       debug_object_fixup(descr->fixup_assert_init, addr,
+                                          ODEBUG_STATE_NOTAVAILABLE);
+               }
                return;
        }
 
@@ -792,6 +805,13 @@ struct self_test {
 
 static __initdata struct debug_obj_descr descr_type_test;
 
+static bool __init is_static_object(void *addr)
+{
+       struct self_test *obj = addr;
+
+       return obj->static_init;
+}
+
 /*
  * fixup_init is called when:
  * - an active object is initialized
@@ -813,7 +833,7 @@ static bool __init fixup_init(void *addr, enum debug_obj_state state)
 /*
  * fixup_activate is called when:
  * - an active object is activated
- * - an unknown object is activated (might be a statically initialized object)
+ * - an unknown non-static object is activated
  */
 static bool __init fixup_activate(void *addr, enum debug_obj_state state)
 {
@@ -821,13 +841,7 @@ static bool __init fixup_activate(void *addr, enum debug_obj_state state)
 
        switch (state) {
        case ODEBUG_STATE_NOTAVAILABLE:
-               if (obj->static_init == 1) {
-                       debug_object_init(obj, &descr_type_test);
-                       debug_object_activate(obj, &descr_type_test);
-                       return false;
-               }
                return true;
-
        case ODEBUG_STATE_ACTIVE:
                debug_object_deactivate(obj, &descr_type_test);
                debug_object_activate(obj, &descr_type_test);
@@ -916,6 +930,7 @@ out:
 
 static __initdata struct debug_obj_descr descr_type_test = {
        .name                   = "selftest",
+       .is_static_object       = is_static_object,
        .fixup_init             = fixup_init,
        .fixup_activate         = fixup_activate,
        .fixup_destroy          = fixup_destroy,