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/appletbdrm: Allocate request/response buffers in begin_fb_access Date: Thu, 04 Jun 2026 15:21:21 +1000 Message-ID: In-Reply-To: <20260530185716.65688-5-tzimmermann@suse.de> References: <20260530185716.65688-1-tzimmermann@suse.de> <20260530185716.65688-5-tzimmermann@suse.de> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review This is the most interesting patch in the series. It moves damage-dependent buffer allocation from `atomic_check` (where damage state isn't finalized) to `begin_fb_access` (which runs during commit, after damage is fully resolved). The refactoring is clean -- `atomic_check` is simplified to just the standard `drm_atomic_helper_check_plane_state()` call, while buffer allocation moves to the new `begin_fb_access` wrapper that calls `drm_gem_begin_shadow_fb_access()` at the end. Key change: `DRM_GEM_SHADOW_PLANE_HELPER_FUNCS` is expanded to its individual components: ```c - DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, + .begin_fb_access = appletbdrm_primary_plane_helper_begin_fb_access, + .end_fb_access = drm_gem_end_shadow_fb_access, ``` The `drm_atomic_helper_damage_iter_init` call now passes NULL for `old_state`: ```c - drm_atomic_helper_damage_iter_init(&iter, old_plane_state, new_plane_state); + drm_atomic_helper_damage_iter_init(&iter, NULL, new_plane_state); ``` This is safe because `old_state` is only used in the current code for `drm_rect_equals(&state->src, &old_state->src)` check, and at this point in the series the `ignore_damage_clips` flag from patch 1 covers the modeset case. However, the src-coordinate comparison is **still** live in the `drm_atomic_helper_damage_iter_init()` code at this point in the series -- it's only removed in patch 6. So passing NULL here would crash on the `!drm_rect_equals(&state->src, &old_state->src)` line if `iter->clips` is non-NULL and `ignore_damage_clips` is false (since it would dereference `old_state` which is NULL). **Potential issue:** In the current tree, `drm_atomic_helper_damage_iter_init()` at line 247 does `!drm_rect_equals(&state->src, &old_state->src)` which would dereference the NULL `old_state`. This is guarded by the short-circuit `!iter->clips || state->ignore_damage_clips ||`, so it's only reached if clips exist AND `ignore_damage_clips` is false. Since `begin_fb_access` runs during the commit phase (after the check phase where `ignore_damage_clips` gets set for modesets), there's a narrow window where clips exist, no modeset was triggered, but src coords changed. In that case, passing NULL would cause a NULL pointer dereference. Patch 6 fixes this by moving the src check into the check phase, but between patches 4 and 6 the appletbdrm code is unsafe. Wait -- re-examining: the series was reordered in v4 specifically "to avoid error-prone intermediate state." Patches 4 comes before patch 5 (which removes the pre-check call) and patch 6 (which adds the src coord check to `drm_atomic_helper_check_plane_damage`). So between patches 4 and 5, the pre-check call at line 1037 still runs and sets `ignore_damage_clips` for modesets, but **not** for src coord changes. The src coord change detection only happens inside `drm_atomic_helper_damage_iter_init()` via the `old_state` param. So passing NULL for `old_state` in appletbdrm's `begin_fb_access` is **potentially unsafe** if src coords change without a modeset. This is a **real bisectability concern** -- if someone bisects to a point between patches 4 and 6, the appletbdrm driver could NULL-deref on a src-coordinate change. In practice, this is unlikely to be hit since appletbdrm is a Touch Bar display driver with fixed src coords, but it's still technically a bug window. --- Generated by Claude Code Patch Reviewer