In the error paths after fb_info structure is successfully allocated,
the memory allocated in fb_deferred_io_init() for info->pagerefs is not
freed. Fix that by adding the cleanup function on the error path.

Fixes: c296d5f9957c ("staging: fbtft: core support")
Signed-off-by: Abdun Nihaal <abdun.nihaal@gmail.com>
---
This patch is compile tested only. Not tested on real hardware.
Bug was found using our prototype static analysis tool.

 drivers/staging/fbtft/fbtft-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index da9c64152a60..39bced400065 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -692,6 +692,7 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 	return info;
 
 release_framebuf:
+	fb_deferred_io_cleanup(info);
 	framebuffer_release(info);
 
 alloc_fail:
-- 
2.43.0

On Thu, Jun 26, 2025 at 10:54:10PM +0530, Abdun Nihaal wrote:
> In the error paths after fb_info structure is successfully allocated,
> the memory allocated in fb_deferred_io_init() for info->pagerefs is not
> freed. Fix that by adding the cleanup function on the error path.
> 
> Fixes: c296d5f9957c ("staging: fbtft: core support")
> Signed-off-by: Abdun Nihaal <abdun.nihaal@gmail.com>
> ---

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter

On Thu, Jun 26, 2025 at 10:54:10PM +0530, Abdun Nihaal wrote:
> In the error paths after fb_info structure is successfully allocated,
> the memory allocated in fb_deferred_io_init() for info->pagerefs is not
> freed. Fix that by adding the cleanup function on the error path.

Thanks for the report and the fix! My comments below.

...

>  release_framebuf:
> +	fb_deferred_io_cleanup(info);
>  	framebuffer_release(info);

While the fix sounds good, there are still problems in the driver in this area:

1) managed resources allocation is mixed up with plain allocations
(as you discovery hints);

2) the order in fbtft_framebuffer_release() is asymmetrical to what
we have in fbtft_framebuffer_alloc().

I would recommend to study this code a bit more and provide the following
patches as a result:

1) fixing the order in fbtft_framebuffer_release();

2) moving vmem allocation closer to when it's needed, i.e. just after
successful allocation of the info; at the same time move txbuf allocation
from managed to unmanaged (drop devm, add respective kfree() calls where
it's required);

3) this patch.

All three should have the respective Fixes tags and hence may be backported.


-- 
With Best Regards,
Andy Shevchenko

On Thu, Jun 26, 2025 at 08:50:43PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 26, 2025 at 10:54:10PM +0530, Abdun Nihaal wrote:
> > In the error paths after fb_info structure is successfully allocated,
> > the memory allocated in fb_deferred_io_init() for info->pagerefs is not
> > freed. Fix that by adding the cleanup function on the error path.
> 
> Thanks for the report and the fix! My comments below.
> 
> ...
> 
> >  release_framebuf:
> > +	fb_deferred_io_cleanup(info);
> >  	framebuffer_release(info);
> 
> While the fix sounds good, there are still problems in the driver in this area:
> 
> 1) managed resources allocation is mixed up with plain allocations
> (as you discovery hints);
> 
> 2) the order in fbtft_framebuffer_release() is asymmetrical to what
> we have in fbtft_framebuffer_alloc().
> 
> I would recommend to study this code a bit more and provide the following
> patches as a result:
> 
> 1) fixing the order in fbtft_framebuffer_release();
> 
> 2) moving vmem allocation closer to when it's needed, i.e. just after
> successful allocation of the info; at the same time move txbuf allocation
> from managed to unmanaged (drop devm, add respective kfree() calls where
> it's required);
> 


Symetrical in this sense means that the cleanup in
fbtft_framebuffer_release() and in fbtft_framebuffer_alloc() are
similar:

	fb_deferred_io_cleanup();
	vfree();
 	framebuffer_release();

I feel like number 1 and 2 are sort of opposite approaches to making the
order symmetrical.  #1 is changing fbtft_framebuffer_release() and #2 is
changing fbtft_framebuffer_alloc().  #2 is the less awkward approach.

> 3) this patch.
> 
> All three should have the respective Fixes tags and hence may be backported.

Changing the order isn't a bug fix so it wouldn't get a Fixes tag.
I agree with Andy that the code isn't beautiful.  But I think it's
easier to just fix the bug, and do the cleanup later as an optional
patch 2/2.  I would also have been fine with a larger patch that does
the cleanup and the bug fix in one patch but I think other people
won't like that.

Diff below.  Except, oops, this doesn't compile.  Change the other
goto alloc_fail places to "return NULL;"  I guess that means you
get authorship credit if you fix that.

So if you want you could resend your patch and you could send these
changes I've suggested as a patch 2/2 and then I think everyone will
be happy.

regards,
dan carpenter

diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index da9c64152a60..abfd7b1157df 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -568,11 +568,6 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 		height = display->height;
 	}
 
