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