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 <[email protected]> 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/[email protected]?part=2
