Re: [PATCH v9 2/4] drm/colorop: make lut(1/3)d_interpolation props correctly behave as mutable

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The function `drm_colorop_atomic_destroy_state` frees the `drm_colorop_state` struct without releasing the reference to the `state->data` blob, leading to a memory leak of `drm_property_blob` objects.
- [High] `__drm_atomic_helper_colorop_duplicate_state` unconditionally resets `bypass = true`, destroying user state.
--

commit af8aea1831e57bd16ec95c73773d4e52e3faa54a
Author: Melissa Wen <mwen@igalia.com>
Subject: drm/colorop: make lut(1/3)d_interpolation props correctly behave as mutable

This commit moves the interpolation properties into drm_colorop_state so
they can correctly behave as mutable properties. It updates the getters,
setters, and state printing to use the state objects instead of the base
colorop.

> diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
> index 44eb823585d2e..5ab77cf47f0a9 100644
> --- a/drivers/gpu/drm/drm_colorop.c
> +++ b/drivers/gpu/drm/drm_colorop.c
[ ... ]
> @@ -488,6 +486,20 @@ static void __drm_colorop_state_reset(struct drm_colorop_state *colorop_state,
>  						      &val);
>  		colorop_state->curve_1d_type = val;
>  	}
> +
> +	if (colorop->lut1d_interpolation_property) {
> +		if (!drm_object_property_get_default_value(&colorop->base,
> +							   colorop->lut1d_interpolation_property,
> +							   &val))
> +			colorop_state->lut1d_interpolation = val;
> +	}
> +
> +	if (colorop->lut3d_interpolation_property) {
> +		if (!drm_object_property_get_default_value(&colorop->base,
> +							   colorop->lut3d_interpolation_property,
> +							   &val))
> +			colorop_state->lut3d_interpolation = val;
> +	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does 
drm_colorop_atomic_destroy_state() leak the state->data blob reference?

During atomic state duplication, a reference is taken on the data blob:

static void __drm_atomic_helper_colorop_duplicate_state(...)
{
        ...
        if (state->data)
                drm_property_blob_get(state->data);
        ...
}

However, the destruction function frees the state without dropping this
reference:

void drm_colorop_atomic_destroy_state(...)
{
        kfree(state);
}

Could this leak system memory on every atomic commit that includes the
colorop? Is it possible this also happens in drm_colorop_reset() when it
calls kfree() on colorop->state?

[Severity: High]
This is also a pre-existing issue, but does 
__drm_atomic_helper_colorop_duplicate_state() corrupt the user state by 
forcing bypass to true?

static void __drm_atomic_helper_colorop_duplicate_state(...)
{
        memcpy(state, colorop->state, sizeof(*state));
        ...
        state->bypass = true;
}

If userspace configures a color pipeline with bypass = false, will a
subsequent unrelated atomic commit (like moving a cursor or doing a pageflip)
that duplicates the colorop state inadvertently reset bypass back to true?

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