[1/2] debugobject: fix concurrency issues with is_static_object

  The is_static_object implementation relay on the initial state of the
object. If multiple places are accessed concurrently, there is a
probability that the debug object has been registered in the system, which
will invalidate the judgment of is_static_object.

The following is the scenario where the problem occurs:

T0                                                   T1
=========================================================================
mod_timer();
  debug_object_assert_init
	db = get_bucket((unsigned long) addr);
	raw_spin_lock_irqsave(&db->lock, flags);
	obj = lookup_object(addr, db);
    if (!obj) {
		raw_spin_unlock_irqrestore(&db->lock, flags);
        << Context switch >>
                                             mod_timer();
                                               debug_object_assert_init
                                               ...
                                               enqueue_timer();
        /*
         * The initial state changed a static timer object, and
         * is_static_object will return false
         */

        if (descr->is_static_object &&
			descr->is_static_object(addr)) {
                debug_object_init();
            } else {
               << Hit here for a static object >>
			   debug_print_object(&o, "assert_init");
			   debug_object_fixup(descr->fixup_assert_init, addr,
					   ODEBUG_STATE_NOTAVAILABLE);
            }
    }

To fix it, we got the is_static_object called within db->lock, and save
it's state to struct debug_obj. This will ensure we won't hit the code
branch not belong to the static object.

For the same static object, debug_object_init may enter multiple times, but
there is a lock in debug_object_init to ensure that there is no problem.

Reported-by: syzbot+5093ba19745994288b53@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?id=22c8a5938eab640d1c6bcc0e3dc7be519d878462
Signed-off-by: Schspa Shi <schspa@gmail.com>
---
 include/linux/debugobjects.h |  1 +
 lib/debugobjects.c           | 71 ++++++++++++++++++++++++++++--------
 2 files changed, 56 insertions(+), 16 deletions(-)
  

On 3/3/23 11:19, Schspa Shi wrote:
> The is_static_object implementation relay on the initial state of the
> object. If multiple places are accessed concurrently, there is a
> probability that the debug object has been registered in the system, which
> will invalidate the judgment of is_static_object.
>
> The following is the scenario where the problem occurs:
>
> T0                                                   T1
> =========================================================================
> mod_timer();
>    debug_object_assert_init
> 	db = get_bucket((unsigned long) addr);
> 	raw_spin_lock_irqsave(&db->lock, flags);
> 	obj = lookup_object(addr, db);
>      if (!obj) {
> 		raw_spin_unlock_irqrestore(&db->lock, flags);
>          << Context switch >>
>                                               mod_timer();
>                                                 debug_object_assert_init
>                                                 ...
>                                                 enqueue_timer();
>          /*
>           * The initial state changed a static timer object, and
>           * is_static_object will return false
>           */
>
>          if (descr->is_static_object &&
> 			descr->is_static_object(addr)) {
>                  debug_object_init();
>              } else {
>                 << Hit here for a static object >>
> 			   debug_print_object(&o, "assert_init");
> 			   debug_object_fixup(descr->fixup_assert_init, addr,
> 					   ODEBUG_STATE_NOTAVAILABLE);
>              }
>      }
>
> To fix it, we got the is_static_object called within db->lock, and save
> it's state to struct debug_obj. This will ensure we won't hit the code
> branch not belong to the static object.
>
> For the same static object, debug_object_init may enter multiple times, but
> there is a lock in debug_object_init to ensure that there is no problem.
>
> Reported-by: syzbot+5093ba19745994288b53@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=22c8a5938eab640d1c6bcc0e3dc7be519d878462
> Signed-off-by: Schspa Shi <schspa@gmail.com>
> ---
>   include/linux/debugobjects.h |  1 +
>   lib/debugobjects.c           | 71 ++++++++++++++++++++++++++++--------
>   2 files changed, 56 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/debugobjects.h b/include/linux/debugobjects.h
> index 32444686b6ff4..544a6111b97f6 100644
> --- a/include/linux/debugobjects.h
> +++ b/include/linux/debugobjects.h
> @@ -30,6 +30,7 @@ struct debug_obj {
>   	enum debug_obj_state	state;
>   	unsigned int		astate;
>   	void			*object;
> +	bool			is_static;
>   	const struct debug_obj_descr *descr;
>   };

The patch looks reasonable. My main concern is the increase in size of 
the debug_obj structure. It is an additional 8 bytes on 64-bit arches. 
How much will we save performance-wise by caching it in the debug_obj. 
Alternatively, you may pack it within the current size by, maybe, 
reducing the size of state.

Cheers,
Longman

