Re: [Intel-gfx] [PATCH v3] debugobjects: stop accessing objects after releasing spinlock

[Andrzej Hajda <andrzej.hajda@xxxxxxxxx>]

On 25.10.2023 23:39, Andrzej Hajda wrote:
> After spinlock release object can be modified/freed by concurrent thread.
> Using it in such case is error prone, even for printing object state.
> To avoid such situation local copy of the object is created if necessary.
>
> Sample buggy scenario:
> 1. Thread tries to deactivate destroyed object, debugobjects detects it,
>     spin lock is released, thread is preempted.
> 2. Other thread frees debugobject, then allocates new one on the same memory
>     location, ie 'obj' variable from 1st thread point to it - it is possible
>     because there is no locking.
> 3. Then preemption occurs, and 1st thread reports error for wrong object.
>
> Signed-off-by: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
> ---
> v2: add missing switch breaks
> v3: abandon single-point-of-unlock approach

Gently ping.

Regards
Andrzej


> ---
>   lib/debugobjects.c | 196 +++++++++++++++++++++--------------------------------
>   1 file changed, 77 insertions(+), 119 deletions(-)
>
> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
> index a517256a270b71..c074dbbec084a6 100644
> --- a/lib/debugobjects.c
> +++ b/lib/debugobjects.c
> @@ -620,9 +620,8 @@ static void debug_objects_fill_pool(void)
>   static void
>   __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack)
>   {
> -	enum debug_obj_state state;
>   	struct debug_bucket *db;
> -	struct debug_obj *obj;
> +	struct debug_obj *obj, o;
>   	unsigned long flags;
>   
>   	debug_objects_fill_pool();
> @@ -643,24 +642,18 @@ __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack
>   	case ODEBUG_STATE_INIT:
>   	case ODEBUG_STATE_INACTIVE:
>   		obj->state = ODEBUG_STATE_INIT;
> -		break;
> -
> -	case ODEBUG_STATE_ACTIVE:
> -		state = obj->state;
>   		raw_spin_unlock_irqrestore(&db->lock, flags);
> -		debug_print_object(obj, "init");
> -		debug_object_fixup(descr->fixup_init, addr, state);
> -		return;
> -
> -	case ODEBUG_STATE_DESTROYED:
> -		raw_spin_unlock_irqrestore(&db->lock, flags);
> -		debug_print_object(obj, "init");
>   		return;
>   	default:
>   		break;
>   	}
>   
> +	o = *obj;
>   	raw_spin_unlock_irqrestore(&db->lock, flags);
> +	debug_print_object(&o, "init");
> +
> +	if (o.state == ODEBUG_STATE_ACTIVE)
> +		debug_object_fixup(descr->fixup_init, addr, o.state);
>   }
>   
>   /**
> @@ -701,11 +694,9 @@ EXPORT_SYMBOL_GPL(debug_object_init_on_stack);
>   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;
>   
>   	if (!debug_objects_enabled)
>   		return 0;
> @@ -717,49 +708,38 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr)
>   	raw_spin_lock_irqsave(&db->lock, flags);
>   
>   	obj = lookup_object_or_alloc(addr, db, descr, false, true);
> -	if (likely(!IS_ERR_OR_NULL(obj))) {
> -		bool print_object = false;
> -
> +	if (unlikely(!obj)) {
> +		raw_spin_unlock_irqrestore(&db->lock, flags);
> +		debug_objects_oom();
> +		return 0;
> +	} else if (likely(!IS_ERR(obj))) {
>   		switch (obj->state) {
> -		case ODEBUG_STATE_INIT:
> -		case ODEBUG_STATE_INACTIVE:
> -			obj->state = ODEBUG_STATE_ACTIVE;
> -			ret = 0;
> -			break;
> -
>   		case ODEBUG_STATE_ACTIVE:
> -			state = obj->state;
> -			raw_spin_unlock_irqrestore(&db->lock, flags);
> -			debug_print_object(obj, "activate");
> -			ret = debug_object_fixup(descr->fixup_activate, addr, state);
> -			return ret ? 0 : -EINVAL;
> -
>   		case ODEBUG_STATE_DESTROYED:
> -			print_object = true;
> -			ret = -EINVAL;
>   			break;
> +		case ODEBUG_STATE_INIT:
> +		case ODEBUG_STATE_INACTIVE:
> +			obj->state = ODEBUG_STATE_ACTIVE;
> +			fallthrough;
>   		default:
> -			ret = 0;
> -			break;
> +			raw_spin_unlock_irqrestore(&db->lock, flags);
> +			return 0;
>   		}
> -		raw_spin_unlock_irqrestore(&db->lock, flags);
> -		if (print_object)
> -			debug_print_object(obj, "activate");
> -		return ret;
>   	}
>   
> +	o = *obj;
>   	raw_spin_unlock_irqrestore(&db->lock, flags);
> +	debug_print_object(&o, "activate");
>   
> -	/* If NULL the allocation has hit OOM */
> -	if (!obj) {
> -		debug_objects_oom();
> -		return 0;
> +	switch (o.state) {
> +	case ODEBUG_STATE_ACTIVE:
> +	case ODEBUG_STATE_NOTAVAILABLE:
> +		if (debug_object_fixup(descr->fixup_activate, addr, o.state))
> +			return 0;
> +		fallthrough;
> +	default:
> +		return -EINVAL;
>   	}
> -
> -	/* 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);
>   
> @@ -770,10 +750,10 @@ EXPORT_SYMBOL_GPL(debug_object_activate);
>    */
>   void debug_object_deactivate(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;
> -	bool print_object = false;
>   
>   	if (!debug_objects_enabled)
>   		return;
> @@ -785,33 +765,24 @@ void debug_object_deactivate(void *addr, const struct debug_obj_descr *descr)
>   	obj = lookup_object(addr, db);
>   	if (obj) {
>   		switch (obj->state) {
> +		case ODEBUG_STATE_DESTROYED:
> +			break;
>   		case ODEBUG_STATE_INIT:
>   		case ODEBUG_STATE_INACTIVE:
>   		case ODEBUG_STATE_ACTIVE:
> -			if (!obj->astate)
> -				obj->state = ODEBUG_STATE_INACTIVE;
> -			else
> -				print_object = true;
> -			break;
> -
> -		case ODEBUG_STATE_DESTROYED:
> -			print_object = true;
> -			break;
> +			if (obj->astate)
> +				break;
> +			obj->state = ODEBUG_STATE_INACTIVE;
> +			fallthrough;
>   		default:
> -			break;
> +			raw_spin_unlock_irqrestore(&db->lock, flags);
> +			return;
>   		}
> +		o = *obj;
>   	}
>   
>   	raw_spin_unlock_irqrestore(&db->lock, flags);
> -	if (!obj) {
> -		struct debug_obj o = { .object = addr,
> -				       .state = ODEBUG_STATE_NOTAVAILABLE,
> -				       .descr = descr };
> -
> -		debug_print_object(&o, "deactivate");
> -	} else if (print_object) {
> -		debug_print_object(obj, "deactivate");
> -	}
> +	debug_print_object(&o, "deactivate");
>   }
>   EXPORT_SYMBOL_GPL(debug_object_deactivate);
>   
> @@ -822,11 +793,9 @@ EXPORT_SYMBOL_GPL(debug_object_deactivate);
>    */
>   void debug_object_destroy(void *addr, const struct debug_obj_descr *descr)
>   {
> -	enum debug_obj_state state;
>   	struct debug_bucket *db;
> -	struct debug_obj *obj;
> +	struct debug_obj *obj, o;
>   	unsigned long flags;
> -	bool print_object = false;
>   
>   	if (!debug_objects_enabled)
>   		return;
> @@ -836,32 +805,31 @@ void debug_object_destroy(void *addr, const struct debug_obj_descr *descr)
>   	raw_spin_lock_irqsave(&db->lock, flags);
>   
>   	obj = lookup_object(addr, db);
> -	if (!obj)
> -		goto out_unlock;
> +	if (!obj) {
> +		raw_spin_unlock_irqrestore(&db->lock, flags);
> +		return;
> +	}
>   
>   	switch (obj->state) {
> +	case ODEBUG_STATE_ACTIVE:
> +	case ODEBUG_STATE_DESTROYED:
> +		break;
>   	case ODEBUG_STATE_NONE:
>   	case ODEBUG_STATE_INIT:
>   	case ODEBUG_STATE_INACTIVE:
>   		obj->state = ODEBUG_STATE_DESTROYED;
> -		break;
> -	case ODEBUG_STATE_ACTIVE:
> -		state = obj->state;
> +		fallthrough;
> +	default:
>   		raw_spin_unlock_irqrestore(&db->lock, flags);
> -		debug_print_object(obj, "destroy");
> -		debug_object_fixup(descr->fixup_destroy, addr, state);
>   		return;
> -
> -	case ODEBUG_STATE_DESTROYED:
> -		print_object = true;
> -		break;
> -	default:
> -		break;
>   	}
> -out_unlock:
> +
> +	o = *obj;
>   	raw_spin_unlock_irqrestore(&db->lock, flags);
> -	if (print_object)
> -		debug_print_object(obj, "destroy");
> +	debug_print_object(&o, "destroy");
> +
> +	if (o.state == ODEBUG_STATE_ACTIVE)
> +		debug_object_fixup(descr->fixup_destroy, addr, o.state);
>   }
>   EXPORT_SYMBOL_GPL(debug_object_destroy);
>   
> @@ -872,9 +840,8 @@ EXPORT_SYMBOL_GPL(debug_object_destroy);
>    */
>   void debug_object_free(void *addr, const struct debug_obj_descr *descr)
>   {
> -	enum debug_obj_state state;
>   	struct debug_bucket *db;
> -	struct debug_obj *obj;
> +	struct debug_obj *obj, o;
>   	unsigned long flags;
>   
>   	if (!debug_objects_enabled)
> @@ -885,24 +852,26 @@ void debug_object_free(void *addr, const struct debug_obj_descr *descr)
>   	raw_spin_lock_irqsave(&db->lock, flags);
>   
>   	obj = lookup_object(addr, db);
> -	if (!obj)
> -		goto out_unlock;
> +	if (!obj) {
> +		raw_spin_unlock_irqrestore(&db->lock, flags);
> +		return;
> +	}
>   
>   	switch (obj->state) {
>   	case ODEBUG_STATE_ACTIVE:
> -		state = obj->state;
> -		raw_spin_unlock_irqrestore(&db->lock, flags);
> -		debug_print_object(obj, "free");
> -		debug_object_fixup(descr->fixup_free, addr, state);
> -		return;
> +		break;
>   	default:
>   		hlist_del(&obj->node);
>   		raw_spin_unlock_irqrestore(&db->lock, flags);
>   		free_object(obj);
>   		return;
>   	}
> -out_unlock:
> +
> +	o = *obj;
>   	raw_spin_unlock_irqrestore(&db->lock, flags);
> +	debug_print_object(&o, "free");
> +
> +	debug_object_fixup(descr->fixup_free, addr, o.state);
>   }
>   EXPORT_SYMBOL_GPL(debug_object_free);
>   
> @@ -954,10 +923,10 @@ void
>   debug_object_active_state(void *addr, const struct debug_obj_descr *descr,
>   			  unsigned int expect, unsigned int next)
>   {
> +	struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr };
>   	struct debug_bucket *db;
>   	struct debug_obj *obj;
>   	unsigned long flags;
> -	bool print_object = false;
>   
>   	if (!debug_objects_enabled)
>   		return;
> @@ -970,28 +939,20 @@ debug_object_active_state(void *addr, const struct debug_obj_descr *descr,
>   	if (obj) {
>   		switch (obj->state) {
>   		case ODEBUG_STATE_ACTIVE:
> -			if (obj->astate == expect)
> +			if (obj->astate == expect) {
>   				obj->astate = next;
> -			else
> -				print_object = true;
> +				raw_spin_unlock_irqrestore(&db->lock, flags);
> +				return;
> +			}
>   			break;
> -
>   		default:
> -			print_object = true;
>   			break;
>   		}
> +		o = *obj;
>   	}
>   
>   	raw_spin_unlock_irqrestore(&db->lock, flags);
> -	if (!obj) {
> -		struct debug_obj o = { .object = addr,
> -				       .state = ODEBUG_STATE_NOTAVAILABLE,
> -				       .descr = descr };
> -
> -		debug_print_object(&o, "active_state");
> -	} else if (print_object) {
> -		debug_print_object(obj, "active_state");
> -	}
> +	debug_print_object(&o, "active_state");
>   }
>   EXPORT_SYMBOL_GPL(debug_object_active_state);
>   
> @@ -999,11 +960,9 @@ EXPORT_SYMBOL_GPL(debug_object_active_state);
>   static void __debug_check_no_obj_freed(const void *address, unsigned long size)
>   {
>   	unsigned long flags, oaddr, saddr, eaddr, paddr, chunks;
> -	const struct debug_obj_descr *descr;
> -	enum debug_obj_state state;
>   	struct debug_bucket *db;
>   	struct hlist_node *tmp;
> -	struct debug_obj *obj;
> +	struct debug_obj *obj, o;
>   	int cnt, objs_checked = 0;
>   
>   	saddr = (unsigned long) address;
> @@ -1026,12 +985,11 @@ static void __debug_check_no_obj_freed(const void *address, unsigned long size)
>   
>   			switch (obj->state) {
>   			case ODEBUG_STATE_ACTIVE:
> -				descr = obj->descr;
> -				state = obj->state;
> +				o = *obj;
>   				raw_spin_unlock_irqrestore(&db->lock, flags);
> -				debug_print_object(obj, "free");
> -				debug_object_fixup(descr->fixup_free,
> -						   (void *) oaddr, state);
> +				debug_print_object(&o, "free");
> +				debug_object_fixup(o.descr->fixup_free,
> +						   (void *) oaddr, o.state);
>   				goto repeat;
>   			default:
>   				hlist_del(&obj->node);
>
> ---
> base-commit: 201c8a7bd1f3f415920a2df4b8a8817e973f42fe
> change-id: 20231025-debugobjects_fix-66e5292557c4
>
> Best regards,