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