>   
> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
> index df86e649d8be0..d1be18158a1f7 100644
> --- a/lib/debugobjects.c
> +++ b/lib/debugobjects.c
> @@ -275,6 +275,8 @@ alloc_object(void *addr, struct debug_bucket *b, const struct debug_obj_descr *d
>   		obj->descr  = descr;
>   		obj->state  = ODEBUG_STATE_NONE;
>   		obj->astate = 0;
> +		obj->is_static = descr->is_static_object &&
> +			descr->is_static_object(addr);
>   		hlist_add_head(&obj->node, &b->list);
>   	}
>   	return obj;
> @@ -581,7 +583,16 @@ __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack
>   			debug_objects_oom();
>   			return;
>   		}
> +
>   		check_stack = true;
> +	} else {
> +		/*
> +		 * The debug object is inited, and we should check this again
> +		 */
> +		if (obj->is_static) {
> +			raw_spin_unlock_irqrestore(&db->lock, flags);
> +			return;
> +		}
>   	}
>   
>   	switch (obj->state) {
> @@ -640,6 +651,29 @@ void debug_object_init_on_stack(void *addr, const struct debug_obj_descr *descr)
>   }
>   EXPORT_SYMBOL_GPL(debug_object_init_on_stack);
>   
> +/*
> + * Check static object.
> + */
> +static bool debug_object_check_static(struct debug_bucket *db, void *addr,
> +			const struct debug_obj_descr *descr)
> +{
> +	struct debug_obj *obj;
> +
> +	/*
> +	 * The is_static_object implementation relay on the initial state of the
> +	 * object. If multiple places are accessed concurrently, there is a
> +	 * probability that the debug object has been registered in the system,
> +	 * which will invalidate the judgment of is_static_object.
> +	 */
> +	lockdep_assert_held(&db->lock);
> +
> +	obj = lookup_object(addr, db);
> +	if (likely(obj))
> +		return obj->is_static;
> +
> +	return descr->is_static_object && descr->is_static_object(addr);
> +}
> +
>   /**
>    * debug_object_activate - debug checks when an object is activated
>    * @addr:	address of the object
> @@ -656,6 +690,7 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr)
>   	struct debug_obj o = { .object = addr,
>   			       .state = ODEBUG_STATE_NOTAVAILABLE,
>   			       .descr = descr };
> +	bool is_static;
>   
>   	if (!debug_objects_enabled)
>   		return 0;
> @@ -696,6 +731,7 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr)
>   		return ret;
>   	}
>   
> +	is_static = debug_object_check_static(db, addr, descr);
>   	raw_spin_unlock_irqrestore(&db->lock, flags);
>   
>   	/*
> @@ -705,7 +741,7 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr)
>   	 * static object is tracked in the object tracker. If
>   	 * not, this must be a bug, so we try to fix it up.
>   	 */
> -	if (descr->is_static_object && descr->is_static_object(addr)) {
> +	if (is_static) {
>   		/* track this static object */
>   		debug_object_init(addr, descr);
>   		debug_object_activate(addr, descr);
> @@ -872,6 +908,7 @@ void debug_object_assert_init(void *addr, const struct debug_obj_descr *descr)
>   	struct debug_bucket *db;
>   	struct debug_obj *obj;
>   	unsigned long flags;
> +	bool is_static;
>   
>   	if (!debug_objects_enabled)
>   		return;
> @@ -886,13 +923,14 @@ void debug_object_assert_init(void *addr, const struct debug_obj_descr *descr)
>   				       .state = ODEBUG_STATE_NOTAVAILABLE,
>   				       .descr = descr };
>   
> +		is_static = debug_object_check_static(db, addr, descr);
>   		raw_spin_unlock_irqrestore(&db->lock, flags);
>   		/*
>   		 * Maybe the object is static, and we let the type specific
>   		 * code confirm. Track this static object if true, else invoke
>   		 * fixup.
>   		 */
> -		if (descr->is_static_object && descr->is_static_object(addr)) {
> +		if (is_static) {
>   			/* Track this static object */
>   			debug_object_init(addr, descr);
>   		} else {
> @@ -1215,7 +1253,8 @@ static __initconst const struct debug_obj_descr descr_type_test = {
>   	.fixup_free		= fixup_free,
>   };
>   
> -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 };
>   
>   static void __init debug_objects_selftest(void)
>   {
> @@ -1256,26 +1295,26 @@ static void __init debug_objects_selftest(void)
>   	if (check_results(&obj, ODEBUG_STATE_NONE, fixups, warnings))
>   		goto out;
>   
> -	obj.static_init = 1;
> -	debug_object_activate(&obj, &descr_type_test);
> -	if (check_results(&obj, ODEBUG_STATE_ACTIVE, fixups, warnings))
> +	debug_object_init(&sobj, &descr_type_test);
> +	debug_object_activate(&sobj, &descr_type_test);
> +	if (check_results(&sobj, ODEBUG_STATE_ACTIVE, fixups, warnings))
>   		goto out;
> -	debug_object_init(&obj, &descr_type_test);
> -	if (check_results(&obj, ODEBUG_STATE_INIT, ++fixups, ++warnings))
> +	debug_object_init(&sobj, &descr_type_test);
> +	if (check_results(&sobj, ODEBUG_STATE_INIT, ++fixups, ++warnings))
>   		goto out;
> -	debug_object_free(&obj, &descr_type_test);
> -	if (check_results(&obj, ODEBUG_STATE_NONE, fixups, warnings))
> +	debug_object_free(&sobj, &descr_type_test);
> +	if (check_results(&sobj, ODEBUG_STATE_NONE, fixups, warnings))
>   		goto out;
>   
>   #ifdef CONFIG_DEBUG_OBJECTS_FREE
> -	debug_object_init(&obj, &descr_type_test);
> -	if (check_results(&obj, ODEBUG_STATE_INIT, fixups, warnings))
> +	debug_object_init(&sobj, &descr_type_test);
> +	if (check_results(&sobj, ODEBUG_STATE_INIT, fixups, warnings))
>   		goto out;
> -	debug_object_activate(&obj, &descr_type_test);
> -	if (check_results(&obj, ODEBUG_STATE_ACTIVE, fixups, warnings))
> +	debug_object_activate(&sobj, &descr_type_test);
> +	if (check_results(&sobj, ODEBUG_STATE_ACTIVE, fixups, warnings))
>   		goto out;
> -	__debug_check_no_obj_freed(&obj, sizeof(obj));
> -	if (check_results(&obj, ODEBUG_STATE_NONE, ++fixups, ++warnings))
> +	__debug_check_no_obj_freed(&sobj, sizeof(sobj));
> +	if (check_results(&sobj, ODEBUG_STATE_NONE, ++fixups, ++warnings))
>   		goto out;
>   #endif
>   	pr_info("selftest passed\n");
  

Waiman Long <longman@redhat.com> writes:

> On 3/3/23 11:19, Schspa Shi wrote:
>> The is_static_object implementation relay on the initial state of the
>> object. If multiple places are accessed concurrently, there is a
>> probability that the debug object has been registered in the system, which
>> will invalidate the judgment of is_static_object.
>>
>> The following is the scenario where the problem occurs:
>>
>> T0                                                   T1
>> =========================================================================
>> mod_timer();
>>    debug_object_assert_init
>> 	db = get_bucket((unsigned long) addr);
>> 	raw_spin_lock_irqsave(&db->lock, flags);
>> 	obj = lookup_object(addr, db);
>>      if (!obj) {
>> 		raw_spin_unlock_irqrestore(&db->lock, flags);
>>          << Context switch >>
>>                                               mod_timer();
>>                                                 debug_object_assert_init
>>                                                 ...
>>                                                 enqueue_timer();
>>          /*
>>           * The initial state changed a static timer object, and
>>           * is_static_object will return false
>>           */
>>
>>          if (descr->is_static_object &&
>> 			descr->is_static_object(addr)) {
>>                  debug_object_init();
>>              } else {
>>                 << Hit here for a static object >>
>> 			   debug_print_object(&o, "assert_init");
>> 			   debug_object_fixup(descr->fixup_assert_init, addr,
>> 					   ODEBUG_STATE_NOTAVAILABLE);
>>              }
>>      }
>>
>> To fix it, we got the is_static_object called within db->lock, and save
>> it's state to struct debug_obj. This will ensure we won't hit the code
>> branch not belong to the static object.
>>
>> For the same static object, debug_object_init may enter multiple times, but
>> there is a lock in debug_object_init to ensure that there is no problem.
>>
>> Reported-by: syzbot+5093ba19745994288b53@syzkaller.appspotmail.com
>> Link: https://syzkaller.appspot.com/bug?id=22c8a5938eab640d1c6bcc0e3dc7be519d878462
>> Signed-off-by: Schspa Shi <schspa@gmail.com>
>> ---
>>   include/linux/debugobjects.h |  1 +
>>   lib/debugobjects.c           | 71 ++++++++++++++++++++++++++++--------
>>   2 files changed, 56 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/linux/debugobjects.h b/include/linux/debugobjects.h
>> index 32444686b6ff4..544a6111b97f6 100644
>> --- a/include/linux/debugobjects.h
>> +++ b/include/linux/debugobjects.h
>> @@ -30,6 +30,7 @@ struct debug_obj {
>>   	enum debug_obj_state	state;
>>   	unsigned int		astate;
>>   	void			*object;
>> +	bool			is_static;
>>   	const struct debug_obj_descr *descr;
>>   };
>
> The patch looks reasonable. My main concern is the increase in size of the
> debug_obj structure. It is an additional 8 bytes on 64-bit arches. How much will
> we save performance-wise by caching it in the debug_obj. Alternatively, you may
> pack it within the current size by, maybe, reducing the size of state.
>

Yes, we can change this to:

struct debug_obj {
	struct hlist_node	node;
	struct {
		enum debug_obj_state	state : 31;
		bool					is_static : 1;
	};
	unsigned int		astate;
	void			*object;
	const struct debug_obj_descr *descr;
};

Which won't increase the object size.

(gdb) ptype /o struct debug_obj
/* offset      |    size */  type = struct debug_obj {
/*      0      |       0 */    struct hlist_node {
                                   <incomplete type>

                                   /* total size (bytes):    0 */
                               } node;
/*     16      |       4 */    struct {
/*     16: 0   |       4 */        enum debug_obj_state state : 31;
/*     19: 7   |       1 */        bool is_static : 1;

                                   /* total size (bytes):    4 */
                               };
/*     20      |       4 */    unsigned int astate;
/*     24      |       8 */    void *object;
/*     32      |       8 */    const struct debug_obj_descr *descr;

                               /* total size (bytes):   40 */
                             }


> Cheers,
> Longman
>
>>   diff --git a/lib/debugobjects.c b/lib/debugobjects.c
>> index df86e649d8be0..d1be18158a1f7 100644
>> --- a/lib/debugobjects.c
>> +++ b/lib/debugobjects.c
>> @@ -275,6 +275,8 @@ alloc_object(void *addr, struct debug_bucket *b, const struct debug_obj_descr *d
>>   		obj->descr  = descr;
>>   		obj->state  = ODEBUG_STATE_NONE;
>>   		obj->astate = 0;
>> +		obj->is_static = descr->is_static_object &&
>> +			descr->is_static_object(addr);
>>   		hlist_add_head(&obj->node, &b->list);
>>   	}
>>   	return obj;
>> @@ -581,7 +583,16 @@ __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack
>>   			debug_objects_oom();
>>   			return;
>>   		}
>> +
>>   		check_stack = true;
>> +	} else {
>> +		/*
>> +		 * The debug object is inited, and we should check this again
>> +		 */
>> +		if (obj->is_static) {
>> +			raw_spin_unlock_irqrestore(&db->lock, flags);
>> +			return;
>> +		}
>>   	}
>>     	switch (obj->state) {
>> @@ -640,6 +651,29 @@ void debug_object_init_on_stack(void *addr, const struct debug_obj_descr *descr)
>>   }
>>   EXPORT_SYMBOL_GPL(debug_object_init_on_stack);
>>   +/*
>> + * Check static object.
>> + */
>> +static bool debug_object_check_static(struct debug_bucket *db, void *addr,
>> +			const struct debug_obj_descr *descr)
>> +{
>> +	struct debug_obj *obj;
>> +
>> +	/*
>> +	 * The is_static_object implementation relay on the initial state of the
>> +	 * object. If multiple places are accessed concurrently, there is a
>> +	 * probability that the debug object has been registered in the system,
>> +	 * which will invalidate the judgment of is_static_object.
>> +	 */
>> +	lockdep_assert_held(&db->lock);
>> +
>> +	obj = lookup_object(addr, db);
>> +	if (likely(obj))
>> +		return obj->is_static;
>> +
>> +	return descr->is_static_object && descr->is_static_object(addr);
>> +}
>> +
>>   /**
>>    * debug_object_activate - debug checks when an object is activated
>>    * @addr:	address of the object
>> @@ -656,6 +690,7 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr)
>>   	struct debug_obj o = { .object = addr,
>>   			       .state = ODEBUG_STATE_NOTAVAILABLE,
>>   			       .descr = descr };
>> +	bool is_static;
>>     	if (!debug_objects_enabled)
>>   		return 0;
>> @@ -696,6 +731,7 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr)
>>   		return ret;
>>   	}
>>   +	is_static = debug_object_check_static(db, addr, descr);
>>   	raw_spin_unlock_irqrestore(&db->lock, flags);
>>     	/*
>> @@ -705,7 +741,7 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr)
>>   	 * static object is tracked in the object tracker. If
>>   	 * not, this must be a bug, so we try to fix it up.
>>   	 */
>> -	if (descr->is_static_object && descr->is_static_object(addr)) {
>> +	if (is_static) {
>>   		/* track this static object */
>>   		debug_object_init(addr, descr);
>>   		debug_object_activate(addr, descr);
>> @@ -872,6 +908,7 @@ void debug_object_assert_init(void *addr, const struct debug_obj_descr *descr)
>>   	struct debug_bucket *db;
>>   	struct debug_obj *obj;
>>   	unsigned long flags;
>> +	bool is_static;
>>     	if (!debug_objects_enabled)
>>   		return;
>> @@ -886,13 +923,14 @@ void debug_object_assert_init(void *addr, const struct debug_obj_descr *descr)
>>   				       .state = ODEBUG_STATE_NOTAVAILABLE,
>>   				       .descr = descr };
>>   +		is_static = debug_object_check_static(db, addr, descr);
>>   		raw_spin_unlock_irqrestore(&db->lock, flags);
>>   		/*
>>   		 * Maybe the object is static, and we let the type specific
>>   		 * code confirm. Track this static object if true, else invoke
>>   		 * fixup.
>>   		 */
>> -		if (descr->is_static_object && descr->is_static_object(addr)) {
>> +		if (is_static) {
>>   			/* Track this static object */
>>   			debug_object_init(addr, descr);
>>   		} else {
>> @@ -1215,7 +1253,8 @@ static __initconst const struct debug_obj_descr descr_type_test = {
>>   	.fixup_free		= fixup_free,
>>   };
>>   -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 };
>>     static void __init debug_objects_selftest(void)
>>   {
>> @@ -1256,26 +1295,26 @@ static void __init debug_objects_selftest(void)
>>   	if (check_results(&obj, ODEBUG_STATE_NONE, fixups, warnings))
>>   		goto out;
>>   -	obj.static_init = 1;
>> -	debug_object_activate(&obj, &descr_type_test);
>> -	if (check_results(&obj, ODEBUG_STATE_ACTIVE, fixups, warnings))
>> +	debug_object_init(&sobj, &descr_type_test);
>> +	debug_object_activate(&sobj, &descr_type_test);
>> +	if (check_results(&sobj, ODEBUG_STATE_ACTIVE, fixups, warnings))
>>   		goto out;
>> -	debug_object_init(&obj, &descr_type_test);
>> -	if (check_results(&obj, ODEBUG_STATE_INIT, ++fixups, ++warnings))
>> +	debug_object_init(&sobj, &descr_type_test);
>> +	if (check_results(&sobj, ODEBUG_STATE_INIT, ++fixups, ++warnings))
>>   		goto out;
>> -	debug_object_free(&obj, &descr_type_test);
>> -	if (check_results(&obj, ODEBUG_STATE_NONE, fixups, warnings))
>> +	debug_object_free(&sobj, &descr_type_test);
>> +	if (check_results(&sobj, ODEBUG_STATE_NONE, fixups, warnings))
>>   		goto out;
>>     #ifdef CONFIG_DEBUG_OBJECTS_FREE
>> -	debug_object_init(&obj, &descr_type_test);
>> -	if (check_results(&obj, ODEBUG_STATE_INIT, fixups, warnings))
>> +	debug_object_init(&sobj, &descr_type_test);
>> +	if (check_results(&sobj, ODEBUG_STATE_INIT, fixups, warnings))
>>   		goto out;
>> -	debug_object_activate(&obj, &descr_type_test);
>> -	if (check_results(&obj, ODEBUG_STATE_ACTIVE, fixups, warnings))
>> +	debug_object_activate(&sobj, &descr_type_test);
>> +	if (check_results(&sobj, ODEBUG_STATE_ACTIVE, fixups, warnings))
>>   		goto out;
>> -	__debug_check_no_obj_freed(&obj, sizeof(obj));
>> -	if (check_results(&obj, ODEBUG_STATE_NONE, ++fixups, ++warnings))
>> +	__debug_check_no_obj_freed(&sobj, sizeof(sobj));
>> +	if (check_results(&sobj, ODEBUG_STATE_NONE, ++fixups, ++warnings))
>>   		goto out;
>>   #endif
>>   	pr_info("selftest passed\n");
  

Hi!

On Sat, Mar 04 2023 at 00:19, Schspa Shi wrote:

> The is_static_object implementation relay on the initial state of the
> object. If multiple places are accessed concurrently, there is a
> probability that the debug object has been registered in the system, which
> will invalidate the judgment of is_static_object.
>
> The following is the scenario where the problem occurs:
>
> T0                                                   T1
> =========================================================================
> mod_timer();
>   debug_object_assert_init
> 	db = get_bucket((unsigned long) addr);
> 	raw_spin_lock_irqsave(&db->lock, flags);
> 	obj = lookup_object(addr, db);
>     if (!obj) {
> 		raw_spin_unlock_irqrestore(&db->lock, flags);
>         << Context switch >>
>                                              mod_timer();
>                                                debug_object_assert_init
>                                                ...
>                                                enqueue_timer();
>         /*
>          * The initial state changed a static timer object, and
>          * is_static_object will return false
>          */
>
>         if (descr->is_static_object &&
> 			descr->is_static_object(addr)) {
>                 debug_object_init();
>             } else {
>                << Hit here for a static object >>
> 			   debug_print_object(&o, "assert_init");
> 			   debug_object_fixup(descr->fixup_assert_init, addr,
> 					   ODEBUG_STATE_NOTAVAILABLE);
>             }
>     }

That analysis is correct.

> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
> index df86e649d8be0..d1be18158a1f7 100644
> --- a/lib/debugobjects.c
> +++ b/lib/debugobjects.c
> @@ -275,6 +275,8 @@ alloc_object(void *addr, struct debug_bucket *b, const struct debug_obj_descr *d
>  		obj->descr  = descr;
>  		obj->state  = ODEBUG_STATE_NONE;
>  		obj->astate = 0;
> +		obj->is_static = descr->is_static_object &&
> +			descr->is_static_object(addr);
>  		hlist_add_head(&obj->node, &b->list);
>  	}
>  	return obj;
> @@ -581,7 +583,16 @@ __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack
>  			debug_objects_oom();
>  			return;
>  		}
> +
>  		check_stack = true;
> +	} 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.

> +/*
> + * Check static object.
> + */
> +static bool debug_object_check_static(struct debug_bucket *db, void *addr,
> +			const struct debug_obj_descr *descr)
> +{
> +	struct debug_obj *obj;
> +
> +	/*
> +	 * The is_static_object implementation relay on the initial state of the
> +	 * object. If multiple places are accessed concurrently, there is a
> +	 * probability that the debug object has been registered in the system,
> +	 * which will invalidate the judgment of is_static_object.
> +	 */
> +	lockdep_assert_held(&db->lock);
> +
> +	obj = lookup_object(addr, db);

What does this lookup solve? See below...

> +	if (likely(obj))
> +		return obj->is_static;
> +
> +	return descr->is_static_object && descr->is_static_object(addr);
> +}

> @@ -696,6 +731,7 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr)
>  		return ret;
>  	}
>  
> +	is_static = debug_object_check_static(db, addr, descr);

