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

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 <[email protected]>
Date: Fri, 2 Nov 2012 09:59:15 +0100

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 <[email protected]>
Date: Sat, 3 Nov 2012 05:53:39 +0100

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.

---
 drivers/ptp/ptp_chardev.c |   22 +++++++++++++++++-----
 1 files changed, 17 insertions(+), 5 deletions(-)

index 4f8ae80..9d7542e 100644
@@ -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

From: Richard Cochran <[email protected]>
Date: Mon, 26 Nov 2012 12:44:34 +0100

Applied.

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

---
 drivers/ptp/ptp_chardev.c |   21 ++++++++++++++-------
 1 files changed, 14 insertions(+), 7 deletions(-)

index 9d7542e..34a0c60 100644
@@ -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 <[email protected]>
Date: Mon, 26 Nov 2012 12:44:35 +0100

Applied.