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 01:55:34 +0800 |
| |
Thomas Gleixner <tglx@linutronix.de> writes:
> On Wed, Mar 22 2023 at 23:40, Schspa Shi wrote: >> Thomas Gleixner <tglx@linutronix.de> writes: >>>> + } else { >>>> + /* >>>> + * The debug object is inited, and we should check this again >>>> + */ >>>> + if (obj->is_static) { >>>> + raw_spin_unlock_irqrestore(&db->lock, flags); >>>> + return; >>> >>> This is broken. If the object is static and already hashed and in active >>> state then this returns and fails to detect the re-initialization of an >>> active object. >>> >> >> Yes, it's right, this can be fixed by pass a skip_ifstatic parameters >> from debug_object_activate. then re-initialization of an active object >> can be detected. > >>>> -static __initdata struct self_test obj = { .static_init = 0 }; >>>> +static struct self_test obj __initdata = { .static_init = 0 }; >>>> +static struct self_test sobj __initdata = { .static_init = 1 }; >>> >>> ... >>> >>>> - obj.static_init = 1; >>> >>> Plus the s/obj/sobj/ which should be equivalent, unless I'm missing >>> something here. >>> >> >> We have saved the is_static state when it is used at the first time, so >> the is_static_object function won't be called in this environment. > > There is zero requirement for saving that state. > >>> lib/debugobjects.c | 127 +++++++++++++++++++++++++++-------------------------- >>> 1 file changed, 67 insertions(+), 60 deletions(-) >>> >>> --- 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) >>> { >>> @@ -273,7 +269,7 @@ alloc_object(void *addr, struct debug_bu >>> if (obj) { >>> obj->object = addr; >>> obj->descr = descr; >>> - obj->state = ODEBUG_STATE_NONE; >>> + obj->state = ODEBUG_STATE_INIT; >> >> This actually droped the ODEBUG_STATE_NONE state. If we active a >> uninitialized object, there will be no error report. > > Indeed. > >> This should be >> >> if (descr->is_static_object && descr->is_static_object(addr)) >> obj->state = ODEBUG_STATE_INIT; >> else >> obj->state = ODEBUG_STATE_NONE; > > Kinda. > >> But this can't resolve the initial state requirement from the >> is_static_object() call. > > Which requirement? The is_static_object() call takes the address of the > actual object and has nothing to do with the tracking object at all. >
This is for the fellowing test case, actually we calls debug_object_free() from a static object in our selftest, if we don't report any thing when call debug_object_free from a static object, we there is no such issues.
obj.static_init = 1; debug_object_activate(&obj, &descr_type_test); if (check_results(&obj, ODEBUG_STATE_ACTIVE, fixups, warnings)) goto out; debug_object_init(&obj, &descr_type_test); if (check_results(&obj, ODEBUG_STATE_INIT, ++fixups, ++warnings)) goto out;
/* * We need to remove the debug_object_free here, because it's not * a legal operation. */ - debug_object_free(&obj, &descr_type_test); - if (check_results(&obj, ODEBUG_STATE_NONE, fixups, warnings)) - goto out;
#if 0 /* * for the static debugobject, it's initial value will be changed * once used. */ obj.static_init = 2; debug_object_activate(&obj, &descr_type_test); if (check_results(&obj, ODEBUG_STATE_ACTIVE, fixups, warnings)) goto out;
/* This test will fail */ #endif
>> I think we can report an error when calling debug_object_free() from a >> static object. If don't do so, there is no way to determine it's a >> static object. > > The memory allocator will tell you loudly when you try to free a static > object. So no point in having another check. > >> When its initialization state changes, the is_static_object() call >> will return the wrong value. > > That call is only relevant on the first invocation when there is no > tracking object yet. So what's the problem you are trying to solve? > >> Please see the fellowing test case: >> >> obj.static_init = 1; > > This is pointless, really. Once the object is tracked it does not matter > at all whether it was statically or dynamically allocated. > >> >> I test this patch, with my above change, and it seems to work well, but >> we still need to add extra flags to store its static state. And >> debug_object_free() should report an error for the static object. > > No, we don't. >
OK, we don't need to store the state if don't take care the debug_object_free() call on static object at all. If so, we should delete the debug_object_free() call on static object at debug_objects_selftest().
>> I think we should introduce lookup_object_or_alloc and is_static at the >> same time. > > What for? >
To report an error when someone calls debug_object_free on a static object.
> Thanks, > > tglx
-- BRs Schspa Shi
| |