Re: [PATCH 1/4] drm/vkms: use the gamma LUT of the state being composed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] drm/vkms: race condition publishing writeback job in
  vkms_wb_atomic_commit() [1]
--

commit e2d912779c4a80188eb9913ca9ba97afaaba49ac
Author: Oarora Etimis <oaroraetimis@gmail.com>

drm/vkms: use the gamma LUT of the state being composed

This commit reads the gamma LUT from crtc_state->base.gamma_lut rather than
crtc->state->gamma_lut in vkms_composer_worker() and clears the LUT fields
when no LUT is present.

> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 0fc915a954bad..1d5658bbd493b 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -609,6 +609,7 @@ void vkms_composer_worker(struct work_struct *work)
>  							  struct vkms_crtc_state,
>  							  composer_work);
>  	struct drm_crtc *crtc = crtc_state->base.crtc;
> +	struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
>  	struct vkms_writeback_job *active_wb = crtc_state->active_writeback;

[Severity: Critical]
This problem wasn't introduced by this patch, but can vkms_composer_worker()
race with vkms_wb_atomic_commit() on crtc_state->active_writeback [1]?

vkms_composer_worker() reads crtc_state->active_writeback before taking
out->composer_lock to read crtc_state->wb_pending, while
vkms_wb_atomic_commit() publishes both under output->composer_lock before
queuing and initializing the job:

drivers/gpu/drm/vkms/vkms_writeback.c:vkms_wb_atomic_commit() {
    ...
	spin_lock_irq(&output->composer_lock);
	crtc_state->active_writeback = active_wb;
	crtc_state->wb_pending = true;
	spin_unlock_irq(&output->composer_lock);
	drm_writeback_queue_job(wb_conn, connector_state);
	active_wb->pixel_write = get_pixel_write_function(wb_format);
	drm_rect_init(&wb_frame_info->src, 0, 0, crtc_width, crtc_height);
	drm_rect_init(&wb_frame_info->dst, 0, 0, crtc_width, crtc_height);
}

When a writeback commit runs while the vblank timer and composer worker are
already active (for example, with CRC capture enabled), the two contexts can
interleave:

CPU0                                  CPU1 (kworker)
vkms_wb_atomic_commit()               vkms_composer_worker()
  spin_lock_irq(&composer_lock)
  crtc_state->active_writeback = ...
  crtc_state->wb_pending = true
  spin_unlock_irq(&composer_lock)
  drm_writeback_queue_job(...)
                                        active_wb =
                                          crtc_state->active_writeback
                                        spin_lock_irq(&composer_lock)
                                        wb_pending = crtc_state->wb_pending
                                        spin_unlock_irq(&composer_lock)
                                        compose_active_planes(active_wb, ...)
                                        drm_writeback_signal_completion(...)
                                          ...
                                            vkms_wb_cleanup_job()
                                              kfree(active_wb)
  active_wb->pixel_write = ...
    <-- use-after-free

Depending on where the worker interleaves, it can also read a NULL or stale
active_wb before composer_lock is taken, hit WARN_ON(!active_wb->pixel_write)
in check_format_funcs(), or call drm_writeback_signal_completion() before
drm_writeback_queue_job(), which triggers WARN_ON(!job) and clears wb_pending
so the queued job never completes.

Initializing active_wb and calling drm_writeback_queue_job() before
publishing active_writeback and wb_pending under composer_lock, and reading
crtc_state->active_writeback under composer_lock in vkms_composer_worker(),
would avoid these races.

[1] https://sashiko.dev/#/bug/linux-420d0359-9fcb-45d5-af1b-74704c78f447

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-lut-v1-0-f762d70ae4cd@gmail.com?part=1