Re: [tip:x86/microcode 1/1] arch/x86/kernel/cpu/microcode/amd.c:714:6: warning: variable 'equiv_id' is used uninitialized whenever 'if' condition is false

[Nathan Chancellor]

On Tue, Jul 30, 2024 at 10:03:26AM +0200, Borislav Petkov wrote:
> On Mon, Jul 29, 2024 at 08:30:08AM -0700, Nathan Chancellor wrote:
> > Right, as clang does not perform interprocedural analysis for the sake
> > of warnings. That's partly why GCC's version of this warning was
> > disabled in commit 78a5255ffb6a ("Stop the ad-hoc games with
> > -Wno-maybe-initialized").
> 
> So why aren't we shutting up clang too?

Because it does not suffer from the same issues. There are many more
instances of local variables that are genuinely uninitialized when used
that get properly flagged than instances such as this where lack of
interprocedural analysis may cause emission of a false positive. Losing
Clang's version of the warning means that no compiler would warn on
instances of obviously uninitialized local variables, which still occurs
quite frequently.

> > While it may be properly handled now, I think this pattern of
> > conditionally avoiding using a variable uninitialized is dubious.
> 
> Well, in this particular case of the microcode loader, this is the best
> compromise I can think of - I have two different paths of handling a microcode
> patch - Zen and newer - and before - so I need those separate flows.
> 
> So sometimes it makes sense.

Sure, I just think it is a potential footgun waiting to happen but it's
obviously fine now.

> > So I guess you can just ignore this if you want but others (maybe even
> > Linus) will likely notice this and report it as well.
> 
> I think this warning should be behind W=1 if it can trigger false positives.

I disagree, any warning can trigger false positives, as they are
heuristics. Feel free to propose a patch if you feel strongly about it
though and we can bring the discussion to a wider audience.

> Anyway:
> 
> From: "Borislav Petkov (AMD)" <bp@xxxxxxxxx>
> Date: Tue, 30 Jul 2024 09:52:43 +0200
> Subject: [PATCH] x86/microcode/AMD: Fix a -Wsometimes-uninitialized clang
>  false positive
> 
> Initialize equiv_id in order to shut up:
> 
>   arch/x86/kernel/cpu/microcode/amd.c:714:6: warning: variable 'equiv_id' is \
>   used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
>         if (x86_family(bsp_cpuid_1_eax) < 0x17) {
>             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> because clang doesn't do interprocedural analysis for warnings to see
> that this variable won't be used uninitialized.
> 
> Fixes: 94838d230a6c ("x86/microcode/AMD: Use the family,model,stepping encoded in the patch ID")
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Closes: https://lore.kernel.org/oe-kbuild-all/202407291815.gJBST0P3-lkp@xxxxxxxxx/
> Signed-off-by: Borislav Petkov (AMD) <bp@xxxxxxxxx>

Reviewed-by: Nathan Chancellor <nathan@xxxxxxxxxx>

> ---
>  arch/x86/kernel/cpu/microcode/amd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index 1f5d36f92b8a..f63b051f25a0 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -703,7 +703,7 @@ static struct ucode_patch *find_patch(unsigned int cpu)
>  {
>  	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
>  	u32 rev, dummy __always_unused;
> -	u16 equiv_id;
> +	u16 equiv_id = 0;
>  
>  	/* fetch rev if not populated yet: */
>  	if (!uci->cpu_sig.rev) {
> -- 
> 2.43.0
> 
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette