diff mbox series

[1/2] drm: Don't unref the same fb many times by mistake due to deadlock handling

Message ID 20231211081625.25704-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [1/2] drm: Don't unref the same fb many times by mistake due to deadlock handling | expand

Commit Message

Ville Syrjälä Dec. 11, 2023, 8:16 a.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

If we get a deadlock after the fb lookup in drm_mode_page_flip_ioctl()
we proceed to unref the fb and then retry the whole thing from the top.
But we forget to reset the fb pointer back to NULL, and so if we then
get another error during the retry, before the fb lookup, we proceed
the unref the same fb again without having gotten another reference.
The end result is that the fb will (eventually) end up being freed
while it's still in use.

Reset fb to NULL once we've unreffed it to avoid doing it again
until we've done another fb lookup.

This turned out to be pretty easy to hit on a DG2 when doing async
flips (and CONFIG_DEBUG_WW_MUTEX_SLOWPATH=y). The first symptom I
saw that drm_closefb() simply got stuck in a busy loop while walking
the framebuffer list. Fortunately I was able to convince it to oops
instead, and from there it was easier to track down the culprit.

Cc: stable@vger.kernel.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_plane.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Javier Martinez Canillas Dec. 20, 2023, 12:04 p.m. UTC | #1
Ville Syrjala <ville.syrjala@linux.intel.com> writes:

Hello Ville,

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> If we get a deadlock after the fb lookup in drm_mode_page_flip_ioctl()
> we proceed to unref the fb and then retry the whole thing from the top.
> But we forget to reset the fb pointer back to NULL, and so if we then
> get another error during the retry, before the fb lookup, we proceed
> the unref the same fb again without having gotten another reference.
> The end result is that the fb will (eventually) end up being freed
> while it's still in use.
>
> Reset fb to NULL once we've unreffed it to avoid doing it again
> until we've done another fb lookup.
>
> This turned out to be pretty easy to hit on a DG2 when doing async
> flips (and CONFIG_DEBUG_WW_MUTEX_SLOWPATH=y). The first symptom I
> saw that drm_closefb() simply got stuck in a busy loop while walking
> the framebuffer list. Fortunately I was able to convince it to oops
> instead, and from there it was easier to track down the culprit.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---

Acked-by: Javier Martinez Canillas <javierm@redhat.com>

>  drivers/gpu/drm/drm_plane.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 9e8e4c60983d..672c655c7a8e 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1503,6 +1503,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  out:
>  	if (fb)
>  		drm_framebuffer_put(fb);
> +	fb = NULL;
>  	if (plane->old_fb)
>  		drm_framebuffer_put(plane->old_fb);
>  	plane->old_fb = NULL;

Interesting that it was done correctly for old_fb.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 9e8e4c60983d..672c655c7a8e 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1503,6 +1503,7 @@  int drm_mode_page_flip_ioctl(struct drm_device *dev,
 out:
 	if (fb)
 		drm_framebuffer_put(fb);
+	fb = NULL;
 	if (plane->old_fb)
 		drm_framebuffer_put(plane->old_fb);
 	plane->old_fb = NULL;