[PATCH v3 0/2] staging: fbtft: cleanup fbtft_framebuffer_alloc()

Fix a potential memory leak and cleanup error handling in
fbtft_framebuffer_alloc().

v3:
- Remove a redundant check before calling kfree

v2:
- Change the earlier patch to also handle the error code returned by
  fb_deferred_io_init() and update Fixes tag to point to the commit that
  introduced the memory allocation (which leads to leak).
- Add second patch to make the error handling order symmetric to
  fbtft_framebuffer_release() and also remove managed allocation for
  txbuf as suggested by Andy and Dan.

Link to v2: https://lore.kernel.org/linux-staging/cover.1751086324.git.abdun.nihaal@gmail.com/T/#md111471ddd69e6ddb0a6b98e565551ffbd791a34
Link to v1: https://lore.kernel.org/all/20250626172412.18355-1-abdun.nihaal@gmail.com/

Abdun Nihaal (2):
  staging: fbtft: fix potential memory leak in fbtft_framebuffer_alloc()
  staging: fbtft: cleanup error handling in fbtft_framebuffer_alloc()

 drivers/staging/fbtft/fbtft-core.c | 38 +++++++++++++++++-------------
 1 file changed, 21 insertions(+), 17 deletions(-)

-- 
2.43.0

After commit 56c134f7f1b5 ("fbdev: Track deferred-I/O pages in pageref
struct"), fb_deferred_io_init() allocates memory for info->pagerefs as
well as return an error code on failure. However the error code is
ignored here and the memory allocated could leak because of not calling
fb_deferred_io_cleanup() on the error path.

Fix them by adding the cleanup function on the error path, and handling
the error code returned by fb_deferred_io_init().

Fixes: 56c134f7f1b5 ("fbdev: Track deferred-I/O pages in pageref struct")
Signed-off-by: Abdun Nihaal <redacted>
---
v2->v3: No change
v1->v2:
- Handle the error code returned by fb_deferred_io_init correctly
- Update Fixes tag to point to the commit that introduced the memory
  allocation which leads to the leak.

 drivers/staging/fbtft/fbtft-core.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index da9c64152a60..8538b6bab6a5 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c

@@ -612,7 +612,8 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 	info->fix.line_length =    width * bpp / 8;
 	info->fix.accel =          FB_ACCEL_NONE;
 	info->fix.smem_len =       vmem_size;
-	fb_deferred_io_init(info);
+	if (fb_deferred_io_init(info))
+		goto release_framebuf;
 
 	info->var.rotate =         pdata->rotate;
 	info->var.xres =           width;

@@ -652,7 +653,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 +670,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,6 +692,8 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 
 	return info;
 
+cleanup_deferred:
+	fb_deferred_io_cleanup(info);
 release_framebuf:
 	framebuffer_release(info);
 

-- 
2.43.0

The error handling in fbtft_framebuffer_alloc() mixes managed allocation
and plain allocation, and performs error handling in an order different
from the order in fbtft_framebuffer_release().

Fix them by moving vmem allocation closer to where it is used, and using
plain kzalloc() for txbuf allocation.

Suggested-by: Andy Shevchenko <redacted>
Suggested-by: Dan Carpenter <redacted>
Signed-off-by: Abdun Nihaal <redacted>
---
v2->v3: 
- Remove the if check before kfree of txbuf.buf, because it is zero
  initialized on allocation, and kfree is NULL aware.

Newly added in v2

 drivers/staging/fbtft/fbtft-core.c | 31 +++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index 8538b6bab6a5..9e7b84071174 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c

@@ -568,18 +568,13 @@ 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;
+		return NULL;
 
 	buf = devm_kzalloc(dev, 128, GFP_KERNEL);
 	if (!buf)
-		goto alloc_fail;
+		return NULL;
 
 	if (display->gamma_num && display->gamma_len) {
 		gamma_curves = devm_kcalloc(dev,

@@ -588,12 +583,17 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 					    sizeof(gamma_curves[0]),
 					    GFP_KERNEL);
 		if (!gamma_curves)
-			goto alloc_fail;
+			return NULL;
 	}
 
 	info = framebuffer_alloc(sizeof(struct fbtft_par), dev);
 	if (!info)
-		goto alloc_fail;
+		return NULL;
+
+	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;

@@ -613,7 +613,7 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 	info->fix.accel =          FB_ACCEL_NONE;
 	info->fix.smem_len =       vmem_size;
 	if (fb_deferred_io_init(info))
-		goto release_framebuf;
+		goto release_screen_buffer;
 
 	info->var.rotate =         pdata->rotate;
 	info->var.xres =           width;

@@ -668,7 +668,7 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 #endif
 
 	if (txbuflen > 0) {
-		txbuf = devm_kzalloc(par->info->device, txbuflen, GFP_KERNEL);
+		txbuf = kzalloc(txbuflen, GFP_KERNEL);
 		if (!txbuf)
 			goto cleanup_deferred;
 		par->txbuf.buf = txbuf;

@@ -694,12 +694,10 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 
 cleanup_deferred:
 	fb_deferred_io_cleanup(info);
+release_screen_buffer:
+	vfree(info->screen_buffer);
 release_framebuf:
 	framebuffer_release(info);
-
-alloc_fail:
-	vfree(vmem);
-
 	return NULL;
 }
 EXPORT_SYMBOL(fbtft_framebuffer_alloc);

@@ -712,6 +710,9 @@ EXPORT_SYMBOL(fbtft_framebuffer_alloc);
  */
 void fbtft_framebuffer_release(struct fb_info *info)
 {
+	struct fbtft_par *par = info->par;
+
+	kfree(par->txbuf.buf);
 	fb_deferred_io_cleanup(info);
 	vfree(info->screen_buffer);
 	framebuffer_release(info);

-- 
2.43.0

On Sun, Jun 29, 2025 at 08:10:09PM +0530, Abdun Nihaal wrote:

Fix a potential memory leak and cleanup error handling in
fbtft_framebuffer_alloc().

Both looks okay to me now.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

On Sun, Jun 29, 2025 at 08:10:09PM +0530, Abdun Nihaal wrote:

Fix a potential memory leak and cleanup error handling in
fbtft_framebuffer_alloc().

Thanks!

Reviewed-by: Dan Carpenter <redacted>

regards,
dan carpenter

On Sun, Jun 29, 2025 at 08:10:11PM +0530, Abdun Nihaal wrote:

The error handling in fbtft_framebuffer_alloc() mixes managed allocation
and plain allocation, and performs error handling in an order different
from the order in fbtft_framebuffer_release().

Fix them by moving vmem allocation closer to where it is used, and using
plain kzalloc() for txbuf allocation.

Suggested-by: Andy Shevchenko <redacted>
Suggested-by: Dan Carpenter <redacted>
Signed-off-by: Abdun Nihaal <redacted>
---
v2->v3: 
- Remove the if check before kfree of txbuf.buf, because it is zero
  initialized on allocation, and kfree is NULL aware.

This patch does not apply to my tree, can you rebase and resend?

thanks,

greg k-h

Hello Greg,

On Mon, Jun 30, 2025 at 07:16:38PM +0200, Greg KH wrote:

This patch does not apply to my tree, can you rebase and resend?

I think you have added both the V1 patch and this current V3 patchset to
your tree, that's why this patch does not apply.

Commit eb2cb7dab60f ("staging: fbtft: fix potential memory leak in fbtft_framebuffer_alloc()") 
on staging-testing is an older version of this patchset, and so it has to be dropped.

Regards,
Nihaal

On Tue, Jul 01, 2025 at 12:47:22AM +0530, Abdun Nihaal wrote:

Hello Greg,

On Mon, Jun 30, 2025 at 07:16:38PM +0200, Greg KH wrote:

quoted

This patch does not apply to my tree, can you rebase and resend?

I think you have added both the V1 patch and this current V3 patchset to
your tree, that's why this patch does not apply.

Commit eb2cb7dab60f ("staging: fbtft: fix potential memory leak in fbtft_framebuffer_alloc()") 
on staging-testing is an older version of this patchset, and so it has to be dropped.

I can't "drop" patches as my tree can not be rebased.  Can you send a
fix-up patch instead, OR a revert?

thanks,

greg k-h

On Tue, Jul 1, 2025 at 8:14 AM Greg KH [off-list ref] wrote:

On Tue, Jul 01, 2025 at 12:47:22AM +0530, Abdun Nihaal wrote:

quoted

On Mon, Jun 30, 2025 at 07:16:38PM +0200, Greg KH wrote:

quoted

This patch does not apply to my tree, can you rebase and resend?

I think you have added both the V1 patch and this current V3 patchset to
your tree, that's why this patch does not apply.

Commit eb2cb7dab60f ("staging: fbtft: fix potential memory leak in fbtft_framebuffer_alloc()")
on staging-testing is an older version of this patchset, and so it has to be dropped.

I can't "drop" patches as my tree can not be rebased.  Can you send a
fix-up patch instead, OR a revert?

I think the cleaner solution will be revert and v3 patches together as
v4. Abdun, can you do that?


-- 
With Best Regards,
Andy Shevchenko

On Tue, Jul 01, 2025 at 10:03:50AM +0300, Andy Shevchenko wrote:

On Tue, Jul 1, 2025 at 8:14 AM Greg KH [off-list ref] wrote:

quoted

I can't "drop" patches as my tree can not be rebased.  Can you send a
fix-up patch instead, OR a revert?

I think the cleaner solution will be revert and v3 patches together as
v4. Abdun, can you do that?

Sure, I'll send a revert in v4.

Regards,
Nihaal

On Tue, Jul 01, 2025 at 10:03:50AM +0300, Andy Shevchenko wrote:

On Tue, Jul 1, 2025 at 8:14 AM Greg KH [off-list ref] wrote:

quoted

On Tue, Jul 01, 2025 at 12:47:22AM +0530, Abdun Nihaal wrote:

quoted

On Mon, Jun 30, 2025 at 07:16:38PM +0200, Greg KH wrote:

quoted

This patch does not apply to my tree, can you rebase and resend?

I think you have added both the V1 patch and this current V3 patchset to
your tree, that's why this patch does not apply.

Commit eb2cb7dab60f ("staging: fbtft: fix potential memory leak in fbtft_framebuffer_alloc()")
on staging-testing is an older version of this patchset, and so it has to be dropped.

I can't "drop" patches as my tree can not be rebased.  Can you send a
fix-up patch instead, OR a revert?

I think the cleaner solution will be revert and v3 patches together as
v4. Abdun, can you do that?

I'm reading my email in the wrong order today.  I thought Abdun came
up with the revert idea on his own instead of you and Greg suggesting
it...

This isn't a case where we revert.  The patch we applied was acceptable
quality and it worked fine.  Just do the additional cleanup on the top.

regards,
dan carpenter