[PATCH] staging: sm750fb: free cmap on framebuffer release

[PATCH] staging: sm750fb: free cmap on framebuffer release

[Lucas Costa]

The driver never calls fb_dealloc_cmap() after it has been allocated,
so when the buffer is torn down we leak.

This happens on every unbind, and when an error is detected in
sm750fb_framebuffer_alloc().

fb_dealloc_cmap() gets called before framebuffer_release() in both places.

Fixes: 81dee67e215b ("staging: sm750fb: add sm750 to staging")

Signed-off-by: Lucas Costa <2000.costalucas@gmail.com>
---
 drivers/staging/sm750fb/sm750.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index 8b93bfeb217b..92412534239c 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -910,6 +910,7 @@ static void sm750fb_framebuffer_release(struct sm750_dev *sm750_dev)
 	while (sm750_dev->fb_count) {
 		fb_info = sm750_dev->fbinfo[sm750_dev->fb_count - 1];
 		unregister_framebuffer(fb_info);
+		fb_dealloc_cmap(&fb_info->cmap);
 		framebuffer_release(fb_info);
 		sm750_dev->fb_count--;
 	}
@@ -943,6 +944,7 @@ static int sm750fb_framebuffer_alloc(struct sm750_dev *sm750_dev, int fbidx)
 	return 0;
 
 release_fb:
+	fb_dealloc_cmap(&fb_info->cmap);
 	framebuffer_release(fb_info);
 	return err;
 }
-- 
2.43.0

Re: [PATCH] staging: sm750fb: free cmap on framebuffer release

[Greg Kroah-Hartman]

On Tue, Sep 22, 2026 at 12:53:02AM -0300, Lucas Costa wrote:
> The driver never calls fb_dealloc_cmap() after it has been allocated,
> so when the buffer is torn down we leak.
> 
> This happens on every unbind, and when an error is detected in
> sm750fb_framebuffer_alloc().
> 
> fb_dealloc_cmap() gets called before framebuffer_release() in both places.
> 
> Fixes: 81dee67e215b ("staging: sm750fb: add sm750 to staging")
> 
> Signed-off-by: Lucas Costa <2000.costalucas@gmail.com>
> ---
>  drivers/staging/sm750fb/sm750.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
> index 8b93bfeb217b..92412534239c 100644
> --- a/drivers/staging/sm750fb/sm750.c
> +++ b/drivers/staging/sm750fb/sm750.c
> @@ -910,6 +910,7 @@ static void sm750fb_framebuffer_release(struct sm750_dev *sm750_dev)
>  	while (sm750_dev->fb_count) {
>  		fb_info = sm750_dev->fbinfo[sm750_dev->fb_count - 1];
>  		unregister_framebuffer(fb_info);
> +		fb_dealloc_cmap(&fb_info->cmap);
>  		framebuffer_release(fb_info);
>  		sm750_dev->fb_count--;
>  	}
> @@ -943,6 +944,7 @@ static int sm750fb_framebuffer_alloc(struct sm750_dev *sm750_dev, int fbidx)
>  	return 0;
>  
>  release_fb:
> +	fb_dealloc_cmap(&fb_info->cmap);
>  	framebuffer_release(fb_info);
>  	return err;
>  }
> -- 
> 2.43.0
> 
> 

Nice, how was this found and tested?

thanks,

greg k-h

Re: [PATCH] staging: sm750fb: free cmap on framebuffer release

[Dan Carpenter]

