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

[Louis Chauvet]

On 29/07/23 - 19:49, Maíra Canal 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>

Hi Maìra,

Thanks for pointing to this patch, it does not apply on drm-misc-next, but 
it was simple to manually rebase (see [0]).

I think it should solve the issue, and the CI seems to agree.

But it seems to be imperfect, as it introduce a warning on mutex unlock 
imbalance [1] (not always reproducable). It is better than a kernel crash 
already.

Do you want/have time to continue this fix?

[0]:https://gitlab.freedesktop.org/louischauvet/kernel/-/commit/017210f48c809730296d1f562e615b666fdbcfdc
[1]:https://gitlab.freedesktop.org/louischauvet/kernel/-/jobs/66118565/viewer#L803

Thanks,
Louis Chauvet

> ---
>  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);
>  		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
>