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

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

[Daeseok Youn]

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@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

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;
 		}
 	}
 
@@ -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);
 	}
 
-- 
1.7.4.4

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

[Aaron Sierra]

----- Original Message -----
> From: "Daeseok Youn" <daeseok.youn@gmail.com>
> 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@gmail.com>
> ---
>  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.

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.

>  
> @@ -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--;
	}

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--;
	}

-Aaron

>  
> --
> 1.7.4.4
> 
> 
> 

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

[DaeSeok Youn]

2014-03-27 3:51 GMT+09:00 Aaron Sierra <asierra@xes-inc.com>:
> ----- Original Message -----
>> From: "Daeseok Youn" <daeseok.youn@gmail.com>
>> 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@gmail.com>
>> ---
>>  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
>>
>>
>>

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

[Aaron Sierra]

----- Original Message -----
> From: "DaeSeok Youn" <daeseok.youn@gmail.com>
> Sent: Wednesday, March 26, 2014 8:47:51 PM
>
> 2014-03-27 3:51 GMT+09:00 Aaron Sierra <asierra@xes-inc.com>:
> > ----- Original Message -----
> >> From: "Daeseok Youn" <daeseok.youn@gmail.com>
> >> 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@gmail.com>
> >> ---
> >>  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.

I do like the error path symmetry provided by your patch.

> >
> > 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. :-)

You are correct that vme_master_free() assumes that the resource being
freed is not NULL. I should have recognized that case. That could easily
be handled by testing the resource again in the loop, but I see the
benefit of the symmetry of your change.

-Aaron

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

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

[DaeSeok Youn]

2014-03-27 23:30 GMT+09:00 Aaron Sierra <asierra@xes-inc.com>:
>
> ----- Original Message -----
>> From: "DaeSeok Youn" <daeseok.youn@gmail.com>
>> Sent: Wednesday, March 26, 2014 8:47:51 PM
>>
>> 2014-03-27 3:51 GMT+09:00 Aaron Sierra <asierra@xes-inc.com>:
>> > ----- Original Message -----
>> >> From: "Daeseok Youn" <daeseok.youn@gmail.com>
>> >> 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@gmail.com>
>> >> ---
>> >>  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.
>
> I do like the error path symmetry provided by your patch.
>
>> >
>> > 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. :-)
>
> You are correct that vme_master_free() assumes that the resource being
> freed is not NULL. I should have recognized that case. That could easily
> be handled by testing the resource again in the loop, but I see the
> benefit of the symmetry of your change.

OK. I would leave the symmetry of my patch. :-)
Thanks for your reply.

Regards,
Daeseok Youn.
>
> -Aaron
>
>>
>> >
>> > 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
>> >>
>> >>
>> >>
>>