[07/19] lirc_dev: remove kmalloc in lirc_dev_fop_read()
diff mbox

Message ID 149839391031.28811.5094791739782133013.stgit@zeus.hardeman.nu
State New
Headers show

Commit Message

David Härdeman June 25, 2017, 12:31 p.m. UTC
lirc_zilog uses a chunk_size of 2 and ir-lirc-codec uses sizeof(int).

Therefore, using stack memory should be perfectly fine.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/lirc_dev.c |    8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

Comments

Mauro Carvalho Chehab Oct. 4, 2017, 4:56 p.m. UTC | #1
Em Sun, 25 Jun 2017 14:31:50 +0200
David Härdeman <david@hardeman.nu> escreveu:

> lirc_zilog uses a chunk_size of 2 and ir-lirc-codec uses sizeof(int).
> 
> Therefore, using stack memory should be perfectly fine.
> 
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
>  drivers/media/rc/lirc_dev.c |    8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
> index 1773a2934484..92048d945ba7 100644
> --- a/drivers/media/rc/lirc_dev.c
> +++ b/drivers/media/rc/lirc_dev.c
> @@ -376,7 +376,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
>  			  loff_t *ppos)
>  {
>  	struct irctl *ir = file->private_data;
> -	unsigned char *buf;
> +	unsigned char buf[ir->buf->chunk_size];

No. We don't do dynamic buffer allocation on stak at the Kernel,
as this could cause the Linux stack to overflow without notice.

This should also generate alerts on static code analyzers like
sparse.

I'll drop this patch from the series.

Thanks,
Mauro
David Härdeman Oct. 9, 2017, 9:45 a.m. UTC | #2
October 4, 2017 6:57 PM, "Mauro Carvalho Chehab" <mchehab@s-opensource.com> wrote:

> Em Sun, 25 Jun 2017 14:31:50 +0200
> David Härdeman <david@hardeman.nu> escreveu:
> 
>> lirc_zilog uses a chunk_size of 2 and ir-lirc-codec uses sizeof(int).
>> 
>> Therefore, using stack memory should be perfectly fine.
>> 
>> Signed-off-by: David Härdeman <david@hardeman.nu>
>> ---
>> drivers/media/rc/lirc_dev.c | 8 +-------
>> 1 file changed, 1 insertion(+), 7 deletions(-)
>> 
>> diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
>> index 1773a2934484..92048d945ba7 100644
>> --- a/drivers/media/rc/lirc_dev.c
>> +++ b/drivers/media/rc/lirc_dev.c
>> @@ -376,7 +376,7 @@ ssize_t lirc_dev_fop_read(struct file *file,
>> loff_t *ppos)
>> {
>> struct irctl *ir = file->private_data;
>> - unsigned char *buf;
>> + unsigned char buf[ir->buf->chunk_size];
> 
> No. We don't do dynamic buffer allocation on stak at the Kernel,
> as this could cause the Linux stack to overflow without notice.

While the general policy is to avoid large stack allocations (in order to not overflow the 4K stack), I'm not aware of a general ban on *any* stack allocations - that sounds like an overly dogmatic approach. If such a generic ban exists, could you please point me to some kind of discussion/message to that effect?

The commit message clearly explained what kind of stack allocations we're talking about here (i.e. sizeof(int) is the maximum), so the stack allocation is clearly not able to cause stack overflows. And once the last lirc driver is gone, this can be changed to a simple int (which would also be allocated on stack).

Regards,
David

Patch
diff mbox

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 1773a2934484..92048d945ba7 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -376,7 +376,7 @@  ssize_t lirc_dev_fop_read(struct file *file,
 			  loff_t *ppos)
 {
 	struct irctl *ir = file->private_data;
-	unsigned char *buf;
+	unsigned char buf[ir->buf->chunk_size];
 	int ret = 0, written = 0;
 	DECLARE_WAITQUEUE(wait, current);
 
@@ -385,10 +385,6 @@  ssize_t lirc_dev_fop_read(struct file *file,
 
 	dev_dbg(ir->d.dev, LOGHEAD "read called\n", ir->d.name, ir->d.minor);
 
-	buf = kzalloc(ir->buf->chunk_size, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
 	if (mutex_lock_interruptible(&ir->irctl_lock)) {
 		ret = -ERESTARTSYS;
 		goto out_unlocked;
@@ -464,8 +460,6 @@  ssize_t lirc_dev_fop_read(struct file *file,
 	mutex_unlock(&ir->irctl_lock);
 
 out_unlocked:
-	kfree(buf);
-
 	return ret ? ret : written;
 }
 EXPORT_SYMBOL(lirc_dev_fop_read);