Re: [PATCH v2] drm/i915/panic: fix panic structure allocation memory leak

[Jocelyn Falempe <jfalempe@redhat.com>]

On 15/10/2025 11:51, Jani Nikula wrote:
> Separating the panic allocation from framebuffer allocation in commit
> 729c5f7ffa83 ("drm/{i915,xe}/panic: move framebuffer allocation where it
> belongs") failed to deallocate the panic structure anywhere.
> 
> The fix is two-fold. First, free the panic structure in
> intel_user_framebuffer_destroy() in the general case. Second, move the
> panic allocation later to intel_framebuffer_init() to not leak the panic
> structure in error paths (if any, now or later) between
> intel_framebuffer_alloc() and intel_framebuffer_init().
> 

Thanks, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>

> v2: Rebase
> 
> Fixes: 729c5f7ffa83 ("drm/{i915,xe}/panic: move framebuffer allocation where it belongs")
> Cc: Jocelyn Falempe <jfalempe@redhat.com>
> Cc: Maarten Lankhorst <dev@lankhorst.se>
> Reported-by: Michał Grzelak <michal.grzelak@intel.com>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Tested-by: Michał Grzelak <michal.grzelak@intel.com> # v1
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_fb.c | 25 +++++++++++++------------
>   1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> index 9c256a2805e4..3958628c73e9 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> @@ -2114,6 +2114,7 @@ static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
>   
>   	intel_frontbuffer_put(intel_fb->frontbuffer);
>   
> +	kfree(intel_fb->panic);
>   	kfree(intel_fb);
>   }
>   
> @@ -2212,16 +2213,22 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>   	struct intel_display *display = to_intel_display(obj->dev);
>   	struct drm_framebuffer *fb = &intel_fb->base;
>   	u32 max_stride;
> -	int ret = -EINVAL;
> +	int ret;
>   	int i;
>   
> +	intel_fb->panic = intel_panic_alloc();
> +	if (!intel_fb->panic)
> +		return -ENOMEM;
> +
>   	/*
>   	 * intel_frontbuffer_get() must be done before
>   	 * intel_fb_bo_framebuffer_init() to avoid set_tiling vs. addfb race.
>   	 */
>   	intel_fb->frontbuffer = intel_frontbuffer_get(obj);
> -	if (!intel_fb->frontbuffer)
> -		return -ENOMEM;
> +	if (!intel_fb->frontbuffer) {
> +		ret = -ENOMEM;
> +		goto err_free_panic;
> +	}
>   
>   	ret = intel_fb_bo_framebuffer_init(obj, mode_cmd);
>   	if (ret)
> @@ -2320,6 +2327,9 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>   	intel_fb_bo_framebuffer_fini(obj);
>   err_frontbuffer_put:
>   	intel_frontbuffer_put(intel_fb->frontbuffer);
> +err_free_panic:
> +	kfree(intel_fb->panic);
> +
>   	return ret;
>   }
>   
> @@ -2346,20 +2356,11 @@ intel_user_framebuffer_create(struct drm_device *dev,
>   struct intel_framebuffer *intel_framebuffer_alloc(void)
>   {
>   	struct intel_framebuffer *intel_fb;
> -	struct intel_panic *panic;
>   
>   	intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
>   	if (!intel_fb)
>   		return NULL;
>   
> -	panic = intel_panic_alloc();
> -	if (!panic) {
> -		kfree(intel_fb);
> -		return NULL;
> -	}
> -
> -	intel_fb->panic = panic;
> -
>   	return intel_fb;
>   }
>