drivers/usb/gadget: add missing kfree calls
PATCH 1/1 drivers/usb/gadget: add missing kfree calls
It seems that there are several memory leaks
due to missing kfree() calls. Also fixed
some coding style issues.
--- drivers/usb/gadget/rndis.c | 59 ++++++++++++++++++++++++++----------------- 1 files changed, 36 insertions(+), 23 deletions(-)
drivers/usb/gadget/rndis.c +36 -23
@@ -31,8 +31,8 @@#include <linux/slab.h> #include <linux/seq_file.h> #include <linux/netdevice.h> +#include <linux/io.h> -#include <asm/io.h> #include <asm/byteorder.h> #include <asm/system.h> #include <asm/unaligned.h>
@@ -71,8 +71,7 @@ static rndis_resp_t *rndis_add_response(int configNr, u32 length);/* supported OIDs */ -static const u32 oid_supported_list[] = -{ +static const u32 oid_supported_list[] = {/* the general stuff */ OID_GEN_SUPPORTED_LIST ,OID_GEN_HARDWARE_STATUS ,
@@ -173,10 +172,12 @@ static int gen_ndis_query_resp(int configNr, u32 OID, u8 *buf,struct rtnl_link_stats64 temp;const struct rtnl_link_stats64 *stats; -if (!r)return -ENOMEM ; +if (!r) +return -ENOMEM ; resp = (rndis_query_cmplt_type *)r->buf; -if (!resp)return -ENOMEM ; +if (!resp) +return -ENOMEM ;if (buf_len && rndis_debug >1 ) {pr_debug ("query OID %08x value, len %d:\n" ,OID , buf_len);
@@ -425,7 +426,7 @@ static int gen_ndis_query_resp(int configNr, u32 OID, u8 *buf,if (rndis_per_dev_params[configNr].dev) { length =ETH_ALEN ;memcpy (outbuf, - rndis_per_dev_params [configNr].host_mac, + rndis_per_dev_params[configNr].host_mac, length); retval =0 ; }
@@ -597,6 +598,7 @@ static int rndis_init_response(int configNr, rndis_init_msg_type *buf) resp->AFListSize =cpu_to_le32 (0 ); params->resp_avail (params->v); +kfree (r);return 0 ; }
@@ -662,9 +664,8 @@ static int rndis_set_response(int configNr, rndis_set_msg_type *buf)pr_debug ("%s: Offset: %d\n" , __func__, BufOffset);pr_debug ("%s: InfoBuffer: " , __func__); -for (i =0 ; i < BufLength; i++) { +for (i =0 ; i < BufLength; i++)pr_debug ("%02x " , *(((u8 *) buf) + i +8 + BufOffset)); - }pr_debug ("\n" );#endif
@@ -700,6 +701,7 @@ static int rndis_reset_response(int configNr, rndis_reset_msg_type *buf) resp->AddressingReset =cpu_to_le32 (1 ); params->resp_avail (params->v); +kfree (r);return 0 ; }
@@ -724,6 +726,7 @@ static int rndis_keepalive_response(int configNr, resp->Status =cpu_to_le32 (RNDIS_STATUS_SUCCESS ); params->resp_avail (params->v); +kfree (r);return 0 ; }
@@ -753,6 +756,7 @@ static int rndis_indicate_status_msg(int configNr, u32 status) resp->StatusBufferOffset =cpu_to_le32 (0 ); params->resp_avail (params->v); +kfree (r);return 0 ; }
@@ -875,13 +879,13 @@ int rndis_msg_parser(u8 configNr, u8 *buf)" %02x %02x %02x %02x" "\n" , i, - buf[i], buf [i+1 ], + buf[i], buf[i+1 ], buf[i+2 ], buf[i+3 ], - buf[i+4 ], buf [i+5 ], + buf[i+4 ], buf[i+5 ], buf[i+6 ], buf[i+7 ], - buf[i+8 ], buf [i+9 ], + buf[i+8 ], buf[i+9 ], buf[i+10 ], buf[i+11 ], - buf[i+12 ], buf [i+13 ], + buf[i+12 ], buf[i+13 ], buf[i+14], buf[i+15]); } }
@@ -916,7 +920,8 @@ void rndis_deregister(int configNr) {pr_debug ("%s:\n" , __func__); -if (configNr >=RNDIS_MAX_CONFIGS )return ; +if (configNr >=RNDIS_MAX_CONFIGS ) +return ; rndis_per_dev_params[configNr].used =0 ; }
@@ -925,7 +930,8 @@ int rndis_set_param_dev(u8 configNr, struct net_device *dev, u16 *cdc_filter)pr_debug ("%s:\n" , __func__);if (!dev)return -EINVAL ; -if (configNr >=RNDIS_MAX_CONFIGS )return -1 ; +if (configNr >=RNDIS_MAX_CONFIGS ) +return -1 ; rndis_per_dev_params[configNr].dev = dev; rndis_per_dev_params[configNr].filter = cdc_filter;
@@ -936,8 +942,10 @@ int rndis_set_param_dev(u8 configNr, struct net_device *dev, u16 *cdc_filter)int rndis_set_param_vendor (u8 configNr,u32 vendorID,const char *vendorDescr) {pr_debug ("%s:\n" , __func__); -if (!vendorDescr)return -1 ; -if (configNr >=RNDIS_MAX_CONFIGS )return -1 ; +if (!vendorDescr) +return -1 ; +if (configNr >=RNDIS_MAX_CONFIGS ) +return -1 ; rndis_per_dev_params[configNr].vendorID = vendorID; rndis_per_dev_params[configNr].vendorDescr = vendorDescr;
@@ -948,7 +956,8 @@ int rndis_set_param_vendor(u8 configNr, u32 vendorID, const char *vendorDescr)int rndis_set_param_medium (u8 configNr,u32 medium,u32 speed) {pr_debug ("%s: %u %u\n" , __func__, medium, speed); -if (configNr >=RNDIS_MAX_CONFIGS )return -1 ; +if (configNr >=RNDIS_MAX_CONFIGS ) +return -1 ; rndis_per_dev_params[configNr].medium = medium; rndis_per_dev_params[configNr].speed = speed;
@@ -991,7 +1000,8 @@ u8 *rndis_get_next_response(int configNr, u32 *length)rndis_resp_t *r;struct list_head *act, *tmp; -if (!length)return NULL ; +if (!length) +return NULL ;list_for_each_safe (act, tmp, &(rndis_per_dev_params[configNr].resp_queue))
@@ -1013,7 +1023,8 @@ static rndis_resp_t *rndis_add_response(int configNr, u32 length)/* NOTE: this gets copied into ether.c USB_BUFSIZ bytes ... */ r =kmalloc (sizeof (rndis_resp_t) + length,GFP_ATOMIC ); -if (!r)return NULL ; +if (!r) +return NULL ; r->buf = (u8 *)(r +1 ); r->length = length;
@@ -1116,8 +1127,10 @@ static ssize_t rndis_proc_write(struct file *file, const char __user *buffer,rndis_signal_disconnect (p->confignr);break ;default : -if (fl_speed) p->speed = speed; -else pr_debug ("%c is not valid\n" , c); +if (fl_speed) + p->speed = speed; +else +pr_debug ("%c is not valid\n" , c);break ; }
@@ -1143,7 +1156,7 @@ static const struct file_operations rndis_proc_fops = {#define NAME_TEMPLATE "driver/rndis-%03d" -static struct proc_dir_entry *rndis_connect_state [RNDIS_MAX_CONFIGS ]; +static struct proc_dir_entry *rndis_connect_state[RNDIS_MAX_CONFIGS ];#endif /* CONFIG_USB_GADGET_DEBUG_FILES */
@@ -1154,7 +1167,7 @@ int rndis_init(void)for (i =0 ; i <RNDIS_MAX_CONFIGS ; i++) {#ifdef CONFIG_USB_GADGET_DEBUG_FILES -char name [20 ]; +char name[20 ];sprintf (name,NAME_TEMPLATE , i); rndis_connect_state[i] = proc_create_data(name, 0660, NULL,
Signature · 1 line
1.7.5.2
drivers/usb/gadget/rndis.c static int rndis_reset_response(int configNr, · line 706
Quoted 8 lines · Andre Bartke
drivers/usb/gadget/rndis.c +1
-0@@ -700,6 +701,7 @@ static int rndis_reset_response(int configNr, rndis_reset_msg_type *buf) resp->AddressingReset = cpu_to_le32(1); params->resp_avail (params->v); +kfree (r);return 0 ; }
Have you tested this? It does not look right to me. rndis_add_response() allocates memory and puts the request on a list. It is later freed in rndis_free_response().
Signature · 4 lines
Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michal "mina86" Nazarewicz (o o) ooo +-----<email/xmpp: [email protected]>-----ooO--(_)--Ooo--
Quoted 15 lines · On Sat, 04 Jun 2011 23:35:16 +0200 "Michal Nazarewicz" <[email protected]>…
drivers/usb/gadget/rndis.c static int rndis_reset_response(int configNr, · line 706
Quoted 8 lines · Andre Bartke
drivers/usb/gadget/rndis.c +1
-0@@ -700,6 +701,7 @@ static int rndis_reset_response(int configNr, rndis_reset_msg_type *buf) resp->AddressingReset = cpu_to_le32(1); params->resp_avail (params->v); +kfree (r);return 0 ; }Have you tested this? It does not look right to me.
rndis_add_response()allocates memory and puts the request on a list. It is later freed inrndis_free_response().
Nope, the only way I can test this is a build - which it does. You might be right though.
Quoted 19 lines · Andre Bartke
Quoted 15 lines · On Sat, 04 Jun 2011 23:35:16 +0200 "Michal Nazarewicz" <[email protected]>…
drivers/usb/gadget/rndis.c static int rndis_reset_response(int configNr, · line 706
Quoted 8 lines · Andre Bartke
drivers/usb/gadget/rndis.c +1
-0@@ -700,6 +701,7 @@ static int rndis_reset_response(int configNr, rndis_reset_msg_type *buf) resp->AddressingReset = cpu_to_le32(1); params->resp_avail(params->v); + kfree(r); return 0; }Have you tested this? It does not look right to me.
rndis_add_response()allocates memory and puts the request on a list. It is later freed inrndis_free_response().
Quoted 2 lines
Nope, the only way I can test this is a build - which it does. You might be right though.
building != correct
r is being added to a list in rndis_add_response Andre is right, rndis_free_response handles list removal
982 if (r && r->buf == buf) {
983 list_del(&r->list);
984 kfree(r);
985 }
in fact all of the kfree calls I believe are incorrect.
Connor
Quoted 6 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Quoted 29 lines · On Sat, 4 Jun 2011 18:35:35 -0700 Connor H <[email protected]> wrote:
Quoted 19 lines · Andre Bartke
Quoted 15 lines · On Sat, 04 Jun 2011 23:35:16 +0200 "Michal Nazarewicz" <[email protected]>…
Quoted 8 lines · Andre Bartke
(quoted diff) +0
-0
b/drivers/usb/gadget/rndis.c@@ -700,6 +701,7 @@ static intrndis_reset_response(int configNr, rndis_reset_msg_type *buf)resp->AddressingReset=cpu_to_le32(1);params->resp_avail(params->v);
+ kfree(r);
return 0;
}Have you tested this? It does not look right to me.
rndis_add_response()allocates memory and puts the request on a list. It is later freed inrndis_free_response().Quoted 2 lines
Nope, the only way I can test this is a build - which it does. You might be right though.
building != correct
r is being added to a list in
rndis_add_responseAndre is right,rndis_free_responsehandles list removal
Quoted 8 lines
982 if (r &&
r->buf== buf) {
983list_del(&r->list);
984 kfree(r);
985 }in fact all of the kfree calls I believe are incorrect.
Connor
alright, then drop this patch please.
Quoted 7 lines
Quoted 7 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/