Messages in this thread | | | From | Stephen Boyd <> | Date | Wed, 12 Apr 2023 17:17:05 -0700 | Subject | Re: [PATCH] debugobject: Prevent init race with static objects |
| |
Quoting Thomas Gleixner (2023-04-12 00:54:39) > Statically initialized objects are usually not initialized via the init() > function of the subsystem. They are special cased and the subsystem [...] > > Rework the code so that the lookup, the static object check and the > tracking object association happens atomically under the hash bucket > lock. This prevents the issue completely as all callers are serialized on > the hash bucket lock and therefore cannot observe inconsistent state. > > Fixes: 3ac7fe5a4aab ("infrastructure to debug (dynamic) objects") > Reported-by: syzbot+5093ba19745994288b53@syzkaller.appspotmail.com > Debugged-by: Schspa Shi <schspa@gmail.com> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Link: https://syzkaller.appspot.com/bug?id=22c8a5938eab640d1c6bcc0e3dc7be519d878462 > Link: https://lore.kernel.org/lkml/20230303161906.831686-1-schspa@gmail.com > ---
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Tiny nitpick below
> > --- a/lib/debugobjects.c > +++ b/lib/debugobjects.c > @@ -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 */
s/allocaction/allocation/
Or is this "alloc action"? Or should it be "lookup_object_or_alloc() 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); > > @@ -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 */
Same comment.
| |