[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87sfdw8yru.ffs@tglx>
Date: Wed, 22 Mar 2023 18:46:29 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Schspa Shi <schspa@...il.com>
Cc: longman@...hat.com, swboyd@...omium.org, linux@...ck-us.net,
wuchi.zero@...il.com, linux-kernel@...r.kernel.org,
syzbot+5093ba19745994288b53@...kaller.appspotmail.com
Subject: Re: [PATCH 1/2] debugobject: fix concurrency issues with
is_static_object
On Wed, Mar 22 2023 at 23:40, Schspa Shi wrote:
> Thomas Gleixner <tglx@...utronix.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
Powered by blists - more mailing lists