[PATCH] USB: Gadget: Composite: Added error handling codes to prevent a memory leak case when the configuration's bind function failed

[PATCH] USB: Gadget: Composite: Added error handling codes to prevent a memory leak case when the configuration's bind function failed

[Yongsul, Oh]

From: yongsul96.oh@samsung.com <yongsul96.oh@samsung.com>

 In some usb gadget driver, for example usb gadget serial driver(serial.c),
multifunction composite driver(multi,c), nokia composite gadget driver(nokia.c),
HID composite driver(hid.c), CDC composite driver(cdc2.c).., the configuration's
bind function by called the 'usb_add_config()' has multiple bind config functions
for each functionality, for example cdc2 configuration bind function -'cdc_do_config()'
has two functionality bind config functions -'ecm_bind_config()' & 'acm_bind_config()'
in CDC composite driver.
 In each functionality bind config function, new instance for each functionality is
allocated & initialized by 'kzalloc()' ,and finally the new instance is added by
'usb_add_function()'. After 'usb_add_function' state, already created the instance
is only handled by its configuration & freed from functionality unbind function.
 So, If an error occurred during the second functionality bind config state, for example
an error occurred at 'acm_bind_config()' after succeeding 'ecm_bind_function()',
The created instance by 'acm_bind_config()' cannot be freed.
And it makes memory leak situation.

This patch fixes this issue.

