* [PATCH] staging: vme: fix memory leak in vme_user_probe() @ 2014-03-26 3:01 Daeseok Youn 2014-03-26 18:51 ` Aaron Sierra 0 siblings, 1 reply; 5+ messages in thread From: Daeseok Youn @ 2014-03-26 3:01 UTC (permalink / raw) To: martyn.welch, manohar.vanga, gregkh Cc: asierra, lisa, yongjun_wei, devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] staging: vme: fix memory leak in vme_user_probe() 2014-03-26 3:01 [PATCH] staging: vme: fix memory leak in vme_user_probe() Daeseok Youn @ 2014-03-26 18:51 ` Aaron Sierra 2014-03-27 1:47 ` DaeSeok Youn 0 siblings, 1 reply; 5+ messages in thread From: Aaron Sierra @ 2014-03-26 18:51 UTC (permalink / raw) To: Daeseok Youn Cc: martyn welch, manohar vanga, gregkh, lisa, yongjun wei, devel, linux-kernel ----- 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 > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] staging: vme: fix memory leak in vme_user_probe() 2014-03-26 18:51 ` Aaron Sierra @ 2014-03-27 1:47 ` DaeSeok Youn 2014-03-27 14:30 ` Aaron Sierra 0 siblings, 1 reply; 5+ messages in thread From: DaeSeok Youn @ 2014-03-27 1:47 UTC (permalink / raw) To: Aaron Sierra Cc: martyn welch, manohar vanga, Greg KH, lisa, yongjun wei, devel, linux-kernel 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 >> >> >> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] staging: vme: fix memory leak in vme_user_probe() 2014-03-27 1:47 ` DaeSeok Youn @ 2014-03-27 14:30 ` Aaron Sierra 2014-03-27 23:53 ` DaeSeok Youn 0 siblings, 1 reply; 5+ messages in thread From: Aaron Sierra @ 2014-03-27 14:30 UTC (permalink / raw) To: DaeSeok Youn Cc: martyn welch, manohar vanga, Greg KH, lisa, yongjun wei, devel, linux-kernel ----- 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 > >> > >> > >> > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] staging: vme: fix memory leak in vme_user_probe() 2014-03-27 14:30 ` Aaron Sierra @ 2014-03-27 23:53 ` DaeSeok Youn 0 siblings, 0 replies; 5+ messages in thread From: DaeSeok Youn @ 2014-03-27 23:53 UTC (permalink / raw) To: Aaron Sierra Cc: martyn welch, manohar vanga, Greg KH, lisa, yongjun wei, devel, linux-kernel 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 >> >> >> >> >> >> >> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-03-27 23:53 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-03-26 3:01 [PATCH] staging: vme: fix memory leak in vme_user_probe() Daeseok Youn 2014-03-26 18:51 ` Aaron Sierra 2014-03-27 1:47 ` DaeSeok Youn 2014-03-27 14:30 ` Aaron Sierra 2014-03-27 23:53 ` DaeSeok Youn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
Powered by JetHome