Messages in this thread | | | Date | Thu, 7 Dec 2023 12:46:34 -0800 | From | Ira Weiny <> | Subject | Re: [PATCH] nvdimm-btt: fix a potential memleak in btt_freelist_init |
| |
Dave Jiang wrote: >
[snip]
First off thanks for the patch. This code seems to have a few things to clean up.
> > On 12/6/23 20:43, Dinghao Liu wrote: > > When an error happens in btt_freelist_init(), its caller > > discover_arenas() will directly free arena, which makes > > arena->freelist allocated in btt_freelist_init() a leaked > > memory. Fix this by freeing arena->freelist in all error > > handling paths of btt_freelist_init(). > > > > Fixes: 5212e11fde4d ("nd_btt: atomic sector updates") > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> > > How about use the new scope based resource management and we can avoid the goto mess altogether? > https://lwn.net/Articles/934679/ >
The freelist is returned as part of arena. I've not traced both paths of btt_freelist_init() completely but devm_kcalloc() looks like a better solution here because this memory needs to live past the function scope.
That said, this patch does not completely fix freelist from leaking in the following error path.
discover_arenas() btt_freelist_init() -> ok (memory allocated) btt_rtt_init() -> fail goto out; (leak because arena is not yet on btt->arena_list) OR btt_maplocks_init() -> fail goto out; (leak because arena is not yet on btt->arena_list)
This error could be fixed by adding to arena_list earlier but devm_*() also takes care of this without having to worry about that logic.
On normal operation all of this memory can be free'ed with the corresponding devm_kfree() and/or devm_add_action_*() calls if arenas come and go. I'm not sure off the top of my head.
In addition, looking at this code. discover_arenas() could make use of the scoped based management for struct btt_sb *super!
Dinghao would you be willing to submit a series of 2 or 3 patches to fix the above issues?
Thanks! Ira
| |