Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1396192 > unrolled thread
| Started by | changbin.du@intel.com |
|---|---|
| First post | 2016-05-07 01:30 +0200 |
| Last post | 2016-05-09 09:20 +0200 |
| Articles | 8 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH] debugobjects: insulate non-fixup logic related to static obj from fixup callbacks changbin.du@intel.com - 2016-05-07 01:30 +0200
Re: [PATCH] debugobjects: insulate non-fixup logic related to static obj from fixup callbacks kbuild test robot <lkp@intel.com> - 2016-05-07 02:20 +0200
Re: [PATCH] debugobjects: insulate non-fixup logic related to static obj from fixup callbacks Thomas Gleixner <tglx@linutronix.de> - 2016-05-07 10:30 +0200
Re: [PATCH] debugobjects: insulate non-fixup logic related to static obj from fixup callbacks "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-05-07 14:50 +0200
RE: [PATCH] debugobjects: insulate non-fixup logic related to static obj from fixup callbacks "Du, Changbin" <changbin.du@intel.com> - 2016-05-08 09:50 +0200
RE: [PATCH] debugobjects: insulate non-fixup logic related to static obj from fixup callbacks Thomas Gleixner <tglx@linutronix.de> - 2016-05-08 18:20 +0200
RE: [PATCH] debugobjects: insulate non-fixup logic related to static obj from fixup callbacks "Du, Changbin" <changbin.du@intel.com> - 2016-05-09 05:10 +0200
[PATCH v2] debugobjects: insulate non-fixup logic related to static obj from fixup callbacks changbin.du@intel.com - 2016-05-09 09:20 +0200
| From | changbin.du@intel.com |
|---|---|
| Date | 2016-05-07 01:30 +0200 |
| Subject | [PATCH] debugobjects: insulate non-fixup logic related to static obj from fixup callbacks |
| Message-ID | <rvXm2-6qk-3@gated-at.bofh.it> |
From: "Du, Changbin" <changbin.du@intel.com>
When activate 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 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.
Signed-off-by: Du, Changbin <changbin.du@intel.com>
---
This patch depends on patch set:
[PATCH 0/7] Make debugobjects fixup functions return bool type
---
include/linux/debugobjects.h | 2 ++
kernel/rcu/update.c | 26 +++-----------------------
kernel/time/hrtimer.c | 7 +------
kernel/time/timer.c | 43 ++++++++++++++-----------------------------
kernel/workqueue.c | 42 ++++++++----------------------------------
lib/debugobjects.c | 38 +++++++++++++++++++++++++-------------
6 files changed, 53 insertions(+), 105 deletions(-)
diff --git a/include/linux/debugobjects.h b/include/linux/debugobjects.h
index a899f10..46056cb 100644
--- a/include/linux/debugobjects.h
+++ b/include/linux/debugobjects.h
@@ -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);
diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index a2897d7..f0d8322 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -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 */
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index f962a58..8c7392c 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -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);
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index be33481..3a95f97 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -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,
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 2932f3e9..fb1bb6d 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -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
@@ -452,39 +459,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,
};
diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index 2f07c8c..acdaf32 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -435,10 +435,15 @@ int debug_object_activate(void *addr, struct debug_obj_descr *descr)
* let the type specific code decide whether this is
* true or not.
*/
- if (debug_object_fixup(descr->fixup_activate, addr,
- ODEBUG_STATE_NOTAVAILABLE)) {
+ if (descr->is_static_object && descr->is_static_object(addr)) {
+ /* Just make sure that it is tracked in the object tracker */
+ 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 +607,17 @@ 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
+ * Maybe the object is static. Let the type specific
* code decide what to do.
*/
- if (debug_object_fixup(descr->fixup_assert_init, addr,
- ODEBUG_STATE_NOTAVAILABLE))
+ if (descr->is_static_object && descr->is_static_object(addr)) {
+ /* Make sure that it is tracked in the object tracker */
+ 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 +802,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 +830,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 +838,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 +927,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,
--
2.7.4
[toc] | [next] | [standalone]
| From | kbuild test robot <lkp@intel.com> |
|---|---|
| Date | 2016-05-07 02:20 +0200 |
| Subject | Re: [PATCH] debugobjects: insulate non-fixup logic related to static obj from fixup callbacks |
| Message-ID | <rvY8q-7ws-3@gated-at.bofh.it> |
| In reply to | #1396192 |
[Multipart message — attachments visible in raw view] — view raw
Hi,
[auto build test WARNING on next-20160506]
[cannot apply to tip/timers/core rcu/rcu/next v4.6-rc6 v4.6-rc5 v4.6-rc4 v4.6-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/changbin-du-intel-com/debugobjects-insulate-non-fixup-logic-related-to-static-obj-from-fixup-callbacks/20160507-072318
reproduce: make htmldocs
All warnings (new ones prefixed by >>):
>> include/linux/debugobjects.h:63: warning: No description found for parameter 'is_static_object'
vim +/is_static_object +63 include/linux/debugobjects.h
3ac7fe5a Thomas Gleixner 2008-04-30 47 * @fixup_destroy: fixup function, which is called when the destroy check
3ac7fe5a Thomas Gleixner 2008-04-30 48 * fails
3ac7fe5a Thomas Gleixner 2008-04-30 49 * @fixup_free: fixup function, which is called when the free check
3ac7fe5a Thomas Gleixner 2008-04-30 50 * fails
b84d435c Christine Chan 2011-11-07 51 * @fixup_assert_init: fixup function, which is called when the assert_init
b84d435c Christine Chan 2011-11-07 52 * check fails
3ac7fe5a Thomas Gleixner 2008-04-30 53 */
3ac7fe5a Thomas Gleixner 2008-04-30 54 struct debug_obj_descr {
3ac7fe5a Thomas Gleixner 2008-04-30 55 const char *name;
99777288 Stanislaw Gruszka 2011-03-07 56 void *(*debug_hint)(void *addr);
ae293450 Du, Changbin 2016-05-07 57 bool (*is_static_object)(void *addr);
5133056d Du, Changbin 2016-05-06 58 bool (*fixup_init)(void *addr, enum debug_obj_state state);
5133056d Du, Changbin 2016-05-06 59 bool (*fixup_activate)(void *addr, enum debug_obj_state state);
5133056d Du, Changbin 2016-05-06 60 bool (*fixup_destroy)(void *addr, enum debug_obj_state state);
5133056d Du, Changbin 2016-05-06 61 bool (*fixup_free)(void *addr, enum debug_obj_state state);
5133056d Du, Changbin 2016-05-06 62 bool (*fixup_assert_init)(void *addr, enum debug_obj_state state);
3ac7fe5a Thomas Gleixner 2008-04-30 @63 };
3ac7fe5a Thomas Gleixner 2008-04-30 64
3ac7fe5a Thomas Gleixner 2008-04-30 65 #ifdef CONFIG_DEBUG_OBJECTS
3ac7fe5a Thomas Gleixner 2008-04-30 66 extern void debug_object_init (void *addr, struct debug_obj_descr *descr);
3ac7fe5a Thomas Gleixner 2008-04-30 67 extern void
3ac7fe5a Thomas Gleixner 2008-04-30 68 debug_object_init_on_stack(void *addr, struct debug_obj_descr *descr);
b778ae25 Paul E. McKenney 2013-04-23 69 extern int debug_object_activate (void *addr, struct debug_obj_descr *descr);
3ac7fe5a Thomas Gleixner 2008-04-30 70 extern void debug_object_deactivate(void *addr, struct debug_obj_descr *descr);
3ac7fe5a Thomas Gleixner 2008-04-30 71 extern void debug_object_destroy (void *addr, struct debug_obj_descr *descr);
:::::: The code at line 63 was first introduced by commit
:::::: 3ac7fe5a4aab409bd5674d0b070bce97f9d20872 infrastructure to debug (dynamic) objects
:::::: TO: Thomas Gleixner <tglx@linutronix.de>
:::::: CC: Linus Torvalds <torvalds@linux-foundation.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2016-05-07 10:30 +0200 |
| Subject | Re: [PATCH] debugobjects: insulate non-fixup logic related to static obj from fixup callbacks |
| Message-ID | <rw5MB-6pI-1@gated-at.bofh.it> |
| In reply to | #1396192 |
On Sat, 7 May 2016, changbin.du@intel.com wrote:
Can you please fix your mail client. Every mail you send has:
Cc: .....
"Du, Changbin" <changbin.du@intel.com>,
Du
And that stray 'Du' is just broken.
> 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.
Well, you have the choice of:
1) Leave the object uninitialized and watch the resulting explosion
2) Assume that the pointer is a valid object and initialize it
The latter has been chosen as the lesser of two evils.
> raw_spin_unlock_irqrestore(&db->lock, flags);
> /*
> - * Maybe the object is static. Let the type specific
> + * Maybe the object is static. Let the type specific
> * code decide what to do.
Instead of doing white space changes you really want to explain the logic
here.
> */
> - if (debug_object_fixup(descr->fixup_assert_init, addr,
> - ODEBUG_STATE_NOTAVAILABLE))
> + if (descr->is_static_object && descr->is_static_object(addr)) {
> + /* Make sure that it is tracked in the object tracker */
> + debug_object_init(addr, descr);
> + } else {
> debug_print_object(&o, "assert_init");
> + debug_object_fixup(descr->fixup_assert_init, addr,
> + ODEBUG_STATE_NOTAVAILABLE);
> + }
> return;
> }
Other than the missing comment this looks good.
Thanks,
tglx
[toc] | [prev] | [next] | [standalone]
| From | "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> |
|---|---|
| Date | 2016-05-07 14:50 +0200 |
| Subject | Re: [PATCH] debugobjects: insulate non-fixup logic related to static obj from fixup callbacks |
| Message-ID | <rw9Qe-1Ak-13@gated-at.bofh.it> |
| In reply to | #1396280 |
On Sat, May 07, 2016 at 10:26:28AM +0200, Thomas Gleixner wrote:
> On Sat, 7 May 2016, changbin.du@intel.com wrote:
>
> Can you please fix your mail client. Every mail you send has:
>
> Cc: .....
> "Du, Changbin" <changbin.du@intel.com>,
> Du
>
> And that stray 'Du' is just broken.
>
> > 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.
>
> Well, you have the choice of:
>
> 1) Leave the object uninitialized and watch the resulting explosion
>
> 2) Assume that the pointer is a valid object and initialize it
>
> The latter has been chosen as the lesser of two evils.
>
> > raw_spin_unlock_irqrestore(&db->lock, flags);
> > /*
> > - * Maybe the object is static. Let the type specific
> > + * Maybe the object is static. Let the type specific
> > * code decide what to do.
>
> Instead of doing white space changes you really want to explain the logic
> here.
>
> > */
> > - if (debug_object_fixup(descr->fixup_assert_init, addr,
> > - ODEBUG_STATE_NOTAVAILABLE))
> > + if (descr->is_static_object && descr->is_static_object(addr)) {
> > + /* Make sure that it is tracked in the object tracker */
> > + debug_object_init(addr, descr);
> > + } else {
> > debug_print_object(&o, "assert_init");
> > + debug_object_fixup(descr->fixup_assert_init, addr,
> > + ODEBUG_STATE_NOTAVAILABLE);
> > + }
> > return;
> > }
>
> Other than the missing comment this looks good.
The transformation to the RCU code looks fine. So given changes so
that Thomas is good with the overall change, I am good with it from an
RCU perspective.
Thanx, Paul
[toc] | [prev] | [next] | [standalone]
| From | "Du, Changbin" <changbin.du@intel.com> |
|---|---|
| Date | 2016-05-08 09:50 +0200 |
| Subject | RE: [PATCH] debugobjects: insulate non-fixup logic related to static obj from fixup callbacks |
| Message-ID | <rwrDr-2ET-5@gated-at.bofh.it> |
| In reply to | #1396280 |
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Can you please fix your mail client. Every mail you send has:
>
> Cc: .....
> "Du, Changbin" <changbin.du@intel.com>,
> Du
>
> And that stray 'Du' is just broken.
>
Yes, I should add "" around my name or fix the git-sendemail perl script.
The script may add a "" auto. :)
> > 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.
>
> Well, you have the choice of:
>
> 1) Leave the object uninitialized and watch the resulting explosion
>
> 2) Assume that the pointer is a valid object and initialize it
>
> The latter has been chosen as the lesser of two evils.
>
Hmm, seems nothing more we can do to reduce further damage.
> > raw_spin_unlock_irqrestore(&db->lock, flags);
> > /*
> > - * Maybe the object is static. Let the type specific
> > + * Maybe the object is static. Let the type specific
> > * code decide what to do.
>
> Instead of doing white space changes you really want to explain the logic
> here.
>
Comments is in following code.
> > */
> > - if (debug_object_fixup(descr->fixup_assert_init, addr,
> > - ODEBUG_STATE_NOTAVAILABLE))
> > + if (descr->is_static_object && descr->is_static_object(addr))
> {
> > + /* Make sure that it is tracked in the object tracker */
> > + debug_object_init(addr, descr);
> > + } else {
> > debug_print_object(&o, "assert_init");
> > + debug_object_fixup(descr->fixup_assert_init, addr,
> > + ODEBUG_STATE_NOTAVAILABLE);
> > + }
> > return;
> > }
>
> Other than the missing comment this looks good.
>
> Thanks,
>
> tglx
Thanks for reviewing.
Du, Changbin
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2016-05-08 18:20 +0200 |
| Subject | RE: [PATCH] debugobjects: insulate non-fixup logic related to static obj from fixup callbacks |
| Message-ID | <rwzB0-1LG-13@gated-at.bofh.it> |
| In reply to | #1396388 |
On Sun, 8 May 2016, Du, Changbin wrote: > > From: Thomas Gleixner [mailto:tglx@linutronix.de] > > > raw_spin_unlock_irqrestore(&db->lock, flags); > > > /* > > > - * Maybe the object is static. Let the type specific > > > + * Maybe the object is static. Let the type specific > > > * code decide what to do. > > > > Instead of doing white space changes you really want to explain the logic > > here. > > > Comments is in following code. Well. It's a comment, but the code you replace has better explanations about statically initialized objects. This should move here. Thanks, tglx
[toc] | [prev] | [next] | [standalone]
| From | "Du, Changbin" <changbin.du@intel.com> |
|---|---|
| Date | 2016-05-09 05:10 +0200 |
| Subject | RE: [PATCH] debugobjects: insulate non-fixup logic related to static obj from fixup callbacks |
| Message-ID | <rwJK1-3lQ-3@gated-at.bofh.it> |
| In reply to | #1396439 |
> From: Thomas Gleixner [mailto:tglx@linutronix.de] > On Sun, 8 May 2016, Du, Changbin wrote: > > > From: Thomas Gleixner [mailto:tglx@linutronix.de] > > > > raw_spin_unlock_irqrestore(&db->lock, flags); > > > > /* > > > > - * Maybe the object is static. Let the type specific > > > > + * Maybe the object is static. Let the type specific > > > > * code decide what to do. > > > > > > Instead of doing white space changes you really want to explain the logic > > > here. > > > > > Comments is in following code. > > Well. It's a comment, but the code you replace has better explanations about > statically initialized objects. This should move here. > > Thanks, > > tglx Ok, let me improve the comment for patch v2. Best Regards, Du, Changbin
[toc] | [prev] | [next] | [standalone]
| From | changbin.du@intel.com |
|---|---|
| Date | 2016-05-09 09:20 +0200 |
| Subject | [PATCH v2] debugobjects: insulate non-fixup logic related to static obj from fixup callbacks |
| Message-ID | <rwNDY-8jo-7@gated-at.bofh.it> |
| In reply to | #1396439 |
From: "Du, Changbin" <changbin.du@intel.com>
When activate 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 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.
Signed-off-by: "Du, Changbin" <changbin.du@intel.com>
---
This patch depends on patch set:
[PATCH 0/7] Make debugobjects fixup functions return bool type
v2:
improve code comments where invoke the new is_static_object callback
---
include/linux/debugobjects.h | 2 ++
kernel/rcu/update.c | 26 +++--------------------
kernel/time/hrtimer.c | 7 +------
kernel/time/timer.c | 43 +++++++++++++-------------------------
kernel/workqueue.c | 42 ++++++++-----------------------------
lib/debugobjects.c | 49 +++++++++++++++++++++++++++++---------------
6 files changed, 60 insertions(+), 109 deletions(-)
diff --git a/include/linux/debugobjects.h b/include/linux/debugobjects.h
index a899f10..46056cb 100644
--- a/include/linux/debugobjects.h
+++ b/include/linux/debugobjects.h
@@ -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);
diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index a2897d7..f0d8322 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -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 */
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index f962a58..8c7392c 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -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);
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index be33481..3a95f97 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -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,
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 2932f3e9..fb1bb6d 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -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
@@ -452,39 +459,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,
};
diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index 2f07c8c..a8e1260 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -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,
--
2.7.4
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web