Skip to contentSkip to conversation

usb: gadget: ffs: Fix sparse error

PATCH In review Rohith Seelaboyina 3 participants 4 messages Dec 24, 2014 – Jan 8, 2015 USB

lore
Share & tools
lore

Rohith Seelaboyina

author
#0 lore raw

PATCH v3 1/1 usb: gadget: ffs: Fix sparse error

This patch fixes the sparse error in functionfs driver.

drivers/usb/gadget/function/f_fs.c:400:44: error: bad constant experssion.

Dynamic memory allocation through kcalloc is more safer than declaring variable array size, Fix this error by using kcalloc for memory allocation, Check if memory allocation is successful and return -ENOMEM on failure.

Signed-off-by Rohith Seelaboyina <[email protected]>

---
Changes in v3:
- Follow kernel coding style to seperate variable declaration and assignment.
Changes in v2:
- Use kcalloc to allocate memory instead of using kmalloc and memset.

 drivers/usb/gadget/function/f_fs.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/usb/gadget/function/f_fs.c +9 -4
index 63314ede7ba6..8af3f7906c83 100644
@@ -397,10 +397,13 @@ static ssize_t __ffs_ep0_read_events(struct ffs_data *ffs, char __user *buf,
 	 * We are holding ffs->ev.waitq.lock and ffs->mutex and we need
 	 * to release them.
 	 */
-	struct usb_functionfs_event events[n];
 	unsigned i = 0;
+	int ret;
+	struct usb_functionfs_event *events;
 
-	memset(events, 0, sizeof events);
+	events = kcalloc(n, sizeof(*events), GFP_KERNEL);
+	if (!events)
+		return -ENOMEM;
 
 	do {
 		events[i].type = ffs->ev.types[i];

@@ -421,8 +424,10 @@ static ssize_t __ffs_ep0_read_events(struct ffs_data *ffs, char __user *buf,
 	spin_unlock_irq(&ffs->ev.waitq.lock);
 	mutex_unlock(&ffs->mutex);
 
-	return unlikely(__copy_to_user(buf, events, sizeof events))
-		? -EFAULT : sizeof events;
+	ret = unlikely(__copy_to_user(buf, events, n * sizeof(*events)))
+		? -EFAULT : n * sizeof(*events);
+	kfree(events);
+	return ret;
 }
 
 static ssize_t ffs_ep0_read(struct file *file, char __user *buf,
Signature · 1 line
1.9.1
#1 lore raw
Quoted 6 lines · Rohith Seelaboyina

This patch fixes the sparse error in functionfs driver.

drivers/usb/gadget/function/f_fs.c:400:44: error: bad constant experssion.

Quoted 6 lines

Dynamic memory allocation through kcalloc is more safer than declaring variable array size, Fix this error by using kcalloc for memory allocation, Check if memory allocation is successful and return -ENOMEM on failure.

Signed-off-by Rohith Seelaboyina <[email protected]>

This has already been addressed in a way that does not require dynamic
allocation: <https://lkml.org/lkml/2014/9/10/513>. I'd rather patch attached at the end would be applied.

Quoted 42 lines
---
 drivers/usb/gadget/function/f_fs.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/usb/gadget/function/f_fs.c +9 -4
index 63314ede7ba6..8af3f7906c83 100644
@@ -397,10 +397,13 @@ static ssize_t __ffs_ep0_read_events(struct ffs_data *ffs, char __user *buf,
 	 * We are holding ffs->ev.waitq.lock and ffs->mutex and we need
 	 * to release them.
 	 */
-	struct usb_functionfs_event events[n];
 	unsigned i = 0;
+	int ret;
+	struct usb_functionfs_event *events;
 
-	memset(events, 0, sizeof events);
+	events = kcalloc(n, sizeof(*events), GFP_KERNEL);
+	if (!events)
+		return -ENOMEM;
 
 	do {
 		events[i].type = ffs->ev.types[i];

@@ -421,8 +424,10 @@ static ssize_t __ffs_ep0_read_events(struct ffs_data *ffs, char __user *buf,
 	spin_unlock_irq(&ffs->ev.waitq.lock);
 	mutex_unlock(&ffs->mutex);
 
-	return unlikely(__copy_to_user(buf, events, sizeof events))
-		? -EFAULT : sizeof events;
+	ret = unlikely(__copy_to_user(buf, events, n * sizeof(*events)))
+		? -EFAULT : n * sizeof(*events);
+	kfree(events);
+	return ret;
 }
 
 static ssize_t ffs_ep0_read(struct file *file, char __user *buf,
Signature · 4 lines
1.9.1


From 8c813b2563cae23c69895833972da1c1ee92d8dd Mon Sep 17 00:00:00 2001

From: Michal Nazarewicz <[email protected]>
Date: Wed, 10 Sep 2014 17:50:24 +0200
Subject: [PATCHv2] usb: f_fs: refactor and document __ffs_ep0_read_events better

Instead of using variable length array, use a static length equal to the size of the ffs->ev.types array. This gets rid of a sparse warning:

	drivers/usb/gadget/function/f_fs.c:401:44: warning:
	Variable length array is used.

and makes it more explicit that the array has a very tight upper size limit. Also add some more documentation about the ev.types array and how its size is limited and affects the rest of the code.

Reported-by Dan Carpenter <[email protected]>
Signed-off-by Michal Nazarewicz <[email protected]>
---
 drivers/usb/gadget/function/f_fs.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)
drivers/usb/gadget/function/f_fs.c +17 -11
index 63314ed..a00ee97 100644
@@ -390,17 +390,20 @@ done_spin:
 	return ret;
 }
 
+/* Called with ffs->ev.waitq.lock and ffs->mutex held, both released on exit. */
 static ssize_t __ffs_ep0_read_events(struct ffs_data *ffs, char __user *buf,
 				     size_t n)
 {
 	/*
-	 * We are holding ffs->ev.waitq.lock and ffs->mutex and we need
-	 * to release them.
+	 * n cannot be bigger than ffs->ev.count, which cannot be bigger than
+	 * size of ffs->ev.types array (which is four) so that's how much space
+	 * we reserve.
 	 */
-	struct usb_functionfs_event events[n];
+	struct usb_functionfs_event events[ARRAY_SIZE(ffs->ev.types)];
+	const size_t size = n * sizeof *events;
 	unsigned i = 0;
 
-	memset(events, 0, sizeof events);
+	memset(events, 0, size);
 
 	do {
 		events[i].type = ffs->ev.types[i];

@@ -410,19 +413,15 @@ static ssize_t __ffs_ep0_read_events(struct ffs_data *ffs, char __user *buf,
 		}
 	} while (++i < n);
 
-	if (n < ffs->ev.count) {
-		ffs->ev.count -= n;
+	ffs->ev.count -= n;
+	if (ffs->ev.count)
 		memmove(ffs->ev.types, ffs->ev.types + n,
 			ffs->ev.count * sizeof *ffs->ev.types);
-	} else {
-		ffs->ev.count = 0;
-	}
 
 	spin_unlock_irq(&ffs->ev.waitq.lock);
 	mutex_unlock(&ffs->mutex);
 
-	return unlikely(__copy_to_user(buf, events, sizeof events))
-		? -EFAULT : sizeof events;
+	return unlikely(__copy_to_user(buf, events, size)) ? -EFAULT : size;
 }
 
 static ssize_t ffs_ep0_read(struct file *file, char __user *buf,

@@ -2377,6 +2376,13 @@ static void __ffs_event_add(struct ffs_data *ffs,
 	if (ffs->setup_state == FFS_SETUP_PENDING)
 		ffs->setup_state = FFS_SETUP_CANCELLED;
 
+	/*
+	 * Logic of this function guarantees that there are at most four pending
+	 * evens on ffs->ev.types queue.  This is important because the queue
+	 * has space for four elements only and __ffs_ep0_read_events function
+	 * depends on that limit as well.  If more event types are added, those
+	 * limits have to be revisited or guaranteed to still hold.
+	 */
 	switch (type) {
 	case FUNCTIONFS_RESUME:
 		rem_type2 = FUNCTIONFS_SUSPEND;
Signature · 4 lines
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<[email protected]>--<xmpp:[email protected]>--ooO--(_)--Ooo--
#2 lore raw

Hi,

Quoted 12 lines · Michal Nazarewicz
Quoted 11 lines · Rohith Seelaboyina

This patch fixes the sparse error in functionfs driver.

drivers/usb/gadget/function/f_fs.c:400:44: error: bad constant experssion.

Dynamic memory allocation through kcalloc is more safer than declaring variable array size, Fix this error by using kcalloc for memory allocation, Check if memory allocation is successful and return -ENOMEM on failure.

Quoted 5 lines
Quoted 1 line
Signed-off-by Rohith Seelaboyina <[email protected]>

This has already been addressed in a way that does not require dynamic
allocation: <https://lkml.org/lkml/2014/9/10/513>. I'd rather patch attached at the end would be applied.

can you send below as a proper patch ?

thanks

Signature · 1 line
balbi
#3 lore raw

PATCH 1/1 usb: f_fs: refactor and document __ffs_ep0_read_events better

Instead of using variable length array, use a static length equal to the size of the ffs->ev.types array. This gets rid of a sparse warning:

	drivers/usb/gadget/function/f_fs.c:401:44: warning:
	Variable length array is used.

and makes it more explicit that the array has a very tight upper size limit. Also add some more documentation about the ev.types array and how its size is limited and affects the rest of the code.

Reported-by Dan Carpenter <[email protected]>
Reported-by Rohith Seelaboyina <[email protected]>
Signed-off-by Michal Nazarewicz <[email protected]>
---
 drivers/usb/gadget/function/f_fs.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)
drivers/usb/gadget/function/f_fs.c +17 -11
index 63314ed..a00ee97 100644
@@ -390,17 +390,20 @@ done_spin:
 	return ret;
 }
 
+/* Called with ffs->ev.waitq.lock and ffs->mutex held, both released on exit. */
 static ssize_t __ffs_ep0_read_events(struct ffs_data *ffs, char __user *buf,
 				     size_t n)
 {
 	/*
-	 * We are holding ffs->ev.waitq.lock and ffs->mutex and we need
-	 * to release them.
+	 * n cannot be bigger than ffs->ev.count, which cannot be bigger than
+	 * size of ffs->ev.types array (which is four) so that's how much space
+	 * we reserve.
 	 */
-	struct usb_functionfs_event events[n];
+	struct usb_functionfs_event events[ARRAY_SIZE(ffs->ev.types)];
+	const size_t size = n * sizeof *events;
 	unsigned i = 0;
 
-	memset(events, 0, sizeof events);
+	memset(events, 0, size);
 
 	do {
 		events[i].type = ffs->ev.types[i];

@@ -410,19 +413,15 @@ static ssize_t __ffs_ep0_read_events(struct ffs_data *ffs, char __user *buf,
 		}
 	} while (++i < n);
 
-	if (n < ffs->ev.count) {
-		ffs->ev.count -= n;
+	ffs->ev.count -= n;
+	if (ffs->ev.count)
 		memmove(ffs->ev.types, ffs->ev.types + n,
 			ffs->ev.count * sizeof *ffs->ev.types);
-	} else {
-		ffs->ev.count = 0;
-	}
 
 	spin_unlock_irq(&ffs->ev.waitq.lock);
 	mutex_unlock(&ffs->mutex);
 
-	return unlikely(__copy_to_user(buf, events, sizeof events))
-		? -EFAULT : sizeof events;
+	return unlikely(__copy_to_user(buf, events, size)) ? -EFAULT : size;
 }
 
 static ssize_t ffs_ep0_read(struct file *file, char __user *buf,

@@ -2377,6 +2376,13 @@ static void __ffs_event_add(struct ffs_data *ffs,
 	if (ffs->setup_state == FFS_SETUP_PENDING)
 		ffs->setup_state = FFS_SETUP_CANCELLED;
 
+	/*
+	 * Logic of this function guarantees that there are at most four pending
+	 * evens on ffs->ev.types queue.  This is important because the queue
+	 * has space for four elements only and __ffs_ep0_read_events function
+	 * depends on that limit as well.  If more event types are added, those
+	 * limits have to be revisited or guaranteed to still hold.
+	 */
 	switch (type) {
 	case FUNCTIONFS_RESUME:
 		rem_type2 = FUNCTIONFS_SUSPEND;
Signature · 1 line
2.2.0.rc0.207.ga3a616c