Re: [PATCH v8 3/8] drm/amd/display: use drmm_writeback_connector_init()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Mismatched object lifetimes in KUnit test causes Use-After-Free when DRM device is torn down.

Pre-existing issues:
- [High] The struct dc_writeback_info allocated dynamically in dm_set_writeback() is leaked on every successful writeback job setup.
--

--- Patch [3]: [PATCH v8 3/8] drm/amd/display: use drmm_writeback_connector_init() ---
commit 4cc9223584993bb5748e535854c7b5fb0eba8f0d
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/amd/display: use drmm_writeback_connector_init()

The driver uses drmm_writeback_connector_init() to ensure proper job queue
cleanup. It also allocates a simple encoder using drmm_plain_encoder_alloc()
for the writeback connector.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 289da9d00b056..ff7b21830fce9 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5048,7 +5048,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);
>  	if (!acrtc) {
>  		drm_err(adev_to_drm(adev), "no amdgpu_crtc found\n");
>  		kfree(wb_info);

[Severity: High]
This isn't a bug introduced by this patch, but does dm_set_writeback() leak
the dynamically allocated wb_info struct on success?

When userspace configures a writeback job, wb_info is allocated dynamically
using kzalloc_obj(). It is then passed to dc_stream_add_writeback():

	dc_stream_add_writeback(dm->dc, crtc_state->stream, wb_info);

	acrtc->wb_conn = wb_conn;
	drm_writeback_queue_job(wb_conn, new_con_state);

However, dc_stream_add_writeback() copies the contents by value into an array
(stream->writeback_info[...] = *wb_info) but does not take ownership of the
pointer.

The function then returns without ever calling kfree(wb_info) on the success
path, which seems to result in a memory leak on every successful writeback
job setup.

> 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 9e7bad4d6ed0c..2bb86218e80b0 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

[ ... ]

> @@ -197,17 +196,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);
> +	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: Medium]
Can this conversion to drmm_writeback_connector_init() cause a use-after-free
in the KUnit tests during DRM device teardown?

In the KUnit test dm_test_wb_connector_init_success(), the mock DRM device is
allocated via dm_kunit_alloc_adev() (which registers a KUnit cleanup action).
The test object wbcon is then allocated using kunit_kzalloc():

	wbcon = kunit_kzalloc(test, sizeof(*wbcon), GFP_KERNEL);
	KUNIT_ASSERT_NOT_NULL(test, wbcon);

	ret = amdgpu_dm_wb_connector_init(dm, wbcon, 0);

Because KUnit cleanup actions run in LIFO order, when the test completes, it
first frees wbcon via kfree(), and then tears down the mock DRM device.

The device teardown triggers the drmm cleanup action registered by this patch
(drm_writeback_connector_cleanup), which will dereference the already-freed
wbcon to lock job_lock and iterate job_queue.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723003205.330362-1-John.Harrison@Igalia.com?part=3