diff mbox series

[2/4] gpiolib: cdev: allocate linereq using kvzalloc()

Message ID 20231220015106.16732-3-warthog618@gmail.com
State New
Headers
Series gpiolib: cdev: guard tidying |

Commit Message

Kent Gibson Dec. 20, 2023, 1:51 a.m. UTC 
  The size of struct linereq may exceed a page, so allocate space for
it using kvzalloc() instead of kzalloc().

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib-cdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

andy@kernel.org Dec. 20, 2023, 2:30 p.m. UTC | #1 
On Wed, Dec 20, 2023 at 09:51:04AM +0800, Kent Gibson wrote:
> The size of struct linereq may exceed a page, so allocate space for
> it using kvzalloc() instead of kzalloc().

It might be this needs a bit of elaboration. The kmalloc() tries to allocate
a contiguous (in physical address space) chunk of memory and with fragmented
memory it might be not possible. So the above issue might (rarely) happen.
In most cases the call to kmalloc() will succeed.
Kent Gibson Dec. 20, 2023, 2:53 p.m. UTC | #2 
On Wed, Dec 20, 2023 at 04:30:24PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 20, 2023 at 09:51:04AM +0800, Kent Gibson wrote:
> > The size of struct linereq may exceed a page, so allocate space for
> > it using kvzalloc() instead of kzalloc().
>
> It might be this needs a bit of elaboration. The kmalloc() tries to allocate
> a contiguous (in physical address space) chunk of memory and with fragmented
> memory it might be not possible. So the above issue might (rarely) happen.
> In most cases the call to kmalloc() will succeed.
>

For sure, the kzalloc() generally works - or we wouldn't've gotten this
far as tests with MAX_LINES would've been failing.
We are targetting a very niche failure mode here.

The size allocated can only be determined at runtime, may be more or
less than a page, and we don't care whether the physical memory allocated
is contiguous.
As such kvzalloc() is the appropriate allocator.

Are you suggesting repeating the relevant sections of the
kmalloc/vmalloc() documentation or Memory Allocation Guide as part of the
checkin comment?

Cheers,
Kent.
andy@kernel.org Dec. 20, 2023, 2:58 p.m. UTC | #3 
On Wed, Dec 20, 2023 at 10:53:07PM +0800, Kent Gibson wrote:
> On Wed, Dec 20, 2023 at 04:30:24PM +0200, Andy Shevchenko wrote:
> > On Wed, Dec 20, 2023 at 09:51:04AM +0800, Kent Gibson wrote:
> > > The size of struct linereq may exceed a page, so allocate space for
> > > it using kvzalloc() instead of kzalloc().
> >
> > It might be this needs a bit of elaboration. The kmalloc() tries to allocate
> > a contiguous (in physical address space) chunk of memory and with fragmented
> > memory it might be not possible. So the above issue might (rarely) happen.
> > In most cases the call to kmalloc() will succeed.
> 
> For sure, the kzalloc() generally works - or we wouldn't've gotten this
> far as tests with MAX_LINES would've been failing.
> We are targetting a very niche failure mode here.
> 
> The size allocated can only be determined at runtime, may be more or
> less than a page, and we don't care whether the physical memory allocated
> is contiguous.
> As such kvzalloc() is the appropriate allocator.
> 
> Are you suggesting repeating the relevant sections of the
> kmalloc/vmalloc() documentation or Memory Allocation Guide as part of the
> checkin comment?

I suggesting to make clear in the commit message that:
- there is no bug per se with the code logic (i.o.w.
  there is no issue to have allocations bigger than one page)
- this is very rarely case when it might be a problem

You can also put a reference to the documentation, if you wish.
This should be harmless and adding not more than a line into
the commit message (or even as a Link: tag to the HTML variant of it).
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 44d864f63130..6fec793f5513 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1723,7 +1723,7 @@  static void linereq_free(struct linereq *lr)
 	kfifo_free(&lr->events);
 	kfree(lr->label);
 	gpio_device_put(lr->gdev);
-	kfree(lr);
+	kvfree(lr);
 }
 
 static int linereq_release(struct inode *inode, struct file *file)
@@ -1788,7 +1788,7 @@  static int linereq_create(struct gpio_device *gdev, void __user *ip)
 	if (ret)
 		return ret;
 
-	lr = kzalloc(struct_size(lr, lines, ulr.num_lines), GFP_KERNEL);
+	lr = kvzalloc(struct_size(lr, lines, ulr.num_lines), GFP_KERNEL);
 	if (!lr)
 		return -ENOMEM;
 	lr->num_lines = ulr.num_lines;