[PATCH] drm/vkms: Fix race-condition between the hrtimer and the atomic commit

[PATCH] drm/vkms: Fix race-condition between the hrtimer and the atomic commit

[Pranav Tyagi]

From: Maíra Canal <mcanal@igalia.com>

[ Upstream commit a0e6a017ab56936c0405fe914a793b241ed25ee0 ]

Currently, it is possible for the composer to be set as enabled and then
as disabled without a proper call for the vkms_vblank_simulate(). This
is problematic, because the driver would skip one CRC output, causing CRC
tests to fail. Therefore, we need to make sure that, for each time the
composer is set as enabled, a composer job is added to the queue.

In order to provide this guarantee, add a mutex that will lock before
the composer is set as enabled and will unlock only after the composer
job is added to the queue. This way, we can have a guarantee that the
driver won't skip a CRC entry.

This race-condition is affecting the IGT test "writeback-check-output",
making the test fail and also, leaking writeback framebuffers, as the
writeback job is queued, but it is not signaled. This patch avoids both
problems.

[v2]:
    * Create a new mutex and keep the spinlock across the atomic commit in
      order to avoid interrupts that could result in deadlocks.

[ Backport to 5.15: context cleanly applied with no semantic changes.
Build-tested. ]

Signed-off-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: Arthur Grillo <arthurgrillo@riseup.net>
Signed-off-by: Maíra Canal <mairacanal@riseup.net>
Link: https://patchwork.freedesktop.org/patch/msgid/20230523123207.173976-1-mcanal@igalia.com
Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_composer.c | 9 +++++++--
 drivers/gpu/drm/vkms/vkms_crtc.c     | 9 +++++----
 drivers/gpu/drm/vkms/vkms_drv.h      | 4 +++-
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 9e8204be9a14..77fced36af55 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -332,10 +332,15 @@ void vkms_set_composer(struct vkms_output *out, bool enabled)
 	if (enabled)
 		drm_crtc_vblank_get(&out->crtc);
 
-	spin_lock_irq(&out->lock);
+	mutex_lock(&out->enabled_lock);
 	old_enabled = out->composer_enabled;
 	out->composer_enabled = enabled;
-	spin_unlock_irq(&out->lock);
+
+	/* the composition wasn't enabled, so unlock the lock to make sure the lock
+	 * will be balanced even if we have a failed commit
+	 */
+	if (!out->composer_enabled)
+		mutex_unlock(&out->enabled_lock);
 
 	if (old_enabled)
 		drm_crtc_vblank_put(&out->crtc);
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 57bbd32e9beb..1b02dee8587a 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -16,7 +16,7 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 	struct drm_crtc *crtc = &output->crtc;
 	struct vkms_crtc_state *state;
 	u64 ret_overrun;
-	bool ret, fence_cookie;
+	bool ret, fence_cookie, composer_enabled;
 
 	fence_cookie = dma_fence_begin_signalling();
 
@@ -25,15 +25,15 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 	if (ret_overrun != 1)
 		pr_warn("%s: vblank timer overrun\n", __func__);
 
-	spin_lock(&output->lock);
 	ret = drm_crtc_handle_vblank(crtc);
 	if (!ret)
 		DRM_ERROR("vkms failure on handling vblank");
 
 	state = output->composer_state;
-	spin_unlock(&output->lock);
+	composer_enabled = output->composer_enabled;
+	mutex_unlock(&output->enabled_lock);
 
