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

From: Thomas Gleixner <tglx@linutronix.de>
To: Andrzej Hajda <andrzej.hajda@intel.com>,
	linux-kernel@vger.kernel.org, intel-gfx@lists.freedesktop.org,
	linux-mm@kvack.org
Cc: Andrzej Hajda <andrzej.hajda@intel.com>,
	Nirmoy Das <nirmoy.das@intel.com>
Subject: Re: [Intel-gfx] [PATCH v2] debugobjects: stop accessing objects after releasing spinlock
Date: Tue, 24 Oct 2023 14:56:47 +0200	[thread overview]
Message-ID: <874jigch68.ffs@tglx> (raw)
In-Reply-To: <20230925131359.2948827-1-andrzej.hajda@intel.com>

On Mon, Sep 25 2023 at 15:13, Andrzej Hajda wrote:
> @@ -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();
> @@ -644,23 +643,19 @@ __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack
>  	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;
> +		obj = NULL;
>  	}
>  
>  	raw_spin_unlock_irqrestore(&db->lock, flags);
> +
> +	if (obj)
> +		return;

Hmm. I'd rather write is this way:

 	case ODEBUG_STATE_INIT:
 	case ODEBUG_STATE_INACTIVE:
 		obj->state = ODEBUG_STATE_INIT;
 		raw_spin_unlock_irqrestore(&db->lock, flags);
 		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);

This spares the 'obj' pointer modification and the conditional. The
extra raw_spin_unlock_irqrestore() is not the end of the world.

>  void debug_object_activate(void *addr, const struct debug_obj_descr *descr)
...
>  		default:
> -			ret = 0;
>  			break;
>  		}
> -		raw_spin_unlock_irqrestore(&db->lock, flags);
> -		if (print_object)
> -			debug_print_object(obj, "activate");
> -		return ret;
> +	} else {
> +		o.object = addr;
> +		o.state = ODEBUG_STATE_NOTAVAILABLE;
> +		o.descr = descr;
> +		obj = NULL;

Hrmm. Just keep the

	struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr };

around and get rid of this else clause.

>  	}
>  
>  	raw_spin_unlock_irqrestore(&db->lock, flags);
>  
> -	/* If NULL the allocation has hit OOM */
> -	if (!obj) {
> -		debug_objects_oom();
> +	if (obj)
>  		return 0;

Plus a similar change as above to get rid of this conditional and just
have the failure path here.

> @@ -788,30 +777,29 @@ void debug_object_deactivate(void *addr, const struct debug_obj_descr *descr)
>  		case ODEBUG_STATE_INIT:
>  		case ODEBUG_STATE_INACTIVE:
>  		case ODEBUG_STATE_ACTIVE:
> -			if (!obj->astate)
> +			if (!obj->astate) {
>  				obj->state = ODEBUG_STATE_INACTIVE;
> -			else
> -				print_object = true;
> -			break;
> -
> +				break;
> +			}
> +			fallthrough;
>  		case ODEBUG_STATE_DESTROYED:
> -			print_object = true;
> +			o = *obj;
> +			obj = NULL;
>  			break;
>  		default:
>  			break;
>  		}
> +	} else {
> +		o.object = addr;
> +		o.state = ODEBUG_STATE_NOTAVAILABLE;
> +		o.descr = descr;
> +		obj = NULL;
>  	}

Same here.
	struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr };
        ...

 	if (obj) {
 		switch (obj->state) {
		case ODEBUG_STATE_DESTROYED:
			o = *obj;
			break;
 		case ODEBUG_STATE_INIT:
 		case ODEBUG_STATE_INACTIVE:
 		case ODEBUG_STATE_ACTIVE:
			if (obj->astate) {
				o = *obj;
				break;
			}
			obj->state = ODEBUG_STATE_INACTIVE;
			fallthrough;
 		default:
			raw_spin_unlock_irqrestore(&db->lock, flags);
			return;
 		}
 	}

 	raw_spin_unlock_irqrestore(&db->lock, flags);
	debug_print_object(&o, "deactivate");

Hmm?

> @@ -970,28 +962,27 @@ 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;
> -			break;
> -
> +				break;
> +			}
> +			fallthrough;
>  		default:
> -			print_object = true;
> +			o = *obj;
> +			obj = NULL;
>  			break;
>  		}
> +	} else {
> +		o.object = addr;
> +		o.state = ODEBUG_STATE_NOTAVAILABLE;
> +		o.descr = descr;
> +		obj = NULL;
>  	}

Same pattern here.

Thanks,

        tglx