Use a do-while loop to iterate through relocation entries to prevent
curr_type from being marked as uninitialized.
Signed-off-by: Charlie Jenkins <[email protected]>
Fixes: d8792a5734b0 ("riscv: Safely remove entries from relocation list")
Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
Closes: https://lore.kernel.org/r/[email protected]/
---
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.
+ */
+ 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);
--
2.43.0
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 <[email protected]>
> Fixes: d8792a5734b0 ("riscv: Safely remove entries from relocation list")
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
> Closes: https://lore.kernel.org/r/[email protected]/
> ---
> 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 <[email protected]>
> > Fixes: d8792a5734b0 ("riscv: Safely remove entries from relocation list")
> > Reported-by: kernel test robot <[email protected]>
> > Reported-by: Dan Carpenter <[email protected]>
> > Closes: https://lore.kernel.org/r/[email protected]/
> > ---
> > 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
>