Re: [PATCH] nvdimm-btt: fix a potential memleak in btt_freelist_init

[dinghao . liu]

> 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@xxxxxxxxxx>
> > 
> > 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)
> 

Thanks for pointing out this issue! I rechecked discover_arenas() and found
that btt_rtt_init() may also trigger a memleak for the same reason as
btt_freelist_init(). Also, I checked another call trace:

    btt_init() -> btt_meta_init() -> btt_maplocks_init()

I think there is a memleak if btt_maplocks_init() succeeds but an error
happens in btt_init() after btt_meta_init() (e.g., when btt_blk_init()
returns an error). Therefore, we may need to fix three functions.

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

Sure. Currently I plan to send 2 patches as follows:
1. Using devm_kcalloc() to replace kcalloc() in btt_freelist_init(), 
   btt_rtt_init(), and btt_maplocks_init(), and removing the corresponding
   kfree in free_arenas(). I checked some uses of devm_kcalloc() and it
   seems that we need not to call devm_kfree(). The memory is automatically
   freed on driver detach, right?
2. Using the scoped based management for struct btt_sb *super (not a bug,
   but it could improve the code).

I'm not quite sure whether my understanding or bug fixing plan is correct.
If there are any issues, please correct me, thanks!

Regards,
Dinghao