On Mon, Jun 15, 2026 at 11:27:12AM -0700, John Harrison wrote: > On 6/12/26 11:58, [email protected] 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 <[email protected]> > > > > 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 > > > > > > if (res) > > > return res; > -- With best wishes Dmitry
