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]

Hi Boris,

On Mon, Jul 29, 2024 at 01:26:14PM +0200, Borislav Petkov wrote:
> + Nathan.
> 
> On Mon, Jul 29, 2024 at 07:04:51PM +0800, kernel test robot wrote:
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/microcode
> > head:   94838d230a6c835ced1bad06b8759e0a5f19c1d3
> > commit: 94838d230a6c835ced1bad06b8759e0a5f19c1d3 [1/1] x86/microcode/AMD: Use the family,model,stepping encoded in the patch ID
> > config: i386-buildonly-randconfig-001-20240729 (https://download.01.org/0day-ci/archive/20240729/202407291815.gJBST0P3-lkp@xxxxxxxxx/config)
> > compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240729/202407291815.gJBST0P3-lkp@xxxxxxxxx/reproduce)
> > 
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202407291815.gJBST0P3-lkp@xxxxxxxxx/
> > 
> > All warnings (new ones prefixed by >>):
> > 
> > >> arch/x86/kernel/cpu/microcode/amd.c:714:6: warning: variable 'equiv_id' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
> >      714 |         if (x86_family(bsp_cpuid_1_eax) < 0x17) {
> >          |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >    arch/x86/kernel/cpu/microcode/amd.c:720:31: note: uninitialized use occurs here
> >      720 |         return cache_find_patch(uci, equiv_id);
> >          |                                      ^~~~~~~~
> >    arch/x86/kernel/cpu/microcode/amd.c:714:2: note: remove the 'if' if its condition is always true
> >      714 |         if (x86_family(bsp_cpuid_1_eax) < 0x17) {
> >          |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >    arch/x86/kernel/cpu/microcode/amd.c:706:14: note: initialize the variable 'equiv_id' to silence this warning
> >      706 |         u16 equiv_id;
> >          |                     ^
> >          |                      = 0
> >    1 warning generated.
> > 
> > 
> > vim +714 arch/x86/kernel/cpu/microcode/amd.c
> > 
> >    701	
> >    702	static struct ucode_patch *find_patch(unsigned int cpu)
> >    703	{
> >    704		struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
> >    705		u32 rev, dummy __always_unused;
> >    706		u16 equiv_id;
> >    707	
> >    708		/* fetch rev if not populated yet: */
> >    709		if (!uci->cpu_sig.rev) {
> >    710			rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
> >    711			uci->cpu_sig.rev = rev;
> >    712		}
> >    713	
> >  > 714		if (x86_family(bsp_cpuid_1_eax) < 0x17) {
> >    715			equiv_id = find_equiv_id(&equiv_table, uci->cpu_sig.sig);
> >    716			if (!equiv_id)
> >    717				return NULL;
> >    718		}
> >    719	
> >    720		return cache_find_patch(uci, equiv_id);
> >    721	}
> >    722	
> 
> That's a false positive, I think.
> 
> clang can't see that equiv_id is used in cache_find_patch() ->
> patch_cpus_equivalent() *only* in the
> 
> 	if (x86_family(bsp_cpuid_1_eax) < 0x17)
> 
> case.

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").

> And that case is handled properly in this function.

While it may be properly handled now, I think this pattern of
conditionally avoiding using a variable uninitialized is dubious.

> So, unless I'm missing something else, it's a good thing this warning is
> behind a W=1. Keep it there.

It's not behind W=1, this happens in a normal build:

  $ make -skj"$(nproc)" ARCH=x86_64 LLVM=1 mrproper defconfig arch/x86/kernel/cpu/microcode/amd.o
  arch/x86/kernel/cpu/microcode/amd.c:714:6: error: variable 'equiv_id' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
    714 |         if (x86_family(bsp_cpuid_1_eax) < 0x17) {
        |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  arch/x86/kernel/cpu/microcode/amd.c:720:31: note: uninitialized use occurs here
    720 |         return cache_find_patch(uci, equiv_id);
        |                                      ^~~~~~~~
  arch/x86/kernel/cpu/microcode/amd.c:714:2: note: remove the 'if' if its condition is always true
    714 |         if (x86_family(bsp_cpuid_1_eax) < 0x17) {
        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  arch/x86/kernel/cpu/microcode/amd.c:706:14: note: initialize the variable 'equiv_id' to silence this warning
    706 |         u16 equiv_id;
        |                     ^
        |                      = 0
  1 error generated.

Even GCC would warn about this code if not for the commit I mentioned
above:

  $ make -skj"$(nproc)" ARCH=x86_64 CROSS_COMPILE=x86_64-linux- KCFLAGS=-Wmaybe-uninitialized mrproper defconfig arch/x86/kernel/cpu/microcode/amd.o
  In function 'cache_find_patch',
      inlined from 'find_patch' at arch/x86/kernel/cpu/microcode/amd.c:720:9:
  arch/x86/kernel/cpu/microcode/amd.c:647:20: error: 'equiv_id' may be used uninitialized [-Werror=maybe-uninitialized]
    647 |                 if (patch_cpus_equivalent(p, &n))
        |                    ^
  arch/x86/kernel/cpu/microcode/amd.c: In function 'find_patch':
  arch/x86/kernel/cpu/microcode/amd.c:706:13: note: 'equiv_id' was declared here
    706 |         u16 equiv_id;
        |             ^~~~~~~~
  cc1: all warnings being treated as errors

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.

Cheers,
Nathan