[RFC PATCH 0/8] Convert colorop pipelines to use proper lists

[RFC PATCH 0/8] Convert colorop pipelines to use proper lists

[John Harrison]

As pointed out by Jani, colorop pipelines are using a locally coded
list system based on a 'next' pointer rather than official kernel list
structures:
https://lore.kernel.org/dri-devel/4452e675c4853faf665b520a8932a960946206bb@intel.com/

This patch set fixes that up.

The result is a little odd. There is no pipeline object as such -
pipelines are just a sequence of colorop objects and defined by
whatever arbitrary colorop object happens to be first in the sequence.
Therefore, there is no clear place to put the list head object. It has
to go inside the first colorop in the pipe. Which means there is a
list head inside *every* colorop object, regardless of whether it is
the head of a pipeline or not. Most of them are just empty lists.

Hence this set is being posted as an RFC - is the end result actually
better than the current version? Outside of open coding vs using
kernel helpers when iterating the list, probably the biggest advantage
is being able to assert that any given colorop is the head of a
pipeline rather than somewhere in the middle (or not in a pipeline at
all). It should not be possible to pass a non-head colorop into a
function that works on pipelines, but extra sanity checks are
generally a good thing.

Signed-off-by: John Harrison <John.Harrison@Igalia.com>
CC: Melissa Wen <mwen@igalia.com>
CC: Suraj Kandpal <suraj.kandpal@intel.com>
CC: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
CC: Jani Nikula <jani.nikula@linux.intel.com>
CC: Alex Hung <alex.hung@amd.com>
CC: Harry Wentland <harry.wentland@amd.com>
CC: kernel-dev@igalia.com
CC: intel-xe@lists.freedesktop.org
CC: intel-gfx@lists.freedesktop.org
CC: amd-gfx@lists.freedesktop.org
CC: dri-devel@lists.freedesktop.org

John Harrison (8):
  drm/colorop: Reduce scope of colorop helpers
  drm/colorop: Create pipelines via 'add to pipeline' instead of 'set
    next'
  drm/i915/colorop: Update to new pipeline API
  drm/amd/colorop: Update to new pipeline API
  drm/vkms/colorop: Update to new pipeline API
  drm/colorop: Remove old add-next API as no longer used
  drm/colorop: Convert pipline 'next' pointer to a list
  drm/colorop: Check that requested colorop is the root of a pipeline

 .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 60 ++++++++++---------
 .../amd/display/amdgpu_dm/amdgpu_dm_colorop.c | 16 ++---
 drivers/gpu/drm/drm_atomic.c                  | 15 ++++-
 drivers/gpu/drm/drm_atomic_uapi.c             | 30 +++++++---
 drivers/gpu/drm/drm_colorop.c                 | 29 +++++----
 .../drm/i915/display/intel_color_pipeline.c   | 14 ++---
 drivers/gpu/drm/i915/display/intel_plane.c    | 14 +++--
 drivers/gpu/drm/vkms/vkms_colorop.c           |  8 ++-
 drivers/gpu/drm/vkms/vkms_composer.c          |  9 +--
 include/drm/drm_colorop.h                     | 40 +++++--------
 10 files changed, 135 insertions(+), 100 deletions(-)

-- 
2.43.0

[RFC PATCH 1/8] drm/colorop: Reduce scope of colorop helpers

[John Harrison]

The drm_colorop_find function had a comment saying that uses outside
of DRM properties are deprecated. Such uses seem to have all been
removed and this function is only ever used once inside the property
assignment code. So make it a static helper local to the file.

Likewise drm_atomic_set_colorop_for_plane is never used outside of the
property set code. So make it a local static helper as well.

Signed-off-by: John Harrison <John.Harrison@Igalia.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c | 23 ++++++++++++++++-------
 include/drm/drm_colorop.h         | 20 --------------------
 2 files changed, 16 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 6441b55cc274..530576850239 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -259,16 +259,16 @@ drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
 EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
 
 /**
- * drm_atomic_set_colorop_for_plane - set colorop for plane
+ * set_colorop_for_plane - set colorop for plane
  * @plane_state: atomic state object for the plane
  * @colorop: colorop to use for the plane
  *
  * Helper function to select the color pipeline on a plane by setting
  * it to the first drm_colorop element of the pipeline.
  */
-void
-drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
-				 struct drm_colorop *colorop)
+static void
+set_colorop_for_plane(struct drm_plane_state *plane_state,
+		      struct drm_colorop *colorop)
 {
 	struct drm_plane *plane = plane_state->plane;
 
@@ -284,7 +284,16 @@ drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
 
 	plane_state->color_pipeline = colorop;
 }
-EXPORT_SYMBOL(drm_atomic_set_colorop_for_plane);
+
+static struct drm_colorop *colorop_find(struct drm_device *dev,
+					struct drm_file *file_priv,
+					uint32_t id)
+{
+	struct drm_mode_object *mo;
+
+	mo = drm_mode_object_find(dev, file_priv, id, DRM_MODE_OBJECT_COLOROP);
+	return mo ? obj_to_colorop(mo) : NULL;
+}
 
 /**
  * drm_atomic_set_crtc_for_connector - set CRTC for connector
@@ -599,12 +608,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
 		/* find DRM colorop object */
 		struct drm_colorop *colorop = NULL;
 
-		colorop = drm_colorop_find(dev, file_priv, val);
+		colorop = colorop_find(dev, file_priv, val);
 
 		if (val && !colorop)
 			return -EACCES;
 
