Re: [PATCH v6 3/9] drm/amd/display: use drmm_writeback_connector_init()

[John Harrison]

On 6/16/26 00:42, Dmitry Baryshkov wrote:
On Mon, Jun 15, 2026 at 11:27:12AM -0700, John Harrison wrote:
On 6/12/26 11:58, sashiko-...@kernel.org wrote:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) 
to consider:
New issues:
- [High] Dangling drmm cleanup action on writeback connector initialization 
error path leads to Use-After-Free, and possible build failure with drmm_kfree.

Pre-existing issues:
- [High] Pre-existing memory leak of `wb_info` and sleeping in atomic context 
in `dm_set_writeback()`.
--

commit 946e1af8401710237d25526b5d87b4357eceb542
Author: Dmitry Baryshkov <dmitry.barysh...@oss.qualcomm.com>

drm/amd/display: use drmm_writeback_connector_init()

The patch converts the driver to use drmm_writeback_connector_init() and
drmm_plain_encoder_alloc() to ensure proper cleanup of the writeback
connector instance and its job queue.

--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
[ ... ]
@@ -11086,7 +11086,7 @@ static void dm_set_writeback(struct 
amdgpu_display_manager *dm,
                return;
        }
-       acrtc = to_amdgpu_crtc(wb_conn->encoder.crtc);
+       acrtc = to_amdgpu_crtc(crtc_state->base.crtc);
[Severity: High]
This is a pre-existing issue, but does dm_set_writeback() allocate memory
using GFP_KERNEL in an atomic context?

It appears that wb_info is allocated using kzalloc_obj() earlier in this
function, which implicitly uses GFP_KERNEL. However, this function is called
from amdgpu_dm_atomic_commit_tail(), which is an atomic context where sleeping
functions are forbidden.

Additionally, dc_stream_add_writeback() copies the contents of wb_info by
value, but the originally allocated pointer seems to never be freed before
dm_set_writeback() returns. Does this introduce a memory leak every time a
writeback job is queued?

        if (!acrtc) {
                drm_err(adev_to_drm(adev), "no amdgpu_crtc found\n");
                kfree(wb_info);
[ ... ]
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c
[ ... ]
@@ -188,17 +187,26 @@ int amdgpu_dm_wb_connector_init(struct 
amdgpu_display_manager *dm,
        struct dc *dc = dm->dc;
        struct dc_link *link = dc_get_link_at_index(dc, link_index);
        int res = 0;
+       struct drm_encoder *encoder;
+
+       encoder = drmm_plain_encoder_alloc(&dm->adev->ddev, NULL,
+                                          DRM_MODE_ENCODER_VIRTUAL, NULL);
+       if (IS_ERR(encoder))
+               return PTR_ERR(encoder);
+
+       drm_encoder_helper_add(encoder, &amdgpu_dm_wb_encoder_helper_funcs);
+
+       encoder->possible_crtcs = amdgpu_dm_get_encoder_crtc_mask(dm->adev);
        wbcon->link = link;
        drm_connector_helper_add(&wbcon->base.base, 
&amdgpu_dm_wb_conn_helper_funcs);
-       res = drm_writeback_connector_init(&dm->adev->ddev, &wbcon->base,
+       res = drmm_writeback_connector_init(&dm->adev->ddev, &wbcon->base,
                                            &amdgpu_dm_wb_connector_funcs,
-                                           &amdgpu_dm_wb_encoder_helper_funcs,
+                                           encoder,
                                            amdgpu_dm_wb_formats,
-                                           ARRAY_SIZE(amdgpu_dm_wb_formats),
-                                           
amdgpu_dm_get_encoder_crtc_mask(dm->adev));
+                                           ARRAY_SIZE(amdgpu_dm_wb_formats));
[Severity: High]
If drmm_writeback_connector_init() fails internally after successfully calling
drmm_connector_init(), will it leave the drmm_connector_cleanup_action
registered?
I'm not following this one. everything that drmm_writeback_connector_init()
does has a check on the return code and only if everything works does it
register the cleanup function as the last action. So how do you get a
dangling anything?

The caller, amdgpu_dm_initialize_drm_device(), appears to handle this failure
by attempting to free the connector using drmm_kfree(). Since drmm_kfree()
requires a dev argument, will this missing argument result in a build failure?
Bizarre that it is complaining about something in a totally different patch.

More importantly, since drmm_kfree() frees the memory but does not unregister
actions, will the managed resource list later execute
drmm_connector_cleanup_action on the already-freed pointer, resulting in a
use-after-free when the DRM device is torn down?
Isn't this just lying? The whole point of drmm_kfree() is surely that it
does unregister the cleanup of the allocation?
The question is not about freeint the memory, but about unrolling the
connector registration. I'll take a look later whether Sashiko is
hallucinating or not
Any updates on this patch set?

I think sashiko is hallucinating at least some of the above. The comment 
about drmm_kfree not unregistering is definitely bogus - because 
drmm_kfree certainly should unregister the cleanup action for the 
allocation being freed. Or maybe it is simply confused and is actually 
meaning the unregister action for wbconn->base rather than for wbconn 
itself. In which case, obviously it is not going to unregister the 
cleanup action for a different object entirely.

And presumably it is meaning "drm_writeback_connector_cleanup" when it 
says "drmm_connector_cleanup_action", given that the latter does not 
exist! But as already noted, that cleanup action is registered as the 
very last operation in the init sequence. So if the action has been 
registered then there will be no cleanup or roll back required because 
everything has succeeded. And at unload time, actions are run in reverse 
order of registration so the connector cleanup action will be run before 
the wbconn memory is freed.

John.



        if (res)
                return res;