On Tue, Sep 22, 2026 at 12:53:02AM -0300, Lucas Costa wrote:
> The driver never calls fb_dealloc_cmap() after it has been allocated,
> so when the buffer is torn down we leak.
> 
> This happens on every unbind, and when an error is detected in
> sm750fb_framebuffer_alloc().
> 
> fb_dealloc_cmap() gets called before framebuffer_release() in both places.
> 
> Fixes: 81dee67e215b ("staging: sm750fb: add sm750 to staging")
> 
> Signed-off-by: Lucas Costa <2000.costalucas@gmail.com>
> ---
>  drivers/staging/sm750fb/sm750.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
> index 8b93bfeb217b..92412534239c 100644
> --- a/drivers/staging/sm750fb/sm750.c
> +++ b/drivers/staging/sm750fb/sm750.c
> @@ -910,6 +910,7 @@ static void sm750fb_framebuffer_release(struct sm750_dev *sm750_dev)
>  	while (sm750_dev->fb_count) {
>  		fb_info = sm750_dev->fbinfo[sm750_dev->fb_count - 1];
>  		unregister_framebuffer(fb_info);
> +		fb_dealloc_cmap(&fb_info->cmap);
>  		framebuffer_release(fb_info);
>  		sm750_dev->fb_count--;
>  	}
> @@ -943,6 +944,7 @@ static int sm750fb_framebuffer_alloc(struct sm750_dev *sm750_dev, int fbidx)
>  	return 0;
>  
>  release_fb:
> +	fb_dealloc_cmap(&fb_info->cmap);
>  	framebuffer_release(fb_info);
>  	return err;

This works, but I don't love One Err label style cleanup.  The label
name is no longer accurate.

The cmap is allocated by:

	err = lynxfb_set_fbinfo(fb_info, fbidx);

So ideally there would be a cleanup function which is called something
like:

	lynxfb_unset_fbinfo(fb_info);

Then we would only call it if lynxfb_set_fbinfo() succeeds instead of
if it succeeds or fails.

unset_fb:
	lynxfb_unset_fbinfo(fb_info);
release_fb:
	framebuffer_release(fb_info);

	return err;

See my blog for more details.
staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

regards,
dan carpenter

Re: [PATCH] staging: sm750fb: free cmap on framebuffer release

[Lucas Costa]

Hi, Greg! Found it by code inspection only, compile-tested it. I don't
have the hardware available.


El mar, 22 sept 2026 a las 2:41, Greg Kroah-Hartman
(<gregkh@linuxfoundation.org>) escribió:
>
> On Tue, Sep 22, 2026 at 12:53:02AM -0300, Lucas Costa wrote:
> > The driver never calls fb_dealloc_cmap() after it has been allocated,
> > so when the buffer is torn down we leak.
> >
> > This happens on every unbind, and when an error is detected in
> > sm750fb_framebuffer_alloc().
> >
> > fb_dealloc_cmap() gets called before framebuffer_release() in both places.
> >
> > Fixes: 81dee67e215b ("staging: sm750fb: add sm750 to staging")
> >
> > Signed-off-by: Lucas Costa <2000.costalucas@gmail.com>
> > ---
> >  drivers/staging/sm750fb/sm750.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
> > index 8b93bfeb217b..92412534239c 100644
> > --- a/drivers/staging/sm750fb/sm750.c
> > +++ b/drivers/staging/sm750fb/sm750.c
> > @@ -910,6 +910,7 @@ static void sm750fb_framebuffer_release(struct sm750_dev *sm750_dev)
> >       while (sm750_dev->fb_count) {
> >               fb_info = sm750_dev->fbinfo[sm750_dev->fb_count - 1];
> >               unregister_framebuffer(fb_info);
> > +             fb_dealloc_cmap(&fb_info->cmap);
> >               framebuffer_release(fb_info);
> >               sm750_dev->fb_count--;
> >       }
> > @@ -943,6 +944,7 @@ static int sm750fb_framebuffer_alloc(struct sm750_dev *sm750_dev, int fbidx)
> >       return 0;
> >
> >  release_fb:
> > +     fb_dealloc_cmap(&fb_info->cmap);
> >       framebuffer_release(fb_info);
> >       return err;
> >  }
> > --
> > 2.43.0
> >
> >
>
> Nice, how was this found and tested?
>
> thanks,
>
> greg k-h

[PATCH v2] staging: sm750fb: free cmap on framebuffer release

[Lucas Costa]