-		drm_atomic_set_colorop_for_plane(state, colorop);
+		set_colorop_for_plane(state, colorop);
 	} else if (property == config->prop_fb_damage_clips) {
 		ret = drm_property_replace_blob_from_id(dev,
 					&state->fb_damage_clips,
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index b4b9e4f558ab..7f0bd58fb625 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -381,26 +381,6 @@ struct drm_colorop {
 
 #define obj_to_colorop(x) container_of(x, struct drm_colorop, base)
 
-/**
- * drm_colorop_find - look up a Colorop object from its ID
- * @dev: DRM device
- * @file_priv: drm file to check for lease against.
- * @id: &drm_mode_object ID
- *
- * This can be used to look up a Colorop from its userspace ID. Only used by
- * drivers for legacy IOCTLs and interface, nowadays extensions to the KMS
- * userspace interface should be done using &drm_property.
- */
-static inline struct drm_colorop *drm_colorop_find(struct drm_device *dev,
-						   struct drm_file *file_priv,
-						   uint32_t id)
-{
-	struct drm_mode_object *mo;
-
-	mo = drm_mode_object_find(dev, file_priv, id, DRM_MODE_OBJECT_COLOROP);
-	return mo ? obj_to_colorop(mo) : NULL;
-}
-
 void drm_colorop_pipeline_destroy(struct drm_device *dev);
 void drm_colorop_cleanup(struct drm_colorop *colorop);
 
-- 
2.43.0

Re: [RFC PATCH 1/8] drm/colorop: Reduce scope of colorop helpers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The prototype for `drm_atomic_set_colorop_for_plane` was not removed from the header file when the function was renamed and made static.
--

commit db8489fc6303b4a6973676321220c79b3ad4273f
Author: John Harrison <John.Harrison@Igalia.com>

drm/colorop: Reduce scope of colorop helpers

This commit reduces the scope of drm_colorop_find and
drm_atomic_set_colorop_for_plane by making them static local helpers within
the drm_atomic_uapi.c file.

> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 6441b55cc2740..530576850239f 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c

[ ... ]

> @@ -259,16 +259,16 @@ drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
>  EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
>  
>  /**
> - * drm_atomic_set_colorop_for_plane - set colorop for plane
> + * set_colorop_for_plane - set colorop for plane
>   * @plane_state: atomic state object for the plane
>   * @colorop: colorop to use for the plane
>   *
>   * Helper function to select the color pipeline on a plane by setting
>   * it to the first drm_colorop element of the pipeline.
>   */
> -void
> -drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
> -				 struct drm_colorop *colorop)
> +static void
> +set_colorop_for_plane(struct drm_plane_state *plane_state,
> +		      struct drm_colorop *colorop)

[Severity: Low]
Since the exported function drm_atomic_set_colorop_for_plane is being renamed
and made static here, should its corresponding declaration in
include/drm/drm_atomic_uapi.h also be removed? The prototype remains in that
header file without an implementation, leaving a dangling declaration.

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

[RFC PATCH 2/8] drm/colorop: Create pipelines via 'add to pipeline' instead of 'set next'

[John Harrison]

Add support for building colorop pipelines using an 'add new entry to
start of pipeline' helper instead of 'add new entry to end of
pipeline'. This is a pre-req of converting the pipeline to a proper
list rather than explicit 'next' pointers.

Signed-off-by: John Harrison <johnarr@igalia.com>
---
 drivers/gpu/drm/drm_colorop.c | 25 +++++++++++++++++++++++++
 include/drm/drm_colorop.h     |  1 +
 2 files changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index c0eecde8c176..b568ae942706 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -655,3 +655,28 @@ void drm_colorop_set_next_property(struct drm_colorop *colorop, struct drm_color
 	colorop->next = next;
 }
 EXPORT_SYMBOL(drm_colorop_set_next_property);
+
+/**
+ * drm_colorop_add_to_pipeline - adds the given colorop to a color pipeline
+ * @pipeline: drm colorop pipeline
+ * @colorop: drm colorop to add
+ *
+ * Should be used when constructing the color pipeline. Adds the new colorop
+ * to the end of the pipeline and sets the 'next' property of the previously
+ * last colorop (if there is one) to point to the new colorop.
+ */
+void drm_colorop_add_to_pipeline(struct drm_colorop *pipeline, struct drm_colorop *colorop)
+{
+	struct drm_colorop *last = pipeline;
+
+	/* Head entry does not need processing */
+	if (colorop == pipeline)
+		return;
+
+	while (last->next)
+		last = last->next;
+
+	drm_object_property_set_value(&last->base, last->next_property, colorop->base.id);
+	last->next = colorop;
+}
+EXPORT_SYMBOL(drm_colorop_add_to_pipeline);
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index 7f0bd58fb625..5c34bc3f115f 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -464,5 +464,6 @@ const char *
 drm_get_colorop_lut3d_interpolation_name(enum drm_colorop_lut3d_interpolation_type type);
 
 void drm_colorop_set_next_property(struct drm_colorop *colorop, struct drm_colorop *next);
+void drm_colorop_add_to_pipeline(struct drm_colorop *pipeline, struct drm_colorop *colorop);
 
 #endif /* __DRM_COLOROP_H__ */
-- 
2.43.0

[RFC PATCH 3/8] drm/i915/colorop: Update to new pipeline API

[John Harrison]

Switch to using the new method for building colorop pipelines.

Signed-off-by: John Harrison <John.Harrison@Igalia.com>
CC: Suraj Kandpal <suraj.kandpal@intel.com>
CC: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
---
 .../gpu/drm/i915/display/intel_color_pipeline.c    | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color_pipeline.c b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
index 6cf8080ee800..49dac9627553 100644
--- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c
+++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
@@ -53,7 +53,7 @@ static bool plane_has_3dlut(struct intel_display *display, enum pipe pipe,
 
 static
 struct intel_colorop *intel_color_pipeline_plane_add_colorop(struct drm_plane *plane,
-							     struct intel_colorop *prev,
+							     struct drm_colorop **pipeline,
 							     enum intel_color_block id)
 {
 	struct drm_device *dev = plane->dev;
@@ -100,8 +100,10 @@ struct intel_colorop *intel_color_pipeline_plane_add_colorop(struct drm_plane *p
 	if (ret)
 		goto cleanup;
 
-	if (prev)
-		drm_colorop_set_next_property(&prev->base, &colorop->base);
+	if (!*pipeline)
+		*pipeline = &colorop->base;
+
+	drm_colorop_add_to_pipeline(*pipeline, &colorop->base);
 
 	return colorop;
 
@@ -117,7 +119,7 @@ int _intel_color_pipeline_plane_init(struct drm_plane *plane, struct drm_prop_en
 	struct drm_device *dev = plane->dev;
 	struct intel_display *display = to_intel_display(dev);
 	struct intel_colorop *colorop[MAX_COLOROP];
-	struct intel_colorop *prev = NULL;
+	struct drm_colorop *pipeline_root = NULL;
 	const enum intel_color_block *pipeline;
 	int pipeline_len;
 	int ret = 0;
@@ -132,14 +134,12 @@ int _intel_color_pipeline_plane_init(struct drm_plane *plane, struct drm_prop_en
 	}
 
 	for (i = 0; i < pipeline_len; i++) {
-		colorop[i] = intel_color_pipeline_plane_add_colorop(plane, prev,
+		colorop[i] = intel_color_pipeline_plane_add_colorop(plane, &pipeline_root,
 								    pipeline[i]);
 		if (IS_ERR(colorop[i])) {
 			ret = PTR_ERR(colorop[i]);
 			goto cleanup;
 		}
-
-		prev = colorop[i];
 	}
 
 	list->type = colorop[0]->base.base.id;
-- 
2.43.0

Re: [RFC PATCH 3/8] drm/i915/colorop: Update to new pipeline API

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Missing `drm_mode_object_unregister()` in colorop cleanup leaves dangling pointers in the DRM device IDR.
- [High] If colorop initialization fails early, the cleanup path will unconditionally dereference uninitialized pointers, causing a kernel panic.
- [High] Missing error handling for `kasprintf` leads to a NULL pointer dereference when creating the pipeline property.
--

commit 430890cda6ad288f9a83188f2b716cf9737d8c83
Author: John Harrison <John.Harrison@Igalia.com>

drm/i915/colorop: Update to new pipeline API

Switch to using the new method for building colorop pipelines.

> diff --git a/drivers/gpu/drm/i915/display/intel_color_pipeline.c b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> index 6cf8080ee8000..49dac9627553a 100644
> --- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> +++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c

[ ... ]

> @@ -100,8 +100,10 @@ struct intel_colorop *intel_color_pipeline_plane_add_colorop(struct drm_plane *p
>  	if (ret)
>  		goto cleanup;
>  
> -	if (prev)
> -		drm_colorop_set_next_property(&prev->base, &colorop->base);
> +	if (!*pipeline)
> +		*pipeline = &colorop->base;
> +
> +	drm_colorop_add_to_pipeline(*pipeline, &colorop->base);
>  
>  	return colorop;

[Severity: High]
This isn't a bug introduced by this patch, but if the colorop initialization
fails before this point, the code jumps to the cleanup label:

	cleanup:
		intel_colorop_destroy(&colorop->base);
		return ERR_PTR(ret);

Could this unconditionally dereference an uninitialized pointer?

The colorop was just allocated, so colorop->dev is NULL.
Calling intel_colorop_destroy() invokes drm_colorop_cleanup(), which
attempts to dereference colorop->dev and call list_del(&colorop->head),
resulting in a NULL pointer dereference.

[Severity: Critical]
This is another pre-existing issue in the teardown path.

If a colorop is later destroyed (either here in the error path, or during
general teardown), drm_colorop_cleanup() is called:

	void drm_colorop_cleanup(struct drm_colorop *colorop)
	{
		...
		list_del(&colorop->head);
		config->num_colorop--;

		if (colorop->state)
			drm_colorop_atomic_destroy_state(colorop,
							 colorop->state);
	}

Is this missing a call to drm_mode_object_unregister()?

The object is registered with the DRM device IDR via drm_mode_object_add()
during initialization. Omitting drm_mode_object_unregister() leaves a
dangling pointer in dev->mode_config.object_idr, which could lead to a
use-after-free or a system crash during device teardown.

[ ... ]

> @@ -132,14 +134,12 @@ int _intel_color_pipeline_plane_init(struct drm_plane *plane, struct drm_prop_en
>  	}
>  
>  	for (i = 0; i < pipeline_len; i++) {
> -		colorop[i] = intel_color_pipeline_plane_add_colorop(plane, prev,
> +		colorop[i] = intel_color_pipeline_plane_add_colorop(plane, &pipeline_root,
>  								    pipeline[i]);
>  		if (IS_ERR(colorop[i])) {
>  			ret = PTR_ERR(colorop[i]);
>  			goto cleanup;
>  		}
> -
> -		prev = colorop[i];
>  	}
>  
>  	list->type = colorop[0]->base.base.id;

[Severity: High]
This is a pre-existing issue, but right after this line in
_intel_color_pipeline_plane_init(), there is an unchecked kasprintf():

	list->type = colorop[0]->base.base.id;
	list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", ...);

	return 0;

If this allocation fails under memory pressure, list->name is set to NULL
and the function unconditionally returns 0 (success).

Could this cause a NULL pointer dereference later when
drm_plane_create_color_pipeline_property() passes this name to
drm_property_create_enum() and drm_property_add_enum(), which performs a
strlen(name)?

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

[RFC PATCH 4/8] drm/amd/colorop: Update to new pipeline API

[John Harrison]

Switch to using the new method for building colorop pipelines.

Signed-off-by: John Harrison <John.Harrison@Igalia.com>
CC: Alex Hung <alex.hung@amd.com>
CC: Harry Wentland <harry.wentland@amd.com>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_colorop.c    | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c
index 48f5c431eaf9..6b5741d9d98b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c
@@ -86,6 +86,8 @@ amdgpu_dm_build_default_pipeline(struct drm_device *dev, struct drm_plane *plane
 	if (ret)
 		goto cleanup;
 
+	drm_colorop_add_to_pipeline(ops[0], ops[i]);
+
 	list->type = ops[i]->base.id;
 
 	i++;
@@ -102,7 +104,7 @@ amdgpu_dm_build_default_pipeline(struct drm_device *dev, struct drm_plane *plane
 	if (ret)
 		goto cleanup;
 
-	drm_colorop_set_next_property(ops[i-1], ops[i]);
+	drm_colorop_add_to_pipeline(ops[0], ops[i]);
 
 	i++;
 
@@ -119,7 +121,7 @@ amdgpu_dm_build_default_pipeline(struct drm_device *dev, struct drm_plane *plane
 	if (ret)
 		goto cleanup;
 
-	drm_colorop_set_next_property(ops[i-1], ops[i]);
+	drm_colorop_add_to_pipeline(ops[0], ops[i]);
 
 	i++;
 
@@ -137,7 +139,7 @@ amdgpu_dm_build_default_pipeline(struct drm_device *dev, struct drm_plane *plane
 		if (ret)
 			goto cleanup;
 
-		drm_colorop_set_next_property(ops[i-1], ops[i]);
+		drm_colorop_add_to_pipeline(ops[0], ops[i]);
 
 		i++;
 
@@ -156,7 +158,7 @@ amdgpu_dm_build_default_pipeline(struct drm_device *dev, struct drm_plane *plane
 		if (ret)
 			goto cleanup;
 
-		drm_colorop_set_next_property(ops[i-1], ops[i]);
+		drm_colorop_add_to_pipeline(ops[0], ops[i]);
 
 		i++;
 
@@ -174,7 +176,7 @@ amdgpu_dm_build_default_pipeline(struct drm_device *dev, struct drm_plane *plane
 		if (ret)
 			goto cleanup;
 
-		drm_colorop_set_next_property(ops[i-1], ops[i]);
+		drm_colorop_add_to_pipeline(ops[0], ops[i]);
 
 		i++;
 	}
@@ -192,7 +194,7 @@ amdgpu_dm_build_default_pipeline(struct drm_device *dev, struct drm_plane *plane
 	if (ret)
 		goto cleanup;
 
-	drm_colorop_set_next_property(ops[i - 1], ops[i]);
+	drm_colorop_add_to_pipeline(ops[0], ops[i]);
 
 	i++;
 
@@ -210,7 +212,7 @@ amdgpu_dm_build_default_pipeline(struct drm_device *dev, struct drm_plane *plane
 	if (ret)
 		goto cleanup;
 
-	drm_colorop_set_next_property(ops[i-1], ops[i]);
+	drm_colorop_add_to_pipeline(ops[0], ops[i]);
 
 	list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", ops[0]->base.id);
 
-- 
2.43.0

Re: [RFC PATCH 4/8] drm/amd/colorop: Update to new pipeline API

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing NULL check for kasprintf() when setting the color pipeline name leads to a potential NULL pointer dereference in strlen().
- [High] Calling list_del on an uninitialized list head during error cleanup causes a kernel panic.
--

commit 3a2ecacea83aea676a95a30af6588c8a29a29749
Author: John Harrison <John.Harrison@Igalia.com>

drm/amd/colorop: Update to new pipeline API

Switch to using the new method for building colorop pipelines.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c
> index 48f5c431eaf91..6b5741d9d98b0 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c
[ ... ]
> @@ -210,7 +212,7 @@ amdgpu_dm_build_default_pipeline(struct drm_device *dev, struct drm_plane *plane
>  	if (ret)
>  		goto cleanup;
>  
> -	drm_colorop_set_next_property(ops[i-1], ops[i]);
> +	drm_colorop_add_to_pipeline(ops[0], ops[i]);
>  
>  	list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", ops[0]->base.id);

[Severity: High]
This isn't a bug introduced by this patch, but does this kasprintf() need a
NULL check?

If the memory allocation in kasprintf() fails, it returns a NULL pointer.
This NULL pointer is blindly assigned to list->name and then passed down to
drm_plane_create_color_pipeline_property() and eventually to
drm_property_add_enum(), which attempts to compute its length:

drivers/gpu/drm/drm_property.c:drm_property_add_enum() {
	...
	if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN))
		return -EINVAL;
	...
}

Calling strlen(NULL) causes an immediate kernel panic during device
initialization. (This same pattern also appears in the Intel and VKMS
drivers' color pipeline initialization).

[Severity: High]
This is another pre-existing issue, but can calling list_del() on an
uninitialized list head during error cleanup cause a kernel panic?

When an error occurs early in color pipeline initialization (e.g.,
drm_mode_object_add() fails), the struct drm_colorop is zero-allocated but
never added to the global colorop_list. The error handling path in the Intel
driver unconditionally calls intel_colorop_destroy(), which delegates to
drm_colorop_cleanup():

drivers/gpu/drm/drm_colorop.c:drm_colorop_cleanup() {
	...
	list_del(&colorop->head);
	...
}

Because the list_head was never initialized with INIT_LIST_HEAD nor linked,
its prev pointer is NULL. The internal __list_del macro dereferences this
NULL pointer, causing an immediate kernel panic on driver probe failure. Is
this expected behavior?

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

[RFC PATCH 5/8] drm/vkms/colorop: Update to new pipeline API

[John Harrison]

Switch to using the new method for building colorop pipelines.

Signed-off-by: John Harrison <John.Harrison@Igalia.com>
CC: Harry Wentland <harry.wentland@amd.com>
---
 drivers/gpu/drm/vkms/vkms_colorop.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_colorop.c b/drivers/gpu/drm/vkms/vkms_colorop.c
index 071f3a8d2e7c..ecb4fd56eb8c 100644
--- a/drivers/gpu/drm/vkms/vkms_colorop.c
+++ b/drivers/gpu/drm/vkms/vkms_colorop.c
@@ -41,6 +41,8 @@ static int vkms_initialize_color_pipeline(struct drm_plane *plane, struct drm_pr
 	if (ret)
 		goto cleanup;
 
+	drm_colorop_add_to_pipeline(ops[0], ops[i]);
+
 	list->type = ops[i]->base.id;
 
 	i++;
@@ -58,7 +60,7 @@ static int vkms_initialize_color_pipeline(struct drm_plane *plane, struct drm_pr
 	if (ret)
 		goto cleanup;
 
-	drm_colorop_set_next_property(ops[i - 1], ops[i]);
+	drm_colorop_add_to_pipeline(ops[0], ops[i]);
 
 	i++;
 
@@ -75,7 +77,7 @@ static int vkms_initialize_color_pipeline(struct drm_plane *plane, struct drm_pr
 	if (ret)
 		goto cleanup;
 
-	drm_colorop_set_next_property(ops[i - 1], ops[i]);
+	drm_colorop_add_to_pipeline(ops[0], ops[i]);
 
 	i++;
 
@@ -93,7 +95,7 @@ static int vkms_initialize_color_pipeline(struct drm_plane *plane, struct drm_pr
 	if (ret)
 		goto cleanup;
 
-	drm_colorop_set_next_property(ops[i - 1], ops[i]);
+	drm_colorop_add_to_pipeline(ops[0], ops[i]);
 
 	list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", ops[0]->base.id);
 
-- 
2.43.0

Re: [RFC PATCH 5/8] drm/vkms/colorop: Update to new pipeline API

[sashiko-bot]

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

Pre-existing issues:
- [High] Unchecked kasprintf return value may lead to a NULL pointer dereference when setting up the color pipeline property.
--

commit e64dca02dc85f9d02e8990c56f98c1559490b722
Author: John Harrison <John.Harrison@Igalia.com>

drm/vkms/colorop: Update to new pipeline API
    
Switch to using the new method for building colorop pipelines.

> diff --git a/drivers/gpu/drm/vkms/vkms_colorop.c b/drivers/gpu/drm/vkms/vkms_colorop.c
> index 071f3a8d2e7c4..ecb4fd56eb8cc 100644
> --- a/drivers/gpu/drm/vkms/vkms_colorop.c
> +++ b/drivers/gpu/drm/vkms/vkms_colorop.c
[ ... ]
> @@ -93,7 +95,7 @@ static int vkms_initialize_color_pipeline(struct drm_plane *plane, struct drm_pr
>  	if (ret)
>  		goto cleanup;
>  
> -	drm_colorop_set_next_property(ops[i - 1], ops[i]);
> +	drm_colorop_add_to_pipeline(ops[0], ops[i]);
>  
>  	list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", ops[0]->base.id);
>  

[Severity: High]
This is a pre-existing issue, but does this kasprintf() need a NULL check?

If kasprintf() fails to allocate memory, it returns NULL. Since
vkms_initialize_color_pipeline() then unconditionally returns 0 (success), the
caller vkms_initialize_colorops() will pass this NULL string down to
drm_plane_create_color_pipeline_property(). This is ultimately passed to
drm_property_add_enum(), where calling strlen() on the NULL name will cause a
NULL pointer dereference.

Should this allocation be checked and routed to the cleanup path on failure?

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

[RFC PATCH 6/8] drm/colorop: Remove old add-next API as no longer used

[John Harrison]

All users have been updated to the new method for building colorop
pipelines. So remove the old version.

Signed-off-by: John Harrison <John.Harrison@Igalia.com>
---
 drivers/gpu/drm/drm_colorop.c | 16 ----------------
 include/drm/drm_colorop.h     |  1 -
 2 files changed, 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index b568ae942706..5a6d05557bc4 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -640,22 +640,6 @@ const char *drm_get_colorop_lut3d_interpolation_name(enum drm_colorop_lut3d_inte
 	return colorop_lu3d_interpolation_name[type];
 }
 
-/**
- * drm_colorop_set_next_property - sets the next pointer
- * @colorop: drm colorop
- * @next: next colorop
- *
- * Should be used when constructing the color pipeline
- */
-void drm_colorop_set_next_property(struct drm_colorop *colorop, struct drm_colorop *next)
-{
-	drm_object_property_set_value(&colorop->base,
-				      colorop->next_property,
-				      next ? next->base.id : 0);
-	colorop->next = next;
-}
-EXPORT_SYMBOL(drm_colorop_set_next_property);
-
 /**
  * drm_colorop_add_to_pipeline - adds the given colorop to a color pipeline
  * @pipeline: drm colorop pipeline
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index 5c34bc3f115f..6888d27e5592 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -463,7 +463,6 @@ drm_get_colorop_lut1d_interpolation_name(enum drm_colorop_lut1d_interpolation_ty
 const char *
 drm_get_colorop_lut3d_interpolation_name(enum drm_colorop_lut3d_interpolation_type type);
 
-void drm_colorop_set_next_property(struct drm_colorop *colorop, struct drm_colorop *next);
 void drm_colorop_add_to_pipeline(struct drm_colorop *pipeline, struct drm_colorop *colorop);
 
 #endif /* __DRM_COLOROP_H__ */
-- 
2.43.0

[RFC PATCH 7/8] drm/colorop: Convert pipline 'next' pointer to a list

[John Harrison]

The colorop pipeline system was using explicit next pointers and open
coded lists rather than the official kernel list helpers. Fix that.

The result is a little odd. There is no pipeline object as such -
pipelines are just a sequence of colorop objects and defined by
whatever arbitrary colorop object happens to be first in the sequence.
Therefore, there is no clear place to put the list head object. It
has to go inside the first colorop in the pipe. Which means there is a
list head inside *every* colorop object, regardless of whether it is the
head of a pipeline or not. Most of them are just empty lists.

Signed-off-by: John Harrison <John.Harrison@Igalia.com>
Suggested-by: Jani Nikula <jani.nikula@linux.intel.com>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 60 ++++++++++---------
 drivers/gpu/drm/drm_atomic.c                  | 15 ++++-
 drivers/gpu/drm/drm_colorop.c                 | 16 +++--
 drivers/gpu/drm/i915/display/intel_plane.c    | 14 +++--
 drivers/gpu/drm/vkms/vkms_composer.c          |  9 +--
 include/drm/drm_colorop.h                     | 18 +++++-
 6 files changed, 79 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
index 86086d10c543..e6de93f25362 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
@@ -1629,21 +1629,21 @@ __set_dm_plane_colorop_multiplier(struct drm_plane_state *plane_state,
 static int
 __set_dm_plane_colorop_shaper(struct drm_plane_state *plane_state,
 			      struct dc_plane_state *dc_plane_state,
-			      struct drm_colorop *colorop)
+			      struct drm_colorop *pipeline)
 {
-	struct drm_colorop *old_colorop;
+	struct drm_colorop *old_colorop, *colorop;
 	struct drm_colorop_state *colorop_state = NULL, *new_colorop_state;
 	struct drm_atomic_commit *state = plane_state->state;
 	enum dc_transfer_func_predefined default_tf = TRANSFER_FUNCTION_LINEAR;
 	struct dc_transfer_func *tf = &dc_plane_state->in_shaper_func;
 	const struct drm_color_lut32 *shaper_lut;
-	struct drm_device *dev = colorop->dev;
+	struct drm_device *dev = pipeline->dev;
 	bool enabled = false;
 	u32 shaper_size;
 	int i = 0, ret = 0;
 
 	/* 1D Curve - SHAPER TF */
-	old_colorop = colorop;
+	old_colorop = pipeline;
 	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
 		if (new_colorop_state->colorop == old_colorop &&
 		    (BIT(new_colorop_state->curve_1d_type) & amdgpu_dm_supported_shaper_tfs)) {
@@ -1664,11 +1664,11 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state *plane_state,
 	}
 
 	/* 1D LUT - SHAPER LUT */
-	colorop = old_colorop->next;
-	if (!colorop) {
+	if (list_is_last(&old_colorop->pipeline_list, &pipeline->pipeline_head)) {
 		drm_dbg(dev, "no Shaper LUT colorop found\n");
 		return -EINVAL;
 	}
+	colorop = list_next_entry(old_colorop, pipeline_list);
 
 	old_colorop = colorop;
 	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
@@ -1793,20 +1793,20 @@ __set_dm_plane_colorop_3dlut(struct drm_plane_state *plane_state,
 static int
 __set_dm_plane_colorop_blend(struct drm_plane_state *plane_state,
 			     struct dc_plane_state *dc_plane_state,
-			     struct drm_colorop *colorop)
+			     struct drm_colorop *pipeline)
 {
-	struct drm_colorop *old_colorop;
+	struct drm_colorop *old_colorop, *colorop;
 	struct drm_colorop_state *colorop_state = NULL, *new_colorop_state;
 	struct drm_atomic_commit *state = plane_state->state;
 	enum dc_transfer_func_predefined default_tf = TRANSFER_FUNCTION_LINEAR;
 	struct dc_transfer_func *tf = &dc_plane_state->blend_tf;
 	const struct drm_color_lut32 *blend_lut = NULL;
-	struct drm_device *dev = colorop->dev;
+	struct drm_device *dev = pipeline->dev;
 	uint32_t blend_size = 0;
 	int i = 0;
 
 	/* 1D Curve - BLND TF */
-	old_colorop = colorop;
+	old_colorop = pipeline;
 	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
 		if (new_colorop_state->colorop == old_colorop &&
 		    (BIT(new_colorop_state->curve_1d_type) & amdgpu_dm_supported_blnd_tfs)) {
@@ -1825,11 +1825,11 @@ __set_dm_plane_colorop_blend(struct drm_plane_state *plane_state,
 	}
 
 	/* 1D Curve - BLND LUT */
-	colorop = old_colorop->next;
-	if (!colorop) {
+	if (list_is_last(&old_colorop->pipeline_list, &pipeline->pipeline_head)) {
 		drm_dbg(dev, "no Blend LUT colorop found\n");
 		return -EINVAL;
 	}
+	colorop = list_next_entry(old_colorop, pipeline_list);
 
 	old_colorop = colorop;
 	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
@@ -1911,14 +1911,15 @@ static int
 amdgpu_dm_plane_set_colorop_properties(struct drm_plane_state *plane_state,
 				       struct dc_plane_state *dc_plane_state)
 {
-	struct drm_colorop *colorop = plane_state->color_pipeline;
+	struct drm_colorop *pipeline = plane_state->color_pipeline;
+	struct drm_colorop *colorop = pipeline;
 	struct drm_device *dev = plane_state->plane->dev;
 	struct amdgpu_device *adev = drm_to_adev(dev);
 	bool has_3dlut = adev->dm.dc->caps.color.dpp.hw_3d_lut || adev->dm.dc->caps.color.mpc.preblend;
 	int ret;
 
 	/* 1D Curve - DEGAM TF */
-	if (!colorop)
+	if (!pipeline)
 		return -EINVAL;
 
 	ret = __set_dm_plane_colorop_degamma(plane_state, dc_plane_state, colorop);
@@ -1926,71 +1927,74 @@ amdgpu_dm_plane_set_colorop_properties(struct drm_plane_state *plane_state,
 		return ret;
 
 	/* Multiplier */
-	colorop = colorop->next;
-	if (!colorop) {
+	if (list_is_last(&colorop->pipeline_list, &pipeline->pipeline_head)) {
 		drm_dbg(dev, "no multiplier colorop found\n");
 		return -EINVAL;
 	}
 
+	colorop = list_next_entry(colorop, pipeline_list);
 	ret = __set_dm_plane_colorop_multiplier(plane_state, dc_plane_state, colorop);
 	if (ret)
 		return ret;
 
 	/* 3x4 matrix */
-	colorop = colorop->next;
-	if (!colorop) {
+	if (list_is_last(&colorop->pipeline_list, &pipeline->pipeline_head)) {
 		drm_dbg(dev, "no 3x4 matrix colorop found\n");
 		return -EINVAL;
 	}
 
+	colorop = list_next_entry(colorop, pipeline_list);
 	ret = __set_dm_plane_colorop_3x4_matrix(plane_state, dc_plane_state, colorop);
 	if (ret)
 		return ret;
 
 	if (has_3dlut) {
 		/* 1D Curve & LUT - SHAPER TF & LUT */
-		colorop = colorop->next;
-		if (!colorop) {
+		if (list_is_last(&colorop->pipeline_list, &pipeline->pipeline_head)) {
 			drm_dbg(dev, "no Shaper TF colorop found\n");
 			return -EINVAL;
 		}
 
+		colorop = list_next_entry(colorop, pipeline_list);
 		ret = __set_dm_plane_colorop_shaper(plane_state, dc_plane_state, colorop);
 		if (ret)
 			return ret;
 
 		/* Shaper LUT colorop is already handled, just skip here */
-		colorop = colorop->next;
-		if (!colorop)
+		if (list_is_last(&colorop->pipeline_list, &pipeline->pipeline_head)) {
+			drm_dbg(dev, "no Shaper LUT colorop found\n");
 			return -EINVAL;
+		}
+		colorop = list_next_entry(colorop, pipeline_list);
 
 		/* 3D LUT */
-		colorop = colorop->next;
-		if (!colorop) {
+		if (list_is_last(&colorop->pipeline_list, &pipeline->pipeline_head)) {
 			drm_dbg(dev, "no 3D LUT colorop found\n");
 			return -EINVAL;
 		}
 
+		colorop = list_next_entry(colorop, pipeline_list);
 		ret = __set_dm_plane_colorop_3dlut(plane_state, dc_plane_state, colorop);
 		if (ret)
 			return ret;
 	}
 
 	/* 1D Curve & LUT - BLND TF & LUT */
-	colorop = colorop->next;
-	if (!colorop) {
+	if (list_is_last(&colorop->pipeline_list, &pipeline->pipeline_head)) {
 		drm_dbg(dev, "no Blend TF colorop found\n");
 		return -EINVAL;
 	}
 
+	colorop = list_next_entry(colorop, pipeline_list);
 	ret = __set_dm_plane_colorop_blend(plane_state, dc_plane_state, colorop);
 	if (ret)
 		return ret;
 
 	/* BLND LUT colorop is already handled, just skip here */
-	colorop = colorop->next;
-	if (!colorop)
+	if (list_is_last(&colorop->pipeline_list, &pipeline->pipeline_head)) {
+		drm_dbg(dev, "no Blend LUT colorop found\n");
 		return -EINVAL;
+	}
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 3af1b9cc9a06..e2a75b70604d 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -893,10 +893,14 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
 }
 
 static void drm_atomic_colorop_print_state(struct drm_printer *p,
+					   struct drm_colorop **pipeline,
 					   const struct drm_colorop_state *state)
 {
 	struct drm_colorop *colorop = state->colorop;
 
+	if (!*pipeline)
+		*pipeline = colorop;
+
 	drm_printf(p, "colorop[%u]:\n", colorop->base.id);
 	drm_printf_indent(p, 1, "type=%s\n", drm_get_colorop_type_name(colorop->type));
 	if (colorop->bypass_property)
@@ -929,7 +933,12 @@ static void drm_atomic_colorop_print_state(struct drm_printer *p,
 		break;
 	}
 
-	drm_printf_indent(p, 1, "next=%d\n", colorop->next ? colorop->next->base.id : 0);
+	if (list_is_last(&colorop->pipeline_list, &(*pipeline)->pipeline_head)) {
+		*pipeline = NULL;
+		return;
+	}
+
+	drm_printf_indent(p, 1, "next=%d\n", list_next_entry(colorop, pipeline_list)->base.id);
 }
 
 static void drm_atomic_plane_print_state(struct drm_printer *p,
@@ -2125,7 +2134,7 @@ static void __drm_state_dump(struct drm_device *dev, struct drm_printer *p,
 			     bool take_locks)
 {
 	struct drm_mode_config *config = &dev->mode_config;
-	struct drm_colorop *colorop;
+	struct drm_colorop *colorop, *colorop_pipeline = NULL;
 	struct drm_plane *plane;
 	struct drm_crtc *crtc;
 	struct drm_connector *connector;
@@ -2138,7 +2147,7 @@ static void __drm_state_dump(struct drm_device *dev, struct drm_printer *p,
 	list_for_each_entry(colorop, &config->colorop_list, head) {
 		if (take_locks)
 			drm_modeset_lock(&colorop->plane->mutex, NULL);
-		drm_atomic_colorop_print_state(p, colorop->state);
+		drm_atomic_colorop_print_state(p, &colorop_pipeline, colorop->state);
 		if (take_locks)
 			drm_modeset_unlock(&colorop->plane->mutex);
 	}
diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index 5a6d05557bc4..5b4e71e90623 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -109,7 +109,8 @@ static int drm_plane_colorop_init(struct drm_device *dev, struct drm_colorop *co
 	colorop->dev = dev;
 	colorop->type = type;
 	colorop->plane = plane;
-	colorop->next = NULL;
+	INIT_LIST_HEAD(&colorop->pipeline_list);
+	INIT_LIST_HEAD(&colorop->pipeline_head);
 	colorop->funcs = funcs;
 
 	list_add_tail(&colorop->head, &config->colorop_list);
@@ -651,16 +652,13 @@ const char *drm_get_colorop_lut3d_interpolation_name(enum drm_colorop_lut3d_inte
  */
 void drm_colorop_add_to_pipeline(struct drm_colorop *pipeline, struct drm_colorop *colorop)
 {
-	struct drm_colorop *last = pipeline;
+	struct drm_colorop *last;
 
-	/* Head entry does not need processing */
-	if (colorop == pipeline)
-		return;
+	last = list_last_entry_or_null(&pipeline->pipeline_head, struct drm_colorop, pipeline_list);
 
-	while (last->next)
-		last = last->next;
+	if (last)
+		drm_object_property_set_value(&last->base, last->next_property, colorop->base.id);
 
-	drm_object_property_set_value(&last->base, last->next_property, colorop->base.id);
-	last->next = colorop;
+	list_add_tail(&colorop->pipeline_list, &pipeline->pipeline_head);
 }
 EXPORT_SYMBOL(drm_colorop_add_to_pipeline);
diff --git a/drivers/gpu/drm/i915/display/intel_plane.c b/drivers/gpu/drm/i915/display/intel_plane.c
index 3eaf82477f49..877c20ed62bd 100644
--- a/drivers/gpu/drm/i915/display/intel_plane.c
+++ b/drivers/gpu/drm/i915/display/intel_plane.c
@@ -413,21 +413,23 @@ intel_plane_color_copy_uapi_to_hw_state(struct intel_atomic_state *state,
 					const struct intel_plane_state *from_plane_state,
 					struct intel_crtc *crtc)
 {
-	struct drm_colorop *iter_colorop, *colorop;
+	struct drm_colorop *iter_colorop, *colorop, *pipeline;
 	struct drm_colorop_state *new_colorop_state;
 	struct intel_colorop *intel_colorop;
 	struct drm_property_blob *blob;
-	struct intel_crtc_state *new_crtc_state = state ?
-		intel_atomic_get_new_crtc_state(state, crtc) : NULL;
+	struct intel_crtc_state *new_crtc_state;
 	bool changed = false;
 	int i = 0;
 
 	if (!state)
 		return;
 
-	iter_colorop = from_plane_state->uapi.color_pipeline;
+	pipeline = from_plane_state->uapi.color_pipeline;
+	if (!pipeline)
+		return;
 
-	while (iter_colorop) {
+	WARN_ON(!list_is_first(&pipeline->pipeline_list, &pipeline->pipeline_head));
+	list_for_each_entry(iter_colorop, &pipeline->pipeline_head, pipeline_list) {
 		for_each_new_colorop_in_state(&state->base, colorop, new_colorop_state, i) {
 			if (new_colorop_state->colorop == iter_colorop) {
 				blob = new_colorop_state->bypass ? NULL : new_colorop_state->data;
@@ -437,9 +439,9 @@ intel_plane_color_copy_uapi_to_hw_state(struct intel_atomic_state *state,
 									    blob);
 			}
 		}
-		iter_colorop = iter_colorop->next;
 	}
 
+	new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
 	if (new_crtc_state && changed)
 		new_crtc_state->plane_color_changed = true;
 }
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 83d217085ad0..fc20b788738a 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -194,9 +194,12 @@ static void pre_blend_color_transform(const struct vkms_plane_state *plane_state
 				      struct line_buffer *output_buffer)
 {
 	struct pixel_argb_s32 pixel;
+	struct drm_colorop *pipeline = plane_state->base.base.color_pipeline;
+
+	WARN_ON(!list_is_first(&pipeline->pipeline_list, &pipeline->pipeline_head));
 
 	for (size_t x = 0; x < output_buffer->n_pixels; x++) {
-		struct drm_colorop *colorop = plane_state->base.base.color_pipeline;
+		struct drm_colorop *colorop;
 
 		/*
 		 * Some operations, such as applying a BT709 encoding matrix,
@@ -213,7 +216,7 @@ static void pre_blend_color_transform(const struct vkms_plane_state *plane_state
 		pixel.g = output_buffer->pixels[x].g;
 		pixel.b = output_buffer->pixels[x].b;
 
-		while (colorop) {
+		list_for_each_entry(colorop, &pipeline->pipeline_head, pipeline_list) {
 			struct drm_colorop_state *colorop_state;
 
 			colorop_state = colorop->state;
@@ -223,8 +226,6 @@ static void pre_blend_color_transform(const struct vkms_plane_state *plane_state
 
 			if (!colorop_state->bypass)
 				apply_colorop(&pixel, colorop);
-
-			colorop = colorop->next;
 		}
 
 		/* clamp values */
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index 6888d27e5592..f500a6077e60 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -271,12 +271,24 @@ struct drm_colorop {
 	enum drm_colorop_type type;
 
 	/**
-	 * @next:
+	 * @pipeline_head:
 	 *
 	 * Read-only
-	 * Pointer to next drm_colorop in pipeline
+	 * List head for the pipeline. Only valid if this drm_colorop object is the
+	 * head of a pipeline. Will be an empty list for all other drm_colorop objects.
+	 * Invariant once the pipeline has been constructed. So does not need locking.
 	 */
-	struct drm_colorop *next;
+	struct list_head pipeline_head;
+
+	/**
+	 * @pipeline_list:
+	 *
+	 * Read-only
+	 * List entry within the @pipeline_head list of the pipeline that this colorop
+	 * is part of. Invariant once the pipeline has been constructed. So does not
+	 * need locking.
+	 */
+	struct list_head pipeline_list;
 
 	/**
 	 * @type_property:
-- 
2.43.0

Re: [RFC PATCH 7/8] drm/colorop: Convert pipline 'next' pointer to a list

[sashiko-bot]

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

New issues:
- [High] Unconditional dereference of potentially NULL color pipeline in VKMS pre_blend_color_transform.
- [Medium] Fragile global state tracking in `drm_atomic_colorop_print_state` leads to misaligned pointer dereferences.

Pre-existing issues:
- [Critical] Data race and Use-After-Free due to asynchronous access of global `colorop->state` in VKMS composer.
--

commit bddf98b16d8f020be8f44f0de1ce3cd9f1f44d2d
Author: John Harrison <John.Harrison@Igalia.com>

drm/colorop: Convert pipline 'next' pointer to a list
    
The colorop pipeline system was using explicit next pointers and open
coded lists rather than the official kernel list helpers. Fix that.

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 170de30c28ae4..8e985fd0562be 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
[ ... ]
> @@ -849,7 +853,12 @@ static void drm_atomic_colorop_print_state(struct drm_printer *p,
>  		break;
>  	}
>  
> -	drm_printf_indent(p, 1, "next=%d\n", colorop->next ? colorop->next->base.id : 0);
> +	if (list_is_last(&colorop->pipeline_list, &(*pipeline)->pipeline_head)) {
> +		*pipeline = NULL;
> +		return;
> +	}
> +
> +	drm_printf_indent(p, 1, "next=%d\n", list_next_entry(colorop, pipeline_list)->base.id);
>  }

[Severity: Medium]
What happens if an orphaned colorop (created but not added to any pipeline)
exists in the global config->colorop_list?

It appears the pipeline pointer would be set to this orphan, but list_is_last
will never evaluate to true since its pipeline_head is empty. Won't the
pipeline tracking get permanently stuck, causing a subsequent valid pipeline's
last node to return false here?

This looks like it would evaluate list_next_entry which computes container_of
on pipeline_head at a different offset, leading to a misaligned pointer
dereference.

> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 83d217085ad09..fc20b788738a3 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -194,9 +194,12 @@ static void pre_blend_color_transform(const struct vkms_plane_state *plane_state
>  				      struct line_buffer *output_buffer)
>  {
>  	struct pixel_argb_s32 pixel;
> +	struct drm_colorop *pipeline = plane_state->base.base.color_pipeline;
> +
> +	WARN_ON(!list_is_first(&pipeline->pipeline_list, &pipeline->pipeline_head));

[Severity: High]
Could this cause a regression if userspace doesn't configure a color pipeline
and leaves the property unset?

The previous code handled a missing pipeline safely, but this looks like it
will unconditionally dereference the pipeline pointer inside the list_is_first
helper before checking if it actually exists. Does this need an early return
or a valid pipeline check first?

>  
>  	for (size_t x = 0; x < output_buffer->n_pixels; x++) {
> -		struct drm_colorop *colorop = plane_state->base.base.color_pipeline;
> +		struct drm_colorop *colorop;
>  
>  		/*
[ ... ]
> @@ -213,7 +216,7 @@ static void pre_blend_color_transform(const struct vkms_plane_state *plane_state
>  		pixel.g = output_buffer->pixels[x].g;
>  		pixel.b = output_buffer->pixels[x].b;
>  
> -		while (colorop) {
> +		list_for_each_entry(colorop, &pipeline->pipeline_head, pipeline_list) {
>  			struct drm_colorop_state *colorop_state;
>  
>  			colorop_state = colorop->state;

[Severity: Critical]
This is a pre-existing issue, but can this direct access to colorop->state
race with concurrent atomic commits?

Since vkms_composer_worker runs asynchronously, couldn't a modeset dynamically
free this state via drm_atomic_helper_commit_cleanup_done while we're reading
it, resulting in a use-after-free regression?

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

[RFC PATCH 8/8] drm/colorop: Check that requested colorop is the root of a pipeline

[John Harrison]

When assigning a colorop pipeline to a plane, the assignment is done
via the id of the root colorop in the pipeline. Add a check to ensure
that only root colorops are assigned and not something further down a
chain.

Signed-off-by: John Harrison <John.Harrison@Igalia.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 530576850239..18193a270d40 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -613,6 +613,13 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
 		if (val && !colorop)
 			return -EACCES;
 
+		/* Colorop must be the start of a pipeline, not the middle */
+		if (colorop && !list_is_first(&colorop->pipeline_list, &colorop->pipeline_head)) {
+			drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] Colorop %d is not head of a pipeline\n",
+				plane->base.id, plane->name, colorop->index);
+			return -EINVAL;
+		}
+
 		set_colorop_for_plane(state, colorop);
 	} else if (property == config->prop_fb_damage_clips) {
 		ret = drm_property_replace_blob_from_id(dev,
-- 
2.43.0