[PATCH] drm/colorop: Fix blob property reference tracking in state lifecycle

[PATCH] drm/colorop: Fix blob property reference tracking in state lifecycle

[Harry Wentland]

The colorop state blob property handling had memory leaks during state
duplication, destruction, and reset operations. The implementation
failed to follow the established pattern from drm_crtc's handling of
DEGAMMA/GAMMA blob properties.

Issues fixed:
- drm_colorop_atomic_destroy_state() was freeing state memory without
  releasing the blob reference, causing a leak
- drm_colorop_reset() was directly freeing old state with kfree()
  instead of properly destroying it, leaking blob references
- drm_colorop_cleanup() had duplicate blob cleanup code

Changes:
- Add __drm_atomic_helper_colorop_destroy_state() helper to properly
  release blob references before freeing state memory
- Update drm_colorop_atomic_destroy_state() to call the helper
- Fix drm_colorop_reset() to use drm_colorop_atomic_destroy_state()
  for proper cleanup of old state
- Simplify drm_colorop_cleanup() to use the common destruction path

This matches the well-tested pattern used by drm_crtc since 2016 and
ensures proper reference counting throughout the state lifecycle.

Co-developed by Claude Sonnet 4.5.

Fixes: cfc27680ee20 ("drm/colorop: Introduce new drm_colorop mode object")
Cc: Simon Ser <contact@emersion.fr>
Cc: Alex Hung <alex.hung@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Daniel Stone <daniels@collabora.com>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Louis Chauvet <louis.chauvet@bootlin.com>
Cc: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Cc: <stable@vger.kernel.org> #v6.19+
Signed-off-by: Harry Wentland <harry.wentland@amd.com>
---
 drivers/gpu/drm/drm_colorop.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index f421c623b3f0..647cf881f413 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -171,12 +171,8 @@ void drm_colorop_cleanup(struct drm_colorop *colorop)
 	list_del(&colorop->head);
 	config->num_colorop--;
 
-	if (colorop->state && colorop->state->data) {
-		drm_property_blob_put(colorop->state->data);
-		colorop->state->data = NULL;
-	}
-
-	kfree(colorop->state);
+	if (colorop->state)
+		drm_colorop_atomic_destroy_state(colorop, colorop->state);
 }
 EXPORT_SYMBOL(drm_colorop_cleanup);
 
@@ -485,9 +481,23 @@ drm_atomic_helper_colorop_duplicate_state(struct drm_colorop *colorop)
 	return state;
 }
 
+/**
+ * __drm_atomic_helper_colorop_destroy_state - release colorop state
+ * @state: colorop state object to release
+ *
+ * Releases all resources stored in the colorop state without actually freeing
+ * the memory of the colorop state. This is useful for drivers that subclass the
+ * colorop state.
+ */
+static void __drm_atomic_helper_colorop_destroy_state(struct drm_colorop_state *state)
+{
+	drm_property_blob_put(state->data);
+}
+
 void drm_colorop_atomic_destroy_state(struct drm_colorop *colorop,
 				      struct drm_colorop_state *state)
 {
+	__drm_atomic_helper_colorop_destroy_state(state);
 	kfree(state);
 }
 
