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

[Maira Canal]

On 8/3/23 17:52, Sebastian Wick wrote:
> 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?

The idea is to hold the lock until we have destroyed the CRTC state.
This way we can make sure that no job was queued between flushing
and destroying the state. So, this lock is unlocked at
vkms_atomic_crtc_destroy_state().

Best Regards,
- Maíra

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