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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxx> 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@xxxxxxxxxxx>

Acked-by: Michal Nazarewicz <mina86@xxxxxxxxxx>

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 {



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux