From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/colorop: Fix blob property reference tracking in state lifecycle Date: Fri, 13 Mar 2026 13:48:41 +1000 Message-ID: In-Reply-To: <20260312204145.829714-1-harry.wentland@amd.com> References: <20260312204145.829714-1-harry.wentland@amd.com> <20260312204145.829714-1-harry.wentland@amd.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Correctness =E2=80=94 the core fix is right:** The existing `drm_colorop_atomic_destroy_state()` at line 488-492 of the cu= rrent source does just `kfree(state)` without releasing the blob reference = obtained via `drm_property_blob_get()` in `__drm_atomic_helper_colorop_dupl= icate_state()`. This is a genuine refcount leak. Adding the `drm_property_b= lob_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_s= tate *state) +{ + drm_property_blob_put(state->data); +} ``` `drm_property_blob_put()` handles NULL gracefully (it's a `drm_property_blo= b_put` =E2=86=92 `kref_put` path guarded by a NULL check), so no explicit N= ULL 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 sub= tle ordering difference from the `drm_crtc` pattern: in `drm_atomic_helper_= crtc_reset()`, the new state is allocated *first*, and then the old state i= s 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= =E2=80=94 `__drm_colorop_reset` assigns it later), then the new state is a= llocated. If `kzalloc_obj` fails, `__drm_colorop_reset` is not called, so `= colorop->state` still points to freed memory (a use-after-free / dangling p= ointer). Looking at the existing code before this patch, this dangling-pointer-on-al= loc-failure bug **already existed** =E2=80=94 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 t= his 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 =3D 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 duplic= ating 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 i= f exported:** The helper is declared `static`, which means drivers subclassing the coloro= p 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 ca= n call it. The analogous `__drm_atomic_helper_crtc_destroy_state()` is expo= rted. If subclassing is anticipated, this should be non-static and exported= . If not, the docstring should be adjusted. This is a minor nit =E2=80=94 m= aking it `static` is fine for now since there are no out-of-tree subclass u= sers, and it can be exported later if needed. **Overall: The patch is correct and fixes real reference leaks. Recommend a= pplying.** --- Generated by Claude Code Patch Reviewer