linux-kernel - Re: [PATCH -next v2 0/2] Remove unused variables in x86/boot

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 27 Sep 2022 14:50:19 -0700
From:   Nathan Chancellor <nathan@...nel.org>
To:     Li Zetao <lizetao1@...wei.com>
Cc:     tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
        kirill.shutemov@...ux.intel.com, akpm@...ux-foundation.org,
        ndesaulniers@...gle.com, masahiroy@...nel.org,
        sathyanarayanan.kuppuswamy@...ux.intel.com, michael.roth@....com,
        brijesh.singh@....com, venu.busireddy@...cle.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH -next v2 0/2] Remove unused variables in x86/boot

Hi Li,

On Tue, Sep 27, 2022 at 08:15:10AM +0000, Li Zetao wrote:
> This patch set removes some unused variables in x86/boot, and add the
> "-Wall" flag to Makefile, which is the old problem of x86 not sharing
> makefiles.
> 
> Changes since v1:
> - Add "-Wall" flag to x86/boot/compressed/Makefile
> - Remove unused variables "et" in efi_get_system_table() and "ret" in
>   efi_get_conf_table()
> - Remove unused variables "ret" in __efi_get_rsdp_addr() and 
>   "nr_tables" in efi_get_rsdp_addr()
> 
> v1 at:
> https://lore.kernel.org/all/20220923113209.3046960-1-lizetao1@huawei.com/
> 
> Li Zetao (2):
>   x86/boot/compressed: Add "-Wall" flag to Makefile
>   x86/boot: Remove unused variables
> 
>  arch/x86/boot/compressed/Makefile | 2 +-
>  arch/x86/boot/compressed/acpi.c   | 2 --
>  arch/x86/boot/compressed/efi.c    | 2 --
>  arch/x86/boot/compressed/sev.c    | 1 -
>  4 files changed, 1 insertion(+), 6 deletions(-)

I took this series for a spin with clang and found a few extra warnings.

1.

  In file included from arch/x86/boot/compressed/misc.c:15:
  In file included from arch/x86/boot/compressed/misc.h:24:
  In file included from ./include/linux/elf.h:6:
  In file included from ./arch/x86/include/asm/elf.h:8:
  In file included from ./include/linux/thread_info.h:60:
  ./arch/x86/include/asm/thread_info.h:175:13: warning: calling '__builtin_frame_address' with a nonzero argument is unsafe [-Wframe-address]
          oldframe = __builtin_frame_address(1);
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~
  ./arch/x86/include/asm/thread_info.h:177:11: warning: calling '__builtin_frame_address' with a nonzero argument is unsafe [-Wframe-address]
                  frame = __builtin_frame_address(2);
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~

This warning is disabled in the main Makefile for this reason so we
should just be able to disable it:

diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index 10abb7c45d04..3f004567f3d5 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -43,6 +43,7 @@ KBUILD_CFLAGS += -mno-mmx -mno-sse
 KBUILD_CFLAGS += -ffreestanding -fshort-wchar
 KBUILD_CFLAGS += -fno-stack-protector
 KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
+KBUILD_CFLAGS += $(call cc-disable-warning, frame-address)
 KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
 KBUILD_CFLAGS += -Wno-pointer-sign
 KBUILD_CFLAGS += $(call cc-option,-fmacro-prefix-map=$(srctree)/=)

2.

  arch/x86/boot/compressed/kaslr.c:627:6: warning: unused variable 'i' [-Wunused-variable]
          int i;
              ^

This happens when CONFIG_MEMORY_HOTREMOVE or CONFIG_ACPI are 'n'. I
think it can just be fixed by aligning arch/x86/boot/compressed with the
rest of the kernel and explicitly compiling with '-std=gnu11', which
will allow us to declare the variable within the for loop, like so.

diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index 3f004567f3d5..6c7e366a437b 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -34,7 +34,7 @@ targets := vmlinux vmlinux.bin vmlinux.bin.gz vmlinux.bin.bz2 vmlinux.bin.lzma \
 # be valid.
 KBUILD_CFLAGS := -m$(BITS) -O2 $(CLANG_FLAGS)
 KBUILD_CFLAGS += -fno-strict-aliasing -fPIE
-KBUILD_CFLAGS += -Wundef -Wall
+KBUILD_CFLAGS += -Wundef -Wall -std=gnu11
 KBUILD_CFLAGS += -DDISABLE_BRANCH_PROFILING
 cflags-$(CONFIG_X86_32) := -march=i386
 cflags-$(CONFIG_X86_64) := -mcmodel=small -mno-red-zone
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 4a3f223973f4..be859c7e7f6b 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -624,7 +624,6 @@ static bool process_mem_region(struct mem_vector *region,
 			       unsigned long minimum,
 			       unsigned long image_size)
 {
-	int i;
 	/*
 	 * If no immovable memory found, or MEMORY_HOTREMOVE disabled,
 	 * use @region directly.
@@ -644,7 +643,7 @@ static bool process_mem_region(struct mem_vector *region,
 	 * If immovable memory found, filter the intersection between
 	 * immovable memory and @region.
 	 */
-	for (i = 0; i < num_immovable_mem; i++) {
+	for (int i = 0; i < num_immovable_mem; i++) {
 		u64 start, end, entry_end, region_end;
 		struct mem_vector entry;
 

Additionally, I think these two patches should be reordered so that the
warnings are fixed before they are enabled.

With those comments addressed, consider the series:

Reviewed-by: Nathan Chancellor <nathan@...nel.org>

Cheers,
Nathan