| Message ID | 20240103-module_loading_fix-v2-4-292b160552c9@rivosinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: modules: Fix module loading error handling | expand |
On Wed, Jan 03, 2024 at 12:22:03PM -0800, Charlie Jenkins wrote: > Use a do-while loop to iterate through relocation entries to prevent > curr_type from being marked as uninitialized. > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > Fixes: d8792a5734b0 ("riscv: Safely remove entries from relocation list") > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <error27@gmail.com> > Closes: https://lore.kernel.org/r/202312130859.wnkuzVWY-lkp@intel.com/ > --- > arch/riscv/kernel/module.c | 26 +++++++++++++++++++++----- > 1 file changed, 21 insertions(+), 5 deletions(-) > > diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c > index ceb0adb38715..581e425686ab 100644 > --- a/arch/riscv/kernel/module.c > +++ b/arch/riscv/kernel/module.c > @@ -633,15 +633,31 @@ process_accumulated_relocations(struct module *me, > bucket_iter->bucket, node) { > buffer = 0; > location = rel_head_iter->location; > - list_for_each_entry_safe(rel_entry_iter, > - rel_entry_iter_tmp, > - rel_head_iter->rel_entry, > - head) { > + rel_entry_iter = > + list_first_entry(rel_head_iter->rel_entry, > + typeof(*rel_entry_iter), head); > + rel_entry_iter_tmp = > + list_next_entry(rel_entry_iter, head); > + > + /* > + * Iterate through all relocation entries that share > + * this location. This uses a do-while loop instead of > + * list_for_each_entry_safe since it is known that there > + * is at least one entry and curr_type needs to be the > + * value of the last entry when the loop exits. > + */ I know that I reported this static checker and all, but actually after reading this comment, I think we should stay with original code. So long as we know the list has "least one entry" which we do then the original code worked fine. To be honest, I probably would not have even reported this static checker warning except that I saw there were some other issues and thought "Eh, why not throw this warning in as well, in case the list can be empty." The other three patches look good. regards, dan carpenter
On Thu, Jan 04, 2024 at 03:35:55PM +0300, Dan Carpenter wrote: > On Wed, Jan 03, 2024 at 12:22:03PM -0800, Charlie Jenkins wrote: > > Use a do-while loop to iterate through relocation entries to prevent > > curr_type from being marked as uninitialized. > > > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > > Fixes: d8792a5734b0 ("riscv: Safely remove entries from relocation list") > > Reported-by: kernel test robot <lkp@intel.com> > > Reported-by: Dan Carpenter <error27@gmail.com> > > Closes: https://lore.kernel.org/r/202312130859.wnkuzVWY-lkp@intel.com/ > > --- > > arch/riscv/kernel/module.c | 26 +++++++++++++++++++++----- > > 1 file changed, 21 insertions(+), 5 deletions(-) > > > > diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c > > index ceb0adb38715..581e425686ab 100644 > > --- a/arch/riscv/kernel/module.c > > +++ b/arch/riscv/kernel/module.c > > @@ -633,15 +633,31 @@ process_accumulated_relocations(struct module *me, > > bucket_iter->bucket, node) { > > buffer = 0; > > location = rel_head_iter->location; > > - list_for_each_entry_safe(rel_entry_iter, > > - rel_entry_iter_tmp, > > - rel_head_iter->rel_entry, > > - head) { > > + rel_entry_iter = > > + list_first_entry(rel_head_iter->rel_entry, > > + typeof(*rel_entry_iter), head); > > + rel_entry_iter_tmp = > > + list_next_entry(rel_entry_iter, head); > > + > > + /* > > + * Iterate through all relocation entries that share > > + * this location. This uses a do-while loop instead of > > + * list_for_each_entry_safe since it is known that there > > + * is at least one entry and curr_type needs to be the > > + * value of the last entry when the loop exits. > > + */ > > I know that I reported this static checker and all, but actually after > reading this comment, I think we should stay with original code. So > long as we know the list has "least one entry" which we do then the > original code worked fine. > > To be honest, I probably would not have even reported this static > checker warning except that I saw there were some other issues and > thought "Eh, why not throw this warning in as well, in case the list > can be empty." That makes sense, I will drop that patch. - Charlie > > The other three patches look good. > > regards, > dan carpenter >
diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c index ceb0adb38715..581e425686ab 100644 --- a/arch/riscv/kernel/module.c +++ b/arch/riscv/kernel/module.c @@ -633,15 +633,31 @@ process_accumulated_relocations(struct module *me, bucket_iter->bucket, node) { buffer = 0; location = rel_head_iter->location; - list_for_each_entry_safe(rel_entry_iter, - rel_entry_iter_tmp, - rel_head_iter->rel_entry, - head) { + rel_entry_iter = + list_first_entry(rel_head_iter->rel_entry, + typeof(*rel_entry_iter), head); + rel_entry_iter_tmp = + list_next_entry(rel_entry_iter, head); + + /* + * Iterate through all relocation entries that share + * this location. This uses a do-while loop instead of + * list_for_each_entry_safe since it is known that there + * is at least one entry and curr_type needs to be the + * value of the last entry when the loop exits. + */ + do { curr_type = rel_entry_iter->type; reloc_handlers[curr_type].reloc_handler( me, &buffer, rel_entry_iter->value); kfree(rel_entry_iter); - } + + rel_entry_iter = rel_entry_iter_tmp; + rel_entry_iter_tmp = list_next_entry(rel_entry_iter_tmp, head); + } while (!list_entry_is_head(rel_entry_iter, + rel_head_iter->rel_entry, + head)); + reloc_handlers[curr_type].accumulate_handler( me, location, buffer); kfree(rel_head_iter);
Use a do-while loop to iterate through relocation entries to prevent curr_type from being marked as uninitialized. Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> Fixes: d8792a5734b0 ("riscv: Safely remove entries from relocation list") Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <error27@gmail.com> Closes: https://lore.kernel.org/r/202312130859.wnkuzVWY-lkp@intel.com/ --- arch/riscv/kernel/module.c | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-)