Messages in this thread | | | Subject | Re: [PATCH 167/190] Revert "gdrom: fix a memory leak bug" | From | Jens Axboe <> | Date | Tue, 27 Apr 2021 11:01:50 -0600 |
| |
On 4/27/21 10:12 AM, Greg KH wrote: > On Tue, Apr 27, 2021 at 08:39:15AM -0600, Jens Axboe wrote: >> On 4/27/21 8:03 AM, Peter Rosin wrote: >>> On 2021-04-27 15:01, Greg KH wrote: >>>> On Fri, Apr 23, 2021 at 08:20:30AM -0600, Jens Axboe wrote: >>>>> On 4/22/21 3:29 PM, Peter Rosin wrote: >>>>>>> This reverts commit 093c48213ee37c3c3ff1cf5ac1aa2a9d8bc66017. >>>>>> >>>>>> The reverted patch looks fishy. >>>>>> >>>>>> gc.cd_info is kzalloc:ed on probe. In case probe fails after this allocation, the >>>>>> memory is kfree:d but the variable is NOT zeroed out. >>>>>> >>>>>> AFAICT, the above leads to a double-free on exit by the added line. >>>>>> >>>>>> I believe gd.cd_info should be kfree:d on remove instead. >>>>>> >>>>>> However, might not gc.toc also be kfree:d twice for similar reasons? >>>>>> >>>>>> I could easily be mistaken. >>>>> >>>>> >From taking a quick look the other day, that's my conclusion too. I >>>>> don't think the patch is correct, but I don't think the surrounding code >>>>> is correct right now either. >>>> >>>> Thanks for the review from both of you, I'll keep this commit in the >>>> tree. >>> Err, which commit is "this" and what tree are you keeping it in? I >>> think you mean that you are keeping the revert in your tree with >>> reverts, and not that you mean that we should keep the original >>> commit in Linus' tree. >>> >>> In any case, I'd think that the original memory leak is somewhat >>> better than the introduced double-free and therefore the revert >>> should be done. >> >> It should probably look like the below, though I doubt it matters >> since only one device is supported anyway. As long as the free >> happens post unregister, it likely won't make a difference. But >> it is cleaner and easier to verify, and should double device support >> ever be introduced, the existing code is buggy. >> >> But given that, I don't think we should keep the revert patch. >> >> diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c >> index 9874fc1c815b..02d369881165 100644 >> --- a/drivers/cdrom/gdrom.c >> +++ b/drivers/cdrom/gdrom.c >> @@ -831,6 +831,8 @@ static int remove_gdrom(struct platform_device *devptr) >> if (gdrom_major) >> unregister_blkdev(gdrom_major, GDROM_DEV_NAME); >> unregister_cdrom(gd.cd_info); >> + kfree(gd.toc); >> + kfree(gd.cd_info); >> >> return 0; >> } >> @@ -862,8 +864,6 @@ static void __exit exit_gdrom(void) >> { >> platform_device_unregister(pd); >> platform_driver_unregister(&gdrom_driver); >> - kfree(gd.toc); >> - kfree(gd.cd_info); >> } >> >> module_init(init_gdrom); >> >> -- >> Jens Axboe >> > > I'll add this fix to the tree after the revert, and give you the credit > for the fix :)
Sounds good, thanks Greg.
-- Jens Axboe
| |