@@ -538,7 +548,9 @@ static void __drm_colorop_reset(struct drm_colorop *colorop,
 
 void drm_colorop_reset(struct drm_colorop *colorop)
 {
-	kfree(colorop->state);
+	if (colorop->state)
+		drm_colorop_atomic_destroy_state(colorop, colorop->state);
+
 	colorop->state = kzalloc_obj(*colorop->state);
 
 	if (colorop->state)
-- 
2.53.0

Re: [PATCH] drm/colorop: Fix blob property reference tracking in state lifecycle

[Alex Hung]

Reviewed-by: Alex Hung <alex.hung@amd.com>

On 3/12/26 14:41, Harry Wentland wrote:
> The colorop state blob property handling had memory leaks during state
> duplication, destruction, and reset operations. The implementation
> failed to follow the established pattern from drm_crtc's handling of
> DEGAMMA/GAMMA blob properties.
> 
> Issues fixed:
> - drm_colorop_atomic_destroy_state() was freeing state memory without
>    releasing the blob reference, causing a leak
> - drm_colorop_reset() was directly freeing old state with kfree()
>    instead of properly destroying it, leaking blob references
> - drm_colorop_cleanup() had duplicate blob cleanup code
> 
> Changes:
> - Add __drm_atomic_helper_colorop_destroy_state() helper to properly
>    release blob references before freeing state memory
> - Update drm_colorop_atomic_destroy_state() to call the helper
> - Fix drm_colorop_reset() to use drm_colorop_atomic_destroy_state()
>    for proper cleanup of old state
> - Simplify drm_colorop_cleanup() to use the common destruction path
> 
> This matches the well-tested pattern used by drm_crtc since 2016 and
> ensures proper reference counting throughout the state lifecycle.
> 
> Co-developed by Claude Sonnet 4.5.
> 
> Fixes: cfc27680ee20 ("drm/colorop: Introduce new drm_colorop mode object")
> Cc: Simon Ser <contact@emersion.fr>
> Cc: Alex Hung <alex.hung@amd.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Daniel Stone <daniels@collabora.com>
> Cc: Melissa Wen <mwen@igalia.com>
> Cc: Sebastian Wick <sebastian.wick@redhat.com>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Louis Chauvet <louis.chauvet@bootlin.com>
> Cc: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> Cc: <stable@vger.kernel.org> #v6.19+
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> ---
>   drivers/gpu/drm/drm_colorop.c | 26 +++++++++++++++++++-------
>   1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
> index f421c623b3f0..647cf881f413 100644
> --- a/drivers/gpu/drm/drm_colorop.c
> +++ b/drivers/gpu/drm/drm_colorop.c
> @@ -171,12 +171,8 @@ void drm_colorop_cleanup(struct drm_colorop *colorop)
>   	list_del(&colorop->head);
>   	config->num_colorop--;
>   
> -	if (colorop->state && colorop->state->data) {
> -		drm_property_blob_put(colorop->state->data);
> -		colorop->state->data = NULL;
> -	}
> -
> -	kfree(colorop->state);
> +	if (colorop->state)
> +		drm_colorop_atomic_destroy_state(colorop, colorop->state);
>   }
>   EXPORT_SYMBOL(drm_colorop_cleanup);
>   
> @@ -485,9 +481,23 @@ drm_atomic_helper_colorop_duplicate_state(struct drm_colorop *colorop)
>   	return state;
>   }
>   
> +/**
> + * __drm_atomic_helper_colorop_destroy_state - release colorop state
> + * @state: colorop state object to release
> + *
> + * Releases all resources stored in the colorop state without actually freeing
> + * the memory of the colorop state. This is useful for drivers that subclass the
> + * colorop state.
> + */
> +static void __drm_atomic_helper_colorop_destroy_state(struct drm_colorop_state *state)
> +{
> +	drm_property_blob_put(state->data);
> +}
> +
>   void drm_colorop_atomic_destroy_state(struct drm_colorop *colorop,
>   				      struct drm_colorop_state *state)
>   {
> +	__drm_atomic_helper_colorop_destroy_state(state);
>   	kfree(state);
>   }
>   
> @@ -538,7 +548,9 @@ static void __drm_colorop_reset(struct drm_colorop *colorop,
>   
>   void drm_colorop_reset(struct drm_colorop *colorop)
>   {
> -	kfree(colorop->state);
> +	if (colorop->state)
> +		drm_colorop_atomic_destroy_state(colorop, colorop->state);
> +
>   	colorop->state = kzalloc_obj(*colorop->state);
>   
>   	if (colorop->state)

Claude review: drm/colorop: Fix blob property reference tracking in state lifecycle

[Claude Code Review Bot]

Overall Series Review

Subject: drm/colorop: Fix blob property reference tracking in state lifecycle
Author: Harry Wentland <harry.wentland@amd.com>
Patches: 2
Reviewed: 2026-03-13T13:48:40.828711

---

This is a single-patch fix for blob property reference leaks in the `drm_colorop` state lifecycle. The patch correctly identifies real bugs: `drm_colorop_atomic_destroy_state()` was calling `kfree(state)` without releasing the blob reference on `state->data`, and `drm_colorop_reset()` was similarly leaking. The fix follows the well-established pattern from `drm_crtc` state helpers. The patch is straightforward, correct, and appropriate for stable backporting.

**Verdict: Looks good, with minor observations below.**

---
Generated by Claude Code Patch Reviewer

Claude review: drm/colorop: Fix blob property reference tracking in state lifecycle

[Claude Code Review Bot]

Patch Review

**Correctness — the core fix is right:**

The existing `drm_colorop_atomic_destroy_state()` at line 488-492 of the current source does just `kfree(state)` without releasing the blob reference obtained via `drm_property_blob_get()` in `__drm_atomic_helper_colorop_duplicate_state()`. This is a genuine refcount leak. Adding the `drm_property_blob_put(state->data)` call before `kfree()` is the correct fix.

**`drm_property_blob_put()` with NULL is safe:**

```c
+static void __drm_atomic_helper_colorop_destroy_state(struct drm_colorop_state *state)
+{
+	drm_property_blob_put(state->data);
+}
```

`drm_property_blob_put()` handles NULL gracefully (it's a `drm_property_blob_put` → `kref_put` path guarded by a NULL check), so no explicit NULL check is needed here. This matches the crtc pattern.

**`drm_colorop_reset()` fix:**

```c
-	kfree(colorop->state);
+	if (colorop->state)
+		drm_colorop_atomic_destroy_state(colorop, colorop->state);
```

This is correct. The old code leaked the blob on reset. However, note a subtle ordering difference from the `drm_crtc` pattern: in `drm_atomic_helper_crtc_reset()`, the new state is allocated *first*, and then the old state is destroyed, so a failed allocation still leaves the old state intact. Here, the old state is destroyed first (and `colorop->state` is not yet cleared — `__drm_colorop_reset` assigns it later), then the new state is allocated. If `kzalloc_obj` fails, `__drm_colorop_reset` is not called, so `colorop->state` still points to freed memory (a use-after-free / dangling pointer).

Looking at the existing code before this patch, this dangling-pointer-on-alloc-failure bug **already existed** — the old `kfree(colorop->state)` had the same problem. So this patch doesn't introduce a regression, but it also doesn't fix it. This is a pre-existing issue and out of scope for this fix, but worth noting for a follow-up.

**`drm_colorop_cleanup()` simplification:**

```c
-	if (colorop->state && colorop->state->data) {
-		drm_property_blob_put(colorop->state->data);
-		colorop->state->data = NULL;
-	}
-
-	kfree(colorop->state);
+	if (colorop->state)
+		drm_colorop_atomic_destroy_state(colorop, colorop->state);
```

Good consolidation. The old code had an ad-hoc blob cleanup that was duplicating what `drm_colorop_atomic_destroy_state()` should be doing. Using the common path is cleaner and less error-prone.

**`__drm_atomic_helper_colorop_destroy_state` is static but documented as if exported:**

The helper is declared `static`, which means drivers subclassing the colorop state cannot use it. The docstring says "This is useful for drivers that subclass the colorop state," but since it's `static`, no external driver can call it. The analogous `__drm_atomic_helper_crtc_destroy_state()` is exported. If subclassing is anticipated, this should be non-static and exported. If not, the docstring should be adjusted. This is a minor nit — making it `static` is fine for now since there are no out-of-tree subclass users, and it can be exported later if needed.

**Overall: The patch is correct and fixes real reference leaks. Recommend applying.**

---
Generated by Claude Code Patch Reviewer