[PATCH 2/2] drm/amdgpu: Dirty cleared blocks on allocation

[Christian König]

On 5/28/25 11:29, Natalie Vock wrote:
> Hi,
> 
> On 5/28/25 09:07, Christian König wrote:
>> On 5/27/25 21:43, Natalie Vock wrote:
>>> If we hand out cleared blocks to users, they are expected to write
>>> at least some non-zero values somewhere. If we keep the CLEAR bit set on
>>> the block, amdgpu_fill_buffer will assume there is nothing to do and
>>> incorrectly skip clearing the block. Ultimately, the (still dirty) block
>>> will be reused as if it were cleared, without any wiping of the memory
>>> contents.
>>>
>>> Most severely, this means that any buffer allocated with
>>> AMDGPU_GEM_CREATE_VRAM_CLEARED | AMDGPU_GEM_CREATE_WIPE_ON_RELEASE
>>> (which is the case for **all userspace buffers**) are neither
>>> guaranteed to contain cleared VRAM, nor are they being wiped on
>>> release, potentially leaking application memory to arbitrary other
>>> applications.
>>>
>>> Fixes: a68c7eaa7a8ff ("drm/amdgpu: Enable clear page functionality")
>>> Cc: stable at vger.kernel.org
>>>
>>> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/3812
>>>
>>> Signed-off-by: Natalie Vock <natalie.vock at gmx.de>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>>> index 2d7f82e98df9..cecc67d0f0b8 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>>> @@ -591,6 +591,13 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
>>>       list_for_each_entry(block, &vres->blocks, link) {
>>>           unsigned long start;
>>>   +        /*
>>> +         * Allocated blocks may be dirtied as soon as we return.
>>> +         * Mark all blocks as dirty here, otherwise we might
>>> +         * incorrectly assume the memory is still zeroed.
>>> +         */
>>> +        drm_buddy_block_set_dirty(block);
>>
>> Exactly that makes no sense.
>>
>> We need the information if it's dirty or not later while clearing the blocks. Otherwise we will clear all blocks and completely loose the advantage of the clear tracking.
> 
> Right, I missed that separate clear on allocation. I was put a bit off-track by assuming DRM_BUDDY_ALLOCATE_CLEARED would guarantee cleared pages, when in reality it's more like a preference.
> 
>>
>> So we should set them dirty as soon as we are done with the clearing.
>>
>> But the problem rather seems to be that we sometimes don't clear the buffers on release for some reason, but still set it as cleared.
> 
> Yes precisely - "some reason" being the aforementioned clear flags. We do always call amdgpu_clear_buffer on release, but that function will perform the same checks as the clear on allocation does - that means, if a block is marked clear then it will skip emitting any actual clears.
> 
> If we don't mark the blocks as dirty after allocating, then the amdgpu_clear_buffer call on release will skip actually performing the clear like it did during allocation - this is obviously really broken.
> 
> After calling amdgpu_clear_buffer, we call amdgpu_vram_mgr_set_cleared which causes the drm_buddy blocks to be marked as "cleared" when freed. This part is correct in itself, but obviously breaks if amdgpu_clear_buffer didn't actually clear the buffer. That's how the dirty blocks end up in the buddy allocator as cleared ones.

IIRC I've pointed out exactly that during internal discussions as well and suggested that amdgpu_vram_mgr_set_cleared() is renamed to amdgpu_vram_mgr_set_state() and given a boolean flag to indicate the state.

So that the clear on allocation will clear the dirty and set the state dirty and the clear on release will clear everything and set the clean state.

Sounds like this was never implemented like it was originally planned.


> I'm testing a v2 that sets the dirty flags after the initial clear, I'll send it once I confirmed it works

Thanks a lot for looking into that!

Regards,
Christian.

.
> 
> Thanks,
> Natalie
> 
>>
>> Regards,
>> Christian.
>>
>>
>>> +
>>>           start = amdgpu_vram_mgr_block_start(block) +
>>>               amdgpu_vram_mgr_block_size(block);
>>>           start >>= PAGE_SHIFT;
>>
>