* Re: [PATCH] staging: sm750fb: free cmap on framebuffer release
2026-09-22 3:53 [PATCH] staging: sm750fb: free cmap on framebuffer release Lucas Costa
@ 2026-09-22 5:41 ` Greg Kroah-Hartman
2026-09-22 14:25 ` Lucas Costa
2026-09-22 7:22 ` Dan Carpenter
2026-09-22 14:28 ` [PATCH v2] " Lucas Costa
2 siblings, 1 reply; 6+ messages in thread
From: Greg Kroah-Hartman @ 2026-09-22 5:41 UTC (permalink / raw)
To: Lucas Costa
Cc: Sudip Mukherjee, Teddy Wang, linux-fbdev, linux-staging, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] staging: sm750fb: free cmap on framebuffer release
2026-09-22 5:41 ` Greg Kroah-Hartman
@ 2026-09-22 14:25 ` Lucas Costa
0 siblings, 0 replies; 6+ messages in thread
From: Lucas Costa @ 2026-09-22 14:25 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Sudip Mukherjee, Teddy Wang, linux-fbdev, linux-staging, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: sm750fb: free cmap on framebuffer release
2026-09-22 3:53 [PATCH] staging: sm750fb: free cmap on framebuffer release Lucas Costa
2026-09-22 5:41 ` Greg Kroah-Hartman
@ 2026-09-22 7:22 ` Dan Carpenter
2026-09-22 14:28 ` [PATCH v2] " Lucas Costa
2 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2026-09-22 7:22 UTC (permalink / raw)
To: Lucas Costa
Cc: Sudip Mukherjee, Teddy Wang, Greg Kroah-Hartman, linux-fbdev,
linux-staging, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2] staging: sm750fb: free cmap on framebuffer release
2026-09-22 3:53 [PATCH] staging: sm750fb: free cmap on framebuffer release Lucas Costa
2026-09-22 5:41 ` Greg Kroah-Hartman
2026-09-22 7:22 ` Dan Carpenter
@ 2026-09-22 14:28 ` Lucas Costa
2026-09-22 16:06 ` Dan Carpenter
2 siblings, 1 reply; 6+ messages in thread
From: Lucas Costa @ 2026-09-22 14:28 UTC (permalink / raw)
To: Sudip Mukherjee, Teddy Wang, Greg Kroah-Hartman
Cc: Dan Carpenter, linux-fbdev, linux-staging, linux-kernel, 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] staging: sm750fb: free cmap on framebuffer release
2026-09-22 14:28 ` [PATCH v2] " Lucas Costa
@ 2026-09-22 16:06 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2026-09-22 16:06 UTC (permalink / raw)
To: Lucas Costa
Cc: Sudip Mukherjee, Teddy Wang, Greg Kroah-Hartman, linux-fbdev,
linux-staging, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread