Re: [PATCH] drm/client: fix restore of partially initialized client

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Memory leak of drm_client_buffer and its virtual memory mapping if register_framebuffer() fails.
--

commit 99c42165eff4a5954aaf41ad682bb169178e6b09
Author: shechenglong <shechenglong@xfusion.com>

drm/client: fix restore of partially initialized client

This commit fixes a NULL pointer dereference that occurs when closing a
DRM file descriptor for a partially initialized fbdev client. It skips
the restore process in drm_fbdev_client_restore() if no valid fbdev
framebuffer is found.

> diff --git a/drivers/gpu/drm/clients/drm_fbdev_client.c b/drivers/gpu/drm/clients/drm_fbdev_client.c
> index 91d196a397cf2..1c16bc1084c40 100644
> --- a/drivers/gpu/drm/clients/drm_fbdev_client.c
> +++ b/drivers/gpu/drm/clients/drm_fbdev_client.c
> @@ -42,6 +42,14 @@ static int drm_fbdev_client_restore(struct drm_client_dev *client, bool force)
>  {
>  	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
>  
> +	/*
> +	 * The client is registered before the initial fbdev probe.
> +	 * If probing failed, the client remains registered but there
> +	 * is no valid fbdev framebuffer to restore.
> +	 */
> +	if (!fb_helper->info || !fb_helper->fb)
> +		return 0;
> +

[Severity: High]
This is a pre-existing issue, but since the commit message mentions mirroring
the logic from drm_fbdev_client_unregister(), does the current cleanup path
leak the drm_client_buffer when probing fails?

In drivers/gpu/drm/drm_fb_helper.c:__drm_fb_helper_initial_config_and_unlock(),
drm_fb_helper_single_fb_probe() allocates fb_helper->buffer (which creates the
framebuffer and maps memory). If the subsequent call to register_framebuffer()
fails, it returns without cleaning up the buffer:

drivers/gpu/drm/drm_fb_helper.c:__drm_fb_helper_initial_config_and_unlock() {
    ...
    ret = register_framebuffer(info);
    if (ret < 0)
        return ret;
    ...
}

The caller handles this error by calling drm_fb_helper_fini(), which frees
fb_helper->info via framebuffer_release() and sets fb_helper->info to NULL.
Importantly, framebuffer_release() only frees the struct without invoking
the fb_destroy callback.

Later, when the client is unregistered, drm_fbdev_client_unregister() takes
the else branch because fb_helper->info is NULL:

drivers/gpu/drm/clients/drm_fbdev_client.c:drm_fbdev_client_unregister() {
    if (fb_helper->info) {
        ...
    } else {
        /*
         * Partially initialized client, no framebuffer device yet
         */
        drm_client_release(&fb_helper->client);
    }
}

This skips drm_fb_helper_unregister_info() and the associated fb_destroy()
callback, permanently leaking fb_helper->buffer, its backing GEM object,
and its virtual memory mapping.

This leak can be triggered by resource exhaustion, limit reached (FB_MAX), or
other failures during register_framebuffer(). Could the error handling path
be updated to ensure the buffer is properly destroyed when this occurs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907035147.1339-1-shechenglong@xfusion.com?part=1