[net-next:master 122/152] drivers/ptp/ptp_chardev.c:36 ptp_ioctl() warn: 'sysoff' puts 832 bytes on stack

Hi Richard,

_just_ FYI and let you aware of it, there are new smatch warnings show up in

tree:   git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git master
head:   b77bc2069d1e437d5a1a71bb5cfcf4556ee40015
commit: 215b13dd288c2e1e4461c1530a801f5f83e8cd90 [122/152] ptp: add an ioctl to compare PHC time with system time

+ drivers/ptp/ptp_chardev.c:36 ptp_ioctl() warn: 'sysoff' puts 832 bytes on stack
  drivers/ptp/ptp_chardev.c:144 ptp_read() warn: 'event' puts 960 bytes on stack

vim +36 +/sysoff drivers/ptp/ptp_chardev.c

d94ba80e Richard Cochran 2011-04-22  20  #include <linux/module.h>
d94ba80e Richard Cochran 2011-04-22  21  #include <linux/posix-clock.h>
d94ba80e Richard Cochran 2011-04-22  22  #include <linux/poll.h>
d94ba80e Richard Cochran 2011-04-22  23  #include <linux/sched.h>
d94ba80e Richard Cochran 2011-04-22  24  
d94ba80e Richard Cochran 2011-04-22  25  #include "ptp_private.h"
d94ba80e Richard Cochran 2011-04-22  26  
d94ba80e Richard Cochran 2011-04-22  27  int ptp_open(struct posix_clock *pc, fmode_t fmode)
d94ba80e Richard Cochran 2011-04-22  28  {
d94ba80e Richard Cochran 2011-04-22  29  	return 0;
d94ba80e Richard Cochran 2011-04-22  30  }
d94ba80e Richard Cochran 2011-04-22  31  
d94ba80e Richard Cochran 2011-04-22  32  long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
d94ba80e Richard Cochran 2011-04-22  33  {
d94ba80e Richard Cochran 2011-04-22  34  	struct ptp_clock_caps caps;
d94ba80e Richard Cochran 2011-04-22  35  	struct ptp_clock_request req;
215b13dd Richard Cochran 2012-10-31 @36  	struct ptp_sys_offset sysoff;
d94ba80e Richard Cochran 2011-04-22  37  	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
d94ba80e Richard Cochran 2011-04-22  38  	struct ptp_clock_info *ops = ptp->info;
215b13dd Richard Cochran 2012-10-31  39  	struct ptp_clock_time *pct;
215b13dd Richard Cochran 2012-10-31  40  	struct timespec ts;
d94ba80e Richard Cochran 2011-04-22  41  	int enable, err = 0;
215b13dd Richard Cochran 2012-10-31  42  	unsigned int i;
d94ba80e Richard Cochran 2011-04-22  43  
d94ba80e Richard Cochran 2011-04-22  44  	switch (cmd) {

---
0-DAY kernel build testing backend         Open Source Technology Center
Fengguang Wu, Yuanhan Liu                              Intel Corporation

On Fri, Nov 02, 2012 at 10:06:31AM +0800, Yuanhan Liu wrote:

Hi Richard,

_just_ FYI and let you aware of it, there are new smatch warnings show up in

tree:   git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git master
head:   b77bc2069d1e437d5a1a71bb5cfcf4556ee40015
commit: 215b13dd288c2e1e4461c1530a801f5f83e8cd90 [122/152] ptp: add an ioctl to compare PHC time with system time

+ drivers/ptp/ptp_chardev.c:36 ptp_ioctl() warn: 'sysoff' puts 832 bytes on stack
  drivers/ptp/ptp_chardev.c:144 ptp_read() warn: 'event' puts 960 bytes on stack

I am aware that these methods use large stack buffers, but I thought
it was okay seeing as they are both under the 1k limit.

Thanks,
Richard

From: Richard Cochran <richardcochran@gmail.com>
Date: Fri, 2 Nov 2012 09:59:15 +0100

On Fri, Nov 02, 2012 at 10:06:31AM +0800, Yuanhan Liu wrote:

quoted

Hi Richard,

_just_ FYI and let you aware of it, there are new smatch warnings show up in

tree:   git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git master
head:   b77bc2069d1e437d5a1a71bb5cfcf4556ee40015
commit: 215b13dd288c2e1e4461c1530a801f5f83e8cd90 [122/152] ptp: add an ioctl to compare PHC time with system time

+ drivers/ptp/ptp_chardev.c:36 ptp_ioctl() warn: 'sysoff' puts 832 bytes on stack
  drivers/ptp/ptp_chardev.c:144 ptp_read() warn: 'event' puts 960 bytes on stack

I am aware that these methods use large stack buffers, but I thought
it was okay seeing as they are both under the 1k limit.

I think you should avoid such a local stack variable here.

It's not that big of a deal to use kmalloc or whatever so just
do that and add the necessary kfree cleanups et al.

On Fri, Nov 02, 2012 at 09:39:28PM -0400, David Miller wrote:

quoted

I am aware that these methods use large stack buffers, but I thought
it was okay seeing as they are both under the 1k limit.

I think you should avoid such a local stack variable here.

It's not that big of a deal to use kmalloc or whatever so just
do that and add the necessary kfree cleanups et al.

The usage pattern will be that the user calls these again and
again. For the sysoff it will be at least once every second, and for
the events it could be ASAP in a tight loop.

Isn't it being nicer to the memory allocation code not to repeatedly
request small chunks?

Thanks,
Richard

From: Richard Cochran <richardcochran@gmail.com>
Date: Sat, 3 Nov 2012 05:53:39 +0100

Isn't it being nicer to the memory allocation code not to repeatedly
request small chunks?

Don't use performance as an argument against writing this code
correctly.

This patch removes the large buffer from the stack of the read file
operation and replaces it with a kmalloced buffer.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/ptp/ptp_chardev.c |   22 +++++++++++++++++-----
 1 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 4f8ae80..9d7542e 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c

@@ -21,6 +21,7 @@
 #include <linux/posix-clock.h>
 #include <linux/poll.h>
 #include <linux/sched.h>
+#include <linux/slab.h>
 
 #include "ptp_private.h"
 

@@ -136,20 +137,23 @@ unsigned int ptp_poll(struct posix_clock *pc, struct file *fp, poll_table *wait)
 	return queue_cnt(&ptp->tsevq) ? POLLIN : 0;
 }
 
+#define EXTTS_BUFSIZE (PTP_BUF_TIMESTAMPS * sizeof(struct ptp_extts_event))
+
 ssize_t ptp_read(struct posix_clock *pc,
 		 uint rdflags, char __user *buf, size_t cnt)
 {
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
 	struct timestamp_event_queue *queue = &ptp->tsevq;
-	struct ptp_extts_event event[PTP_BUF_TIMESTAMPS];
+	struct ptp_extts_event *event;
 	unsigned long flags;
 	size_t qcnt, i;
+	int result;
 
 	if (cnt % sizeof(struct ptp_extts_event) != 0)
 		return -EINVAL;
 
-	if (cnt > sizeof(event))
-		cnt = sizeof(event);
+	if (cnt > EXTTS_BUFSIZE)
+		cnt = EXTTS_BUFSIZE;
 
 	cnt = cnt / sizeof(struct ptp_extts_event);
 

@@ -167,6 +171,12 @@ ssize_t ptp_read(struct posix_clock *pc,
 		return -ENODEV;
 	}
 
+	event = kmalloc(EXTTS_BUFSIZE, GFP_KERNEL);
+	if (!event) {
+		mutex_unlock(&ptp->tsevq_mux);
+		return -ENOMEM;
+	}
+
 	spin_lock_irqsave(&queue->lock, flags);
 
 	qcnt = queue_cnt(queue);

@@ -185,8 +195,10 @@ ssize_t ptp_read(struct posix_clock *pc,
 
 	mutex_unlock(&ptp->tsevq_mux);
 
+	result = cnt;
 	if (copy_to_user(buf, event, cnt))
-		return -EFAULT;
+		result = -EFAULT;
 
-	return cnt;
+	kfree(event);
+	return result;
 }

-- 
1.7.2.5

This patch removes the large buffer from the stack of the system
offset ioctl and replaces it with a kmalloced buffer.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/ptp/ptp_chardev.c |   21 ++++++++++++++-------
 1 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 9d7542e..34a0c60 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c

@@ -34,7 +34,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 {
 	struct ptp_clock_caps caps;
 	struct ptp_clock_request req;
-	struct ptp_sys_offset sysoff;
+	struct ptp_sys_offset *sysoff = NULL;
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
 	struct ptp_clock_info *ops = ptp->info;
 	struct ptp_clock_time *pct;

@@ -94,17 +94,22 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		break;
 
 	case PTP_SYS_OFFSET:
-		if (copy_from_user(&sysoff, (void __user *)arg,
-				   sizeof(sysoff))) {
+		sysoff = kmalloc(sizeof(*sysoff), GFP_KERNEL);
+		if (!sysoff) {
+			err = -ENOMEM;
+			break;
+		}
+		if (copy_from_user(sysoff, (void __user *)arg,
+				   sizeof(*sysoff))) {
 			err = -EFAULT;
 			break;
 		}
-		if (sysoff.n_samples > PTP_MAX_SAMPLES) {
+		if (sysoff->n_samples > PTP_MAX_SAMPLES) {
 			err = -EINVAL;
 			break;
 		}
-		pct = &sysoff.ts[0];
-		for (i = 0; i < sysoff.n_samples; i++) {
+		pct = &sysoff->ts[0];
+		for (i = 0; i < sysoff->n_samples; i++) {
 			getnstimeofday(&ts);
 			pct->sec = ts.tv_sec;
 			pct->nsec = ts.tv_nsec;

@@ -117,7 +122,7 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		getnstimeofday(&ts);
 		pct->sec = ts.tv_sec;
 		pct->nsec = ts.tv_nsec;
-		if (copy_to_user((void __user *)arg, &sysoff, sizeof(sysoff)))
+		if (copy_to_user((void __user *)arg, sysoff, sizeof(*sysoff)))
 			err = -EFAULT;
 		break;
 

@@ -125,6 +130,8 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		err = -ENOTTY;
 		break;
 	}
+
+	kfree(sysoff);
 	return err;
 }
 

-- 
1.7.2.5

From: Richard Cochran <richardcochran@gmail.com>
Date: Mon, 26 Nov 2012 12:44:34 +0100

This patch removes the large buffer from the stack of the read file
operation and replaces it with a kmalloced buffer.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>

Applied.

From: Richard Cochran <richardcochran@gmail.com>
Date: Mon, 26 Nov 2012 12:44:35 +0100

This patch removes the large buffer from the stack of the system
offset ioctl and replaces it with a kmalloced buffer.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>

Applied.