Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] VKMS composer accesses global colorop->state directly without shadowing it during atomic commit, leading to a data race and potential Use-After-Free. -- commit 7feb24a5b3e1c9cb2aa23694b44e46aece887a34 Author: Shixiong Ou <[email protected]> drm/vkms: Skip pre_blend_color_transform when pipeline is all bypassed This patch adds an optimization to pre_blend_color_transform() to skip the entire pixel processing loop when all colorops in the pipeline are bypassed or the pipeline is empty. > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c > b/drivers/gpu/drm/vkms/vkms_composer.c > index 83d217085ad09..18f58569f0169 100644 > --- a/drivers/gpu/drm/vkms/vkms_composer.c > +++ b/drivers/gpu/drm/vkms/vkms_composer.c > @@ -190,13 +190,29 @@ static void apply_colorop(struct pixel_argb_s32 *pixel, > struct drm_colorop *colo > } > } > > +static bool pipeline_all_bypassed(struct drm_colorop *colorop) > +{ > + while (colorop) { > + struct drm_colorop_state *colorop_state = colorop->state; [Severity: High] This is a pre-existing issue, but does this direct access to colorop->state introduce a potential data race and use-after-free? The VKMS composer processes frames asynchronously in a workqueue (vkms_composer_worker). While plane states are properly duplicated and shadowed, reading the global colorop->state pointer directly here in pipeline_all_bypassed() within drivers/gpu/drm/vkms/vkms_composer.c seems unsafe. If a non-blocking atomic commit swaps the state via drm_atomic_helper_swap_state() and frees the old state via drm_atomic_state_put() while the composer worker is preempted, will the worker dereference a freed state pointer when it resumes? Local userspace with DRM master privileges could potentially trigger this by submitting rapid non-blocking atomic commits that modify the color pipeline while the virtual hardware is compositing frames. This same unsafe access pattern already exists in the inner loop of pre_blend_color_transform(), but this patch expands the pattern into the new pipeline_all_bypassed() function. Should the colorop state be properly shadowed for the atomic commit instead of being read globally? > + > + if (!colorop_state || !colorop_state->bypass) > + return false; > + colorop = colorop->next; > + } > + return true; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
