From: Andi Shyti <andi.shyti@linux.intel.com>
To: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: linux-kernel@vger.kernel.org, intel-gfx@lists.freedesktop.org,
linux-mm@kvack.org, Thomas Gleixner <tglx@linutronix.de>,
Andi Shyti <andi.shyti@linux.intel.com>,
Nirmoy Das <nirmoy.das@intel.com>,
Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Subject: Re: [PATCH v2] debugobjects: stop accessing objects after releasing spinlock
Date: Tue, 10 Oct 2023 14:10:36 +0200 [thread overview]
Message-ID: <ZSU/PD5QhLA663Re@ashyti-mobl2.lan> (raw)
In-Reply-To: <e0f9f143-b2b7-4281-8954-e981e744f0f6@intel.com>
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);
WARNING: multiple messages have this Message-ID (diff)
From: Andi Shyti <andi.shyti@linux.intel.com>
To: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org,
linux-mm@kvack.org, Thomas Gleixner <tglx@linutronix.de>,
Nirmoy Das <nirmoy.das@intel.com>
Subject: Re: [Intel-gfx] [PATCH v2] debugobjects: stop accessing objects after releasing spinlock
Date: Tue, 10 Oct 2023 14:10:36 +0200 [thread overview]
Message-ID: <ZSU/PD5QhLA663Re@ashyti-mobl2.lan> (raw)
In-Reply-To: <e0f9f143-b2b7-4281-8954-e981e744f0f6@intel.com>
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);
next prev parent reply other threads:[~2023-10-10 12:10 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-25 13:13 [PATCH v2] debugobjects: stop accessing objects after releasing spinlock Andrzej Hajda
2023-09-25 13:13 ` [Intel-gfx] " Andrzej Hajda
2023-09-26 2:03 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for debugobjects: stop accessing objects after releasing spinlock (rev2) Patchwork
2023-09-26 2:16 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2023-09-26 9:50 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2023-10-10 12:02 ` [PATCH v2] debugobjects: stop accessing objects after releasing spinlock Andrzej Hajda
2023-10-10 12:02 ` [Intel-gfx] " Andrzej Hajda
2023-10-10 12:10 ` Andi Shyti [this message]
2023-10-10 12:10 ` Andi Shyti
2023-10-13 13:15 ` Thomas Gleixner
2023-10-13 13:15 ` [Intel-gfx] " Thomas Gleixner
2023-10-19 10:31 ` Andrzej Hajda
2023-10-23 16:11 ` Thomas Gleixner
2023-10-24 12:56 ` Thomas Gleixner
2023-10-24 12:56 ` Thomas Gleixner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZSU/PD5QhLA663Re@ashyti-mobl2.lan \
--to=andi.shyti@linux.intel.com \
--cc=andrzej.hajda@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=janusz.krzysztofik@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=nirmoy.das@intel.com \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.