It's already clear that the object is not hashed because activate does:

        lock()
        lookup()
        if (ob) {
           ....
           unlock();
        }

At this point nothing has changed after the lookup because
the hash bucket is still locked.

>  	raw_spin_unlock_irqrestore(&db->lock, flags);
 
>  	/*
> @@ -705,7 +741,7 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr)
>  	 * static object is tracked in the object tracker. If
>  	 * not, this must be a bug, so we try to fix it up.
>  	 */
> -	if (descr->is_static_object && descr->is_static_object(addr)) {
> +	if (is_static) {
>  		/* track this static object */
>  		debug_object_init(addr, descr);

This cannot work as the change in debug_object_init() is not correct and
there is no way to make it correct.

> -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.

But that also changes the actual test:

-	obj.static_init = 1;
-	debug_object_activate(&obj, &descr_type_test);
-	if (check_results(&obj, ODEBUG_STATE_ACTIVE, fixups, warnings))
+	debug_object_init(&sobj, &descr_type_test);
+	debug_object_activate(&sobj, &descr_type_test);
+	if (check_results(&sobj, ODEBUG_STATE_ACTIVE, fixups, warnings))
		goto out;

That's _not_ equivalent.

The original code checks exactly for the case where the object
initialization does not happen before debug_object_activate() is invoked
which is perfectly correct for statically initialized
objects. Initializing the statically allocated object breaks that
test. The changelog is utterly silent about that.

The proper fix for this is obvious from the analysis. The problem
happens because the failed lookup drops the hash bucket lock which opens
the window for the concurrent caller. So why dropping the hash bucket
lock at all?

Uncompiled and untested fix below.

Thanks,

        tglx
---
 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;
 		obj->astate = 0;
 		hlist_add_head(&obj->node, &b->list);
 	}
@@ -552,11 +548,49 @@ static void debug_object_is_on_stack(voi
 	WARN_ON(1);
 }
 
+static struct debug_obj *lookup_object_or_alloc(void *addr, struct debug_bucket *b,
+						const struct debug_obj_descr *descr,
+						bool onstack, bool alloc_ifstatic)
+{
+	struct debug_obj *obj = lookup_object(addr, b);
+
+	if (likely(obj))
+		return obj;
+
+	/*
+	 * debug_object_init() unconditionally allocates untracked
+	 * objects. It does not matter whether it is a static object or
+	 * not.
+	 *
+	 * debug_object_assert_init() and debug_object_activate() allow
+	 * allocation only if the descriptor callback confirms that the
+	 * object is static and considered initialized. For non-static
+	 * objects the allocation needs to be done from the fixup callback.
+	 */
+	if (unlikely(alloc_ifstatic)) {
+		if (!descr->is_static_object || !descr->is_static_object(addr))
+			return ERR_PTR(-ENOENT);
+	}
+
+	/*
+	 * On success the returned object is in INIT state as that's the
+	 * correct state for all callers.
+	 */
+	obj = alloc_object(addr, b, descr);
+	if (likely(obj)) {
+		debug_object_is_on_stack(addr, onstack);
+		return obj;
+	}
+
+	/* Out of memory. Do the cleanup outside of the locked region */
+	debug_objects_enabled = 0;
+	return NULL;
+}
+
 static void
 __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack)
 {
 	enum debug_obj_state state;
-	bool check_stack = false;
 	struct debug_bucket *db;
 	struct debug_obj *obj;
 	unsigned long flags;
@@ -572,16 +606,11 @@ static void
 
 	raw_spin_lock_irqsave(&db->lock, flags);
 
-	obj = lookup_object(addr, db);
-	if (!obj) {
-		obj = alloc_object(addr, db, descr);
-		if (!obj) {
-			debug_objects_enabled = 0;
-			raw_spin_unlock_irqrestore(&db->lock, flags);
-			debug_objects_oom();
-			return;
-		}
-		check_stack = true;
+	obj = lookup_object_or_alloc(addr, db, descr, onstack, false);
+	if (unlikely(!obj)) {
+		raw_spin_unlock_irqrestore(&db->lock, flags);
+		debug_objects_oom();
+		return;
 	}
 
 	switch (obj->state) {
@@ -607,8 +636,6 @@ static void
 	}
 
 	raw_spin_unlock_irqrestore(&db->lock, flags);
-	if (check_stack)
-		debug_object_is_on_stack(addr, onstack);
 }
 
 /**
@@ -648,14 +675,12 @@ EXPORT_SYMBOL_GPL(debug_object_init_on_s
  */
 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;
 	unsigned long flags;
 	int ret;
-	struct debug_obj o = { .object = addr,
-			       .state = ODEBUG_STATE_NOTAVAILABLE,
-			       .descr = descr };
 
 	if (!debug_objects_enabled)
 		return 0;
@@ -664,8 +689,8 @@ int debug_object_activate(void *addr, co
 
 	raw_spin_lock_irqsave(&db->lock, flags);
 
-	obj = lookup_object(addr, db);
-	if (obj) {
+	obj = lookup_object_or_alloc(addr, db, descr, false, true);
+	if (likely(!IS_ERR_OR_NULL(obj))) {
 		bool print_object = false;
 
 		switch (obj->state) {
@@ -698,24 +723,16 @@ int debug_object_activate(void *addr, co
 
 	raw_spin_unlock_irqrestore(&db->lock, flags);
 
-	/*
-	 * We are here when a static object is activated. We
-	 * let the type specific code confirm whether this is
-	 * true or not. if true, we just make sure that the
-	 * static object is tracked in the object tracker. If
-	 * not, this must be a bug, so we try to fix it up.
-	 */
-	if (descr->is_static_object && descr->is_static_object(addr)) {
-		/* track this static object */
-		debug_object_init(addr, descr);
-		debug_object_activate(addr, descr);
-	} else {
-		debug_print_object(&o, "activate");
-		ret = debug_object_fixup(descr->fixup_activate, addr,
-					ODEBUG_STATE_NOTAVAILABLE);
-		return ret ? 0 : -EINVAL;
+	/* If NULL the allocaction has hit OOM */
+	if (!obj) {
+		debug_objects_oom();
+		return 0;
 	}
-	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;
 }
 EXPORT_SYMBOL_GPL(debug_object_activate);
 
@@ -869,6 +886,7 @@ EXPORT_SYMBOL_GPL(debug_object_free);
  */
 void debug_object_assert_init(void *addr, const struct debug_obj_descr *descr)
 {
+	struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr };
 	struct debug_bucket *db;
 	struct debug_obj *obj;
 	unsigned long flags;
@@ -879,31 +897,20 @@ void debug_object_assert_init(void *addr
 	db = get_bucket((unsigned long) addr);
 
 	raw_spin_lock_irqsave(&db->lock, flags);
+	obj = lookup_object_or_alloc(addr, db, descr, false, true);
+	raw_spin_unlock_irqrestore(&db->lock, flags);
+	if (likely(!IS_ERR_OR_NULL(obj)))
+		return;
 
-	obj = lookup_object(addr, db);
+	/* If NULL the allocaction has hit OOM */
 	if (!obj) {
-		struct debug_obj o = { .object = addr,
-				       .state = ODEBUG_STATE_NOTAVAILABLE,
-				       .descr = descr };
-
-		raw_spin_unlock_irqrestore(&db->lock, flags);
-		/*
-		 * Maybe the object is static, and we let the type specific
-		 * code confirm. Track this static object if true, else invoke
-		 * fixup.
-		 */
-		if (descr->is_static_object && descr->is_static_object(addr)) {
-			/* Track this static object */
-			debug_object_init(addr, descr);
-		} else {
-			debug_print_object(&o, "assert_init");
-			debug_object_fixup(descr->fixup_assert_init, addr,
-					   ODEBUG_STATE_NOTAVAILABLE);
-		}
+		debug_objects_oom();
 		return;
 	}
 
-	raw_spin_unlock_irqrestore(&db->lock, flags);
+	/* Object is neither tracked nor static. It's not initialized. */
+	debug_print_object(&o, "assert_init");
+	debug_object_fixup(descr->fixup_assert_init, addr, ODEBUG_STATE_NOTAVAILABLE);
 }
 EXPORT_SYMBOL_GPL(debug_object_assert_init);
  

On Sat, Mar 04 2023 at 01:53, Schspa Shi wrote:
> Waiman Long <longman@redhat.com> writes:
>>> diff --git a/include/linux/debugobjects.h b/include/linux/debugobjects.h
>>> index 32444686b6ff4..544a6111b97f6 100644
>>> --- a/include/linux/debugobjects.h
>>> +++ b/include/linux/debugobjects.h
>>> @@ -30,6 +30,7 @@ struct debug_obj {
>>>   	enum debug_obj_state	state;
>>>   	unsigned int		astate;
>>>   	void			*object;
>>> +	bool			is_static;
>>>   	const struct debug_obj_descr *descr;
>>>   };
>>
>> The patch looks reasonable. My main concern is the increase in size of the
>> debug_obj structure. It is an additional 8 bytes on 64-bit arches. How much will
>> we save performance-wise by caching it in the debug_obj. Alternatively, you may
>> pack it within the current size by, maybe, reducing the size of state.
>>
>
> Yes, we can change this to:
>
> struct debug_obj {
> 	struct hlist_node	node;
> 	struct {
> 		enum debug_obj_state	state : 31;
> 		bool					is_static : 1;
> 	};

and thereby making debugobjects even more slower than it is right
now. Please check the resulting assembly code before proposing quick
"solutions".

Thanks,

        tglx
  

Thomas Gleixner <tglx@linutronix.de> writes:

> Hi!
>
> On Sat, Mar 04 2023 at 00:19, Schspa Shi wrote:
>
>> The is_static_object implementation relay on the initial state of the
>> object. If multiple places are accessed concurrently, there is a
>> probability that the debug object has been registered in the system, which
>> will invalidate the judgment of is_static_object.
>>
>> The following is the scenario where the problem occurs:
>>
>> T0                                                   T1
>> =========================================================================
>> mod_timer();
>>   debug_object_assert_init
>> 	db = get_bucket((unsigned long) addr);
>> 	raw_spin_lock_irqsave(&db->lock, flags);
>> 	obj = lookup_object(addr, db);
>>     if (!obj) {
>> 		raw_spin_unlock_irqrestore(&db->lock, flags);
>>         << Context switch >>
>>                                              mod_timer();
>>                                                debug_object_assert_init
>>                                                ...
>>                                                enqueue_timer();
>>         /*
>>          * The initial state changed a static timer object, and
>>          * is_static_object will return false
>>          */
>>
>>         if (descr->is_static_object &&
>> 			descr->is_static_object(addr)) {
>>                 debug_object_init();
>>             } else {
>>                << Hit here for a static object >>
>> 			   debug_print_object(&o, "assert_init");
>> 			   debug_object_fixup(descr->fixup_assert_init, addr,
>> 					   ODEBUG_STATE_NOTAVAILABLE);
>>             }
>>     }
>
> That analysis is correct.
>
>> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
>> index df86e649d8be0..d1be18158a1f7 100644
>> --- a/lib/debugobjects.c
>> +++ b/lib/debugobjects.c
>> @@ -275,6 +275,8 @@ alloc_object(void *addr, struct debug_bucket *b, const struct debug_obj_descr *d
>>  		obj->descr  = descr;
>>  		obj->state  = ODEBUG_STATE_NONE;
>>  		obj->astate = 0;
>> +		obj->is_static = descr->is_static_object &&
>> +			descr->is_static_object(addr);
>>  		hlist_add_head(&obj->node, &b->list);
>>  	}
>>  	return obj;
>> @@ -581,7 +583,16 @@ __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack
>>  			debug_objects_oom();
>>  			return;
>>  		}
>> +
>>  		check_stack = true;
>> +	} 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.

>> +/*
>> + * Check static object.
>> + */
>> +static bool debug_object_check_static(struct debug_bucket *db, void *addr,
>> +			const struct debug_obj_descr *descr)
>> +{
>> +	struct debug_obj *obj;
>> +
>> +	/*
>> +	 * The is_static_object implementation relay on the initial state of the
>> +	 * object. If multiple places are accessed concurrently, there is a
>> +	 * probability that the debug object has been registered in the system,
>> +	 * which will invalidate the judgment of is_static_object.
>> +	 */
>> +	lockdep_assert_held(&db->lock);
>> +
>> +	obj = lookup_object(addr, db);
>
> What does this lookup solve? See below...
>

This lookup can be droped by add a extra obj parameter.

>> +	if (likely(obj))
>> +		return obj->is_static;
>> +
>> +	return descr->is_static_object && descr->is_static_object(addr);
>> +}
>
>> @@ -696,6 +731,7 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr)
>>  		return ret;
>>  	}
>>  
>> +	is_static = debug_object_check_static(db, addr, descr);
>
> It's already clear that the object is not hashed because activate does:
>
>         lock()
>         lookup()
>         if (ob) {
>            ....
>            unlock();
>         }
>
> At this point nothing has changed after the lookup because
> the hash bucket is still locked.
>

Yes, the obj variable should be passed to debug_object_check_static.

>>  	raw_spin_unlock_irqrestore(&db->lock, flags);
>  
>>  	/*
>> @@ -705,7 +741,7 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr)
>>  	 * static object is tracked in the object tracker. If
>>  	 * not, this must be a bug, so we try to fix it up.
>>  	 */
>> -	if (descr->is_static_object && descr->is_static_object(addr)) {
>> +	if (is_static) {
>>  		/* track this static object */
>>  		debug_object_init(addr, descr);
>
> This cannot work as the change in debug_object_init() is not correct and
> there is no way to make it correct.
>

Function debug_object_init() can be fixed as explained above.

>> -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.

> But that also changes the actual test:
>
> -	obj.static_init = 1;
> -	debug_object_activate(&obj, &descr_type_test);
> -	if (check_results(&obj, ODEBUG_STATE_ACTIVE, fixups, warnings))
> +	debug_object_init(&sobj, &descr_type_test);
> +	debug_object_activate(&sobj, &descr_type_test);
> +	if (check_results(&sobj, ODEBUG_STATE_ACTIVE, fixups, warnings))
> 		goto out;
>
> That's _not_ equivalent.
>
> The original code checks exactly for the case where the object
> initialization does not happen before debug_object_activate() is invoked
> which is perfectly correct for statically initialized
> objects. Initializing the statically allocated object breaks that
> test. The changelog is utterly silent about that.
>

This debug_object_init(&sobj, &descr_type_test) can be droped. I'm
sorry to add this extra change.

> The proper fix for this is obvious from the analysis. The problem
> happens because the failed lookup drops the hash bucket lock which opens
> the window for the concurrent caller. So why dropping the hash bucket
> lock at all?
>
> Uncompiled and untested fix below.
>
> Thanks,
>
>         tglx
> ---
>  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.

This should be

if (descr->is_static_object && descr->is_static_object(addr))
	obj->state  = ODEBUG_STATE_INIT;
else
	obj->state  = ODEBUG_STATE_NONE;

But this can't resolve the initial state requirement from the
is_static_object() call.

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. When its initialization state changes, the
is_static_object() call will return the wrong value.

Please see the fellowing test case:

	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;
	debug_object_free(&obj, &descr_type_test);
	if (check_results(&obj, ODEBUG_STATE_NONE, fixups, warnings))
		goto out;

    /*
     * 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 */

>  		obj->astate = 0;
>  		hlist_add_head(&obj->node, &b->list);
>  	}
> @@ -552,11 +548,49 @@ static void debug_object_is_on_stack(voi
>  	WARN_ON(1);
>  }
>  
> +static struct debug_obj *lookup_object_or_alloc(void *addr, struct debug_bucket *b,
> +						const struct debug_obj_descr *descr,
> +						bool onstack, bool alloc_ifstatic)
> +{
> +	struct debug_obj *obj = lookup_object(addr, b);
> +
> +	if (likely(obj))
> +		return obj;
> +
> +	/*
> +	 * debug_object_init() unconditionally allocates untracked
> +	 * objects. It does not matter whether it is a static object or
> +	 * not.
> +	 *
> +	 * debug_object_assert_init() and debug_object_activate() allow
> +	 * allocation only if the descriptor callback confirms that the
> +	 * object is static and considered initialized. For non-static
> +	 * objects the allocation needs to be done from the fixup callback.
> +	 */
> +	if (unlikely(alloc_ifstatic)) {
> +		if (!descr->is_static_object || !descr->is_static_object(addr))
> +			return ERR_PTR(-ENOENT);
> +	}
> +
> +	/*
> +	 * On success the returned object is in INIT state as that's the
> +	 * correct state for all callers.
> +	 */
> +	obj = alloc_object(addr, b, descr);
> +	if (likely(obj)) {
> +		debug_object_is_on_stack(addr, onstack);
> +		return obj;
> +	}
> +
> +	/* Out of memory. Do the cleanup outside of the locked region */
> +	debug_objects_enabled = 0;
> +	return NULL;
> +}
> +
>  static void
>  __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack)
>  {
>  	enum debug_obj_state state;
> -	bool check_stack = false;
>  	struct debug_bucket *db;
>  	struct debug_obj *obj;
>  	unsigned long flags;
> @@ -572,16 +606,11 @@ static void
>  
>  	raw_spin_lock_irqsave(&db->lock, flags);
>  
> -	obj = lookup_object(addr, db);
> -	if (!obj) {
> -		obj = alloc_object(addr, db, descr);
> -		if (!obj) {
> -			debug_objects_enabled = 0;
> -			raw_spin_unlock_irqrestore(&db->lock, flags);
> -			debug_objects_oom();
> -			return;
> -		}
> -		check_stack = true;
> +	obj = lookup_object_or_alloc(addr, db, descr, onstack, false);
> +	if (unlikely(!obj)) {
> +		raw_spin_unlock_irqrestore(&db->lock, flags);
> +		debug_objects_oom();
> +		return;
>  	}
>  
>  	switch (obj->state) {
> @@ -607,8 +636,6 @@ static void
>  	}
>  
>  	raw_spin_unlock_irqrestore(&db->lock, flags);
> -	if (check_stack)
> -		debug_object_is_on_stack(addr, onstack);
>  }
>  
>  /**
> @@ -648,14 +675,12 @@ EXPORT_SYMBOL_GPL(debug_object_init_on_s
>   */
>  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;
>  	unsigned long flags;
>  	int ret;
> -	struct debug_obj o = { .object = addr,
> -			       .state = ODEBUG_STATE_NOTAVAILABLE,
> -			       .descr = descr };
>  
>  	if (!debug_objects_enabled)
>  		return 0;
> @@ -664,8 +689,8 @@ int debug_object_activate(void *addr, co
>  
>  	raw_spin_lock_irqsave(&db->lock, flags);
>  
> -	obj = lookup_object(addr, db);
> -	if (obj) {
> +	obj = lookup_object_or_alloc(addr, db, descr, false, true);
> +	if (likely(!IS_ERR_OR_NULL(obj))) {
>  		bool print_object = false;
>  
>  		switch (obj->state) {
> @@ -698,24 +723,16 @@ int debug_object_activate(void *addr, co
>  
>  	raw_spin_unlock_irqrestore(&db->lock, flags);
>  
> -	/*
> -	 * We are here when a static object is activated. We
> -	 * let the type specific code confirm whether this is
> -	 * true or not. if true, we just make sure that the
> -	 * static object is tracked in the object tracker. If
> -	 * not, this must be a bug, so we try to fix it up.
> -	 */
> -	if (descr->is_static_object && descr->is_static_object(addr)) {
> -		/* track this static object */
> -		debug_object_init(addr, descr);
> -		debug_object_activate(addr, descr);
> -	} else {
> -		debug_print_object(&o, "activate");
> -		ret = debug_object_fixup(descr->fixup_activate, addr,
> -					ODEBUG_STATE_NOTAVAILABLE);
> -		return ret ? 0 : -EINVAL;
> +	/* If NULL the allocaction has hit OOM */
> +	if (!obj) {
> +		debug_objects_oom();
> +		return 0;
>  	}
> -	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;
>  }
>  EXPORT_SYMBOL_GPL(debug_object_activate);
>  
> @@ -869,6 +886,7 @@ EXPORT_SYMBOL_GPL(debug_object_free);
>   */
>  void debug_object_assert_init(void *addr, const struct debug_obj_descr *descr)
>  {
> +	struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr };
>  	struct debug_bucket *db;
>  	struct debug_obj *obj;
>  	unsigned long flags;
> @@ -879,31 +897,20 @@ void debug_object_assert_init(void *addr
>  	db = get_bucket((unsigned long) addr);
>  
>  	raw_spin_lock_irqsave(&db->lock, flags);
> +	obj = lookup_object_or_alloc(addr, db, descr, false, true);
> +	raw_spin_unlock_irqrestore(&db->lock, flags);
> +	if (likely(!IS_ERR_OR_NULL(obj)))
> +		return;
>  
> -	obj = lookup_object(addr, db);
> +	/* If NULL the allocaction has hit OOM */
>  	if (!obj) {
> -		struct debug_obj o = { .object = addr,
> -				       .state = ODEBUG_STATE_NOTAVAILABLE,
> -				       .descr = descr };
> -
> -		raw_spin_unlock_irqrestore(&db->lock, flags);
> -		/*
> -		 * Maybe the object is static, and we let the type specific
> -		 * code confirm. Track this static object if true, else invoke
> -		 * fixup.
> -		 */
> -		if (descr->is_static_object && descr->is_static_object(addr)) {
> -			/* Track this static object */
> -			debug_object_init(addr, descr);
> -		} else {
> -			debug_print_object(&o, "assert_init");
> -			debug_object_fixup(descr->fixup_assert_init, addr,
> -					   ODEBUG_STATE_NOTAVAILABLE);
> -		}
> +		debug_objects_oom();
>  		return;
>  	}
>  
> -	raw_spin_unlock_irqrestore(&db->lock, flags);
> +	/* Object is neither tracked nor static. It's not initialized. */
> +	debug_print_object(&o, "assert_init");
> +	debug_object_fixup(descr->fixup_assert_init, addr, ODEBUG_STATE_NOTAVAILABLE);
>  }
>  EXPORT_SYMBOL_GPL(debug_object_assert_init);
>  

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.

I think we should introduce lookup_object_or_alloc and is_static at the
same time.
  

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
  

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
  

On Wed, Mar 22 2023 at 18:46, Thomas Gleixner wrote:
> On Wed, Mar 22 2023 at 23:40, Schspa Shi wrote:
>> I think we should introduce lookup_object_or_alloc and is_static at the
>> same time.
>
> What for?

The below has the NONE/INIT issue addressed and plugs that race
completely, so no further action required.

Thanks,

        tglx
---
--- 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)
 {
@@ -552,11 +548,49 @@ static void debug_object_is_on_stack(voi
 	WARN_ON(1);
 }
 
+static struct debug_obj *lookup_object_or_alloc(void *addr, struct debug_bucket *b,
+						const struct debug_obj_descr *descr,
+						bool onstack, bool alloc_ifstatic)
+{
+	struct debug_obj *obj = lookup_object(addr, b);
+	enum debug_obj_state state = ODEBUG_STATE_NONE;
+
+	if (likely(obj))
+		return obj;
+
+	/*
+	 * debug_object_init() unconditionally allocates untracked
+	 * objects. It does not matter whether it is a static object or
+	 * not.
+	 *
+	 * debug_object_assert_init() and debug_object_activate() allow
+	 * allocation only if the descriptor callback confirms that the
+	 * object is static and considered initialized. For non-static
+	 * objects the allocation needs to be done from the fixup callback.
+	 */
+	if (unlikely(alloc_ifstatic)) {
+		if (!descr->is_static_object || !descr->is_static_object(addr))
+			return ERR_PTR(-ENOENT);
+		/* Statically allocated objects are considered initialized */
+		state = ODEBUG_STATE_INIT;
+	}
+
+	obj = alloc_object(addr, b, descr);
+	if (likely(obj)) {
+		obj->state = state;
+		debug_object_is_on_stack(addr, onstack);
+		return obj;
+	}
+
+	/* Out of memory. Do the cleanup outside of the locked region */
+	debug_objects_enabled = 0;
+	return NULL;
+}
+
 static void
 __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack)
 {
 	enum debug_obj_state state;
-	bool check_stack = false;
 	struct debug_bucket *db;
 	struct debug_obj *obj;
 	unsigned long flags;
@@ -572,16 +606,11 @@ static void
 
 	raw_spin_lock_irqsave(&db->lock, flags);
 
-	obj = lookup_object(addr, db);
-	if (!obj) {
-		obj = alloc_object(addr, db, descr);
-		if (!obj) {
-			debug_objects_enabled = 0;
-			raw_spin_unlock_irqrestore(&db->lock, flags);
-			debug_objects_oom();
-			return;
-		}
-		check_stack = true;
+	obj = lookup_object_or_alloc(addr, db, descr, onstack, false);
+	if (unlikely(!obj)) {
+		raw_spin_unlock_irqrestore(&db->lock, flags);
+		debug_objects_oom();
+		return;
 	}
 
 	switch (obj->state) {
@@ -607,8 +636,6 @@ static void
 	}
 
 	raw_spin_unlock_irqrestore(&db->lock, flags);
-	if (check_stack)
-		debug_object_is_on_stack(addr, onstack);
 }
 
 /**
@@ -648,14 +675,12 @@ EXPORT_SYMBOL_GPL(debug_object_init_on_s
  */
 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;
 	unsigned long flags;
 	int ret;
-	struct debug_obj o = { .object = addr,
-			       .state = ODEBUG_STATE_NOTAVAILABLE,
-			       .descr = descr };
 
 	if (!debug_objects_enabled)
 		return 0;
@@ -664,8 +689,8 @@ int debug_object_activate(void *addr, co
 
 	raw_spin_lock_irqsave(&db->lock, flags);
 
-	obj = lookup_object(addr, db);
-	if (obj) {
+	obj = lookup_object_or_alloc(addr, db, descr, false, true);
+	if (likely(!IS_ERR_OR_NULL(obj))) {
 		bool print_object = false;
 
 		switch (obj->state) {
@@ -698,24 +723,16 @@ int debug_object_activate(void *addr, co
 
 	raw_spin_unlock_irqrestore(&db->lock, flags);
 
-	/*
-	 * We are here when a static object is activated. We
-	 * let the type specific code confirm whether this is
-	 * true or not. if true, we just make sure that the
-	 * static object is tracked in the object tracker. If
-	 * not, this must be a bug, so we try to fix it up.
-	 */
-	if (descr->is_static_object && descr->is_static_object(addr)) {
-		/* track this static object */
-		debug_object_init(addr, descr);
-		debug_object_activate(addr, descr);
-	} else {
-		debug_print_object(&o, "activate");
-		ret = debug_object_fixup(descr->fixup_activate, addr,
-					ODEBUG_STATE_NOTAVAILABLE);
-		return ret ? 0 : -EINVAL;
+	/* If NULL the allocaction has hit OOM */
+	if (!obj) {
+		debug_objects_oom();
+		return 0;
 	}
-	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;
 }
 EXPORT_SYMBOL_GPL(debug_object_activate);
 
@@ -869,6 +886,7 @@ EXPORT_SYMBOL_GPL(debug_object_free);
  */
 void debug_object_assert_init(void *addr, const struct debug_obj_descr *descr)
 {
+	struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr };
 	struct debug_bucket *db;
 	struct debug_obj *obj;
 	unsigned long flags;
@@ -879,31 +897,20 @@ void debug_object_assert_init(void *addr
 	db = get_bucket((unsigned long) addr);
 
 	raw_spin_lock_irqsave(&db->lock, flags);
+	obj = lookup_object_or_alloc(addr, db, descr, false, true);
+	raw_spin_unlock_irqrestore(&db->lock, flags);
+	if (likely(!IS_ERR_OR_NULL(obj)))
+		return;
 
-	obj = lookup_object(addr, db);
+	/* If NULL the allocaction has hit OOM */
 	if (!obj) {
-		struct debug_obj o = { .object = addr,
-				       .state = ODEBUG_STATE_NOTAVAILABLE,
-				       .descr = descr };
-
-		raw_spin_unlock_irqrestore(&db->lock, flags);
-		/*
-		 * Maybe the object is static, and we let the type specific
-		 * code confirm. Track this static object if true, else invoke
-		 * fixup.
-		 */
-		if (descr->is_static_object && descr->is_static_object(addr)) {
-			/* Track this static object */
-			debug_object_init(addr, descr);
-		} else {
-			debug_print_object(&o, "assert_init");
-			debug_object_fixup(descr->fixup_assert_init, addr,
-					   ODEBUG_STATE_NOTAVAILABLE);
-		}
+		debug_objects_oom();
 		return;
 	}
 
-	raw_spin_unlock_irqrestore(&db->lock, flags);
+	/* Object is neither tracked nor static. It's not initialized. */
+	debug_print_object(&o, "assert_init");
+	debug_object_fixup(descr->fixup_assert_init, addr, ODEBUG_STATE_NOTAVAILABLE);
 }
 EXPORT_SYMBOL_GPL(debug_object_assert_init);
  

Thomas Gleixner <tglx@linutronix.de> writes:

> On Wed, Mar 22 2023 at 18:46, Thomas Gleixner wrote:
>> On Wed, Mar 22 2023 at 23:40, Schspa Shi wrote:
>>> I think we should introduce lookup_object_or_alloc and is_static at the
>>> same time.
>>
>> What for?
>
> The below has the NONE/INIT issue addressed and plugs that race
> completely, so no further action required.
>
> Thanks,
>
>         tglx
> ---
> --- 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)
>  {
> @@ -552,11 +548,49 @@ static void debug_object_is_on_stack(voi
>  	WARN_ON(1);
>  }
>  
> +static struct debug_obj *lookup_object_or_alloc(void *addr, struct debug_bucket *b,
> +						const struct debug_obj_descr *descr,
> +						bool onstack, bool alloc_ifstatic)
> +{
> +	struct debug_obj *obj = lookup_object(addr, b);
> +	enum debug_obj_state state = ODEBUG_STATE_NONE;
> +
> +	if (likely(obj))
> +		return obj;
> +
> +	/*
> +	 * debug_object_init() unconditionally allocates untracked
> +	 * objects. It does not matter whether it is a static object or
> +	 * not.
> +	 *
> +	 * debug_object_assert_init() and debug_object_activate() allow
> +	 * allocation only if the descriptor callback confirms that the
> +	 * object is static and considered initialized. For non-static
> +	 * objects the allocation needs to be done from the fixup callback.
> +	 */
> +	if (unlikely(alloc_ifstatic)) {
> +		if (!descr->is_static_object || !descr->is_static_object(addr))
> +			return ERR_PTR(-ENOENT);
> +		/* Statically allocated objects are considered initialized */
> +		state = ODEBUG_STATE_INIT;
> +	}
> +
> +	obj = alloc_object(addr, b, descr);
> +	if (likely(obj)) {
> +		obj->state = state;
> +		debug_object_is_on_stack(addr, onstack);
> +		return obj;
> +	}
> +
> +	/* Out of memory. Do the cleanup outside of the locked region */
> +	debug_objects_enabled = 0;
> +	return NULL;
> +}
> +
>  static void
>  __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack)
>  {
>  	enum debug_obj_state state;
> -	bool check_stack = false;
>  	struct debug_bucket *db;
>  	struct debug_obj *obj;
>  	unsigned long flags;
> @@ -572,16 +606,11 @@ static void
>  
>  	raw_spin_lock_irqsave(&db->lock, flags);
>  
> -	obj = lookup_object(addr, db);
> -	if (!obj) {
> -		obj = alloc_object(addr, db, descr);
> -		if (!obj) {
> -			debug_objects_enabled = 0;
> -			raw_spin_unlock_irqrestore(&db->lock, flags);
> -			debug_objects_oom();
> -			return;
> -		}
> -		check_stack = true;
> +	obj = lookup_object_or_alloc(addr, db, descr, onstack, false);
> +	if (unlikely(!obj)) {
> +		raw_spin_unlock_irqrestore(&db->lock, flags);
> +		debug_objects_oom();
> +		return;
>  	}
>  
>  	switch (obj->state) {
> @@ -607,8 +636,6 @@ static void
>  	}
>  
>  	raw_spin_unlock_irqrestore(&db->lock, flags);
> -	if (check_stack)
> -		debug_object_is_on_stack(addr, onstack);
>  }
>  
>  /**
> @@ -648,14 +675,12 @@ EXPORT_SYMBOL_GPL(debug_object_init_on_s
>   */
>  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;
>  	unsigned long flags;
>  	int ret;
> -	struct debug_obj o = { .object = addr,
> -			       .state = ODEBUG_STATE_NOTAVAILABLE,
> -			       .descr = descr };
>  
>  	if (!debug_objects_enabled)
>  		return 0;
> @@ -664,8 +689,8 @@ int debug_object_activate(void *addr, co
>  
>  	raw_spin_lock_irqsave(&db->lock, flags);
>  
> -	obj = lookup_object(addr, db);
> -	if (obj) {
> +	obj = lookup_object_or_alloc(addr, db, descr, false, true);
> +	if (likely(!IS_ERR_OR_NULL(obj))) {
>  		bool print_object = false;
>  
>  		switch (obj->state) {
> @@ -698,24 +723,16 @@ int debug_object_activate(void *addr, co
>  
>  	raw_spin_unlock_irqrestore(&db->lock, flags);
>  
> -	/*
> -	 * We are here when a static object is activated. We
> -	 * let the type specific code confirm whether this is
> -	 * true or not. if true, we just make sure that the
> -	 * static object is tracked in the object tracker. If
> -	 * not, this must be a bug, so we try to fix it up.
> -	 */
> -	if (descr->is_static_object && descr->is_static_object(addr)) {
> -		/* track this static object */
> -		debug_object_init(addr, descr);
> -		debug_object_activate(addr, descr);
> -	} else {
> -		debug_print_object(&o, "activate");
> -		ret = debug_object_fixup(descr->fixup_activate, addr,
> -					ODEBUG_STATE_NOTAVAILABLE);
> -		return ret ? 0 : -EINVAL;
> +	/* If NULL the allocaction has hit OOM */
> +	if (!obj) {
> +		debug_objects_oom();
> +		return 0;
>  	}
> -	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;
>  }
>  EXPORT_SYMBOL_GPL(debug_object_activate);
>  
> @@ -869,6 +886,7 @@ EXPORT_SYMBOL_GPL(debug_object_free);
>   */
>  void debug_object_assert_init(void *addr, const struct debug_obj_descr *descr)
>  {
> +	struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr };
>  	struct debug_bucket *db;
>  	struct debug_obj *obj;
>  	unsigned long flags;
> @@ -879,31 +897,20 @@ void debug_object_assert_init(void *addr
>  	db = get_bucket((unsigned long) addr);
>  
>  	raw_spin_lock_irqsave(&db->lock, flags);
> +	obj = lookup_object_or_alloc(addr, db, descr, false, true);
> +	raw_spin_unlock_irqrestore(&db->lock, flags);
> +	if (likely(!IS_ERR_OR_NULL(obj)))
> +		return;
>  
> -	obj = lookup_object(addr, db);
> +	/* If NULL the allocaction has hit OOM */
>  	if (!obj) {
> -		struct debug_obj o = { .object = addr,
> -				       .state = ODEBUG_STATE_NOTAVAILABLE,
> -				       .descr = descr };
> -
> -		raw_spin_unlock_irqrestore(&db->lock, flags);
> -		/*
> -		 * Maybe the object is static, and we let the type specific
> -		 * code confirm. Track this static object if true, else invoke
> -		 * fixup.
> -		 */
> -		if (descr->is_static_object && descr->is_static_object(addr)) {
> -			/* Track this static object */
> -			debug_object_init(addr, descr);
> -		} else {
> -			debug_print_object(&o, "assert_init");
> -			debug_object_fixup(descr->fixup_assert_init, addr,
> -					   ODEBUG_STATE_NOTAVAILABLE);
> -		}
> +		debug_objects_oom();
>  		return;
>  	}
>  
> -	raw_spin_unlock_irqrestore(&db->lock, flags);
> +	/* Object is neither tracked nor static. It's not initialized. */
> +	debug_print_object(&o, "assert_init");
> +	debug_object_fixup(descr->fixup_assert_init, addr, ODEBUG_STATE_NOTAVAILABLE);
>  }
>  EXPORT_SYMBOL_GPL(debug_object_assert_init);
>  

It's OK if you don't think debug_object_free() should report a error when
the object is static. But I still think we should report an error and
modify the autotest case in debug_objects_selftest() to remove
debug_object_free() call on the static object.
  

On Thu, Mar 23 2023 at 01:55, Schspa Shi wrote:
> Thomas Gleixner <tglx@linutronix.de> writes:
>> 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.

That's an artifical and completely pointless test case. As I told you
before the memory subsystem will warn when it's tried to free a static
object. debug_objects_free() is invoked from the memory subsystem *free*
functions.

What is the value of another warning?

Nothing at all.

So why would we add extra code just to keep track of something
completely redundant?

Thanks,

        tglx
  

Thomas Gleixner <tglx@linutronix.de> writes:

> On Thu, Mar 23 2023 at 01:55, Schspa Shi wrote:
>> Thomas Gleixner <tglx@linutronix.de> writes:
>>> 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.
>
> That's an artifical and completely pointless test case. As I told you
> before the memory subsystem will warn when it's tried to free a static
> object. debug_objects_free() is invoked from the memory subsystem *free*
> functions.
>
> What is the value of another warning?
>
> Nothing at all.
>
> So why would we add extra code just to keep track of something
> completely redundant?
>

OK, there is really no need to do this extra check. And should I
submit a new patch version with your change 😃?

> Thanks,
>
>         tglx