Messages in this thread | | | From | Schspa Shi <> | Subject | Re: [PATCH 1/2] debugobject: fix concurrency issues with is_static_object | Date | Thu, 23 Mar 2023 02:18:10 +0800 |
| |
Thomas Gleixner <tglx@linutronix.de> writes:
> On Wed, Mar 22 2023 at 18:46, Thomas Gleixner wrote: >> On Wed, Mar 22 2023 at 23:40, Schspa Shi wrote: >>> I think we should introduce lookup_object_or_alloc and is_static at the >>> same time. >> >> What for? > > The below has the NONE/INIT issue addressed and plugs that race > completely, so no further action required. > > Thanks, > > tglx > --- > --- a/lib/debugobjects.c > +++ b/lib/debugobjects.c > @@ -216,10 +216,6 @@ static struct debug_obj *__alloc_object( > return obj; > } > > -/* > - * Allocate a new object. If the pool is empty, switch off the debugger. > - * Must be called with interrupts disabled. > - */ > static struct debug_obj * > alloc_object(void *addr, struct debug_bucket *b, const struct debug_obj_descr *descr) > { > @@ -552,11 +548,49 @@ static void debug_object_is_on_stack(voi > WARN_ON(1); > } > > +static struct debug_obj *lookup_object_or_alloc(void *addr, struct debug_bucket *b, > + const struct debug_obj_descr *descr, > + bool onstack, bool alloc_ifstatic) > +{ > + struct debug_obj *obj = lookup_object(addr, b); > + enum debug_obj_state state = ODEBUG_STATE_NONE; > + > + if (likely(obj)) > + return obj; > + > + /* > + * debug_object_init() unconditionally allocates untracked > + * objects. It does not matter whether it is a static object or > + * not. > + * > + * debug_object_assert_init() and debug_object_activate() allow > + * allocation only if the descriptor callback confirms that the > + * object is static and considered initialized. For non-static > + * objects the allocation needs to be done from the fixup callback. > + */ > + if (unlikely(alloc_ifstatic)) { > + if (!descr->is_static_object || !descr->is_static_object(addr)) > + return ERR_PTR(-ENOENT); > + /* Statically allocated objects are considered initialized */ > + state = ODEBUG_STATE_INIT; > + } > + > + obj = alloc_object(addr, b, descr); > + if (likely(obj)) { > + obj->state = state; > + debug_object_is_on_stack(addr, onstack); > + return obj; > + } > + > + /* Out of memory. Do the cleanup outside of the locked region */ > + debug_objects_enabled = 0; > + return NULL; > +} > + > static void > __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack) > { > enum debug_obj_state state; > - bool check_stack = false; > struct debug_bucket *db; > struct debug_obj *obj; > unsigned long flags; > @@ -572,16 +606,11 @@ static void > > raw_spin_lock_irqsave(&db->lock, flags); > > - obj = lookup_object(addr, db); > - if (!obj) { > - obj = alloc_object(addr, db, descr); > - if (!obj) { > - debug_objects_enabled = 0; > - raw_spin_unlock_irqrestore(&db->lock, flags); > - debug_objects_oom(); > - return; > - } > - check_stack = true; > + obj = lookup_object_or_alloc(addr, db, descr, onstack, false); > + if (unlikely(!obj)) { > + raw_spin_unlock_irqrestore(&db->lock, flags); > + debug_objects_oom(); > + return; > } > > switch (obj->state) { > @@ -607,8 +636,6 @@ static void > } > > raw_spin_unlock_irqrestore(&db->lock, flags); > - if (check_stack) > - debug_object_is_on_stack(addr, onstack); > } > > /** > @@ -648,14 +675,12 @@ EXPORT_SYMBOL_GPL(debug_object_init_on_s > */ > int debug_object_activate(void *addr, const struct debug_obj_descr *descr) > { > + struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr }; > enum debug_obj_state state; > struct debug_bucket *db; > struct debug_obj *obj; > unsigned long flags; > int ret; > - struct debug_obj o = { .object = addr, > - .state = ODEBUG_STATE_NOTAVAILABLE, > - .descr = descr }; > > if (!debug_objects_enabled) > return 0; > @@ -664,8 +689,8 @@ int debug_object_activate(void *addr, co > > raw_spin_lock_irqsave(&db->lock, flags); > > - obj = lookup_object(addr, db); > - if (obj) { > + obj = lookup_object_or_alloc(addr, db, descr, false, true); > + if (likely(!IS_ERR_OR_NULL(obj))) { > bool print_object = false; > > switch (obj->state) { > @@ -698,24 +723,16 @@ int debug_object_activate(void *addr, co > > raw_spin_unlock_irqrestore(&db->lock, flags); > > - /* > - * 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 (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"); > - ret = debug_object_fixup(descr->fixup_activate, addr, > - ODEBUG_STATE_NOTAVAILABLE); > - return ret ? 0 : -EINVAL; > + /* If NULL the allocaction has hit OOM */ > + if (!obj) { > + debug_objects_oom(); > + return 0; > } > - return 0; > + > + /* Object is neither static nor tracked. It's not initialized */ > + debug_print_object(&o, "activate"); > + ret = debug_object_fixup(descr->fixup_activate, addr, ODEBUG_STATE_NOTAVAILABLE); > + return ret ? 0 : -EINVAL; > } > EXPORT_SYMBOL_GPL(debug_object_activate); > > @@ -869,6 +886,7 @@ EXPORT_SYMBOL_GPL(debug_object_free); > */ > void debug_object_assert_init(void *addr, const struct debug_obj_descr *descr) > { > + struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr }; > struct debug_bucket *db; > struct debug_obj *obj; > unsigned long flags; > @@ -879,31 +897,20 @@ void debug_object_assert_init(void *addr > db = get_bucket((unsigned long) addr); > > raw_spin_lock_irqsave(&db->lock, flags); > + obj = lookup_object_or_alloc(addr, db, descr, false, true); > + raw_spin_unlock_irqrestore(&db->lock, flags); > + if (likely(!IS_ERR_OR_NULL(obj))) > + return; > > - obj = lookup_object(addr, db); > + /* If NULL the allocaction has hit OOM */ > if (!obj) { > - struct debug_obj o = { .object = addr, > - .state = ODEBUG_STATE_NOTAVAILABLE, > - .descr = 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)) { > - /* 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); > - } > + debug_objects_oom(); > return; > } > > - raw_spin_unlock_irqrestore(&db->lock, flags); > + /* Object is neither tracked nor static. It's not initialized. */ > + debug_print_object(&o, "assert_init"); > + debug_object_fixup(descr->fixup_assert_init, addr, ODEBUG_STATE_NOTAVAILABLE); > } > EXPORT_SYMBOL_GPL(debug_object_assert_init); >
It's OK if you don't think debug_object_free() should report a error when the object is static. But I still think we should report an error and modify the autotest case in debug_objects_selftest() to remove debug_object_free() call on the static object.
-- BRs Schspa Shi
| |