linux-kernel - Re: [PATCH v2] arm64/cache: fix -Woverride-init compiler warnings

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]

Message-ID: <20190807105307.GB54191@lakrids.cambridge.arm.com>
Date:   Wed, 7 Aug 2019 11:53:07 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Qian Cai <cai@....pw>
Cc:     will@...nel.org, catalin.marinas@....com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] arm64/cache: fix -Woverride-init compiler warnings

On Tue, Aug 06, 2019 at 03:34:34PM -0400, Qian Cai wrote:
> The commit 155433cb365e ("arm64: cache: Remove support for ASID-tagged
> VIVT I-caches") introduced some compiation warnings from GCC (and
> Clang),
> 
> arch/arm64/kernel/cpuinfo.c:38:26: warning: initialized field
> overwritten [-Woverride-init]
>  [ICACHE_POLICY_VIPT]  = "VIPT",
>                          ^~~~~~
> arch/arm64/kernel/cpuinfo.c:38:26: note: (near initialization for
> 'icache_policy_str[2]')
> arch/arm64/kernel/cpuinfo.c:39:26: warning: initialized field
> overwritten [-Woverride-init]
>  [ICACHE_POLICY_PIPT]  = "PIPT",
>                          ^~~~~~
> arch/arm64/kernel/cpuinfo.c:39:26: note: (near initialization for
> 'icache_policy_str[3]')
> arch/arm64/kernel/cpuinfo.c:40:27: warning: initialized field
> overwritten [-Woverride-init]
>  [ICACHE_POLICY_VPIPT]  = "VPIPT",
>                           ^~~~~~~
> arch/arm64/kernel/cpuinfo.c:40:27: note: (near initialization for
> 'icache_policy_str[0]')
> 
> because it initializes icache_policy_str[0 ... 3] twice. Since the array
> is only used in cpuinfo_detect_icache_policy(), fix it by initializing
> a specific field there just before using.
> 
> Fixes: 155433cb365e ("arm64: cache: Remove support for ASID-tagged VIVT I-caches")
> Signed-off-by: Qian Cai <cai@....pw>

Rather than trying to "fix" correct code like this (and making it harder
to read), could you instead look into where/whether the warning is
actually useful?

I had a look at an arm64 defconfig, where I see:

[mark@...rids:~/src/linux]% grep override-init err.log | grep -o '^[^[:space:]:]\+' | sort | uniq -c | sort -rn
    434 arch/arm64/kernel/sys32.c			// all benign
    291 arch/arm64/kernel/sys.c				// all benign
     48 ./arch/arm64/include/asm/perf_event.h		// all benign
     37 arch/arm64/kernel/traps.c			// all benign
     21 arch/arm64/kvm/handle_exit.c			// all benign
     12 drivers/ata/ahci.h				// all benign
      6 arch/arm64/kernel/perf_event.c			// all benign
      4 kernel/time/hrtimer.c				// all benign
      3 arch/arm64/kernel/cpuinfo.c			// all benign
      2 drivers/pinctrl/tegra/pinctrl-tegra194.c	// unclear to me
      1 ./include/linux/blkdev.h			// all benign
      1 drivers/gpu/drm/sun4i/sun4i_drv.c		// all benign
      1 drivers/ata/sata_sil24.c			// all benign
      1 ./arch/arm64/include/asm/mmu.h			// all benign

... so that's 862 warnings where at least 860 are unhelpful (and I
suspect those last two are also fine, but I haven't untangled the set of
macros).

Given that, what's the point in enabling this warning? It forces us to
write worse code that's harder to maintain, and it doesn't spot anything
useful.

> ---
> 
> v2: Initialize a specific field in cpuinfo_detect_icache_policy().
> 
>  arch/arm64/kernel/cpuinfo.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> index 876055e37352..a0c495a3f4fd 100644
> --- a/arch/arm64/kernel/cpuinfo.c
> +++ b/arch/arm64/kernel/cpuinfo.c
> @@ -34,10 +34,7 @@ DEFINE_PER_CPU(struct cpuinfo_arm64, cpu_data);
>  static struct cpuinfo_arm64 boot_cpu_data;
>  
>  static char *icache_policy_str[] = {
> -	[0 ... ICACHE_POLICY_PIPT]	= "RESERVED/UNKNOWN",
> -	[ICACHE_POLICY_VIPT]		= "VIPT",
> -	[ICACHE_POLICY_PIPT]		= "PIPT",
> -	[ICACHE_POLICY_VPIPT]		= "VPIPT",
> +	[0 ... ICACHE_POLICY_PIPT]	= "RESERVED/UNKNOWN"
>  };
>  
>  unsigned long __icache_flags;
> @@ -310,13 +307,16 @@ static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info)
>  
>  	switch (l1ip) {
>  	case ICACHE_POLICY_PIPT:
> +		icache_policy_str[ICACHE_POLICY_PIPT] = "PIPT";
>  		break;
>  	case ICACHE_POLICY_VPIPT:
> +		icache_policy_str[ICACHE_POLICY_VPIPT] = "VPIPT";
>  		set_bit(ICACHEF_VPIPT, &__icache_flags);
>  		break;
>  	default:
>  		/* Fallthrough */
>  	case ICACHE_POLICY_VIPT:
> +		icache_policy_str[ICACHE_POLICY_VIPT] = "VIPT";
>  		/* Assume aliasing */
>  		set_bit(ICACHEF_ALIASING, &__icache_flags);

NAK to this. Please leave this code as-is.

Thanks,
Mark.

>  	}
> -- 
> 2.20.1 (Apple Git-117)
>