[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

[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

[kernel test robot]

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@intel.com/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@intel.com/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@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407291815.gJBST0P3-lkp@intel.com/

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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Borislav Petkov]

+ 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@intel.com/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@intel.com/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@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202407291815.gJBST0P3-lkp@intel.com/
> 
> 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.

And that case is handled properly in this function.

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

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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@intel.com/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@intel.com/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@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202407291815.gJBST0P3-lkp@intel.com/
> > 
> > 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

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

[Borislav Petkov]

On Mon, Jul 29, 2024 at 08:30:08AM -0700, Nathan Chancellor wrote:
> It's not behind W=1, this happens in a normal build:

"reproduce (this is a W=1 build):"

I guess that mail formulation is misleading.

Ok, lemme fix that then.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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 Mon, Jul 29, 2024 at 06:50:36PM +0200, Borislav Petkov wrote:
> On Mon, Jul 29, 2024 at 08:30:08AM -0700, Nathan Chancellor wrote:
> > It's not behind W=1, this happens in a normal build:
> 
> "reproduce (this is a W=1 build):"
> 
> I guess that mail formulation is misleading.

Yeah, I can definitely see how that can be misleading. It might be nice
if the robot could do a regular and a W=1 build at the same time to
report which warnings show up at which levels. For the record/future,
-Wsometimes-uninitialized and -Wuninitialized from clang should always
appear in a normal build; if they don't, that's a bug.

> Ok, lemme fix that then.

Thanks a lot :)

Cheers,
Nathan

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

[Borislav Petkov]

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?

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

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

Anyway:

From: "Borislav Petkov (AMD)" <bp@alien8.de>
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@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202407291815.gJBST0P3-lkp@intel.com/
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
---
 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

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@alien8.de>
> 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@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202407291815.gJBST0P3-lkp@intel.com/
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>

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

> ---
>  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