Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
References: <20230303161906.831686-1-schspa@gmail.com> <87bkl9jt3a.ffs@tglx>
 <m2ttycd8kx.fsf@gmail.com> <87sfdw8yru.ffs@tglx>
User-agent: mu4e 1.7.5; emacs 28.2
From:   Schspa Shi <schspa@gmail.com>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     longman@redhat.com, swboyd@chromium.org, linux@roeck-us.net,
        wuchi.zero@gmail.com, linux-kernel@vger.kernel.org,
        syzbot+5093ba19745994288b53@syzkaller.appspotmail.com
Subject: Re: [PATCH 1/2] debugobject: fix concurrency issues with
 is_static_object
Date:   Thu, 23 Mar 2023 01:55:34 +0800
In-reply-to: <87sfdw8yru.ffs@tglx>
Message-ID: <m2r0tgmze9.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


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