Signed-off-by: yongsul96.oh@samsung.com <yongsul96.oh@samsung.com>
---
 drivers/usb/gadget/composite.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index baaebf2..4cb1801 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -737,6 +737,19 @@ int usb_add_config(struct usb_composite_dev *cdev,
 
 	status = bind(config);
 	if (status < 0) {
+		while (!list_empty(&config->functions)) {
+			struct usb_function		*f;
+
+			f = list_first_entry(&config->functions,
+					struct usb_function, list);
+			list_del(&f->list);
+			if (f->unbind) {
+				DBG(cdev, "unbind function '%s'/%p\n",
+					f->name, f);
+				f->unbind(config, f);
+				/* may free memory for "f" */
+			}
+		}
 		list_del(&config->list);
 		config->cdev = NULL;
 	} else {
-- 
1.7.1

Re: [PATCH] USB: Gadget: Composite: Added error handling codes to prevent a memory leak case when the configuration's bind function failed

[Michal Nazarewicz]

On Thu, 15 Mar 2012 02:23:55 +0100, <Yongsul@vger.kernel.org> wrote:
>  In some usb gadget driver, for example usb gadget serial driver(serial.c),
> multifunction composite driver(multi,c), nokia composite gadget driver(nokia.c),
> HID composite driver(hid.c), CDC composite driver(cdc2.c).., the configuration's
> bind function by called the 'usb_add_config()' has multiple bind config functions
> for each functionality, for example cdc2 configuration bind function -'cdc_do_config()'
> has two functionality bind config functions -'ecm_bind_config()' & 'acm_bind_config()'
> in CDC composite driver.
>  In each functionality bind config function, new instance for each functionality is
> allocated & initialized by 'kzalloc()' ,and finally the new instance is added by
> 'usb_add_function()'. After 'usb_add_function' state, already created the instance
> is only handled by its configuration & freed from functionality unbind function.
>  So, If an error occurred during the second functionality bind config state, for example
> an error occurred at 'acm_bind_config()' after succeeding 'ecm_bind_function()',
> The created instance by 'acm_bind_config()' cannot be freed.
> And it makes memory leak situation.

It may be my poor English skills, but this is very hard to read.

Also, should this be handled in usb_gadget_probe_driver() instead?  Ie. on error
recovery path, should usb_gadget_probe_driver() call usb_gadget_remove_driver()?

In particular, if I'm not mistaken, for (some?) gadgets with two configurations
(like g_multi), if the second configuration fails, the first one will not get
removed, so the memory will still leak, no?

> Signed-off-by: yongsul96.oh@samsung.com <yongsul96.oh@samsung.com>
> ---
>  drivers/usb/gadget/composite.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index baaebf2..4cb1801 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -737,6 +737,19 @@ int usb_add_config(struct usb_composite_dev *cdev,
> 	status = bind(config);
>  	if (status < 0) {
> +		while (!list_empty(&config->functions)) {
> +			struct usb_function		*f;
> +
> +			f = list_first_entry(&config->functions,
> +					struct usb_function, list);
> +			list_del(&f->list);
> +			if (f->unbind) {
> +				DBG(cdev, "unbind function '%s'/%p\n",
> +					f->name, f);
> +				f->unbind(config, f);
> +				/* may free memory for "f" */
> +			}
> +		}
>  		list_del(&config->list);
>  		config->cdev = NULL;
>  	} else {


-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

Re: [PATCH] USB: Gadget: Composite: Added error handling codes to prevent a memory leak case when the configuration's bind function failed

[Sergei Shtylyov]

Hello.

On 15-03-2012 5:23, Yongsul@vger.kernel.org wrote:

> From: yongsul96.oh@samsung.com<yongsul96.oh@samsung.com>

>   In some usb gadget driver, for example usb gadget serial driver(serial.c),
> multifunction composite driver(multi,c), nokia composite gadget driver(nokia.c),
> HID composite driver(hid.c), CDC composite driver(cdc2.c).., the configuration's
> bind function by called the 'usb_add_config()' has multiple bind config functions
> for each functionality, for example cdc2 configuration bind function -'cdc_do_config()'
> has two functionality bind config functions -'ecm_bind_config()'&  'acm_bind_config()'
> in CDC composite driver.
>   In each functionality bind config function, new instance for each functionality is
> allocated&  initialized by 'kzalloc()' ,and finally the new instance is added by
> 'usb_add_function()'. After 'usb_add_function' state, already created the instance
> is only handled by its configuration&  freed from functionality unbind function.
>   So, If an error occurred during the second functionality bind config state, for example
> an error occurred at 'acm_bind_config()' after succeeding 'ecm_bind_function()',
> The created instance by 'acm_bind_config()' cannot be freed.
> And it makes memory leak situation.

> This patch fixes this issue.

> Signed-off-by: yongsul96.oh@samsung.com <yongsul96.oh@samsung.com>

    Real name is required on the left, not another copy of email.

WBR, Sergei

[PATCH] USB: Gadget: Composite: Added error handling codes to prevent a memory leak case when the configuration's bind function failed

[Yongsul Oh]

 In some usb gadget driver, (for example usb gadget serial driver(serial.c),
multifunction composite driver(multi,c), nokia composite gadget driver(nokia.c),
HID composite driver(hid.c), CDC composite driver(cdc2.c)..) the configuration's
bind function by called the 'usb_add_config()' has multiple bind config functions
for each functionality. (for example cdc2 configuration bind function -'cdc_do_config()'
has two functionality bind config functions -'ecm_bind_config()' & 'acm_bind_config()'
in CDC composite driver.)

 In each functionality bind config function, new instance for each functionality is
allocated & initialized by 'kzalloc()' and finally the new instance is added by
'usb_add_function()'. After 'usb_add_function' state, already created the instance
is only handled by it's configuration & freed from functionality unbind function.

 So, If an error occurred during the second functionality bind config state,
(for example an error occurred at 'acm_bind_config()' after succeeding of
'ecm_bind_function()') the created instance by 'acm_bind_config()'
cannot be freed. And it makes memory leak situation.

This patch fixes this issue

Signed-off-by: Yongsul Oh <yongsul96.oh@samsung.com>
---
 drivers/usb/gadget/composite.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index baaebf2..4cb1801 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -737,6 +737,19 @@ int usb_add_config(struct usb_composite_dev *cdev,
 
 	status = bind(config);
 	if (status < 0) {
+		while (!list_empty(&config->functions)) {
+			struct usb_function		*f;
+
+			f = list_first_entry(&config->functions,
+					struct usb_function, list);
+			list_del(&f->list);
+			if (f->unbind) {
+				DBG(cdev, "unbind function '%s'/%p\n",
+					f->name, f);
+				f->unbind(config, f);
+				/* may free memory for "f" */
+			}
+		}
 		list_del(&config->list);
 		config->cdev = NULL;
 	} else {
-- 
1.7.1

Re: [PATCH] USB: Gadget: Composite: Added error handling codes to prevent a memory leak case when the configuration's bind function failed

[Michal Nazarewicz]

On Fri, 16 Mar 2012 07:27:15 +0100, Yongsul Oh <yongsul96.oh@samsung.com> wrote:

>  In some usb gadget driver, (for example usb gadget serial driver(serial.c),

s/usb/USB/g
s/gadget driver/composite gadget drivers/

I also don't think the list of examples is necessary here.  Maybe just a list file
names? My personal opinion, others may disagree though.

Also, comma should go after the closing paren.

> multifunction composite driver(multi,c), nokia composite gadget driver(nokia.c),
> HID composite driver(hid.c), CDC composite driver(cdc2.c)..) the configuration's
> bind function by called the 'usb_add_config()' has multiple bind config functions

s/by called the/called by the/
s/has/calls/

> for each functionality. (for example cdc2 configuration bind function -'cdc_do_config()'

s/functions for each functionality/functions, one for each USB functionality/

> has two functionality bind config functions -'ecm_bind_config()' & 'acm_bind_config()'
> in CDC composite driver.)
>
>  In each functionality bind config function, new instance for each functionality is

s/for each functionality//

> allocated & initialized by 'kzalloc()' and finally the new instance is added by

s/& initialized by 'kzalloc()' and finally the new instance/and/

> 'usb_add_function()'. After 'usb_add_function' state, already created the instance
> is only handled by it's configuration & freed from functionality unbind function.

I'm not sure what you meant by this last sentence.

>  So, If an error occurred during the second functionality bind config state,

s/If/if/
s/state//

> (for example an error occurred at 'acm_bind_config()' after succeeding of
> 'ecm_bind_function()') the created instance by 'acm_bind_config()'

s/the created instance by 'acm_bind_config()'/the instance created by acm_bind_config()/

> cannot be freed. And it makes memory leak situation.

s/freed. And it makes memory leak situation./freed creating a memory leak./

> This patch fixes this issue

s/issue/issue./

Also, drop apostrophes around function names.  It looks weird.

> Signed-off-by: Yongsul Oh <yongsul96.oh@samsung.com>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

Also, like written previously, I think there might be memory leak in other error
recovery paths as well.  If you could take a look whether I'm right, that would be
awesome.

> ---
>  drivers/usb/gadget/composite.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index baaebf2..4cb1801 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -737,6 +737,19 @@ int usb_add_config(struct usb_composite_dev *cdev,
> 	status = bind(config);
>  	if (status < 0) {
> +		while (!list_empty(&config->functions)) {
> +			struct usb_function		*f;
> +
> +			f = list_first_entry(&config->functions,
> +					struct usb_function, list);
> +			list_del(&f->list);
> +			if (f->unbind) {
> +				DBG(cdev, "unbind function '%s'/%p\n",
> +					f->name, f);
> +				f->unbind(config, f);
> +				/* may free memory for "f" */
> +			}
> +		}
>  		list_del(&config->list);
>  		config->cdev = NULL;
>  	} else {


-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

Re: [PATCH] USB: Gadget: Composite: Added error handling codes to prevent a memory leak case when the configuration's bind function failed

[Yongsul Oh]

Dear Michal Nazarewicz

Thank you very much for your kindness.
And i also agree with your comments.
I wil try to change my commit-msg & re-send it as soon as possible.

And about your last commets,

 > Also, like written previously, I think there might be memory leak in 
other error
 > recovery paths as well.  If you could take a look whether I'm right, 
that would be
 > awesome.

I will try to find out if the memory leaks in other error recovery paths 
or not,
  but i think that might be another case and need different patch-set.
(If i find out that, i will try to make another patch-set and send it 
all again)

Best regards.
Yongsul Oh

On 2012년 03월 16일 20:14, Michal Nazarewicz wrote:
> On Fri, 16 Mar 2012 07:27:15 +0100, Yongsul Oh 
> <yongsul96.oh@samsung.com> wrote:
>
>>  In some usb gadget driver, (for example usb gadget serial 
>> driver(serial.c),
>
> s/usb/USB/g
> s/gadget driver/composite gadget drivers/
>
> I also don't think the list of examples is necessary here.  Maybe just 
> a list file
> names? My personal opinion, others may disagree though.
>
> Also, comma should go after the closing paren.
>
>> multifunction composite driver(multi,c), nokia composite gadget 
>> driver(nokia.c),
>> HID composite driver(hid.c), CDC composite driver(cdc2.c)..) the 
>> configuration's
>> bind function by called the 'usb_add_config()' has multiple bind 
>> config functions
>
> s/by called the/called by the/
> s/has/calls/
>
>> for each functionality. (for example cdc2 configuration bind function 
>> -'cdc_do_config()'
>
> s/functions for each functionality/functions, one for each USB 
> functionality/
>
>> has two functionality bind config functions -'ecm_bind_config()' & 
>> 'acm_bind_config()'
>> in CDC composite driver.)
>>
>>  In each functionality bind config function, new instance for each 
>> functionality is
>
> s/for each functionality//
>
>> allocated & initialized by 'kzalloc()' and finally the new instance 
>> is added by
>
> s/& initialized by 'kzalloc()' and finally the new instance/and/
>
>> 'usb_add_function()'. After 'usb_add_function' state, already created 
>> the instance
>> is only handled by it's configuration & freed from functionality 
>> unbind function.
>
> I'm not sure what you meant by this last sentence.
>
>>  So, If an error occurred during the second functionality bind config 
>> state,
>
> s/If/if/
> s/state//
>
>> (for example an error occurred at 'acm_bind_config()' after 
>> succeeding of
>> 'ecm_bind_function()') the created instance by 'acm_bind_config()'
>
> s/the created instance by 'acm_bind_config()'/the instance created by 
> acm_bind_config()/
>
>> cannot be freed. And it makes memory leak situation.
>
> s/freed. And it makes memory leak situation./freed creating a memory 
> leak./
>
>> This patch fixes this issue
>
> s/issue/issue./
>
> Also, drop apostrophes around function names.  It looks weird.
>
>> Signed-off-by: Yongsul Oh <yongsul96.oh@samsung.com>
>
> Acked-by: Michal Nazarewicz <mina86@mina86.com>
>
> Also, like written previously, I think there might be memory leak in 
> other error
> recovery paths as well.  If you could take a look whether I'm right, 
> that would be
> awesome.
>
>> ---
>>  drivers/usb/gadget/composite.c |   13 +++++++++++++
>>  1 files changed, 13 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/composite.c 
>> b/drivers/usb/gadget/composite.c
>> index baaebf2..4cb1801 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -737,6 +737,19 @@ int usb_add_config(struct usb_composite_dev *cdev,
>>     status = bind(config);
>>      if (status < 0) {
>> +        while (!list_empty(&config->functions)) {
>> +            struct usb_function        *f;
>> +
>> +            f = list_first_entry(&config->functions,
>> +                    struct usb_function, list);
>> +            list_del(&f->list);
>> +            if (f->unbind) {
>> +                DBG(cdev, "unbind function '%s'/%p\n",
>> +                    f->name, f);
>> +                f->unbind(config, f);
>> +                /* may free memory for "f" */
>> +            }
>> +        }
>>          list_del(&config->list);
>>          config->cdev = NULL;
>>      } else {
>
>

[PATCH] USB: Gadget: Composite: Added error handling codes to prevent a memory leak case when the configuration's bind function failed

[Yongsul Oh]

 In some USB composite gadget drivers, the configuration's bind function
  called by the usb_add_config() calls multiple bind config functions.
  (for example cdc2 configuration bind function in the cdc_do_config()
  of the cdc2.c has two functionality bind config functions.
  - the ecm_bind_config() & the acm_bind_config())

 In each functionality bind config function, new instance is allocated and
  finally added by the usb_add_function().

 So if an error occurred during the second functionality bind config
  (for example an error occurred at the acm_bind_config() after succeeding of
  the ecm_bind_function()), the instance created by the acm_bind_config()
  cannot be freed creating a memory leak.

 This patch fixes this issue.

Signed-off-by: Yongsul Oh <yongsul96.oh@samsung.com>
---
 drivers/usb/gadget/composite.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index baaebf2..4cb1801 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -737,6 +737,19 @@ int usb_add_config(struct usb_composite_dev *cdev,
 
 	status = bind(config);
 	if (status < 0) {
+		while (!list_empty(&config->functions)) {
+			struct usb_function		*f;
+
+			f = list_first_entry(&config->functions,
+					struct usb_function, list);
+			list_del(&f->list);
+			if (f->unbind) {
+				DBG(cdev, "unbind function '%s'/%p\n",
+					f->name, f);
+				f->unbind(config, f);
+				/* may free memory for "f" */
+			}
+		}
 		list_del(&config->list);
 		config->cdev = NULL;
 	} else {
-- 
1.7.1

Re: [PATCH] USB: Gadget: Composite: Added error handling codes to prevent a memory leak case when the configuration's bind function failed

[Michal Nazarewicz]

On Tue, 20 Mar 2012 02:38:38 +0100, Yongsul Oh <yongsul96.oh@samsung.com> wrote:

>  In some USB composite gadget drivers, the configuration's bind function
>   called by the usb_add_config() calls multiple bind config functions.
>   (for example cdc2 configuration bind function in the cdc_do_config()
>   of the cdc2.c has two functionality bind config functions.
>   - the ecm_bind_config() & the acm_bind_config())
>
>  In each functionality bind config function, new instance is allocated and
>   finally added by the usb_add_function().
>
>  So if an error occurred during the second functionality bind config
>   (for example an error occurred at the acm_bind_config() after succeeding of
>   the ecm_bind_function()), the instance created by the acm_bind_config()
>   cannot be freed creating a memory leak.
>
>  This patch fixes this issue.
>
> Signed-off-by: Yongsul Oh <yongsul96.oh@samsung.com>

Like before:

Acked-by: Michal Nazarewicz <mina86@mina86.com>

> ---
>  drivers/usb/gadget/composite.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index baaebf2..4cb1801 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -737,6 +737,19 @@ int usb_add_config(struct usb_composite_dev *cdev,
> 	status = bind(config);
>  	if (status < 0) {
> +		while (!list_empty(&config->functions)) {
> +			struct usb_function		*f;
> +
> +			f = list_first_entry(&config->functions,
> +					struct usb_function, list);
> +			list_del(&f->list);
> +			if (f->unbind) {
> +				DBG(cdev, "unbind function '%s'/%p\n",
> +					f->name, f);
> +				f->unbind(config, f);
> +				/* may free memory for "f" */
> +			}
> +		}
>  		list_del(&config->list);
>  		config->cdev = NULL;
>  	} else {


-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--