Re: [PATCH 1/3] drm/amd/display: Fix some memory lifetime issues in KUnit tests

[sashiko-bot@kernel.org]

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 <John.Harrison@Igalia.com>

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/20261022173655.2318987-1-John.Harrison@Igalia.com?part=1