[PATCH v3] usb: gadget: ffs: Fix sparse error

[PATCH v3] usb: gadget: ffs: Fix sparse error

[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.

Signed-off-by: Rohith Seelaboyina <rseelaboyina@nvidia.com>
---
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(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 63314ede7ba6..8af3f7906c83 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -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,
-- 
1.9.1

Re: [PATCH v3] usb: gadget: ffs: Fix sparse error

[Michal Nazarewicz]

On Wed, Dec 24 2014, Rohith Seelaboyina wrote:
> 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 <rseelaboyina@nvidia.com>

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.

> ---
>  drivers/usb/gadget/function/f_fs.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 63314ede7ba6..8af3f7906c83 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -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,
> -- 
> 1.9.1
>

>From 8c813b2563cae23c69895833972da1c1ee92d8dd Mon Sep 17 00:00:00 2001
From: Michal Nazarewicz <mina86@mina86.com>
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 <dan.carpenter@oracle.com>
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/function/f_fs.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 63314ed..a00ee97 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -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;
-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

Re: [PATCH v3] usb: gadget: ffs: Fix sparse error

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 852 bytes --]

Hi,

On Wed, Dec 24, 2014 at 03:23:07PM +0100, Michal Nazarewicz wrote:
> On Wed, Dec 24 2014, Rohith Seelaboyina wrote:
> > 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 <rseelaboyina@nvidia.com>
> 
> 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

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH] usb: f_fs: refactor and document __ffs_ep0_read_events better

[Michal Nazarewicz]

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 <dan.carpenter@oracle.com>
Reported-by: Rohith Seelaboyina <rseelaboyina@nvidia.com>
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/function/f_fs.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 63314ed..a00ee97 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -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;
-- 
2.2.0.rc0.207.ga3a616c