On 2021-06-18 10:46 a.m., Łukasz Bartosik wrote:
> czw., 17 cze 2021 o 16:18 Michel Dänzer <michel@xxxxxxxxxxx> napisał(a):
>>
>> On 2021-06-17 10:18 a.m., Lukasz Bartosik wrote:
>>> With option CONFIG_DEBUG_LIST enabled the kernel logs show list_add
>>> corruption warning. The warning originates from drm_framebuffer_init()
>>> function which adds framebuffer to a framebuffers list and is called by
>>> amdgpu_display_gem_fb_verify_and_init().
>>> If amdgpu_display_gem_fb_verify_and_init() encounters an error after
>>> calling drm_framebuffer_init() then framebuffer memory is released
>>> in amdgpu_display_user_framebuffer_create() without removing framebuffer
>>> from the list where it was added. Reuse of that memory by any other
>>> party cause corruption of the framebuffers linked list. This fix removes
>>> framebuffer from the linked list and unregisters it in case of failure.
>>>
>>> [...]
>>>
>>> Fixes: 6eed95b00b45 ("drm/amd/display: Store tiling_flags in the framebuffer.")
>>
>> I didn't realize there was already an issue before f258907fdd835e "drm/amdgpu: Verify bo size can fit framebuffer size on init.". Looking at
>> the Git history again, I agree there's already at least a theoretical issue in 5.11, though I suspect it's harder to hit in practice.
>>
>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> index c13985fb35be..933190281b91 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> @@ -1085,14 +1085,17 @@ int amdgpu_display_gem_fb_verify_and_init(
>>> mode_cmd->modifier[0]);
>>>
>>> ret = -EINVAL;
>>> - goto err;
>>> + goto err_fb_cleanup;
>>> }
>>>
>>> ret = amdgpu_display_framebuffer_init(dev, rfb, mode_cmd, obj);
>>> if (ret)
>>> - goto err;
>>> + goto err_fb_cleanup;
>>>
>>> return 0;
>>> +err_fb_cleanup:
>>> + drm_framebuffer_unregister_private(&rfb->base);
>>> + drm_framebuffer_cleanup(&rfb->base);
>>> err:
>>> drm_dbg_kms(dev, "Failed to verify and init gem fb: %d\n", ret);
>>> rfb->base.obj[0] = NULL;
>>>
>>
>> There's a similar issue in amdgpu_display_gem_fb_init. https://patchwork.freedesktop.org/patch/439542/ fixes that as well, and seems simpler (though I'm biased obviously :).
>
> I agree your patch is simpler and covers more cases, but IMHO my
> approach with explicit framebuffer cleanup has the advantage
> that it will be hard to miss in case of future code reorganizations in
> that area.
Fair enough.
FWIW, I went the "call drm_framebuffer_init last" route because it matches what all other drivers do AFAICT.
--
Earthling Michel Dänzer | https://redhat.com
Libre software enthusiast | Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
