diff mbox series

[v2,4/4] riscv: Convert relocation iterator to do-while

Message ID 20240103-module_loading_fix-v2-4-292b160552c9@rivosinc.com
State New
Headers
Series riscv: modules: Fix module loading error handling |

Commit Message

Charlie Jenkins Jan. 3, 2024, 8:22 p.m. UTC 
  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(-)

Comments

Dan Carpenter Jan. 4, 2024, 12:35 p.m. UTC | #1 
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
Charlie Jenkins Jan. 4, 2024, 7:36 p.m. UTC | #2 
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 mbox series

Patch

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);