Local variable 'i' is referenced only when CONFIG_MEMORY_HOTREMOVE and
CONFIG_ACPI are defined, but definition of variable 'i' is out of guard.
If any of the two macros is undefined, below warning triggers during
build, fix it by moving 'i' in the guard.
arch/x86/boot/compressed/kaslr.c:698:6: warning: unused variable ‘i’ [-Wunused-variable]
Also use true/false instead of 1/0 for boolean return.
Fixes: 690eaa532057 ("x86/boot/KASLR: Limit KASLR to extract the kernel in immovable memory only")
Signed-off-by: Zhenzhong Duan <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Chao Fan <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
v2: update description per Boris.
arch/x86/boot/compressed/kaslr.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index d7408af55738..fff24a55bfd5 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -695,7 +695,6 @@ static bool process_mem_region(struct mem_vector *region,
unsigned long long minimum,
unsigned long long image_size)
{
- int i;
/*
* If no immovable memory found, or MEMORY_HOTREMOVE disabled,
* use @region directly.
@@ -705,12 +704,13 @@ static bool process_mem_region(struct mem_vector *region,
if (slot_area_index == MAX_SLOT_AREA) {
debug_putstr("Aborted e820/efi memmap scan (slot_areas full)!\n");
- return 1;
+ return true;
}
- return 0;
+ return false;
}
#if defined(CONFIG_MEMORY_HOTREMOVE) && defined(CONFIG_ACPI)
+ int i;
/*
* If immovable memory found, filter the intersection between
* immovable memory and @region.
@@ -734,11 +734,11 @@ static bool process_mem_region(struct mem_vector *region,
if (slot_area_index == MAX_SLOT_AREA) {
debug_putstr("Aborted e820/efi memmap scan when walking immovable regions(slot_areas full)!\n");
- return 1;
+ return true;
}
}
#endif
- return 0;
+ return false;
}
#ifdef CONFIG_EFI
--
2.17.1
On Fri, Jan 03, 2020 at 11:39:29AM +0800, Zhenzhong Duan wrote:
> Local variable 'i' is referenced only when CONFIG_MEMORY_HOTREMOVE and
> CONFIG_ACPI are defined, but definition of variable 'i' is out of guard.
> If any of the two macros is undefined, below warning triggers during
> build, fix it by moving 'i' in the guard.
>
> arch/x86/boot/compressed/kaslr.c:698:6: warning: unused variable ‘i’ [-Wunused-variable]
How do you trigger this?
I have:
$ grep -E "(CONFIG_MEMORY_HOTREMOVE|CONFIG_ACPI)" .config
# CONFIG_ACPI is not set
but no warning. Neither with gcc 8 nor with gcc 9.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, Jan 09, 2020 at 07:40:55PM +0100, Borislav Petkov wrote:
> On Fri, Jan 03, 2020 at 11:39:29AM +0800, Zhenzhong Duan wrote:
> > Local variable 'i' is referenced only when CONFIG_MEMORY_HOTREMOVE and
> > CONFIG_ACPI are defined, but definition of variable 'i' is out of guard.
> > If any of the two macros is undefined, below warning triggers during
> > build, fix it by moving 'i' in the guard.
> >
> > arch/x86/boot/compressed/kaslr.c:698:6: warning: unused variable ‘i’ [-Wunused-variable]
>
> How do you trigger this?
>
> I have:
>
> $ grep -E "(CONFIG_MEMORY_HOTREMOVE|CONFIG_ACPI)" .config
> # CONFIG_ACPI is not set
>
> but no warning. Neither with gcc 8 nor with gcc 9.
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
The boot/compressed Makefile resets KBUILD_CFLAGS. Following hack and
building with W=1 shows it, or just add -Wunused in there.
diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index 56aa5fa0a66b..791c0d5a952a 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -35,6 +35,9 @@ KBUILD_CFLAGS += $(cflags-y)
KBUILD_CFLAGS += -mno-mmx -mno-sse
KBUILD_CFLAGS += $(call cc-option,-ffreestanding)
KBUILD_CFLAGS += $(call cc-option,-fno-stack-protector)
+
+include scripts/Makefile.extrawarn
+
KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
KBUILD_CFLAGS += -Wno-pointer-sign
Drop [email protected] from Cc because it bounces.
On Thu, Jan 09, 2020 at 03:46:41PM -0500, Arvind Sankar wrote:
> The boot/compressed Makefile resets KBUILD_CFLAGS. Following hack and
> building with W=1 shows it, or just add -Wunused in there.
I'm interested in how he reproduced it on the stock tree, without
additional hacks or changes.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Zhenzhong Duan <[email protected]> writes:
> Local variable 'i' is referenced only when CONFIG_MEMORY_HOTREMOVE and
> CONFIG_ACPI are defined, but definition of variable 'i' is out of guard.
> If any of the two macros is undefined, below warning triggers during
> build, fix it by moving 'i' in the guard.
>
> arch/x86/boot/compressed/kaslr.c:698:6: warning: unused variable ‘i’ [-Wunused-variable]
>
> Also use true/false instead of 1/0 for boolean return.
No. This is not the scope of the unused variable issue. This want's to
be a separate patch.
Thanks,
tglx
On Fri, Jan 10, 2020 at 4:50 AM Borislav Petkov <[email protected]> wrote:
>
> Drop [email protected] from Cc because it bounces.
>
> On Thu, Jan 09, 2020 at 03:46:41PM -0500, Arvind Sankar wrote:
> > The boot/compressed Makefile resets KBUILD_CFLAGS. Following hack and
> > building with W=1 shows it, or just add -Wunused in there.
>
> I'm interested in how he reproduced it on the stock tree, without
> additional hacks or changes.
I indeed used additional parameters as below for daily build.
# make O=/build/kernel/ -j4 EXTRA_CFLAGS=-Wall binrpm-pkg
Regards
Zhenzhong
On Fri, Jan 10, 2020 at 4:46 AM Arvind Sankar <[email protected]> wrote:
>
> On Thu, Jan 09, 2020 at 07:40:55PM +0100, Borislav Petkov wrote:
> > On Fri, Jan 03, 2020 at 11:39:29AM +0800, Zhenzhong Duan wrote:
> > > Local variable 'i' is referenced only when CONFIG_MEMORY_HOTREMOVE and
> > > CONFIG_ACPI are defined, but definition of variable 'i' is out of guard.
> > > If any of the two macros is undefined, below warning triggers during
> > > build, fix it by moving 'i' in the guard.
> > >
> > > arch/x86/boot/compressed/kaslr.c:698:6: warning: unused variable ‘i’ [-Wunused-variable]
> >
> > How do you trigger this?
> >
> > I have:
> >
> > $ grep -E "(CONFIG_MEMORY_HOTREMOVE|CONFIG_ACPI)" .config
> > # CONFIG_ACPI is not set
> >
> > but no warning. Neither with gcc 8 nor with gcc 9.
> >
> > --
> > Regards/Gruss,
> > Boris.
> >
> > https://people.kernel.org/tglx/notes-about-netiquette
>
> The boot/compressed Makefile resets KBUILD_CFLAGS. Following hack and
> building with W=1 shows it, or just add -Wunused in there.
>
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index 56aa5fa0a66b..791c0d5a952a 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -35,6 +35,9 @@ KBUILD_CFLAGS += $(cflags-y)
> KBUILD_CFLAGS += -mno-mmx -mno-sse
> KBUILD_CFLAGS += $(call cc-option,-ffreestanding)
> KBUILD_CFLAGS += $(call cc-option,-fno-stack-protector)
> +
> +include scripts/Makefile.extrawarn
> +
> KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
> KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
> KBUILD_CFLAGS += -Wno-pointer-sign
Yes. Will you send this formally? Not clear if there is other reason making
KBUILD_CFLAGS for arch/x86/boot/compressed different from other part.
Regards
Zhenzhong
On Fri, Jan 10, 2020 at 10:09:38AM +0800, Zhenzhong Duan wrote:
> I indeed used additional parameters as below for daily build.
> # make O=/build/kernel/ -j4 EXTRA_CFLAGS=-Wall binrpm-pkg
And in no point in time it did occur to you that you should mention this
important piece of information in your commit message so that a person
looking at the patch knows how you triggered it?!?
Geez.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Fri, Jan 10, 2020 at 4:27 PM Borislav Petkov <[email protected]> wrote:
>
> On Fri, Jan 10, 2020 at 10:09:38AM +0800, Zhenzhong Duan wrote:
> > I indeed used additional parameters as below for daily build.
> > # make O=/build/kernel/ -j4 EXTRA_CFLAGS=-Wall binrpm-pkg
>
> And in no point in time it did occur to you that you should mention this
> important piece of information in your commit message so that a person
> looking at the patch knows how you triggered it?!?
I'm sorry on that. Will do next time!
Regards
Zhenzhong
On Fri, Jan 10, 2020 at 5:00 AM Thomas Gleixner <[email protected]> wrote:
>
> Zhenzhong Duan <[email protected]> writes:
>
> > Local variable 'i' is referenced only when CONFIG_MEMORY_HOTREMOVE and
> > CONFIG_ACPI are defined, but definition of variable 'i' is out of guard.
> > If any of the two macros is undefined, below warning triggers during
> > build, fix it by moving 'i' in the guard.
> >
> > arch/x86/boot/compressed/kaslr.c:698:6: warning: unused variable ‘i’ [-Wunused-variable]
> >
> > Also use true/false instead of 1/0 for boolean return.
>
> No. This is not the scope of the unused variable issue. This want's to
> be a separate patch.
I'm trying to combine trivial changes into one, so you maintainers
don't mind to pick two trivial patches? :)
Regards
Zhenzhong
On Fri, Jan 10, 2020 at 10:27:02AM +0800, Zhenzhong Duan wrote:
> Yes. Will you send this formally?
And then a flood of fix-this-trivial-warning patches ensues?
You should know that they have the lowest prio when it comes to looking
at them.
> Not clear if there is other reason making KBUILD_CFLAGS for
> arch/x86/boot/compressed different from other part.
Maybe because the kernel proper build system should not break the
compressed kernel's build as the two are quite different... maybe for
historical raisins...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Fri, Jan 10, 2020 at 04:36:26PM +0800, Zhenzhong Duan wrote:
> I'm sorry on that. Will do next time!
Do that *every* time from now on!
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Zhenzhong Duan <[email protected]> writes:
> On Fri, Jan 10, 2020 at 5:00 AM Thomas Gleixner <[email protected]> wrote:
>>
>> Zhenzhong Duan <[email protected]> writes:
>>
>> > Local variable 'i' is referenced only when CONFIG_MEMORY_HOTREMOVE and
>> > CONFIG_ACPI are defined, but definition of variable 'i' is out of guard.
>> > If any of the two macros is undefined, below warning triggers during
>> > build, fix it by moving 'i' in the guard.
>> >
>> > arch/x86/boot/compressed/kaslr.c:698:6: warning: unused variable ‘i’ [-Wunused-variable]
>> >
>> > Also use true/false instead of 1/0 for boolean return.
>>
>> No. This is not the scope of the unused variable issue. This want's to
>> be a separate patch.
>
> I'm trying to combine trivial changes into one, so you maintainers
> don't mind to pick two trivial patches? :)
See Documentation/process/submitting-patches.rst:
Solve only one problem per patch.
Thanks,
tglx
On Fri, Jan 10, 2020 at 5:00 PM Borislav Petkov <[email protected]> wrote:
>
> On Fri, Jan 10, 2020 at 10:27:02AM +0800, Zhenzhong Duan wrote:
> > Yes. Will you send this formally?
>
> And then a flood of fix-this-trivial-warning patches ensues?
>
> You should know that they have the lowest prio when it comes to looking
> at them.
I see.
Just tried Arvind's patch, result is not that bad. Below are all
warnings during build:
In fact, only gop.c and kaslr.c have compile warning.
/root/kernel/drivers/firmware/efi/libstub/gop.c: In function ‘efi_setup_gop’:
/root/kernel/drivers/firmware/efi/libstub/gop.c:174:18: warning:
‘pixel_format’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
setup_pixel_info(si, pixels_per_scan_line, pixel_info, pixel_format);
^
/root/kernel/drivers/firmware/efi/libstub/gop.c:97:6: note:
‘pixel_format’ was declared here
int pixel_format;
^
/root/kernel/drivers/firmware/efi/libstub/gop.c:166:45: warning:
‘fb_base’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
ext_lfb_base = (u64)(unsigned long)fb_base >> 32;
^
/root/kernel/drivers/firmware/efi/libstub/gop.c:95:6: note: ‘fb_base’
was declared here
u64 fb_base;
^
/root/kernel/drivers/firmware/efi/libstub/gop.c:174:18: warning:
‘pixels_per_scan_line’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
setup_pixel_info(si, pixels_per_scan_line, pixel_info, pixel_format);
^
/root/kernel/drivers/firmware/efi/libstub/gop.c:93:6: note:
‘pixels_per_scan_line’ was declared here
u32 pixels_per_scan_line;
^
/root/kernel/drivers/firmware/efi/libstub/gop.c:163:17: warning:
‘height’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
si->lfb_height = height;
^
/root/kernel/drivers/firmware/efi/libstub/gop.c:92:13: note: ‘height’
was declared here
u16 width, height;
^
/root/kernel/drivers/firmware/efi/libstub/gop.c:162:16: warning:
‘width’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
si->lfb_width = width;
^
/root/kernel/drivers/firmware/efi/libstub/gop.c:92:6: note: ‘width’
was declared here
u16 width, height;
^
/root/kernel/drivers/firmware/efi/libstub/gop.c:271:18: warning:
‘pixel_format’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
setup_pixel_info(si, pixels_per_scan_line, pixel_info, pixel_format);
^
/root/kernel/drivers/firmware/efi/libstub/gop.c:194:6: note:
‘pixel_format’ was declared here
int pixel_format;
^
/root/kernel/drivers/firmware/efi/libstub/gop.c:263:45: warning:
‘fb_base’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
ext_lfb_base = (u64)(unsigned long)fb_base >> 32;
^
/root/kernel/drivers/firmware/efi/libstub/gop.c:192:6: note: ‘fb_base’
was declared here
u64 fb_base;
^
/root/kernel/drivers/firmware/efi/libstub/gop.c:271:18: warning:
‘pixels_per_scan_line’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
setup_pixel_info(si, pixels_per_scan_line, pixel_info, pixel_format);
^
/root/kernel/drivers/firmware/efi/libstub/gop.c:190:6: note:
‘pixels_per_scan_line’ was declared here
u32 pixels_per_scan_line;
^
/root/kernel/drivers/firmware/efi/libstub/gop.c:260:17: warning:
‘height’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
si->lfb_height = height;
^
/root/kernel/drivers/firmware/efi/libstub/gop.c:189:13: note: ‘height’
was declared here
u16 width, height;
^
/root/kernel/drivers/firmware/efi/libstub/gop.c:259:16: warning:
‘width’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
si->lfb_width = width;
^
/root/kernel/drivers/firmware/efi/libstub/gop.c:189:6: note: ‘width’
was declared here
u16 width, height;
/root/kernel/arch/x86/boot/compressed/kaslr.c: In function ‘process_mem_region’:
/root/kernel/arch/x86/boot/compressed/kaslr.c:698:6: warning: unused
variable ‘i’ [-Wunused-variable]
int i;
Regards
Zhenzhong
On Mon, Jan 13, 2020 at 10:43:14AM +0800, Zhenzhong Duan wrote:
> Just tried Arvind's patch, result is not that bad. Below are all
> warnings during build:
> In fact, only gop.c and kaslr.c have compile warning.
gop.c is not part of arch/x86/boot/compressed/
So I'd take a patch adding -Wunused to arch/x86/boot/compressed/Makefile
and fixing the single warning in kaslr.c
The extrawarn W=1 stuff gives a lot more (below) and there I guess I'd
take only well thought out patches, each fixing all -Wmissing-prototypes
in a single compilation unit.
In file included from arch/x86/boot/compressed/string.c:11:
arch/x86/boot/compressed/../string.c:43:5: warning: no previous prototype for ‘bcmp’ [-Wmissing-prototypes]
arch/x86/boot/compressed/../string.c:146:6: warning: no previous prototype for ‘simple_strtol’ [-Wmissing-prototypes]
arch/x86/boot/compressed/string.c:53:7: warning: no previous prototype for ‘memmove’ [-Wmissing-prototypes]
In file included from arch/x86/boot/compressed/string.c:11:
arch/x86/boot/compressed/../string.c:43:5: warning: no previous prototype for ‘bcmp’ [-Wmissing-prototypes]
arch/x86/boot/compressed/../string.c:146:6: warning: no previous prototype for ‘simple_strtol’ [-Wmissing-prototypes]
arch/x86/boot/compressed/string.c:53:7: warning: no previous prototype for ‘memmove’ [-Wmissing-prototypes]
In file included from arch/x86/boot/compressed/cmdline.c:14:
arch/x86/boot/compressed/../cmdline.c:28:5: warning: no previous prototype for ‘__cmdline_find_option’ [-Wmissing-prototypes]
arch/x86/boot/compressed/../cmdline.c:100:5: warning: no previous prototype for ‘__cmdline_find_option_bool’ [-Wmissing-prototypes]
arch/x86/boot/compressed/cmdline.c:15:15: warning: no previous prototype for ‘get_cmd_line_ptr’ [-Wmissing-prototypes]
arch/x86/boot/compressed/eboot.c:26:28: warning: no previous prototype for ‘efi_system_table’ [-Wmissing-prototypes]
arch/x86/boot/compressed/eboot.c:318:6: warning: no previous prototype for ‘setup_graphics’ [-Wmissing-prototypes]
arch/x86/boot/compressed/eboot.c:357:23: warning: no previous prototype for ‘efi_pe_entry’ [-Wmissing-prototypes]
arch/x86/boot/compressed/pgtable_64.c:110:22: warning: no previous prototype for ‘paging_prepare’ [-Wmissing-prototypes]
arch/x86/boot/compressed/pgtable_64.c:193:6: warning: no previous prototype for ‘cleanup_trampoline’ [-Wmissing-prototypes]
arch/x86/boot/compressed/eboot.c:711:21: warning: no previous prototype for ‘efi_main’ [-Wmissing-prototypes]
arch/x86/boot/compressed/misc.c:340:28: warning: no previous prototype for ‘extract_kernel’ [-Wmissing-prototypes]
...
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette