Re: [PATCH v3 25/40] drm/amdgpu_dm: Convert to atomic_create_state

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing Error Handling / Null Pointer Dereference in `amdgpu_dm_crtc_init`.
- [High] Use After Free / List Corruption on plane initialization error paths.
--

commit b7f19de1d51c67315c5de464e67807b88c862ea5
Author: Maxime Ripard <mripard@kernel.org>

drm/amdgpu_dm: Convert to atomic_create_state

This commit converts the plane reset implementation to use
atomic_create_state. It allocates and initializes a pristine state without
resetting hardware, updating the plane functions and state initialization
paths to explicitly return errors on allocation failure.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> index 824ef3ce5de0a..f9655f534d3b6 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
[ ... ]
> @@ -2278,9 +2277,16 @@ int amdgpu_dm_plane_init(struct amdgpu_display_manager *dm,
>  		return res;
>  #endif
>  
> -	/* Create (reset) the plane state */
> -	if (plane->funcs->reset)
> -		plane->funcs->reset(plane);
> +	/* Create the plane state */
> +	if (plane->funcs->atomic_create_state) {
> +		struct drm_plane_state *plane_state;
> +
> +		plane_state = plane->funcs->atomic_create_state(plane);
> +		if (IS_ERR(plane_state))
> +			return PTR_ERR(plane_state);

[Severity: High]
This is a pre-existing issue, but returning this explicit error code exposes
an ignored return value in one of the callers.

In amdgpu_dm_crtc_init(), the error returned here for the cursor plane is
overwritten without being checked:

	cursor_plane->type = DRM_PLANE_TYPE_CURSOR;
	res = amdgpu_dm_plane_init(dm, cursor_plane, 0, NULL);

	acrtc = kzalloc_obj(struct amdgpu_crtc);
	if (!acrtc)
		goto fail;

	res = drm_crtc_init_with_planes( ... );

If amdgpu_dm_plane_init() fails, does the overwritten res variable mean the
cursor plane will be left with a NULL state and trigger a kernel panic in
the DRM atomic helpers later?

[Severity: High]
This is also a pre-existing issue, but this new error return path triggers
improper cleanup in the callers when atomic_create_state() fails.

If amdgpu_dm_plane_init() returns an error here after
drm_universal_plane_init() has already registered the plane, the callers
handle the failure by simply calling kfree() on the plane:

initialize_plane() in amdgpu_dm.c:
	ret = amdgpu_dm_plane_init(dm, plane, possible_crtcs, plane_cap);
	if (ret) {
		drm_err(adev_to_drm(dm->adev), "KMS: Failed to initialize plane\n");
		kfree(plane);
		return ret;
	}

amdgpu_dm_crtc_init() in amdgpu_dm_crtc.c:
fail:
	kfree(acrtc);
	kfree(cursor_plane);
	return res;

Since the plane is not properly cleaned up with drm_plane_cleanup() first,
could this leak a dangling pointer into the mode_config.plane_list and cause
list corruption when the DRM plane list is iterated?

> +
> +		plane->state = plane_state;
> +	}
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-drm-no-more-plane-reset-v3-0-1877c7aa57b3@kernel.org?part=25