[PATCH] drm/vkms: avoid race-condition between flushing and destroying

[Sebastian Wick]

On Sun, Jul 30, 2023 at 12:51 AM Maíra Canal <mairacanal at riseup.net> wrote:
>
> After we flush the workqueue at the commit tale, we need to make sure
> that no work is queued until we destroy the state. Currently, new work
> can be queued in the workqueue, even after the commit tale, as the
> vblank thread is still running.
>
> Therefore, to avoid a race-condition that will lead to the trigger of a
> WARN_ON() at the function vkms_atomic_crtc_destroy_state(), add a mutex
> to protect the sections where the queue is manipulated.
>
> This way we can make sure that no work will be added to the workqueue
> between flushing the queue (at the commit tail) and destroying the
> state.
>
> Signed-off-by: Maíra Canal <mairacanal at riseup.net>
> ---
>  drivers/gpu/drm/vkms/vkms_crtc.c | 10 +++++++++-
>  drivers/gpu/drm/vkms/vkms_drv.c  |  1 +
>  drivers/gpu/drm/vkms/vkms_drv.h  |  8 ++++++++
>  3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 3c5ebf106b66..e5ec21a0da05 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -49,7 +49,9 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
>                 state->crc_pending = true;
>                 spin_unlock(&output->composer_lock);
>
> +               mutex_lock(&state->queue_lock);
>                 ret = queue_work(output->composer_workq, &state->composer_work);
> +               mutex_unlock(&state->queue_lock);
>                 if (!ret)
>                         DRM_DEBUG_DRIVER("Composer worker already queued\n");
>         }
> @@ -129,6 +131,7 @@ vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
>
>         __drm_atomic_helper_crtc_duplicate_state(crtc, &vkms_state->base);
>
> +       mutex_init(&vkms_state->queue_lock);
>         INIT_WORK(&vkms_state->composer_work, vkms_composer_worker);
>
>         return &vkms_state->base;
> @@ -142,6 +145,9 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
>         __drm_atomic_helper_crtc_destroy_state(state);
>
>         WARN_ON(work_pending(&vkms_state->composer_work));
> +       mutex_unlock(&vkms_state->queue_lock);
> +
> +       mutex_destroy(&vkms_state->queue_lock);
>         kfree(vkms_state->active_planes);
>         kfree(vkms_state);
>  }
> @@ -155,8 +161,10 @@ static void vkms_atomic_crtc_reset(struct drm_crtc *crtc)
>                 vkms_atomic_crtc_destroy_state(crtc, crtc->state);
>
>         __drm_atomic_helper_crtc_reset(crtc, &vkms_state->base);
> -       if (vkms_state)
> +       if (vkms_state) {
> +               mutex_init(&vkms_state->queue_lock);
>                 INIT_WORK(&vkms_state->composer_work, vkms_composer_worker);
> +       }
>  }
>
>  static const struct drm_crtc_funcs vkms_crtc_funcs = {
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index dd0af086e7fa..9212686ca88a 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -84,6 +84,7 @@ static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
>                 struct vkms_crtc_state *vkms_state =
>                         to_vkms_crtc_state(old_crtc_state);
>
> +               mutex_lock(&vkms_state->queue_lock);

I haven't looked at the code in detail but doesn't this need to be
unlocked after flush_work again?

>                 flush_work(&vkms_state->composer_work);
>         }
>
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index c7ae6c2ba1df..83767692469a 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -89,6 +89,14 @@ struct vkms_crtc_state {
>         struct vkms_writeback_job *active_writeback;
>         struct vkms_color_lut gamma_lut;
>
> +       /* protects the access to the workqueue
> +        *
> +        * we need to hold this lock between flushing the workqueue and
> +        * destroying the state to avoid work to be queued by the worker
> +        * thread
> +        */
> +       struct mutex queue_lock;
> +
>         /* below four are protected by vkms_output.composer_lock */
>         bool crc_pending;
>         bool wb_pending;
> --
> 2.41.0
>