Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Message-ID: <62e16250-63f4-4fbb-b00e-db808b600664@intel.com>
Date:   Thu, 19 Oct 2023 12:31:38 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [Intel-gfx] [PATCH v2] debugobjects: stop accessing objects after
 releasing spinlock
Content-Language: en-US
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>
References: <20230925131359.2948827-1-andrzej.hajda@intel.com>
 <87v8bak6iy.ffs@tglx>
From:   Andrzej Hajda <andrzej.hajda@intel.com>
Organization: Intel Technology Poland sp. z o.o. - ul. Slowackiego 173, 80-298
 Gdansk - KRS 101882 - NIP 957-07-52-316
In-Reply-To: <87v8bak6iy.ffs@tglx>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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