| Message ID | 20241127142519.3038691-1-cleger@rivosinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] riscv: module: use a plain variable for list_head instead of a pointer | expand |
On Wed, Nov 27, 2024 at 03:25:17PM +0100, Clément Léger wrote: > rel_head's list_head member, rel_entry, doesn't need to be allocated, > its storage can just be part of the allocated rel_head. Remove the > pointer which allows to get rid of the allocation as well as an existing > memory leak found by Kai Zang using kmemleak. > > Reported-by: Kai Zhang <zhangkai@iscas.ac.cn> > Signed-off-by: Clément Léger <cleger@rivosinc.com> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> Thanks for looking into this! Reviewed-by: Charlie Jenkins <charlie@rivosinc.com> Tested-by: Charlie Jenkins <charlie@rivosinc.com> > --- > > V2: > - Add Kai Reported-by > - Reword the commit description (Andrew) > > --- > arch/riscv/kernel/module.c | 18 ++++-------------- > 1 file changed, 4 insertions(+), 14 deletions(-) > > diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c > index 1cd461f3d872..47d0ebeec93c 100644 > --- a/arch/riscv/kernel/module.c > +++ b/arch/riscv/kernel/module.c > @@ -23,7 +23,7 @@ struct used_bucket { > > struct relocation_head { > struct hlist_node node; > - struct list_head *rel_entry; > + struct list_head rel_entry; > void *location; > }; > > @@ -634,7 +634,7 @@ process_accumulated_relocations(struct module *me, > location = rel_head_iter->location; > list_for_each_entry_safe(rel_entry_iter, > rel_entry_iter_tmp, > - rel_head_iter->rel_entry, > + &rel_head_iter->rel_entry, > head) { > curr_type = rel_entry_iter->type; > reloc_handlers[curr_type].reloc_handler( > @@ -704,16 +704,7 @@ static int add_relocation_to_accumulate(struct module *me, int type, > return -ENOMEM; > } > > - rel_head->rel_entry = > - kmalloc(sizeof(struct list_head), GFP_KERNEL); > - > - if (!rel_head->rel_entry) { > - kfree(entry); > - kfree(rel_head); > - return -ENOMEM; > - } > - > - INIT_LIST_HEAD(rel_head->rel_entry); > + INIT_LIST_HEAD(&rel_head->rel_entry); > rel_head->location = location; > INIT_HLIST_NODE(&rel_head->node); > if (!current_head->first) { > @@ -722,7 +713,6 @@ static int add_relocation_to_accumulate(struct module *me, int type, > > if (!bucket) { > kfree(entry); > - kfree(rel_head->rel_entry); > kfree(rel_head); > return -ENOMEM; > } > @@ -735,7 +725,7 @@ static int add_relocation_to_accumulate(struct module *me, int type, > } > > /* Add relocation to head of discovered rel_head */ > - list_add_tail(&entry->head, rel_head->rel_entry); > + list_add_tail(&entry->head, &rel_head->rel_entry); > > return 0; > } > -- > 2.45.2 >
On Wed, 2024-11-27 at 15:25 +0100, Clément Léger wrote: > rel_head's list_head member, rel_entry, doesn't need to be allocated, > its storage can just be part of the allocated rel_head. Remove the Oh my poor English. OK, it's more better than just add the lost kfree. > pointer which allows to get rid of the allocation as well as an > existing > memory leak found by Kai Zang using kmemleak. ^ Zhang BTW. Doesn't it need a fixes tag like what you suggested? (The bug might come from 6.7) > > Reported-by: Kai Zhang <zhangkai@iscas.ac.cn> > Signed-off-by: Clément Léger <cleger@rivosinc.com> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > --- > > V2: > - Add Kai Reported-by > - Reword the commit description (Andrew) > > --- > arch/riscv/kernel/module.c | 18 ++++-------------- > 1 file changed, 4 insertions(+), 14 deletions(-) > > diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c > index 1cd461f3d872..47d0ebeec93c 100644 > --- a/arch/riscv/kernel/module.c > +++ b/arch/riscv/kernel/module.c > @@ -23,7 +23,7 @@ struct used_bucket { > > struct relocation_head { > struct hlist_node node; > - struct list_head *rel_entry; > + struct list_head rel_entry; > void *location; > }; > > @@ -634,7 +634,7 @@ process_accumulated_relocations(struct module > *me, > location = rel_head_iter->location; > list_for_each_entry_safe(rel_entry_iter, > rel_entry_iter_tmp, > - rel_head_iter- > >rel_entry, > + &rel_head_iter- > >rel_entry, > head) { > curr_type = rel_entry_iter->type; > reloc_handlers[curr_type].reloc_handl > er( > @@ -704,16 +704,7 @@ static int add_relocation_to_accumulate(struct > module *me, int type, > return -ENOMEM; > } > > - rel_head->rel_entry = > - kmalloc(sizeof(struct list_head), > GFP_KERNEL); > - > - if (!rel_head->rel_entry) { > - kfree(entry); > - kfree(rel_head); > - return -ENOMEM; > - } > - > - INIT_LIST_HEAD(rel_head->rel_entry); > + INIT_LIST_HEAD(&rel_head->rel_entry); > rel_head->location = location; > INIT_HLIST_NODE(&rel_head->node); > if (!current_head->first) { > @@ -722,7 +713,6 @@ static int add_relocation_to_accumulate(struct > module *me, int type, > > if (!bucket) { > kfree(entry); > - kfree(rel_head->rel_entry); > kfree(rel_head); > return -ENOMEM; > } > @@ -735,7 +725,7 @@ static int add_relocation_to_accumulate(struct > module *me, int type, > } > > /* Add relocation to head of discovered rel_head */ > - list_add_tail(&entry->head, rel_head->rel_entry); > + list_add_tail(&entry->head, &rel_head->rel_entry); > > return 0; > }
On 28/11/2024 02:01, laokz wrote: > On Wed, 2024-11-27 at 15:25 +0100, Clément Léger wrote: >> rel_head's list_head member, rel_entry, doesn't need to be allocated, >> its storage can just be part of the allocated rel_head. Remove the > > Oh my poor English. OK, it's more better than just add the lost kfree. > >> pointer which allows to get rid of the allocation as well as an >> existing >> memory leak found by Kai Zang using kmemleak. > ^ > Zhang > > BTW. Doesn't it need a fixes tag like what you suggested? (The bug > might come from 6.7) Hey Kai, Apologies for misspelling your name, I'll fix that ! That's a good question, I guess I could have added it indeed but since it was rather a rework, I left it out. But it's probably better to add it anyway. I'll send a V3 with the Fixes tag as well as fixing your name spelling. Thanks, Clément > >> >> Reported-by: Kai Zhang <zhangkai@iscas.ac.cn> >> Signed-off-by: Clément Léger <cleger@rivosinc.com> >> Reviewed-by: Andrew Jones <ajones@ventanamicro.com> >> --- >> >> V2: >> - Add Kai Reported-by >> - Reword the commit description (Andrew) >> >> --- >> arch/riscv/kernel/module.c | 18 ++++-------------- >> 1 file changed, 4 insertions(+), 14 deletions(-) >> >> diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c >> index 1cd461f3d872..47d0ebeec93c 100644 >> --- a/arch/riscv/kernel/module.c >> +++ b/arch/riscv/kernel/module.c >> @@ -23,7 +23,7 @@ struct used_bucket { >> >> struct relocation_head { >> struct hlist_node node; >> - struct list_head *rel_entry; >> + struct list_head rel_entry; >> void *location; >> }; >> >> @@ -634,7 +634,7 @@ process_accumulated_relocations(struct module >> *me, >> location = rel_head_iter->location; >> list_for_each_entry_safe(rel_entry_iter, >> rel_entry_iter_tmp, >> - rel_head_iter- >>> rel_entry, >> + &rel_head_iter- >>> rel_entry, >> head) { >> curr_type = rel_entry_iter->type; >> reloc_handlers[curr_type].reloc_handl >> er( >> @@ -704,16 +704,7 @@ static int add_relocation_to_accumulate(struct >> module *me, int type, >> return -ENOMEM; >> } >> >> - rel_head->rel_entry = >> - kmalloc(sizeof(struct list_head), >> GFP_KERNEL); >> - >> - if (!rel_head->rel_entry) { >> - kfree(entry); >> - kfree(rel_head); >> - return -ENOMEM; >> - } >> - >> - INIT_LIST_HEAD(rel_head->rel_entry); >> + INIT_LIST_HEAD(&rel_head->rel_entry); >> rel_head->location = location; >> INIT_HLIST_NODE(&rel_head->node); >> if (!current_head->first) { >> @@ -722,7 +713,6 @@ static int add_relocation_to_accumulate(struct >> module *me, int type, >> >> if (!bucket) { >> kfree(entry); >> - kfree(rel_head->rel_entry); >> kfree(rel_head); >> return -ENOMEM; >> } >> @@ -735,7 +725,7 @@ static int add_relocation_to_accumulate(struct >> module *me, int type, >> } >> >> /* Add relocation to head of discovered rel_head */ >> - list_add_tail(&entry->head, rel_head->rel_entry); >> + list_add_tail(&entry->head, &rel_head->rel_entry); >> >> return 0; >> } >
On Thu, Nov 28, 2024 at 09:01:52AM +0800, laokz wrote: > On Wed, 2024-11-27 at 15:25 +0100, Clément Léger wrote: > > rel_head's list_head member, rel_entry, doesn't need to be allocated, > > its storage can just be part of the allocated rel_head. Remove the > > Oh my poor English. OK, it's more better than just add the lost kfree. > It wasn't the English I was correcting. That was fine, but just saying the object could be "a plain variable" wasn't giving any justification for the change and, to me, even implied that rel_entry was locally scoped. So, when I first skimmed the patch and saw that it was getting appended to a list, I almost stated the patch was wrong. It was clear after looking closer, but it could have been clear the first time through if the commit message had better guided me. Thanks, drew
On Thu, 2024-11-28 at 09:32 +0100, Andrew Jones wrote: > On Thu, Nov 28, 2024 at 09:01:52AM +0800, laokz wrote: > > On Wed, 2024-11-27 at 15:25 +0100, Clément Léger wrote: > > > rel_head's list_head member, rel_entry, doesn't need to be > > > allocated, > > > its storage can just be part of the allocated rel_head. Remove > > > the > > > > Oh my poor English. OK, it's more better than just add the lost > > kfree. > > > > It wasn't the English I was correcting. That was fine, but just > saying > the object could be "a plain variable" wasn't giving any > justification > for the change and, to me, even implied that rel_entry was locally > scoped. So, when I first skimmed the patch and saw that it was > getting > appended to a list, I almost stated the patch was wrong. It was clear > after looking closer, but it could have been clear the first time > through if the commit message had better guided me. Thanks for the explanation. laokz > Thanks, > drew
diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c index 1cd461f3d872..47d0ebeec93c 100644 --- a/arch/riscv/kernel/module.c +++ b/arch/riscv/kernel/module.c @@ -23,7 +23,7 @@ struct used_bucket { struct relocation_head { struct hlist_node node; - struct list_head *rel_entry; + struct list_head rel_entry; void *location; }; @@ -634,7 +634,7 @@ process_accumulated_relocations(struct module *me, location = rel_head_iter->location; list_for_each_entry_safe(rel_entry_iter, rel_entry_iter_tmp, - rel_head_iter->rel_entry, + &rel_head_iter->rel_entry, head) { curr_type = rel_entry_iter->type; reloc_handlers[curr_type].reloc_handler( @@ -704,16 +704,7 @@ static int add_relocation_to_accumulate(struct module *me, int type, return -ENOMEM; } - rel_head->rel_entry = - kmalloc(sizeof(struct list_head), GFP_KERNEL); - - if (!rel_head->rel_entry) { - kfree(entry); - kfree(rel_head); - return -ENOMEM; - } - - INIT_LIST_HEAD(rel_head->rel_entry); + INIT_LIST_HEAD(&rel_head->rel_entry); rel_head->location = location; INIT_HLIST_NODE(&rel_head->node); if (!current_head->first) { @@ -722,7 +713,6 @@ static int add_relocation_to_accumulate(struct module *me, int type, if (!bucket) { kfree(entry); - kfree(rel_head->rel_entry); kfree(rel_head); return -ENOMEM; } @@ -735,7 +725,7 @@ static int add_relocation_to_accumulate(struct module *me, int type, } /* Add relocation to head of discovered rel_head */ - list_add_tail(&entry->head, rel_head->rel_entry); + list_add_tail(&entry->head, &rel_head->rel_entry); return 0; }