* Re: [PATCH] staging: ft1000: fix error path
2010-09-26 13:11 ` Dan Carpenter
@ 2010-09-26 16:56 ` Belisko Marek
2010-09-26 17:18 ` walter harms
2010-09-26 19:41 ` Vasiliy Kulikov
2010-10-03 17:58 ` [PATCH v2] " Vasiliy Kulikov
2 siblings, 1 reply; 6+ messages in thread
From: Belisko Marek @ 2010-09-26 16:56 UTC (permalink / raw)
To: Dan Carpenter, Vasiliy Kulikov, kernel-janitors,
Greg Kroah-Hartman, Marek Belisko, devel, linux-kernel
On Sun, Sep 26, 2010 at 3:11 PM, Dan Carpenter <error27@gmail.com> wrote:
> On Sun, Sep 26, 2010 at 12:59:55PM +0400, Vasiliy Kulikov wrote:
>> +err_free:
>> + for (i--; i>=0; i--) {
>> + kfree(pdpram_blk->pbuffer);
>> + kfree(pdpram_blk);
>> + }
>
> This is wrong. I don't have linux-next so I can't see the context, why
> are we looping here? The second iteration through the loop will cause a
> NULL dereference.
Some lines upper there is allocation of structure and it's internal
buffer in loop:
for (i=0; i<NUM_OF_FREE_BUFFERS; i++) {
// Get memory for DPRAM_DATA link list
pdpram_blk = kmalloc ( sizeof(DPRAM_BLK), GFP_KERNEL );
// Get a block of memory to store command data
pdpram_blk->pbuffer = kmalloc ( MAX_CMD_SQSIZE, GFP_KERNEL );
// link provisioning data
list_add_tail (&pdpram_blk->list, &freercvpool);
}
Free loop is correct in my opinion but kfree should be extended by checking
of NULL pointer because allocation of pdpram_blk could fail and we free also
pdpram_blk->pbuffer.
>
> Also there should be spaces before and after the ">=".
>
> regards,
> dan carpenter
>
>> + return STATUS_FAILURE;
>> }
>>
>
>
marek
--
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer
Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
icq: 290551086
web: http://open-nandra.com
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] staging: ft1000: fix error path
2010-09-26 16:56 ` Belisko Marek
@ 2010-09-26 17:18 ` walter harms
0 siblings, 0 replies; 6+ messages in thread
From: walter harms @ 2010-09-26 17:18 UTC (permalink / raw)
To: Belisko Marek
Cc: Dan Carpenter, kernel-janitors, Greg Kroah-Hartman, devel,
linux-kernel
hi Marek,
IMHO the alloc sequencs is building a linked list of allocated buffer.
And if kfree() does no magiclly follow the list this is not working.
re,
wh
Belisko Marek schrieb:
> On Sun, Sep 26, 2010 at 3:11 PM, Dan Carpenter <error27@gmail.com> wrote:
>> On Sun, Sep 26, 2010 at 12:59:55PM +0400, Vasiliy Kulikov wrote:
>>> +err_free:
>>> + for (i--; i>=0; i--) {
>>> + kfree(pdpram_blk->pbuffer);
>>> + kfree(pdpram_blk);
>>> + }
>> This is wrong. I don't have linux-next so I can't see the context, why
>> are we looping here? The second iteration through the loop will cause a
>> NULL dereference.
> Some lines upper there is allocation of structure and it's internal
> buffer in loop:
> for (i=0; i<NUM_OF_FREE_BUFFERS; i++) {
> // Get memory for DPRAM_DATA link list
> pdpram_blk = kmalloc ( sizeof(DPRAM_BLK), GFP_KERNEL );
> // Get a block of memory to store command data
> pdpram_blk->pbuffer = kmalloc ( MAX_CMD_SQSIZE, GFP_KERNEL );
> // link provisioning data
> list_add_tail (&pdpram_blk->list, &freercvpool);
> }
>
> Free loop is correct in my opinion but kfree should be extended by checking
> of NULL pointer because allocation of pdpram_blk could fail and we free also
> pdpram_blk->pbuffer.
>> Also there should be spaces before and after the ">=".
>>
>> regards,
>> dan carpenter
>>
>>> + return STATUS_FAILURE;
>>> }
>>>
>>
>
> marek
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: ft1000: fix error path
2010-09-26 13:11 ` Dan Carpenter
2010-09-26 16:56 ` Belisko Marek
@ 2010-09-26 19:41 ` Vasiliy Kulikov
2010-10-03 17:58 ` [PATCH v2] " Vasiliy Kulikov
2 siblings, 0 replies; 6+ messages in thread
From: Vasiliy Kulikov @ 2010-09-26 19:41 UTC (permalink / raw)
To: Dan Carpenter, kernel-janitors, Greg Kroah-Hartman, Marek Belisko,
devel, linux-kernel
On Sun, Sep 26, 2010 at 15:11 +0200, Dan Carpenter wrote:
> On Sun, Sep 26, 2010 at 12:59:55PM +0400, Vasiliy Kulikov wrote:
> > +err_free:
> > + for (i--; i>=0; i--) {
> > + kfree(pdpram_blk->pbuffer);
> > + kfree(pdpram_blk);
> > + }
>
> This is wrong.
Of course, I'm very careless. Pointers are linked into the list and the right code
should loop the list and free both pointers. I'll post patch v2 tomorrow.
--
Vasiliy
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2] staging: ft1000: fix error path
2010-09-26 13:11 ` Dan Carpenter
2010-09-26 16:56 ` Belisko Marek
2010-09-26 19:41 ` Vasiliy Kulikov
@ 2010-10-03 17:58 ` Vasiliy Kulikov
2 siblings, 0 replies; 6+ messages in thread
From: Vasiliy Kulikov @ 2010-10-03 17:58 UTC (permalink / raw)
To: Dan Carpenter, kernel-janitors, Greg Kroah-Hartman, Marek Belisko,
devel, linux-kernel
init_ft1000_netdev() calls kfree(netdev) instead of free_netdev(netdev).
It doesn't check kmalloc() return value.
ft1000_read_fifo_reg() doesn't free dr on error and calls kfree(urb)
instead of usb_free_urb(urb). Also kfree(NULL) is OK.
Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
---
drivers/staging/ft1000/ft1000-usb/ft1000_hw.c | 21 +++++++++++++++++++--
1 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/ft1000/ft1000-usb/ft1000_hw.c b/drivers/staging/ft1000/ft1000-usb/ft1000_hw.c
index 51ed0dd..8609add 100644
--- a/drivers/staging/ft1000/ft1000-usb/ft1000_hw.c
+++ b/drivers/staging/ft1000/ft1000-usb/ft1000_hw.c
@@ -1013,6 +1013,7 @@ u16 init_ft1000_netdev(struct ft1000_device *ft1000dev)
FT1000_INFO *pInfo = NULL;
PDPRAM_BLK pdpram_blk;
int i;
+ struct list_head *cur, *tmp;
gCardIndex=0; //mbelian
@@ -1048,7 +1049,7 @@ u16 init_ft1000_netdev(struct ft1000_device *ft1000dev)
}
else {
printk(KERN_ERR "ft1000: Invalid device name\n");
- kfree(netdev);
+ free_netdev(netdev);
return STATUS_FAILURE;
}
}
@@ -1121,8 +1122,14 @@ u16 init_ft1000_netdev(struct ft1000_device *ft1000dev)
for (i=0; i<NUM_OF_FREE_BUFFERS; i++) {
// Get memory for DPRAM_DATA link list
pdpram_blk = kmalloc ( sizeof(DPRAM_BLK), GFP_KERNEL );
+ if (pdpram_blk == NULL)
+ goto err_free;
// Get a block of memory to store command data
pdpram_blk->pbuffer = kmalloc ( MAX_CMD_SQSIZE, GFP_KERNEL );
+ if (pdpram_blk->pbuffer == NULL) {
+ kfree(pdpram_blk);
+ goto err_free;
+ }
// link provisioning data
list_add_tail (&pdpram_blk->list, &freercvpool);
}
@@ -1131,6 +1138,15 @@ u16 init_ft1000_netdev(struct ft1000_device *ft1000dev)
return STATUS_SUCCESS;
+
+err_free:
+ list_for_each_safe(cur, tmp, &pdpram_blk->list) {
+ pdpram_blk = list_entry(cur, DPRAM_BLK, list);
+ list_del(&pdpram_blk->list);
+ kfree(pdpram_blk->pbuffer);
+ kfree(pdpram_blk);
+ }
+ return STATUS_FAILURE;
}
@@ -1287,7 +1303,8 @@ static int ft1000_read_fifo_reg(struct ft1000_device *ft1000dev,unsigned int pip
if(!urb || !dr)
{
- if(urb) kfree(urb);
+ kfree(dr);
+ usb_free_urb(urb);
spin_unlock(&ft1000dev->device_lock);
return -ENOMEM;
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 6+ messages in thread