Received-SPF: pass (google.com: domain of linux-kernel+bounces-16644-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 4 Jan 2024 15:35:55 +0300
From: Dan Carpenter <dan.carpenter@linaro.org>
To: Charlie Jenkins <charlie@rivosinc.com>
Cc: Paul Walmsley <paul.walmsley@sifive.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	Palmer Dabbelt <palmer@rivosinc.com>,
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
	kernel test robot <lkp@intel.com>,
	Dan Carpenter <error27@gmail.com>
Subject: Re: [PATCH v2 4/4] riscv: Convert relocation iterator to do-while
Message-ID: <39727434-a265-484b-9ee6-da6611487b47@moroto.mountain>
References: <20240103-module_loading_fix-v2-0-292b160552c9@rivosinc.com>
 <20240103-module_loading_fix-v2-4-292b160552c9@rivosinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240103-module_loading_fix-v2-4-292b160552c9@rivosinc.com>

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