Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/amd/display: memory leak of wbcon in dm_test_wb_connector_init_success() - [Medium] drm/amd/display: memory leak of port in dm_mst_test_alloc_dsc_link_ctx() --
commit 46c185efd7ffdbf05586b8f2bd12c8e611ccd969 Author: John Harrison <[email protected]> drm/amd/display: Fix some memory lifetime issues in KUnit tests This commit updates memory allocations in several amdgpu display KUnit tests to avoid use-after-free errors during test teardown when memory debugging is enabled. [ ... ] > diff --git > a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c > index eafa3e6733a64..5dc6cd8801691 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c > @@ -3833,7 +3833,7 @@ dm_mst_test_alloc_dsc_link_ctx(struct kunit *test, int > total_avail_slots) > link_srv = kunit_kzalloc(test, sizeof(*link_srv), GFP_KERNEL); > res_pool = kunit_kzalloc(test, sizeof(*res_pool), GFP_KERNEL); > sink = kunit_kzalloc(test, sizeof(*sink), GFP_KERNEL); > - port = kunit_kzalloc(test, sizeof(*port), GFP_KERNEL); > + port = kzalloc(sizeof(*port), GFP_KERNEL); > KUNIT_ASSERT_NOT_NULL(test, link_srv); > KUNIT_ASSERT_NOT_NULL(test, res_pool); > KUNIT_ASSERT_NOT_NULL(test, sink); [Severity: Medium] Does this leak port in dm_mst_test_alloc_dsc_link_ctx() in drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c when running the dm_mst_test_dsc_link_* tests? Later in dm_mst_test_alloc_dsc_link_ctx(), port->malloc_kref is initialized to 1: drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c: dm_mst_test_alloc_dsc_link_ctx() { ... /* The payload allocation takes a reference, so start above zero. */ kref_init(&port->malloc_kref); port->mgr = &ctx->aconnector->mst_mgr; port->connector = &ctx->aconnector->base; port->full_pbn = 2000; ... } When pre_compute_mst_dsc_configs_for_state() runs compute_mst_dsc_configs_for_link() -> drm_dp_atomic_find_time_slots(), it adds a payload to ctx->mst_state->payloads and calls drm_dp_mst_get_port_malloc(port), incrementing port->malloc_kref from 1 to 2. On test exit, tearing down ctx->state calls drm_dp_mst_destroy_state() in drivers/gpu/drm/display/drm_dp_mst_topology.c: drivers/gpu/drm/display/drm_dp_mst_topology.c:drm_dp_mst_destroy_state() { ... list_for_each_entry_safe(pos, tmp, &mst_state->payloads, next) { /* We only keep references to ports with active payloads */ if (!pos->delete) drm_dp_mst_put_port_malloc(pos->port); kfree(pos); } ... } That drops only the payload reference, decrementing port->malloc_kref from 2 back to 1. Because ctx->aconnector is initialized with dm_mst_test_dsc_link_conn_funcs rather than dm_dp_mst_connector_destroy() and port is not attached to any branch in ctx->aconnector->mst_mgr, nothing ever drops the initial malloc_kref reference (and if it did reach 0, drm_dp_free_mst_port() would call drm_dp_mst_put_mstb_malloc() on a null port->parent). The original use-after-free happened because kunit_kzalloc() for port was registered after drm_kunit_helper_atomic_state_alloc(), so KUnit's last-in, first-out cleanup freed port before ctx->state teardown ran drm_dp_mst_destroy_state(). Could port be allocated with drmm_kzalloc(ctx->drm, ...) or allocated with kunit_kzalloc() prior to drm_kunit_helper_atomic_state_alloc() instead? > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_wb_test.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_wb_test.c > index 4bc033fdfa97a..f3b64a66cf28d 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_wb_test.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_wb_test.c > @@ -500,7 +500,7 @@ static void dm_test_wb_connector_init_success(struct > kunit *test) > dc->links[0] = link; > dm->dc = dc; > > - wbcon = kunit_kzalloc(test, sizeof(*wbcon), GFP_KERNEL); > + wbcon = kzalloc_obj(*wbcon); > KUNIT_ASSERT_NOT_NULL(test, wbcon); > > ret = amdgpu_dm_wb_connector_init(dm, wbcon, 0); [Severity: Medium] Won't changing wbcon to kzalloc_obj(*wbcon) in dm_test_wb_connector_init_success() in drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_wb_test.c leak wbcon on every run of this test? In drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c, amdgpu_dm_wb_connector_init() initializes wbcon->base through drm_writeback_connector_init() using amdgpu_dm_wb_connector_funcs: drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c: static const struct drm_connector_funcs amdgpu_dm_wb_connector_funcs = { .fill_modes = drm_helper_probe_single_connector_modes, .destroy = drm_connector_cleanup, .reset = amdgpu_dm_connector_funcs_reset, .atomic_duplicate_state = amdgpu_dm_connector_atomic_duplicate_state, .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, }; Unlike regular amdgpu_dm_connector_funcs (which uses amdgpu_dm_connector_destroy() to kfree() the connector), amdgpu_dm_wb_connector_funcs sets .destroy to drm_connector_cleanup(), which in drivers/gpu/drm/drm_connector.c only cleans up internal connector state without freeing the enclosing struct amdgpu_dm_wb_connector. The original use-after-free occurred because wbcon was allocated with kunit_kzalloc() after adev = dm_kunit_alloc_adev(test), causing KUnit to free wbcon before drm_mode_config_cleanup(&adev->ddev) cleaned up the writeback connector and encoder. Would allocating wbcon with kunit_kzalloc() before dm_kunit_alloc_adev(test) (or via drmm_kzalloc()) fix the ordering issue without leaking wbcon? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