-	if (state && output->composer_enabled) {
+	if (state && composer_enabled) {
 		u64 frame = drm_crtc_accurate_vblank_count(crtc);
 
 		/* update frame_start only if a queued vkms_composer_worker()
@@ -293,6 +293,7 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 
 	spin_lock_init(&vkms_out->lock);
 	spin_lock_init(&vkms_out->composer_lock);
+	mutex_init(&vkms_out->enabled_lock);
 
 	vkms_out->composer_workq = alloc_ordered_workqueue("vkms_composer", 0);
 	if (!vkms_out->composer_workq)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index d48c23d40ce5..666997e2bcab 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -83,8 +83,10 @@ struct vkms_output {
 	struct workqueue_struct *composer_workq;
 	/* protects concurrent access to composer */
 	spinlock_t lock;
+	/* guarantees that if the composer is enabled, a job will be queued */
+	struct mutex enabled_lock;
 
-	/* protected by @lock */
+	/* protected by @enabled_lock */
 	bool composer_enabled;
 	struct vkms_crtc_state *composer_state;
 
-- 
2.49.0

Re: [PATCH] drm/vkms: Fix race-condition between the hrtimer and the atomic commit

[Greg KH]

On Thu, Jun 26, 2025 at 07:52:43PM +0530, Pranav Tyagi wrote:
> From: Maíra Canal <mcanal@igalia.com>
> 
> [ Upstream commit a0e6a017ab56936c0405fe914a793b241ed25ee0 ]
> 
> Currently, it is possible for the composer to be set as enabled and then
> as disabled without a proper call for the vkms_vblank_simulate(). This
> is problematic, because the driver would skip one CRC output, causing CRC
> tests to fail. Therefore, we need to make sure that, for each time the
> composer is set as enabled, a composer job is added to the queue.
> 
> In order to provide this guarantee, add a mutex that will lock before
> the composer is set as enabled and will unlock only after the composer
> job is added to the queue. This way, we can have a guarantee that the
> driver won't skip a CRC entry.
> 
> This race-condition is affecting the IGT test "writeback-check-output",
> making the test fail and also, leaking writeback framebuffers, as the
> writeback job is queued, but it is not signaled. This patch avoids both
> problems.
> 
> [v2]:
>     * Create a new mutex and keep the spinlock across the atomic commit in
>       order to avoid interrupts that could result in deadlocks.
> 
> [ Backport to 5.15: context cleanly applied with no semantic changes.
> Build-tested. ]

Did you forget about 6.1.y?

confused,

greg k-h

Re: [PATCH] drm/vkms: Fix race-condition between the hrtimer and the atomic commit

[Maíra Canal]

Hi Pranav,

On 26/06/25 11:22, Pranav Tyagi wrote:
> From: Maíra Canal <mcanal@igalia.com>
> 
> [ Upstream commit a0e6a017ab56936c0405fe914a793b241ed25ee0 ]
> 
> Currently, it is possible for the composer to be set as enabled and then
> as disabled without a proper call for the vkms_vblank_simulate(). This
> is problematic, because the driver would skip one CRC output, causing CRC
> tests to fail. Therefore, we need to make sure that, for each time the
> composer is set as enabled, a composer job is added to the queue.
> 
> In order to provide this guarantee, add a mutex that will lock before
> the composer is set as enabled and will unlock only after the composer
> job is added to the queue. This way, we can have a guarantee that the
> driver won't skip a CRC entry.
> 
> This race-condition is affecting the IGT test "writeback-check-output",
> making the test fail and also, leaking writeback framebuffers, as the
> writeback job is queued, but it is not signaled. This patch avoids both
> problems.
> 
> [v2]:
>      * Create a new mutex and keep the spinlock across the atomic commit in
>        order to avoid interrupts that could result in deadlocks.
> 
> [ Backport to 5.15: context cleanly applied with no semantic changes.
> Build-tested. ]
> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> Reviewed-by: Arthur Grillo <arthurgrillo@riseup.net>
> Signed-off-by: Maíra Canal <mairacanal@riseup.net>
> Link: https://patchwork.freedesktop.org/patch/msgid/20230523123207.173976-1-mcanal@igalia.com
> Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>

This patch violates locking rules and it was reversed a while ago.
Please, check commit 7908632f2927 ("Revert "drm/vkms: Fix race-condition
between the hrtimer and the atomic commit"").

Best Regards,
- Maíra

Re: [PATCH] drm/vkms: Fix race-condition between the hrtimer and the atomic commit

[Pranav Tyagi]

On Thu, Jun 26, 2025 at 8:27 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jun 26, 2025 at 07:52:43PM +0530, Pranav Tyagi wrote:
> > From: Maíra Canal <mcanal@igalia.com>
> >
> > [ Upstream commit a0e6a017ab56936c0405fe914a793b241ed25ee0 ]
> >
> > Currently, it is possible for the composer to be set as enabled and then
> > as disabled without a proper call for the vkms_vblank_simulate(). This
> > is problematic, because the driver would skip one CRC output, causing CRC
> > tests to fail. Therefore, we need to make sure that, for each time the
> > composer is set as enabled, a composer job is added to the queue.
> >
> > In order to provide this guarantee, add a mutex that will lock before
> > the composer is set as enabled and will unlock only after the composer
> > job is added to the queue. This way, we can have a guarantee that the
> > driver won't skip a CRC entry.
> >
> > This race-condition is affecting the IGT test "writeback-check-output",
> > making the test fail and also, leaking writeback framebuffers, as the
> > writeback job is queued, but it is not signaled. This patch avoids both
> > problems.
> >
> > [v2]:
> >     * Create a new mutex and keep the spinlock across the atomic commit in
> >       order to avoid interrupts that could result in deadlocks.
> >
> > [ Backport to 5.15: context cleanly applied with no semantic changes.
> > Build-tested. ]
>
> Did you forget about 6.1.y?
>
> confused,
>
> greg k-h

Apologies for the oversight.

Regards
Pranav Tyagi

Re: [PATCH] drm/vkms: Fix race-condition between the hrtimer and the atomic commit

[Pranav Tyagi]

On Thu, Jun 26, 2025 at 8:32 PM Maíra Canal <mcanal@igalia.com> wrote:
>
> Hi Pranav,
>
> On 26/06/25 11:22, Pranav Tyagi wrote:
> > From: Maíra Canal <mcanal@igalia.com>
> >
> > [ Upstream commit a0e6a017ab56936c0405fe914a793b241ed25ee0 ]
> >
> > Currently, it is possible for the composer to be set as enabled and then
> > as disabled without a proper call for the vkms_vblank_simulate(). This
> > is problematic, because the driver would skip one CRC output, causing CRC
> > tests to fail. Therefore, we need to make sure that, for each time the
> > composer is set as enabled, a composer job is added to the queue.
> >
> > In order to provide this guarantee, add a mutex that will lock before
> > the composer is set as enabled and will unlock only after the composer
> > job is added to the queue. This way, we can have a guarantee that the
> > driver won't skip a CRC entry.
> >
> > This race-condition is affecting the IGT test "writeback-check-output",
> > making the test fail and also, leaking writeback framebuffers, as the
> > writeback job is queued, but it is not signaled. This patch avoids both
> > problems.
> >
> > [v2]:
> >      * Create a new mutex and keep the spinlock across the atomic commit in
> >        order to avoid interrupts that could result in deadlocks.
> >
> > [ Backport to 5.15: context cleanly applied with no semantic changes.
> > Build-tested. ]
> >
> > Signed-off-by: Maíra Canal <mcanal@igalia.com>
> > Reviewed-by: Arthur Grillo <arthurgrillo@riseup.net>
> > Signed-off-by: Maíra Canal <mairacanal@riseup.net>
> > Link: https://patchwork.freedesktop.org/patch/msgid/20230523123207.173976-1-mcanal@igalia.com
> > Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
>
> This patch violates locking rules and it was reversed a while ago.
> Please, check commit 7908632f2927 ("Revert "drm/vkms: Fix race-condition
> between the hrtimer and the atomic commit"").
>
> Best Regards,
> - Maíra

Thanks for pointing that out.

I missed the revert. I now see that commit 7908632f2927 reversed
this due to locking issues. I’ll drop this backport from 5.15
accordingly.

Regards
Pranav Tyagi

Re: [PATCH] drm/vkms: Fix race-condition between the hrtimer and the atomic commit

[Sasha Levin]

[ Sasha's backport helper bot ]

Hi,

Summary of potential issues:
❌ Commit was reverted in mainline

The upstream commit SHA1 provided is correct: a0e6a017ab56936c0405fe914a793b241ed25ee0

WARNING: Author mismatch between patch and upstream commit:
Backport author: Pranav Tyagi<pranav.tyagi03@gmail.com>
Commit author: Maíra Canal<mcanal@igalia.com>

Found revert commits:
7908632f2927 Revert "drm/vkms: Fix race-condition between the hrtimer and the atomic commit"

Note: The patch differs from the upstream commit:
---
1:  a0e6a017ab569 < -:  ------------- drm/vkms: Fix race-condition between the hrtimer and the atomic commit
-:  ------------- > 1:  a2b47f77e740a Linux 6.15.3
---

Results of testing on various branches:

| Branch                    | Patch Apply | Build Test |
|---------------------------|-------------|------------|
| stable/linux-6.15.y       |  Success    |  Success   |
| stable/linux-6.12.y       |  Success    |  Success   |
| stable/linux-6.6.y        |  Success    |  Success   |
| stable/linux-6.1.y        |  Success    |  Success   |
| stable/linux-5.15.y       |  Success    |  Success   |
| stable/linux-5.10.y       |  Success    |  Success   |
| stable/linux-5.4.y        |  Success    |  Success   |