| Message ID | 20231207034332.24107-1-dinghao.liu@zju.edu.cn (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | nvdimm-btt: fix a potential memleak in btt_freelist_init | expand |
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/ > --- > drivers/nvdimm/btt.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c > index d5593b0dc700..d8c4ba8bfdda 100644 > --- a/drivers/nvdimm/btt.c > +++ b/drivers/nvdimm/btt.c > @@ -544,8 +544,10 @@ static int btt_freelist_init(struct arena_info *arena) > > for (i = 0; i < arena->nfree; i++) { > new = btt_log_read(arena, i, &log_new, LOG_NEW_ENT); > - if (new < 0) > - return new; > + if (new < 0) { > + ret = new; > + goto out_free; > + } > > /* old and new map entries with any flags stripped out */ > log_oldmap = ent_lba(le32_to_cpu(log_new.old_map)); > @@ -577,7 +579,7 @@ static int btt_freelist_init(struct arena_info *arena) > ret = btt_map_read(arena, le32_to_cpu(log_new.lba), &map_entry, > NULL, NULL, 0); > if (ret) > - return ret; > + goto out_free; > > /* > * The map_entry from btt_read_map is stripped of any flag bits, > @@ -594,11 +596,16 @@ static int btt_freelist_init(struct arena_info *arena) > ret = btt_map_write(arena, le32_to_cpu(log_new.lba), > le32_to_cpu(log_new.new_map), 0, 0, 0); > if (ret) > - return ret; > + goto out_free; > } > } > > return 0; > + > +out_free: > + kfree(arena->freelist); > + arena->freelist = NULL; > + return ret; > } > > static bool ent_is_padding(struct log_entry *ent)
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
> 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) > 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
dinghao.liu@ wrote: > > Dave Jiang wrote: [snip] > > 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. Yea I think we need to trace this code better. This is why devm_ is nice for memory allocated for the life of the device. > > > 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? On device put yes. So if these allocations are scoped to the life of the device there would be no reason to call devm_kfree() on them at all. I was not sure if they got reallocated at some point or not. > 2. Using the scoped based management for struct btt_sb *super (not a bug, > but it could improve the code). Good! > > I'm not quite sure whether my understanding or bug fixing plan is correct. > If there are any issues, please correct me, thanks! The above sounds right. Ira
> dinghao.liu@ wrote: > > > Dave Jiang wrote: > > [snip] > > > > 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. > > Yea I think we need to trace this code better. This is why devm_ is nice for > memory allocated for the life of the device. > > > > > > 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? > > On device put yes. So if these allocations are scoped to the life of the > device there would be no reason to call devm_kfree() on them at all. I was not > sure if they got reallocated at some point or not. > > > 2. Using the scoped based management for struct btt_sb *super (not a bug, > > but it could improve the code). > > Good! > > > > > I'm not quite sure whether my understanding or bug fixing plan is correct. > > If there are any issues, please correct me, thanks! > > The above sounds right. > Ira Thanks for the review! I will send the patches soon. Regards, Dinghao
diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c index d5593b0dc700..d8c4ba8bfdda 100644 --- a/drivers/nvdimm/btt.c +++ b/drivers/nvdimm/btt.c @@ -544,8 +544,10 @@ static int btt_freelist_init(struct arena_info *arena) for (i = 0; i < arena->nfree; i++) { new = btt_log_read(arena, i, &log_new, LOG_NEW_ENT); - if (new < 0) - return new; + if (new < 0) { + ret = new; + goto out_free; + } /* old and new map entries with any flags stripped out */ log_oldmap = ent_lba(le32_to_cpu(log_new.old_map)); @@ -577,7 +579,7 @@ static int btt_freelist_init(struct arena_info *arena) ret = btt_map_read(arena, le32_to_cpu(log_new.lba), &map_entry, NULL, NULL, 0); if (ret) - return ret; + goto out_free; /* * The map_entry from btt_read_map is stripped of any flag bits, @@ -594,11 +596,16 @@ static int btt_freelist_init(struct arena_info *arena) ret = btt_map_write(arena, le32_to_cpu(log_new.lba), le32_to_cpu(log_new.new_map), 0, 0, 0); if (ret) - return ret; + goto out_free; } } return 0; + +out_free: + kfree(arena->freelist); + arena->freelist = NULL; + return ret; } static bool ent_is_padding(struct log_entry *ent)
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> --- drivers/nvdimm/btt.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)