Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Calling `drmm_kfree` on `wbcon` after `drmm_writeback_connector_init` fails causes a Use-After-Free during DRM device teardown. - [Medium] The patch breaks the `amdgpu_dm_wb_test` KUnit tests by failing to update test assertions to match the new encoder allocation mechanism. --
--- Patch [3]: [PATCH v7 3/9] drm/amd/display: use drmm_writeback_connector_init() --- commit fecbcb335bbb22bbd24d07e0177c5af99179cac1 Author: Dmitry Baryshkov <[email protected]> drm/amd/display: use drmm_writeback_connector_init() The driver uses drm_writeback_connector_init() instead of its drmm counterpart, but it doesn't perform the job queue cleanup (neither manually nor by calling drm_writeback_connector_cleanup()). On the contrary, the drmm_writeback_connector_init() function ensures the proper cleanup of the job queue. Use drmm_plain_encoder_alloc() to allocate simple encoder and drmm_writeback_connector_init() in order to initialize writeback connector instance. > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c > index 0bf82e46f773f..3ee31634549e4 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c > @@ -193,17 +192,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; > + struct drm_encoder *encoder; > + > + encoder = drmm_plain_encoder_alloc(&dm->adev->ddev, NULL, > + DRM_MODE_ENCODER_VIRTUAL, NULL); [Severity: Medium] Does this change break the amdgpu_dm_wb_test KUnit tests? The test suite in drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_wb_test.c asserts on the state of the embedded encoder: dm_test_wb_connector_init_success() { ... KUNIT_EXPECT_TRUE(test, wbcon->base.encoder.funcs != NULL); KUNIT_EXPECT_EQ(test, wbcon->base.encoder.possible_crtcs, 0x1); } Since the code now dynamically allocates a separate encoder, the embedded one is left zero-initialized, which will likely cause the assertions to fail. > + 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)); [Severity: High] Can this cause a use-after-free during DRM device teardown if drmm_writeback_connector_init() fails? If drmm_writeback_connector_init() encounters a failure (like memory allocation failure in __drm_writeback_connector_init()), it returns an error but leaves drm_connector_cleanup_action registered from its prior call to drmm_connector_init(): drivers/gpu/drm/drm_writeback.c:drmm_writeback_connector_init() { ret = drmm_connector_init(dev, connector, con_funcs, ...); if (ret) return ret; ret = __drm_writeback_connector_init(dev, wb_connector, enc, ...); if (ret) return ret; // cleanup action remains registered } When this error propagates up, the caller amdgpu_dm_initialize_drm_device() explicitly frees the connector: drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:amdgpu_dm_initialize_drm_device() { if (amdgpu_dm_wb_connector_init(dm, wbcon, i)) { drm_err(adev_to_drm(adev), "amdgpu_dm_wb_connector_init failed\n"); drmm_kfree(adev_to_drm(adev), wbcon); continue; } } drmm_kfree() will free the memory, but it doesn't unregister the separate cleanup action. Later, during device teardown, drm_managed_release() will execute the dangling action and call drm_connector_cleanup() on the already freed wbcon pointer. > > if (res) > return res; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
