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

[Javier Martinez Canillas <javierm@redhat.com>]

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.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat