Messages in this thread | | | From | Thomas Gleixner <> | Subject | Re: [PATCH 1/2] debugobject: fix concurrency issues with is_static_object | Date | Wed, 22 Mar 2023 18:46:29 +0100 |
| |
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.
> 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.
> I think we should introduce lookup_object_or_alloc and is_static at the > same time.
What for?
Thanks,
tglx
| |