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

[Maíra Canal]

Hi Louis,

On 05/11/24 10:59, Louis Chauvet wrote:
> 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.
> 

Yeah, I think it needs improvement indeed. Usually, unbalanced mutexes
aren't a good idea.

> Do you want/have time to continue this fix?

I don't plan to keep working in the patch. Feel free to pick the idea
and implement a proper fix.

Best Regards,
- Maíra

> 
> [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
>>