Re: [PATCH 07/19] lirc_dev: remove kmalloc in lirc_dev_fop_read()

[David Härdeman]

October 4, 2017 6:57 PM, "Mauro Carvalho Chehab"  
wrote:

> Em Sun, 25 Jun 2017 14:31:50 +0200
> David Härdeman  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 
>> ---
>> 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

Re: [PATCH 07/19] lirc_dev: remove kmalloc in lirc_dev_fop_read()

[Mauro Carvalho Chehab]

Em Sun, 25 Jun 2017 14:31:50 +0200
David Härdeman  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 
> ---
>  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

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

[David Härdeman]

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 
---
 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];
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(>irctl_lock)) {
ret = -ERESTARTSYS;
goto out_unlocked;
@@ -464,8 +460,6 @@ ssize_t lirc_dev_fop_read(struct file *file,
mutex_unlock(>irctl_lock);
 
 out_unlocked:
-   kfree(buf);
-
return ret ? ret : written;
 }
 EXPORT_SYMBOL(lirc_dev_fop_read);