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? 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? 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? > > if (res) > return res; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
