Re: [PATCH] staging: vme: fix memory leak in vme_user_probe()

[DaeSeok Youn]

2014-03-27 3:51 GMT+09:00 Aaron Sierra <asierra@xxxxxxxxxxx>:
> ----- Original Message -----
>> From: "Daeseok Youn" <daeseok.youn@xxxxxxxxx>
>> Sent: Tuesday, March 25, 2014 10:01:48 PM
>> Subject: [PATCH] staging: vme: fix memory leak in vme_user_probe()
>>
>>
>> If vme_master_request() returns NULL when it failed,
>> it need to free buffers for master.
>>
>> And also removes unreachable code in vme_user_probe().
>>
>> Signed-off-by: Daeseok Youn <daeseok.youn@xxxxxxxxx>
>> ---
>>  drivers/staging/vme/devices/vme_user.c |    9 +++------
>>  1 files changed, 3 insertions(+), 6 deletions(-)
>
> Nice catches Daeseok. I don't maintain this driver, but I have some
> suggestions below.
>
>>
>> diff --git a/drivers/staging/vme/devices/vme_user.c
>> b/drivers/staging/vme/devices/vme_user.c
>> index 7927927..ffb4eee 100644
>> --- a/drivers/staging/vme/devices/vme_user.c
>> +++ b/drivers/staging/vme/devices/vme_user.c
>> @@ -776,7 +776,8 @@ static int vme_user_probe(struct vme_dev *vdev)
>>               image[i].kern_buf = kmalloc(image[i].size_buf, GFP_KERNEL);
>>               if (image[i].kern_buf == NULL) {
>>                       err = -ENOMEM;
>> -                     goto err_master_buf;
>> +                     vme_master_free(image[i].resource);
>> +                     goto err_master;
>>               }
>>       }
>
> I think it would be nice to keep all of the cleanup under the err_master
> label.
Actually, I changed like "err_slave" doing. When it failed to alloc
buffer for slave,
just called vme_slave_free(image[i].slave) and cleanup under the err_slave.

>
> That could be done by changing the kern_buf allocation in this part to
> a devm_kmalloc. Then devm handles the kern_buf freeing entirely.
I didn't know about devm_kmalloc(), I will check that function. Thanks!

>
>>
>> @@ -819,8 +820,6 @@ static int vme_user_probe(struct vme_dev *vdev)
>>
>>       return 0;
>>
>> -     /* Ensure counter set correcty to destroy all sysfs devices */
>> -     i = VME_DEVS;
>>  err_sysfs:
>>       while (i > 0) {
>>               i--;
>> @@ -830,12 +829,10 @@ err_sysfs:
>>
>>       /* Ensure counter set correcty to unalloc all master windows */
>>       i = MASTER_MAX + 1;
>> -err_master_buf:
>> -     for (i = MASTER_MINOR; i < (MASTER_MAX + 1); i++)
>> -             kfree(image[i].kern_buf);
>>  err_master:
>>       while (i > MASTER_MINOR) {
>>               i--;
>> +             kfree(image[i].kern_buf);
>>               vme_master_free(image[i].resource);
>>       }
>
> Using devm_kmalloc as mentioned above, the while loop could be
> simplified to this:
>
> err_master:
>         while (i >= MASTER_MINOR) {
>                 vme_master_free(image[i].resource);
>                 i--;
>         }
It would be nice, but when it failed to vme_master_request() and than
go to err_master,
image[i].resource must be NULL. So a NULL exception has occurred in
vme_master_free().

I think vme_master{slave}_free() need to check NULL and it can be
possible to change code as your comment.
please check for me. :-)

>
> If not moving to devm, this should be safe even though the first
> kern_buf may be NULL:
>
> err_master:
>         while (i >= MASTER_MINOR) {
>                 kfree(image[i].kern_buf);
>                 vme_master_free(image[i].resource);
>                 i--;
>         }
kfree() is ok. But vme_master_free() function has an problem as mentioned above.

Thanks for review.
Daeseok Youn.
>
> -Aaron
>
>>
>> --
>> 1.7.4.4
>>
>>
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/