Message-ID: <814636e6-9a8a-9ab1-03a0-ed3702024227@redhat.com>
Date:   Fri, 3 Mar 2023 12:00:02 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH 1/2] debugobject: fix concurrency issues with
 is_static_object
Content-Language: en-US
To:     Schspa Shi <schspa@gmail.com>, tglx@linutronix.de,
        swboyd@chromium.org, linux@roeck-us.net, wuchi.zero@gmail.com
Cc:     linux-kernel@vger.kernel.org,
        syzbot+5093ba19745994288b53@syzkaller.appspotmail.com
References: <20230303161906.831686-1-schspa@gmail.com>
From:   Waiman Long <longman@redhat.com>
In-Reply-To: <20230303161906.831686-1-schspa@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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");