-	vmem_size = display->width * display->height * bpp / 8;
-	vmem = vzalloc(vmem_size);
-	if (!vmem)
-		goto alloc_fail;
-
 	fbdefio = devm_kzalloc(dev, sizeof(struct fb_deferred_io), GFP_KERNEL);
 	if (!fbdefio)
 		goto alloc_fail;
@@ -595,6 +590,11 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 	if (!info)
 		goto alloc_fail;
 
+	vmem_size = display->width * display->height * bpp / 8;
+	vmem = vzalloc(vmem_size);
+	if (!vmem)
+		goto release_framebuf;
+
 	info->screen_buffer = vmem;
 	info->fbops = &fbtft_ops;
 	info->fbdefio = fbdefio;
@@ -652,7 +652,7 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 	if (par->gamma.curves && gamma) {
 		if (fbtft_gamma_parse_str(par, par->gamma.curves, gamma,
 					  strlen(gamma)))
-			goto release_framebuf;
+			goto cleanup_deferred;
 	}
 
 	/* Transmit buffer */
@@ -669,7 +669,7 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 	if (txbuflen > 0) {
 		txbuf = devm_kzalloc(par->info->device, txbuflen, GFP_KERNEL);
 		if (!txbuf)
-			goto release_framebuf;
+			goto cleanup_deferred;
 		par->txbuf.buf = txbuf;
 		par->txbuf.len = txbuflen;
 	}
@@ -691,12 +691,12 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 
 	return info;
 
+cleanup_deferred:
+	fb_deferred_io_cleanup(info);
+	vfree(info->screen_buffer);
 release_framebuf:
 	framebuffer_release(info);
 
-alloc_fail:
-	vfree(vmem);
-
 	return NULL;
 }
 EXPORT_SYMBOL(fbtft_framebuffer_alloc);

Thu, Jun 26, 2025 at 11:11:39PM +0300, Dan Carpenter kirjoitti:
> On Thu, Jun 26, 2025 at 08:50:43PM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 26, 2025 at 10:54:10PM +0530, Abdun Nihaal wrote:

...

> > >  release_framebuf:
> > > +	fb_deferred_io_cleanup(info);
> > >  	framebuffer_release(info);
> > 
> > While the fix sounds good, there are still problems in the driver in this area:
> > 
> > 1) managed resources allocation is mixed up with plain allocations
> > (as you discovery hints);
> > 
> > 2) the order in fbtft_framebuffer_release() is asymmetrical to what
> > we have in fbtft_framebuffer_alloc().
> > 
> > I would recommend to study this code a bit more and provide the following
> > patches as a result:
> > 
> > 1) fixing the order in fbtft_framebuffer_release();
> > 
> > 2) moving vmem allocation closer to when it's needed, i.e. just after
> > successful allocation of the info; at the same time move txbuf allocation
> > from managed to unmanaged (drop devm, add respective kfree() calls where
> > it's required);
> 
> Symetrical in this sense means that the cleanup in
> fbtft_framebuffer_release() and in fbtft_framebuffer_alloc() are
> similar:
> 
> 	fb_deferred_io_cleanup();
> 	vfree();
>  	framebuffer_release();
> 
> I feel like number 1 and 2 are sort of opposite approaches to making the
> order symmetrical.  #1 is changing fbtft_framebuffer_release() and #2 is
> changing fbtft_framebuffer_alloc().  #2 is the less awkward approach.
> 
> > 3) this patch.
> > 
> > All three should have the respective Fixes tags and hence may be backported.
> 
> Changing the order isn't a bug fix so it wouldn't get a Fixes tag.
> I agree with Andy that the code isn't beautiful.  But I think it's
> easier to just fix the bug, and do the cleanup later as an optional
> patch 2/2.  I would also have been fine with a larger patch that does
> the cleanup and the bug fix in one patch but I think other people
> won't like that.

Ah, you have a point. Yes, the moving vmem allocation will solve the ordering
issue.

-- 
With Best Regards,
Andy Shevchenko

On Fri, Jun 27, 2025 at 12:59:30AM +0300, Andy Shevchenko wrote:
> Thu, Jun 26, 2025 at 11:11:39PM +0300, Dan Carpenter kirjoitti:
> > On Thu, Jun 26, 2025 at 08:50:43PM +0300, Andy Shevchenko wrote:
> > > On Thu, Jun 26, 2025 at 10:54:10PM +0530, Abdun Nihaal wrote:

...

> Ah, you have a point. Yes, the moving vmem allocation will solve the ordering
> issue.

...with moving from devm for the txbuf. Otherwise we would still have a
problematic ordering with it.

-- 
With Best Regards,
Andy Shevchenko

Thanks Andy and Dan for your detailed comments. I'll send a V2 with an
another clean up patch that fixes the order and removes devm for txbuf.

Regards,
Nihaal