The cmap is allocated by lynxfb_set_fbinfo() but never freed, so it is
leaked when the framebuffer is torn down.  This happens on every unbind,
and on the register_framebuffer() error path in
sm750fb_framebuffer_alloc().

Add a lynxfb_unset_fbinfo() cleanup counterpart to lynxfb_set_fbinfo()
which deallocates the cmap, and call it on teardown.  In the alloc error
path it is only called once lynxfb_set_fbinfo() has succeeded, so the
cmap is only freed when it was actually allocated.

Fixes: 81dee67e215b ("staging: sm750fb: add sm750 to staging")
Suggested-by: Dan Carpenter <error27@gmail.com>
Signed-off-by: Lucas Costa <2000.costalucas@gmail.com>
---
v2:
 - Move the cmap deallocation into a lynxfb_unset_fbinfo() cleanup
   counterpart to lynxfb_set_fbinfo(), instead of open-coding
   fb_dealloc_cmap() at each call site.
 - In the alloc error path, only unwind the cmap once
   lynxfb_set_fbinfo() has succeeded (Dan Carpenter).

 drivers/staging/sm750fb/sm750.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index 8b93bfeb217b..68b6d4fe79ca 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -825,6 +825,11 @@ static int lynxfb_set_fbinfo(struct fb_info *info, int index)
 	return ret;
 }
 
+static void lynxfb_unset_fbinfo(struct fb_info *info)
+{
+	fb_dealloc_cmap(&info->cmap);
+}
+
 /*	chip specific g_option configuration routine */
 static void sm750fb_setup(struct sm750_dev *sm750_dev, char *src)
 {
@@ -910,6 +915,7 @@ static void sm750fb_framebuffer_release(struct sm750_dev *sm750_dev)
 	while (sm750_dev->fb_count) {
 		fb_info = sm750_dev->fbinfo[sm750_dev->fb_count - 1];
 		unregister_framebuffer(fb_info);
+		lynxfb_unset_fbinfo(fb_info);
 		framebuffer_release(fb_info);
 		sm750_dev->fb_count--;
 	}
@@ -936,12 +942,14 @@ static int sm750fb_framebuffer_alloc(struct sm750_dev *sm750_dev, int fbidx)
 
 	err = register_framebuffer(fb_info);
 	if (err < 0)
-		goto release_fb;
+		goto unset_fb;
 
 	sm750_dev->fb_count++;
 
 	return 0;
 
+unset_fb:
+	lynxfb_unset_fbinfo(fb_info);
 release_fb:
 	framebuffer_release(fb_info);
 	return err;
-- 
2.43.0

Re: [PATCH v2] staging: sm750fb: free cmap on framebuffer release

[Dan Carpenter]

On Tue, Sep 22, 2026 at 11:28:51AM -0300, Lucas Costa wrote:
> The cmap is allocated by lynxfb_set_fbinfo() but never freed, so it is
> leaked when the framebuffer is torn down.  This happens on every unbind,
> and on the register_framebuffer() error path in
> sm750fb_framebuffer_alloc().
> 
> Add a lynxfb_unset_fbinfo() cleanup counterpart to lynxfb_set_fbinfo()
> which deallocates the cmap, and call it on teardown.  In the alloc error
> path it is only called once lynxfb_set_fbinfo() has succeeded, so the
> cmap is only freed when it was actually allocated.
> 
> Fixes: 81dee67e215b ("staging: sm750fb: add sm750 to staging")
> Suggested-by: Dan Carpenter <error27@gmail.com>
> Signed-off-by: Lucas Costa <2000.costalucas@gmail.com>
> ---
> v2:
>  - Move the cmap deallocation into a lynxfb_unset_fbinfo() cleanup
>    counterpart to lynxfb_set_fbinfo(), instead of open-coding
>    fb_dealloc_cmap() at each call site.
>  - In the alloc error path, only unwind the cmap once
>    lynxfb_set_fbinfo() has succeeded (Dan Carpenter).

Thanks!

Reviewed-by: Dan Carpenter <error27@gmail.com>

regards,
dan carpenter