Re: [PATCH v9 3/4] drm/atomic: track individual colorop updates

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing `async_flip` validation for `DRM_MODE_OBJECT_COLOROP` allows colorop properties to be modified during async page flips, resulting in silently discarded updates.
- [High] Leaked `drm_property_blob` references in colorop state destruction.
--

commit e431074495b7fd11806af7e6be2bb0ec2ea72838
Author: Melissa Wen <mwen@igalia.com>

drm/atomic: track individual colorop updates

As we do for CRTC color mgmt properties, this commit uses the
color_mgmt_changed flag to track value changes in the color pipeline
of a given plane. This allows drivers to update color blocks as soon
as plane color pipeline or individual colorop values change.

> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index cd0fb5dc7d3ec..f799863c350c7 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
[ ... ]
> @@ -1271,8 +1296,10 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>  		break;
>  	}
>  	case DRM_MODE_OBJECT_COLOROP: {
> +		struct drm_plane_state *plane_state;
>  		struct drm_colorop *colorop = obj_to_colorop(obj);

[Severity: High]
This isn't a bug introduced by this patch, but does this block lack
validation for async_flip?

Unlike the blocks for planes, CRTCs, and connectors, there is no check for
async_flip here. If userspace submits an atomic commit with
DRM_MODE_PAGE_FLIP_ASYNC that modifies colorop properties, the ioctl
succeeds and modifies colorop_state inside the transaction.

However, drm_atomic_helper_async_commit() completely ignores colorops and
does not swap state->colorops. Could this lead to the updated state being
silently discarded and never applied to hardware, causing state divergence
between userspace expectations and kernel state?

>  		struct drm_colorop_state *colorop_state;
> +		bool replaced = false;
>  
>  		colorop_state = drm_atomic_get_colorop_state(state, colorop);
>  		if (IS_ERR(colorop_state)) {


[Severity: High]
This is a pre-existing issue, but is there a leaked drm_property_blob
reference in colorop state destruction?

Looking at drm_colorop_atomic_destroy_state() in
drivers/gpu/drm/drm_colorop.c:

void drm_colorop_atomic_destroy_state(struct drm_colorop *colorop,
				      struct drm_colorop_state *state)
{
	kfree(state);
}

When an atomic state is committed or aborted, the atomic framework frees the
associated state objects by calling their respective destroy functions. This
function simply calls kfree(state) without dropping the reference to the data
blob (state->data) via drm_property_blob_put().

Because drm_atomic_helper_colorop_duplicate_state() explicitly increments the
blob's reference count, and property setting acquires new blob references,
could failing to release the blob reference during state teardown lead to an
unbounded memory leak for every atomic update touching the color pipeline?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609110420.1298352-1-mwen@igalia.com?part=3