| Message ID | 20230925131359.2948827-1-andrzej.hajda@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] debugobjects: stop accessing objects after releasing spinlock | expand |
On 25.09.2023 15:13, Andrzej Hajda wrote: > After spinlock release object can be modified/freed by concurrent thread. > Using it in such case is error prone, even for printing object state. > To avoid such situation local copy of the object is created if necessary. > > Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com> > --- > v2: add missing switch breaks > --- Ping, any volunteer to review? Regards Andrzej > lib/debugobjects.c | 206 +++++++++++++++++++++------------------------ > 1 file changed, 97 insertions(+), 109 deletions(-) > > diff --git a/lib/debugobjects.c b/lib/debugobjects.c > index a517256a270b71..3afff2f668fc1e 100644 > --- a/lib/debugobjects.c > +++ b/lib/debugobjects.c > @@ -620,9 +620,8 @@ static void debug_objects_fill_pool(void) > static void > __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack) > { > - enum debug_obj_state state; > struct debug_bucket *db; > - struct debug_obj *obj; > + struct debug_obj *obj, o; > unsigned long flags; > > debug_objects_fill_pool(); > @@ -644,23 +643,19 @@ __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack > case ODEBUG_STATE_INACTIVE: > obj->state = ODEBUG_STATE_INIT; > break; > - > - case ODEBUG_STATE_ACTIVE: > - state = obj->state; > - raw_spin_unlock_irqrestore(&db->lock, flags); > - debug_print_object(obj, "init"); > - debug_object_fixup(descr->fixup_init, addr, state); > - return; > - > - case ODEBUG_STATE_DESTROYED: > - raw_spin_unlock_irqrestore(&db->lock, flags); > - debug_print_object(obj, "init"); > - return; > default: > - break; > + o = *obj; > + obj = NULL; > } > > raw_spin_unlock_irqrestore(&db->lock, flags); > + > + if (obj) > + return; > + > + debug_print_object(&o, "init"); > + if (o.state == ODEBUG_STATE_ACTIVE) > + debug_object_fixup(descr->fixup_init, addr, o.state); > } > > /** > @@ -700,12 +695,9 @@ EXPORT_SYMBOL_GPL(debug_object_init_on_stack); > */ > 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; > + struct debug_obj *obj, o; > unsigned long flags; > - int ret; > > if (!debug_objects_enabled) > return 0; > @@ -717,49 +709,47 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr) > raw_spin_lock_irqsave(&db->lock, flags); > > obj = lookup_object_or_alloc(addr, db, descr, false, true); > - if (likely(!IS_ERR_OR_NULL(obj))) { > - bool print_object = false; > - > + if (unlikely(!obj)) { > + raw_spin_unlock_irqrestore(&db->lock, flags); > + debug_objects_oom(); > + return 0; > + } else if (likely(!IS_ERR(obj))) { > switch (obj->state) { > case ODEBUG_STATE_INIT: > case ODEBUG_STATE_INACTIVE: > obj->state = ODEBUG_STATE_ACTIVE; > - ret = 0; > break; > - > case ODEBUG_STATE_ACTIVE: > - state = obj->state; > - raw_spin_unlock_irqrestore(&db->lock, flags); > - debug_print_object(obj, "activate"); > - ret = debug_object_fixup(descr->fixup_activate, addr, state); > - return ret ? 0 : -EINVAL; > - > case ODEBUG_STATE_DESTROYED: > - print_object = true; > - ret = -EINVAL; > + o = *obj; > + obj = NULL; > break; > default: > - ret = 0; > break; > } > - raw_spin_unlock_irqrestore(&db->lock, flags); > - if (print_object) > - debug_print_object(obj, "activate"); > - return ret; > + } else { > + o.object = addr; > + o.state = ODEBUG_STATE_NOTAVAILABLE; > + o.descr = descr; > + obj = NULL; > } > > raw_spin_unlock_irqrestore(&db->lock, flags); > > - /* If NULL the allocation has hit OOM */ > - if (!obj) { > - debug_objects_oom(); > + if (obj) > 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; > + > + switch (o.state) { > + case ODEBUG_STATE_ACTIVE: > + case ODEBUG_STATE_NOTAVAILABLE: > + if (debug_object_fixup(descr->fixup_activate, addr, o.state)) > + return 0; > + fallthrough; > + default: > + return -EINVAL; > + } > } > EXPORT_SYMBOL_GPL(debug_object_activate); > > @@ -771,9 +761,8 @@ EXPORT_SYMBOL_GPL(debug_object_activate); > void debug_object_deactivate(void *addr, const struct debug_obj_descr *descr) > { > struct debug_bucket *db; > - struct debug_obj *obj; > + struct debug_obj *obj, o; > unsigned long flags; > - bool print_object = false; > > if (!debug_objects_enabled) > return; > @@ -788,30 +777,29 @@ void debug_object_deactivate(void *addr, const struct debug_obj_descr *descr) > case ODEBUG_STATE_INIT: > case ODEBUG_STATE_INACTIVE: > case ODEBUG_STATE_ACTIVE: > - if (!obj->astate) > + if (!obj->astate) { > obj->state = ODEBUG_STATE_INACTIVE; > - else > - print_object = true; > - break; > - > + break; > + } > + fallthrough; > case ODEBUG_STATE_DESTROYED: > - print_object = true; > + o = *obj; > + obj = NULL; > break; > default: > break; > } > + } else { > + o.object = addr; > + o.state = ODEBUG_STATE_NOTAVAILABLE; > + o.descr = descr; > + obj = NULL; > } > > raw_spin_unlock_irqrestore(&db->lock, flags); > - if (!obj) { > - struct debug_obj o = { .object = addr, > - .state = ODEBUG_STATE_NOTAVAILABLE, > - .descr = descr }; > > + if (!obj) > debug_print_object(&o, "deactivate"); > - } else if (print_object) { > - debug_print_object(obj, "deactivate"); > - } > } > EXPORT_SYMBOL_GPL(debug_object_deactivate); > > @@ -822,11 +810,9 @@ EXPORT_SYMBOL_GPL(debug_object_deactivate); > */ > void debug_object_destroy(void *addr, const struct debug_obj_descr *descr) > { > - enum debug_obj_state state; > struct debug_bucket *db; > - struct debug_obj *obj; > + struct debug_obj *obj, o; > unsigned long flags; > - bool print_object = false; > > if (!debug_objects_enabled) > return; > @@ -836,8 +822,10 @@ void debug_object_destroy(void *addr, const struct debug_obj_descr *descr) > raw_spin_lock_irqsave(&db->lock, flags); > > obj = lookup_object(addr, db); > - if (!obj) > - goto out_unlock; > + if (!obj) { > + raw_spin_unlock_irqrestore(&db->lock, flags); > + return; > + } > > switch (obj->state) { > case ODEBUG_STATE_NONE: > @@ -846,22 +834,23 @@ void debug_object_destroy(void *addr, const struct debug_obj_descr *descr) > obj->state = ODEBUG_STATE_DESTROYED; > break; > case ODEBUG_STATE_ACTIVE: > - state = obj->state; > - raw_spin_unlock_irqrestore(&db->lock, flags); > - debug_print_object(obj, "destroy"); > - debug_object_fixup(descr->fixup_destroy, addr, state); > - return; > - > case ODEBUG_STATE_DESTROYED: > - print_object = true; > + o = *obj; > + obj = NULL; > break; > default: > break; > } > -out_unlock: > + > raw_spin_unlock_irqrestore(&db->lock, flags); > - if (print_object) > - debug_print_object(obj, "destroy"); > + > + if (obj) > + return; > + > + debug_print_object(&o, "destroy"); > + > + if (o.state == ODEBUG_STATE_ACTIVE) > + debug_object_fixup(descr->fixup_destroy, addr, o.state); > } > EXPORT_SYMBOL_GPL(debug_object_destroy); > > @@ -872,9 +861,8 @@ EXPORT_SYMBOL_GPL(debug_object_destroy); > */ > void debug_object_free(void *addr, const struct debug_obj_descr *descr) > { > - enum debug_obj_state state; > struct debug_bucket *db; > - struct debug_obj *obj; > + struct debug_obj *obj, o; > unsigned long flags; > > if (!debug_objects_enabled) > @@ -885,24 +873,29 @@ void debug_object_free(void *addr, const struct debug_obj_descr *descr) > raw_spin_lock_irqsave(&db->lock, flags); > > obj = lookup_object(addr, db); > - if (!obj) > - goto out_unlock; > + if (!obj) { > + raw_spin_unlock_irqrestore(&db->lock, flags); > + return; > + } > > switch (obj->state) { > case ODEBUG_STATE_ACTIVE: > - state = obj->state; > - raw_spin_unlock_irqrestore(&db->lock, flags); > - debug_print_object(obj, "free"); > - debug_object_fixup(descr->fixup_free, addr, state); > - return; > + o = *obj; > + obj = NULL; > + break; > default: > hlist_del(&obj->node); > - raw_spin_unlock_irqrestore(&db->lock, flags); > + } > + > + raw_spin_unlock_irqrestore(&db->lock, flags); > + > + if (obj) { > free_object(obj); > return; > } > -out_unlock: > - raw_spin_unlock_irqrestore(&db->lock, flags); > + > + debug_print_object(&o, "free"); > + debug_object_fixup(descr->fixup_free, addr, o.state); > } > EXPORT_SYMBOL_GPL(debug_object_free); > > @@ -955,9 +948,8 @@ debug_object_active_state(void *addr, const struct debug_obj_descr *descr, > unsigned int expect, unsigned int next) > { > struct debug_bucket *db; > - struct debug_obj *obj; > + struct debug_obj *obj, o; > unsigned long flags; > - bool print_object = false; > > if (!debug_objects_enabled) > return; > @@ -970,28 +962,27 @@ debug_object_active_state(void *addr, const struct debug_obj_descr *descr, > if (obj) { > switch (obj->state) { > case ODEBUG_STATE_ACTIVE: > - if (obj->astate == expect) > + if (obj->astate == expect) { > obj->astate = next; > - else > - print_object = true; > - break; > - > + break; > + } > + fallthrough; > default: > - print_object = true; > + o = *obj; > + obj = NULL; > break; > } > + } else { > + o.object = addr; > + o.state = ODEBUG_STATE_NOTAVAILABLE; > + o.descr = descr; > + obj = NULL; > } > > raw_spin_unlock_irqrestore(&db->lock, flags); > - if (!obj) { > - struct debug_obj o = { .object = addr, > - .state = ODEBUG_STATE_NOTAVAILABLE, > - .descr = descr }; > > + if (!obj) > debug_print_object(&o, "active_state"); > - } else if (print_object) { > - debug_print_object(obj, "active_state"); > - } > } > EXPORT_SYMBOL_GPL(debug_object_active_state); > > @@ -999,11 +990,9 @@ EXPORT_SYMBOL_GPL(debug_object_active_state); > static void __debug_check_no_obj_freed(const void *address, unsigned long size) > { > unsigned long flags, oaddr, saddr, eaddr, paddr, chunks; > - const struct debug_obj_descr *descr; > - enum debug_obj_state state; > struct debug_bucket *db; > struct hlist_node *tmp; > - struct debug_obj *obj; > + struct debug_obj *obj, o; > int cnt, objs_checked = 0; > > saddr = (unsigned long) address; > @@ -1026,12 +1015,11 @@ static void __debug_check_no_obj_freed(const void *address, unsigned long size) > > switch (obj->state) { > case ODEBUG_STATE_ACTIVE: > - descr = obj->descr; > - state = obj->state; > + o = *obj; > raw_spin_unlock_irqrestore(&db->lock, flags); > - debug_print_object(obj, "free"); > - debug_object_fixup(descr->fixup_free, > - (void *) oaddr, state); > + debug_print_object(&o, "free"); > + debug_object_fixup(o.descr->fixup_free, > + (void *) oaddr, o.state); > goto repeat; > default: > hlist_del(&obj->node);
Hi Andrzej, On Tue, Oct 10, 2023 at 02:02:54PM +0200, Andrzej Hajda wrote: > On 25.09.2023 15:13, Andrzej Hajda wrote: > > After spinlock release object can be modified/freed by concurrent thread. > > Using it in such case is error prone, even for printing object state. > > To avoid such situation local copy of the object is created if necessary. > > > > Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com> > > --- > > v2: add missing switch breaks > > --- > > Ping, any volunteer to review? ops... sorry... this slipped from my review list. Will look at it soon Andi > Regards > Andrzej > > > > > > lib/debugobjects.c | 206 +++++++++++++++++++++------------------------ > > 1 file changed, 97 insertions(+), 109 deletions(-) > > > > diff --git a/lib/debugobjects.c b/lib/debugobjects.c > > index a517256a270b71..3afff2f668fc1e 100644 > > --- a/lib/debugobjects.c > > +++ b/lib/debugobjects.c > > @@ -620,9 +620,8 @@ static void debug_objects_fill_pool(void) > > static void > > __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack) > > { > > - enum debug_obj_state state; > > struct debug_bucket *db; > > - struct debug_obj *obj; > > + struct debug_obj *obj, o; > > unsigned long flags; > > debug_objects_fill_pool(); > > @@ -644,23 +643,19 @@ __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack > > case ODEBUG_STATE_INACTIVE: > > obj->state = ODEBUG_STATE_INIT; > > break; > > - > > - case ODEBUG_STATE_ACTIVE: > > - state = obj->state; > > - raw_spin_unlock_irqrestore(&db->lock, flags); > > - debug_print_object(obj, "init"); > > - debug_object_fixup(descr->fixup_init, addr, state); > > - return; > > - > > - case ODEBUG_STATE_DESTROYED: > > - raw_spin_unlock_irqrestore(&db->lock, flags); > > - debug_print_object(obj, "init"); > > - return; > > default: > > - break; > > + o = *obj; > > + obj = NULL; > > } > > raw_spin_unlock_irqrestore(&db->lock, flags); > > + > > + if (obj) > > + return; > > + > > + debug_print_object(&o, "init"); > > + if (o.state == ODEBUG_STATE_ACTIVE) > > + debug_object_fixup(descr->fixup_init, addr, o.state); > > } > > /** > > @@ -700,12 +695,9 @@ EXPORT_SYMBOL_GPL(debug_object_init_on_stack); > > */ > > 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; > > + struct debug_obj *obj, o; > > unsigned long flags; > > - int ret; > > if (!debug_objects_enabled) > > return 0; > > @@ -717,49 +709,47 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr) > > raw_spin_lock_irqsave(&db->lock, flags); > > obj = lookup_object_or_alloc(addr, db, descr, false, true); > > - if (likely(!IS_ERR_OR_NULL(obj))) { > > - bool print_object = false; > > - > > + if (unlikely(!obj)) { > > + raw_spin_unlock_irqrestore(&db->lock, flags); > > + debug_objects_oom(); > > + return 0; > > + } else if (likely(!IS_ERR(obj))) { > > switch (obj->state) { > > case ODEBUG_STATE_INIT: > > case ODEBUG_STATE_INACTIVE: > > obj->state = ODEBUG_STATE_ACTIVE; > > - ret = 0; > > break; > > - > > case ODEBUG_STATE_ACTIVE: > > - state = obj->state; > > - raw_spin_unlock_irqrestore(&db->lock, flags); > > - debug_print_object(obj, "activate"); > > - ret = debug_object_fixup(descr->fixup_activate, addr, state); > > - return ret ? 0 : -EINVAL; > > - > > case ODEBUG_STATE_DESTROYED: > > - print_object = true; > > - ret = -EINVAL; > > + o = *obj; > > + obj = NULL; > > break; > > default: > > - ret = 0; > > break; > > } > > - raw_spin_unlock_irqrestore(&db->lock, flags); > > - if (print_object) > > - debug_print_object(obj, "activate"); > > - return ret; > > + } else { > > + o.object = addr; > > + o.state = ODEBUG_STATE_NOTAVAILABLE; > > + o.descr = descr; > > + obj = NULL; > > } > > raw_spin_unlock_irqrestore(&db->lock, flags); > > - /* If NULL the allocation has hit OOM */ > > - if (!obj) { > > - debug_objects_oom(); > > + if (obj) > > 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; > > + > > + switch (o.state) { > > + case ODEBUG_STATE_ACTIVE: > > + case ODEBUG_STATE_NOTAVAILABLE: > > + if (debug_object_fixup(descr->fixup_activate, addr, o.state)) > > + return 0; > > + fallthrough; > > + default: > > + return -EINVAL; > > + } > > } > > EXPORT_SYMBOL_GPL(debug_object_activate); > > @@ -771,9 +761,8 @@ EXPORT_SYMBOL_GPL(debug_object_activate); > > void debug_object_deactivate(void *addr, const struct debug_obj_descr *descr) > > { > > struct debug_bucket *db; > > - struct debug_obj *obj; > > + struct debug_obj *obj, o; > > unsigned long flags; > > - bool print_object = false; > > if (!debug_objects_enabled) > > return; > > @@ -788,30 +777,29 @@ void debug_object_deactivate(void *addr, const struct debug_obj_descr *descr) > > case ODEBUG_STATE_INIT: > > case ODEBUG_STATE_INACTIVE: > > case ODEBUG_STATE_ACTIVE: > > - if (!obj->astate) > > + if (!obj->astate) { > > obj->state = ODEBUG_STATE_INACTIVE; > > - else > > - print_object = true; > > - break; > > - > > + break; > > + } > > + fallthrough; > > case ODEBUG_STATE_DESTROYED: > > - print_object = true; > > + o = *obj; > > + obj = NULL; > > break; > > default: > > break; > > } > > + } else { > > + o.object = addr; > > + o.state = ODEBUG_STATE_NOTAVAILABLE; > > + o.descr = descr; > > + obj = NULL; > > } > > raw_spin_unlock_irqrestore(&db->lock, flags); > > - if (!obj) { > > - struct debug_obj o = { .object = addr, > > - .state = ODEBUG_STATE_NOTAVAILABLE, > > - .descr = descr }; > > + if (!obj) > > debug_print_object(&o, "deactivate"); > > - } else if (print_object) { > > - debug_print_object(obj, "deactivate"); > > - } > > } > > EXPORT_SYMBOL_GPL(debug_object_deactivate); > > @@ -822,11 +810,9 @@ EXPORT_SYMBOL_GPL(debug_object_deactivate); > > */ > > void debug_object_destroy(void *addr, const struct debug_obj_descr *descr) > > { > > - enum debug_obj_state state; > > struct debug_bucket *db; > > - struct debug_obj *obj; > > + struct debug_obj *obj, o; > > unsigned long flags; > > - bool print_object = false; > > if (!debug_objects_enabled) > > return; > > @@ -836,8 +822,10 @@ void debug_object_destroy(void *addr, const struct debug_obj_descr *descr) > > raw_spin_lock_irqsave(&db->lock, flags); > > obj = lookup_object(addr, db); > > - if (!obj) > > - goto out_unlock; > > + if (!obj) { > > + raw_spin_unlock_irqrestore(&db->lock, flags); > > + return; > > + } > > switch (obj->state) { > > case ODEBUG_STATE_NONE: > > @@ -846,22 +834,23 @@ void debug_object_destroy(void *addr, const struct debug_obj_descr *descr) > > obj->state = ODEBUG_STATE_DESTROYED; > > break; > > case ODEBUG_STATE_ACTIVE: > > - state = obj->state; > > - raw_spin_unlock_irqrestore(&db->lock, flags); > > - debug_print_object(obj, "destroy"); > > - debug_object_fixup(descr->fixup_destroy, addr, state); > > - return; > > - > > case ODEBUG_STATE_DESTROYED: > > - print_object = true; > > + o = *obj; > > + obj = NULL; > > break; > > default: > > break; > > } > > -out_unlock: > > + > > raw_spin_unlock_irqrestore(&db->lock, flags); > > - if (print_object) > > - debug_print_object(obj, "destroy"); > > + > > + if (obj) > > + return; > > + > > + debug_print_object(&o, "destroy"); > > + > > + if (o.state == ODEBUG_STATE_ACTIVE) > > + debug_object_fixup(descr->fixup_destroy, addr, o.state); > > } > > EXPORT_SYMBOL_GPL(debug_object_destroy); > > @@ -872,9 +861,8 @@ EXPORT_SYMBOL_GPL(debug_object_destroy); > > */ > > void debug_object_free(void *addr, const struct debug_obj_descr *descr) > > { > > - enum debug_obj_state state; > > struct debug_bucket *db; > > - struct debug_obj *obj; > > + struct debug_obj *obj, o; > > unsigned long flags; > > if (!debug_objects_enabled) > > @@ -885,24 +873,29 @@ void debug_object_free(void *addr, const struct debug_obj_descr *descr) > > raw_spin_lock_irqsave(&db->lock, flags); > > obj = lookup_object(addr, db); > > - if (!obj) > > - goto out_unlock; > > + if (!obj) { > > + raw_spin_unlock_irqrestore(&db->lock, flags); > > + return; > > + } > > switch (obj->state) { > > case ODEBUG_STATE_ACTIVE: > > - state = obj->state; > > - raw_spin_unlock_irqrestore(&db->lock, flags); > > - debug_print_object(obj, "free"); > > - debug_object_fixup(descr->fixup_free, addr, state); > > - return; > > + o = *obj; > > + obj = NULL; > > + break; > > default: > > hlist_del(&obj->node); > > - raw_spin_unlock_irqrestore(&db->lock, flags); > > + } > > + > > + raw_spin_unlock_irqrestore(&db->lock, flags); > > + > > + if (obj) { > > free_object(obj); > > return; > > } > > -out_unlock: > > - raw_spin_unlock_irqrestore(&db->lock, flags); > > + > > + debug_print_object(&o, "free"); > > + debug_object_fixup(descr->fixup_free, addr, o.state); > > } > > EXPORT_SYMBOL_GPL(debug_object_free); > > @@ -955,9 +948,8 @@ debug_object_active_state(void *addr, const struct debug_obj_descr *descr, > > unsigned int expect, unsigned int next) > > { > > struct debug_bucket *db; > > - struct debug_obj *obj; > > + struct debug_obj *obj, o; > > unsigned long flags; > > - bool print_object = false; > > if (!debug_objects_enabled) > > return; > > @@ -970,28 +962,27 @@ debug_object_active_state(void *addr, const struct debug_obj_descr *descr, > > if (obj) { > > switch (obj->state) { > > case ODEBUG_STATE_ACTIVE: > > - if (obj->astate == expect) > > + if (obj->astate == expect) { > > obj->astate = next; > > - else > > - print_object = true; > > - break; > > - > > + break; > > + } > > + fallthrough; > > default: > > - print_object = true; > > + o = *obj; > > + obj = NULL; > > break; > > } > > + } else { > > + o.object = addr; > > + o.state = ODEBUG_STATE_NOTAVAILABLE; > > + o.descr = descr; > > + obj = NULL; > > } > > raw_spin_unlock_irqrestore(&db->lock, flags); > > - if (!obj) { > > - struct debug_obj o = { .object = addr, > > - .state = ODEBUG_STATE_NOTAVAILABLE, > > - .descr = descr }; > > + if (!obj) > > debug_print_object(&o, "active_state"); > > - } else if (print_object) { > > - debug_print_object(obj, "active_state"); > > - } > > } > > EXPORT_SYMBOL_GPL(debug_object_active_state); > > @@ -999,11 +990,9 @@ EXPORT_SYMBOL_GPL(debug_object_active_state); > > static void __debug_check_no_obj_freed(const void *address, unsigned long size) > > { > > unsigned long flags, oaddr, saddr, eaddr, paddr, chunks; > > - const struct debug_obj_descr *descr; > > - enum debug_obj_state state; > > struct debug_bucket *db; > > struct hlist_node *tmp; > > - struct debug_obj *obj; > > + struct debug_obj *obj, o; > > int cnt, objs_checked = 0; > > saddr = (unsigned long) address; > > @@ -1026,12 +1015,11 @@ static void __debug_check_no_obj_freed(const void *address, unsigned long size) > > switch (obj->state) { > > case ODEBUG_STATE_ACTIVE: > > - descr = obj->descr; > > - state = obj->state; > > + o = *obj; > > raw_spin_unlock_irqrestore(&db->lock, flags); > > - debug_print_object(obj, "free"); > > - debug_object_fixup(descr->fixup_free, > > - (void *) oaddr, state); > > + debug_print_object(&o, "free"); > > + debug_object_fixup(o.descr->fixup_free, > > + (void *) oaddr, o.state); > > goto repeat; > > default: > > hlist_del(&obj->node);
On Mon, Sep 25 2023 at 15:13, Andrzej Hajda wrote: > After spinlock release object can be modified/freed by concurrent thread. > Using it in such case is error prone, even for printing object state. It cannot be freed. If that happens then the calling code will have an UAF problem on the tracked item too. If there is a concurrent modification then again, the calling code is lacking serialization on the tracked object. debugobject fundamentally relies on the call site being consistent simply because it _cannot_ invoke the fixup callbacks with the hash bucket lock held. What's the actualy problem you are trying to solve here. The changelog does not explain anything except of handwaving about modified/freed. Thanks, tglx
On 13.10.2023 15:15, Thomas Gleixner wrote: > On Mon, Sep 25 2023 at 15:13, Andrzej Hajda wrote: >> After spinlock release object can be modified/freed by concurrent thread. >> Using it in such case is error prone, even for printing object state. > > It cannot be freed. If that happens then the calling code will have an > UAF problem on the tracked item too. Yes, and I have assumed that debugobjects are created also for detecting UAFs. They should be able to handle/detect 'issues due to incorrectly serialized concurrent accesses' scenarios as well, at least some of them. And even if it cannot recover it should at least provide reliable reporting. Now we can have scenario: 1. Thread tries to deactivate destroyed object, debugobjects detects it, spin lock is released, thread is preempted. 2. Other thread frees debugobject, then allocates new one on the same memory location, ie 'obj' variable from 1st thread point to it - it is possible because there is no locking. 3. Then preemption occurs, and 1st thread reports error for wrong object. This seems the most drastic for me, but also with lowest chances to happen due to delayed freeing, but there are also other more probable scenarios when we print the same object but in state different from the one when debugobject detected issue, due to modification by concurrent thread. > > If there is a concurrent modification then again, the calling code is > lacking serialization on the tracked object. > > debugobject fundamentally relies on the call site being consistent > simply because it _cannot_ invoke the fixup callbacks with the hash > bucket lock held. Hmm, if call site is consistent then 'fixup' seems unnecessary, together with debugobjects. I guess 'fixup' users should take care of locking on they own in such case, as it is currently, nothing changed. > > What's the actualy problem you are trying to solve here. The changelog > does not explain anything except of handwaving about modified/freed. Presented above. Regards Andrzej > > Thanks, > > tglx
On Thu, Oct 19 2023 at 12:31, Andrzej Hajda wrote: > On 13.10.2023 15:15, Thomas Gleixner wrote: >> It cannot be freed. If that happens then the calling code will have an >> UAF problem on the tracked item too. > > Yes, and I have assumed that debugobjects are created also for detecting > UAFs. Kinda. > They should be able to handle/detect 'issues due to incorrectly > serialized concurrent accesses' scenarios as well, at least some of > them. And even if it cannot recover it should at least provide reliable > reporting. Fair enough. > Now we can have scenario: > 1. Thread tries to deactivate destroyed object, debugobjects detects it, > spin lock is released, thread is preempted. > 2. Other thread frees debugobject, then allocates new one on the same > memory location, ie 'obj' variable from 1st thread point to it - it is > possible because there is no locking. > 3. Then preemption occurs, and 1st thread reports error for wrong object. > > This seems the most drastic for me, but also with lowest chances to > happen due to delayed freeing, but there are also other more probable > scenarios when we print the same object but in state different from the > one when debugobject detected issue, due to modification by concurrent > thread. Now I understand what you mean. This information should be in the changelog, no? Let me stare at the patch once more.
On Mon, Sep 25 2023 at 15:13, Andrzej Hajda wrote: > @@ -620,9 +620,8 @@ static void debug_objects_fill_pool(void) > static void > __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack) > { > - enum debug_obj_state state; > struct debug_bucket *db; > - struct debug_obj *obj; > + struct debug_obj *obj, o; > unsigned long flags; > > debug_objects_fill_pool(); > @@ -644,23 +643,19 @@ __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack > case ODEBUG_STATE_INACTIVE: > obj->state = ODEBUG_STATE_INIT; > break; > - > - case ODEBUG_STATE_ACTIVE: > - state = obj->state; > - raw_spin_unlock_irqrestore(&db->lock, flags); > - debug_print_object(obj, "init"); > - debug_object_fixup(descr->fixup_init, addr, state); > - return; > - > - case ODEBUG_STATE_DESTROYED: > - raw_spin_unlock_irqrestore(&db->lock, flags); > - debug_print_object(obj, "init"); > - return; > default: > - break; > + o = *obj; > + obj = NULL; > } > > raw_spin_unlock_irqrestore(&db->lock, flags); > + > + if (obj) > + return; Hmm. I'd rather write is this way: case ODEBUG_STATE_INIT: case ODEBUG_STATE_INACTIVE: obj->state = ODEBUG_STATE_INIT; raw_spin_unlock_irqrestore(&db->lock, flags); return; default: break; } o = *obj; raw_spin_unlock_irqrestore(&db->lock, flags); debug_print_object(&o, "init"); if (o.state == ODEBUG_STATE_ACTIVE) debug_object_fixup(descr->fixup_init, addr, o.state); This spares the 'obj' pointer modification and the conditional. The extra raw_spin_unlock_irqrestore() is not the end of the world. > void debug_object_activate(void *addr, const struct debug_obj_descr *descr) ... > default: > - ret = 0; > break; > } > - raw_spin_unlock_irqrestore(&db->lock, flags); > - if (print_object) > - debug_print_object(obj, "activate"); > - return ret; > + } else { > + o.object = addr; > + o.state = ODEBUG_STATE_NOTAVAILABLE; > + o.descr = descr; > + obj = NULL; Hrmm. Just keep the struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr }; around and get rid of this else clause. > } > > raw_spin_unlock_irqrestore(&db->lock, flags); > > - /* If NULL the allocation has hit OOM */ > - if (!obj) { > - debug_objects_oom(); > + if (obj) > return 0; Plus a similar change as above to get rid of this conditional and just have the failure path here. > @@ -788,30 +777,29 @@ void debug_object_deactivate(void *addr, const struct debug_obj_descr *descr) > case ODEBUG_STATE_INIT: > case ODEBUG_STATE_INACTIVE: > case ODEBUG_STATE_ACTIVE: > - if (!obj->astate) > + if (!obj->astate) { > obj->state = ODEBUG_STATE_INACTIVE; > - else > - print_object = true; > - break; > - > + break; > + } > + fallthrough; > case ODEBUG_STATE_DESTROYED: > - print_object = true; > + o = *obj; > + obj = NULL; > break; > default: > break; > } > + } else { > + o.object = addr; > + o.state = ODEBUG_STATE_NOTAVAILABLE; > + o.descr = descr; > + obj = NULL; > } Same here. struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr }; ... if (obj) { switch (obj->state) { case ODEBUG_STATE_DESTROYED: o = *obj; break; case ODEBUG_STATE_INIT: case ODEBUG_STATE_INACTIVE: case ODEBUG_STATE_ACTIVE: if (obj->astate) { o = *obj; break; } obj->state = ODEBUG_STATE_INACTIVE; fallthrough; default: raw_spin_unlock_irqrestore(&db->lock, flags); return; } } raw_spin_unlock_irqrestore(&db->lock, flags); debug_print_object(&o, "deactivate"); Hmm? > @@ -970,28 +962,27 @@ debug_object_active_state(void *addr, const struct debug_obj_descr *descr, > if (obj) { > switch (obj->state) { > case ODEBUG_STATE_ACTIVE: > - if (obj->astate == expect) > + if (obj->astate == expect) { > obj->astate = next; > - else > - print_object = true; > - break; > - > + break; > + } > + fallthrough; > default: > - print_object = true; > + o = *obj; > + obj = NULL; > break; > } > + } else { > + o.object = addr; > + o.state = ODEBUG_STATE_NOTAVAILABLE; > + o.descr = descr; > + obj = NULL; > } Same pattern here. Thanks, tglx
diff --git a/lib/debugobjects.c b/lib/debugobjects.c index a517256a270b71..3afff2f668fc1e 100644 --- a/lib/debugobjects.c +++ b/lib/debugobjects.c @@ -620,9 +620,8 @@ static void debug_objects_fill_pool(void) static void __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack) { - enum debug_obj_state state; struct debug_bucket *db; - struct debug_obj *obj; + struct debug_obj *obj, o; unsigned long flags; debug_objects_fill_pool(); @@ -644,23 +643,19 @@ __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack case ODEBUG_STATE_INACTIVE: obj->state = ODEBUG_STATE_INIT; break; - - case ODEBUG_STATE_ACTIVE: - state = obj->state; - raw_spin_unlock_irqrestore(&db->lock, flags); - debug_print_object(obj, "init"); - debug_object_fixup(descr->fixup_init, addr, state); - return; - - case ODEBUG_STATE_DESTROYED: - raw_spin_unlock_irqrestore(&db->lock, flags); - debug_print_object(obj, "init"); - return; default: - break; + o = *obj; + obj = NULL; } raw_spin_unlock_irqrestore(&db->lock, flags); + + if (obj) + return; + + debug_print_object(&o, "init"); + if (o.state == ODEBUG_STATE_ACTIVE) + debug_object_fixup(descr->fixup_init, addr, o.state); } /** @@ -700,12 +695,9 @@ EXPORT_SYMBOL_GPL(debug_object_init_on_stack); */ 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; + struct debug_obj *obj, o; unsigned long flags; - int ret; if (!debug_objects_enabled) return 0; @@ -717,49 +709,47 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr) raw_spin_lock_irqsave(&db->lock, flags); obj = lookup_object_or_alloc(addr, db, descr, false, true); - if (likely(!IS_ERR_OR_NULL(obj))) { - bool print_object = false; - + if (unlikely(!obj)) { + raw_spin_unlock_irqrestore(&db->lock, flags); + debug_objects_oom(); + return 0; + } else if (likely(!IS_ERR(obj))) { switch (obj->state) { case ODEBUG_STATE_INIT: case ODEBUG_STATE_INACTIVE: obj->state = ODEBUG_STATE_ACTIVE; - ret = 0; break; - case ODEBUG_STATE_ACTIVE: - state = obj->state; - raw_spin_unlock_irqrestore(&db->lock, flags); - debug_print_object(obj, "activate"); - ret = debug_object_fixup(descr->fixup_activate, addr, state); - return ret ? 0 : -EINVAL; - case ODEBUG_STATE_DESTROYED: - print_object = true; - ret = -EINVAL; + o = *obj; + obj = NULL; break; default: - ret = 0; break; } - raw_spin_unlock_irqrestore(&db->lock, flags); - if (print_object) - debug_print_object(obj, "activate"); - return ret; + } else { + o.object = addr; + o.state = ODEBUG_STATE_NOTAVAILABLE; + o.descr = descr; + obj = NULL; } raw_spin_unlock_irqrestore(&db->lock, flags); - /* If NULL the allocation has hit OOM */ - if (!obj) { - debug_objects_oom(); + if (obj) 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; + + switch (o.state) { + case ODEBUG_STATE_ACTIVE: + case ODEBUG_STATE_NOTAVAILABLE: + if (debug_object_fixup(descr->fixup_activate, addr, o.state)) + return 0; + fallthrough; + default: + return -EINVAL; + } } EXPORT_SYMBOL_GPL(debug_object_activate); @@ -771,9 +761,8 @@ EXPORT_SYMBOL_GPL(debug_object_activate); void debug_object_deactivate(void *addr, const struct debug_obj_descr *descr) { struct debug_bucket *db; - struct debug_obj *obj; + struct debug_obj *obj, o; unsigned long flags; - bool print_object = false; if (!debug_objects_enabled) return; @@ -788,30 +777,29 @@ void debug_object_deactivate(void *addr, const struct debug_obj_descr *descr) case ODEBUG_STATE_INIT: case ODEBUG_STATE_INACTIVE: case ODEBUG_STATE_ACTIVE: - if (!obj->astate) + if (!obj->astate) { obj->state = ODEBUG_STATE_INACTIVE; - else - print_object = true; - break; - + break; + } + fallthrough; case ODEBUG_STATE_DESTROYED: - print_object = true; + o = *obj; + obj = NULL; break; default: break; } + } else { + o.object = addr; + o.state = ODEBUG_STATE_NOTAVAILABLE; + o.descr = descr; + obj = NULL; } raw_spin_unlock_irqrestore(&db->lock, flags); - if (!obj) { - struct debug_obj o = { .object = addr, - .state = ODEBUG_STATE_NOTAVAILABLE, - .descr = descr }; + if (!obj) debug_print_object(&o, "deactivate"); - } else if (print_object) { - debug_print_object(obj, "deactivate"); - } } EXPORT_SYMBOL_GPL(debug_object_deactivate); @@ -822,11 +810,9 @@ EXPORT_SYMBOL_GPL(debug_object_deactivate); */ void debug_object_destroy(void *addr, const struct debug_obj_descr *descr) { - enum debug_obj_state state; struct debug_bucket *db; - struct debug_obj *obj; + struct debug_obj *obj, o; unsigned long flags; - bool print_object = false; if (!debug_objects_enabled) return; @@ -836,8 +822,10 @@ void debug_object_destroy(void *addr, const struct debug_obj_descr *descr) raw_spin_lock_irqsave(&db->lock, flags); obj = lookup_object(addr, db); - if (!obj) - goto out_unlock; + if (!obj) { + raw_spin_unlock_irqrestore(&db->lock, flags); + return; + } switch (obj->state) { case ODEBUG_STATE_NONE: @@ -846,22 +834,23 @@ void debug_object_destroy(void *addr, const struct debug_obj_descr *descr) obj->state = ODEBUG_STATE_DESTROYED; break; case ODEBUG_STATE_ACTIVE: - state = obj->state; - raw_spin_unlock_irqrestore(&db->lock, flags); - debug_print_object(obj, "destroy"); - debug_object_fixup(descr->fixup_destroy, addr, state); - return; - case ODEBUG_STATE_DESTROYED: - print_object = true; + o = *obj; + obj = NULL; break; default: break; } -out_unlock: + raw_spin_unlock_irqrestore(&db->lock, flags); - if (print_object) - debug_print_object(obj, "destroy"); + + if (obj) + return; + + debug_print_object(&o, "destroy"); + + if (o.state == ODEBUG_STATE_ACTIVE) + debug_object_fixup(descr->fixup_destroy, addr, o.state); } EXPORT_SYMBOL_GPL(debug_object_destroy); @@ -872,9 +861,8 @@ EXPORT_SYMBOL_GPL(debug_object_destroy); */ void debug_object_free(void *addr, const struct debug_obj_descr *descr) { - enum debug_obj_state state; struct debug_bucket *db; - struct debug_obj *obj; + struct debug_obj *obj, o; unsigned long flags; if (!debug_objects_enabled) @@ -885,24 +873,29 @@ void debug_object_free(void *addr, const struct debug_obj_descr *descr) raw_spin_lock_irqsave(&db->lock, flags); obj = lookup_object(addr, db); - if (!obj) - goto out_unlock; + if (!obj) { + raw_spin_unlock_irqrestore(&db->lock, flags); + return; + } switch (obj->state) { case ODEBUG_STATE_ACTIVE: - state = obj->state; - raw_spin_unlock_irqrestore(&db->lock, flags); - debug_print_object(obj, "free"); - debug_object_fixup(descr->fixup_free, addr, state); - return; + o = *obj; + obj = NULL; + break; default: hlist_del(&obj->node); - raw_spin_unlock_irqrestore(&db->lock, flags); + } + + raw_spin_unlock_irqrestore(&db->lock, flags); + + if (obj) { free_object(obj); return; } -out_unlock: - raw_spin_unlock_irqrestore(&db->lock, flags); + + debug_print_object(&o, "free"); + debug_object_fixup(descr->fixup_free, addr, o.state); } EXPORT_SYMBOL_GPL(debug_object_free); @@ -955,9 +948,8 @@ debug_object_active_state(void *addr, const struct debug_obj_descr *descr, unsigned int expect, unsigned int next) { struct debug_bucket *db; - struct debug_obj *obj; + struct debug_obj *obj, o; unsigned long flags; - bool print_object = false; if (!debug_objects_enabled) return; @@ -970,28 +962,27 @@ debug_object_active_state(void *addr, const struct debug_obj_descr *descr, if (obj) { switch (obj->state) { case ODEBUG_STATE_ACTIVE: - if (obj->astate == expect) + if (obj->astate == expect) { obj->astate = next; - else - print_object = true; - break; - + break; + } + fallthrough; default: - print_object = true; + o = *obj; + obj = NULL; break; } + } else { + o.object = addr; + o.state = ODEBUG_STATE_NOTAVAILABLE; + o.descr = descr; + obj = NULL; } raw_spin_unlock_irqrestore(&db->lock, flags); - if (!obj) { - struct debug_obj o = { .object = addr, - .state = ODEBUG_STATE_NOTAVAILABLE, - .descr = descr }; + if (!obj) debug_print_object(&o, "active_state"); - } else if (print_object) { - debug_print_object(obj, "active_state"); - } } EXPORT_SYMBOL_GPL(debug_object_active_state); @@ -999,11 +990,9 @@ EXPORT_SYMBOL_GPL(debug_object_active_state); static void __debug_check_no_obj_freed(const void *address, unsigned long size) { unsigned long flags, oaddr, saddr, eaddr, paddr, chunks; - const struct debug_obj_descr *descr; - enum debug_obj_state state; struct debug_bucket *db; struct hlist_node *tmp; - struct debug_obj *obj; + struct debug_obj *obj, o; int cnt, objs_checked = 0; saddr = (unsigned long) address; @@ -1026,12 +1015,11 @@ static void __debug_check_no_obj_freed(const void *address, unsigned long size) switch (obj->state) { case ODEBUG_STATE_ACTIVE: - descr = obj->descr; - state = obj->state; + o = *obj; raw_spin_unlock_irqrestore(&db->lock, flags); - debug_print_object(obj, "free"); - debug_object_fixup(descr->fixup_free, - (void *) oaddr, state); + debug_print_object(&o, "free"); + debug_object_fixup(o.descr->fixup_free, + (void *) oaddr, o.state); goto repeat; default: hlist_del(&obj->node);
After spinlock release object can be modified/freed by concurrent thread. Using it in such case is error prone, even for printing object state. To avoid such situation local copy of the object is created if necessary. Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com> --- v2: add missing switch breaks --- lib/debugobjects.c | 206 +++++++++++++++++++++------------------------ 1 file changed, 97 insertions(+), 109 deletions(-)