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

From: Andrzej Hajda <andrzej.hajda@intel.com>
To: Thomas Gleixner <tglx@linutronix.de>,
	linux-kernel@vger.kernel.org, intel-gfx@lists.freedesktop.org,
	linux-mm@kvack.org
Cc: Nirmoy Das <nirmoy.das@intel.com>
Subject: Re: [Intel-gfx] [PATCH v2] debugobjects: stop accessing objects after releasing spinlock
Date: Thu, 19 Oct 2023 12:31:38 +0200	[thread overview]
Message-ID: <62e16250-63f4-4fbb-b00e-db808b600664@intel.com> (raw)
In-Reply-To: <87v8bak6iy.ffs@tglx>

On 13.10.2023 15:15, Thomas Gleixner wrote:
> On Mon, Sep 25 2023 at 15:13, 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.
> 
> It cannot be freed. If that happens then the calling code will have an
> UAF problem on the tracked item too.

Yes, and I have assumed that debugobjects are created also for detecting 
UAFs. They should be able to handle/detect 'issues due to incorrectly 
serialized concurrent accesses' scenarios as well, at least some of 
them. And even if it cannot recover it should at least provide reliable 
reporting.

Now we can have 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.

This seems the most drastic for me, but also with lowest chances to 
happen due to delayed freeing, but there are also other more probable 
scenarios when we print the same object but in state different from the 
one when debugobject detected issue, due to modification by concurrent 
thread.

> 
> If there is a concurrent modification then again, the calling code is
> lacking serialization on the tracked object.
> 
> debugobject fundamentally relies on the call site being consistent
> simply because it _cannot_ invoke the fixup callbacks with the hash
> bucket lock held.

Hmm, if call site is consistent then 'fixup' seems unnecessary, together 
with debugobjects.
I guess 'fixup' users should take care of locking on they own in such 
case, as it is currently, nothing changed.

> 
> What's the actualy problem you are trying to solve here. The changelog
> does not explain anything except of handwaving about modified/freed.

Presented above.

Regards
Andrzej

> 
> Thanks,
> 
>          tglx