[PATCH] perf/x86/amd/uncore: fix error codes in amd_uncore_init()

[PATCH] perf/x86/amd/uncore: fix error codes in amd_uncore_init()

[Dan Carpenter]

Some of the error paths in this function return don't initialize the
error code.  Return -ENODEV.

Fixes: d6389d3ccc13 ("perf/x86/amd/uncore: Refactor uncore management")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 arch/x86/events/amd/uncore.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
index 9b444ce24108..a389828f378c 100644
--- a/arch/x86/events/amd/uncore.c
+++ b/arch/x86/events/amd/uncore.c
@@ -1009,7 +1009,8 @@ static struct amd_uncore uncores[UNCORE_TYPE_MAX] = {
 static int __init amd_uncore_init(void)
 {
 	struct amd_uncore *uncore;
-	int ret, i;
+	int ret = -ENODEV;
+	int i;
 
 	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD &&
 	    boot_cpu_data.x86_vendor != X86_VENDOR_HYGON)
-- 
2.39.2

Re: [PATCH] perf/x86/amd/uncore: fix error codes in amd_uncore_init()

[Ingo Molnar]

* Dan Carpenter <dan.carpenter@linaro.org> wrote:

> Some of the error paths in this function return don't initialize the
> error code.  Return -ENODEV.
> 
> Fixes: d6389d3ccc13 ("perf/x86/amd/uncore: Refactor uncore management")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  arch/x86/events/amd/uncore.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
> index 9b444ce24108..a389828f378c 100644
> --- a/arch/x86/events/amd/uncore.c
> +++ b/arch/x86/events/amd/uncore.c
> @@ -1009,7 +1009,8 @@ static struct amd_uncore uncores[UNCORE_TYPE_MAX] = {
>  static int __init amd_uncore_init(void)
>  {
>  	struct amd_uncore *uncore;
> -	int ret, i;
> +	int ret = -ENODEV;
> +	int i;
>  
>  	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD &&
>  	    boot_cpu_data.x86_vendor != X86_VENDOR_HYGON)

Ugh, why on Earth didn't GCC warn about this? The bad pattern is pretty 
simple & obvious once pointed out ... compilers should have no trouble 
realizing that 'ret' is returned uninitialized in some of these control 
paths. Yet not a peep from the compiler ...

Thanks for the fix!

	Ingo

Re: [PATCH] perf/x86/amd/uncore: fix error codes in amd_uncore_init()

[Dan Carpenter]

On Fri, Oct 13, 2023 at 09:30:46AM +0200, Ingo Molnar wrote:
> Ugh, why on Earth didn't GCC warn about this? The bad pattern is pretty 
> simple & obvious once pointed out ... compilers should have no trouble 
> realizing that 'ret' is returned uninitialized in some of these control 
> paths. Yet not a peep from the compiler ...

We disabled that warning years ago (5?) because GCC had too many false
positives.

regards,
dan carpenter

Re: [PATCH] perf/x86/amd/uncore: fix error codes in amd_uncore_init()

[Ingo Molnar]

* Dan Carpenter <dan.carpenter@linaro.org> wrote:

> On Fri, Oct 13, 2023 at 09:30:46AM +0200, Ingo Molnar wrote:
>
> > Ugh, why on Earth didn't GCC warn about this? The bad pattern is pretty 
> > simple & obvious once pointed out ... compilers should have no trouble 
> > realizing that 'ret' is returned uninitialized in some of these control 
> > paths. Yet not a peep from the compiler ...
> 
> We disabled that warning years ago (5?) because GCC had too many false 
> positives.

GCC had some pretty bogus notions about 'possible' uninitialized use that 
encouraged some bad code patterns, but in this case there's readily 
provable uninitialized use, that a compiler should warn about.

Is it possible to disable just the unreliable, probabilistic part of GCC's 
uninitialized variables warnings?

Thanks,

	Ingo

Re: [PATCH] perf/x86/amd/uncore: fix error codes in amd_uncore_init()

[Uros Bizjak]

On Fri, Oct 13, 2023 at 11:06 AM Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> > On Fri, Oct 13, 2023 at 09:30:46AM +0200, Ingo Molnar wrote:
> >
> > > Ugh, why on Earth didn't GCC warn about this? The bad pattern is pretty
> > > simple & obvious once pointed out ... compilers should have no trouble
> > > realizing that 'ret' is returned uninitialized in some of these control
> > > paths. Yet not a peep from the compiler ...
> >
> > We disabled that warning years ago (5?) because GCC had too many false
> > positives.
>
> GCC had some pretty bogus notions about 'possible' uninitialized use that
> encouraged some bad code patterns, but in this case there's readily
> provable uninitialized use, that a compiler should warn about.
>
> Is it possible to disable just the unreliable, probabilistic part of GCC's
> uninitialized variables warnings?

-Wno-maybe-uninitialized?

Uros.

Re: [PATCH] perf/x86/amd/uncore: fix error codes in amd_uncore_init()

[Ingo Molnar]

* Uros Bizjak <ubizjak@gmail.com> wrote:

> On Fri, Oct 13, 2023 at 11:06 AM Ingo Molnar <mingo@kernel.org> wrote:
> >
> >
> > * Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > > On Fri, Oct 13, 2023 at 09:30:46AM +0200, Ingo Molnar wrote:
> > >
> > > > Ugh, why on Earth didn't GCC warn about this? The bad pattern is pretty
> > > > simple & obvious once pointed out ... compilers should have no trouble
> > > > realizing that 'ret' is returned uninitialized in some of these control
> > > > paths. Yet not a peep from the compiler ...
> > >
> > > We disabled that warning years ago (5?) because GCC had too many false
> > > positives.
> >
> > GCC had some pretty bogus notions about 'possible' uninitialized use that
> > encouraged some bad code patterns, but in this case there's readily
> > provable uninitialized use, that a compiler should warn about.
> >
> > Is it possible to disable just the unreliable, probabilistic part of GCC's
> > uninitialized variables warnings?
> 
> -Wno-maybe-uninitialized?

No combination of the relevant compiler options appears to be able to get 
GCC to notice this bug.

On top of tip:master, the patch below produces no compiler warnings with 
GCC 12.3.0:

  $ git revert 7543365739a4
  $ rm -f arch/x86/events/amd/uncore.o
  $ make V=1 W=1e arch/x86/events/amd/uncore.o

The "W=1e" incantation activates, with the patch below applied, among many 
other GCC options, the following options:

  -Wall
  -Wuninitialized
  -Wmaybe-uninitialized
  -Werror 

Which should have found this bug, right?

[ The V=1 helps double checking the compiler options. ]

Thanks,

	Ingo

 scripts/Makefile.extrawarn | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 2fe6f2828d37..9d245fcff419 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -108,6 +108,8 @@ KBUILD_CFLAGS += $(call cc-option, -Wformat-overflow)
 KBUILD_CFLAGS += $(call cc-option, -Wformat-truncation)
 KBUILD_CFLAGS += $(call cc-option, -Wstringop-overflow)
 KBUILD_CFLAGS += $(call cc-option, -Wstringop-truncation)
+KBUILD_CFLAGS += $(call cc-option, -Wuninitialized)
+KBUILD_CFLAGS += $(call cc-option, -Wmaybe-uninitialized)
 
 KBUILD_CPPFLAGS += -Wundef
 KBUILD_CPPFLAGS += -DKBUILD_EXTRA_WARN1
@@ -176,7 +178,7 @@ KBUILD_CFLAGS += -Wno-shift-negative-value
 ifdef CONFIG_CC_IS_CLANG
 KBUILD_CFLAGS += -Wno-initializer-overrides
 else
-KBUILD_CFLAGS += -Wno-maybe-uninitialized
+#KBUILD_CFLAGS += -Wno-maybe-uninitialized
 endif
 
 endif

Re: [PATCH] perf/x86/amd/uncore: fix error codes in amd_uncore_init()

[Dan Carpenter]

The surprising thing is the Clang doesn't detect the bug either.  It's
strange.  (I found this bug with Smatch).

Also I notice that my Fixes tag wasn't correct either.  That patch did
have a missing error code bug, but "ret" was set to zero.  :/

regards,
dan carpenter

Re: [PATCH] perf/x86/amd/uncore: fix error codes in amd_uncore_init()

[Ingo Molnar]

* Dan Carpenter <dan.carpenter@linaro.org> wrote:

> The surprising thing is the Clang doesn't detect the bug either.  It's
> strange.  (I found this bug with Smatch).

Yeah, that's weird and kind of concerning. I don't think either compiler is 
able to see that the init function return values are always ignored. I had 
to dig into init/main.c to convince myself.

> Also I notice that my Fixes tag wasn't correct either.  That patch did 
> have a missing error code bug, but "ret" was set to zero.  :/

Yeah, so I left the Fixes tag out of the commit anyway, because this isn't 
really a fix that -stable should concern itself with. After the first 
commit it's not even a fix per se, but an improvement in the resolution & 
meaning of error codes or so.

Thanks,

	Ingo

Re: [PATCH] perf/x86/amd/uncore: fix error codes in amd_uncore_init()

[Sandipan Das]

On 10/13/2023 12:48 PM, Dan Carpenter wrote:
> Some of the error paths in this function return don't initialize the
> error code.  Return -ENODEV.
> 
> Fixes: d6389d3ccc13 ("perf/x86/amd/uncore: Refactor uncore management")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  arch/x86/events/amd/uncore.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
> index 9b444ce24108..a389828f378c 100644
> --- a/arch/x86/events/amd/uncore.c
> +++ b/arch/x86/events/amd/uncore.c
> @@ -1009,7 +1009,8 @@ static struct amd_uncore uncores[UNCORE_TYPE_MAX] = {
>  static int __init amd_uncore_init(void)
>  {
>  	struct amd_uncore *uncore;
> -	int ret, i;
> +	int ret = -ENODEV;
> +	int i;
>  
>  	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD &&
>  	    boot_cpu_data.x86_vendor != X86_VENDOR_HYGON)


Thanks for catching this. I see that 'ret' remains uninitialized for cases
where the hotplug callback registration fails and was thinking if the
following is a better fix for this as the reason might not be ENODEV.

diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
index 91f01d6c8f7d..7d768dd01522 100644
--- a/arch/x86/events/amd/uncore.c
+++ b/arch/x86/events/amd/uncore.c
@@ -1039,20 +1039,25 @@ static int __init amd_uncore_init(void)
        /*
         * Install callbacks. Core will call them for each online cpu.
         */
-       if (cpuhp_setup_state(CPUHP_PERF_X86_AMD_UNCORE_PREP,
-                             "perf/x86/amd/uncore:prepare",
-                             NULL, amd_uncore_cpu_dead))
+       ret = cpuhp_setup_state(CPUHP_PERF_X86_AMD_UNCORE_PREP,
+                               "perf/x86/amd/uncore:prepare",
+                               NULL, amd_uncore_cpu_dead);
+       if (ret)
                goto fail;

-       if (cpuhp_setup_state(CPUHP_AP_PERF_X86_AMD_UNCORE_STARTING,
-                             "perf/x86/amd/uncore:starting",
-                             amd_uncore_cpu_starting, NULL))
+       ret = cpuhp_setup_state(CPUHP_AP_PERF_X86_AMD_UNCORE_STARTING,
+                               "perf/x86/amd/uncore:starting",
+                               amd_uncore_cpu_starting, NULL);
+       if (ret)
                goto fail_prep;
-       if (cpuhp_setup_state(CPUHP_AP_PERF_X86_AMD_UNCORE_ONLINE,
-                             "perf/x86/amd/uncore:online",
-                             amd_uncore_cpu_online,
-                             amd_uncore_cpu_down_prepare))
+
+       ret = cpuhp_setup_state(CPUHP_AP_PERF_X86_AMD_UNCORE_ONLINE,
+                               "perf/x86/amd/uncore:online",
+                               amd_uncore_cpu_online,
+                               amd_uncore_cpu_down_prepare);
+       if (ret)
                goto fail_start;
+
        return 0;

 fail_start:
 

Re: [PATCH] perf/x86/amd/uncore: fix error codes in amd_uncore_init()

[Ingo Molnar]

* Sandipan Das <sandipan.das@amd.com> wrote:

> On 10/13/2023 12:48 PM, Dan Carpenter wrote:
> > Some of the error paths in this function return don't initialize the
> > error code.  Return -ENODEV.
> > 
> > Fixes: d6389d3ccc13 ("perf/x86/amd/uncore: Refactor uncore management")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> >  arch/x86/events/amd/uncore.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
> > index 9b444ce24108..a389828f378c 100644
> > --- a/arch/x86/events/amd/uncore.c
> > +++ b/arch/x86/events/amd/uncore.c
> > @@ -1009,7 +1009,8 @@ static struct amd_uncore uncores[UNCORE_TYPE_MAX] = {
> >  static int __init amd_uncore_init(void)
> >  {
> >  	struct amd_uncore *uncore;
> > -	int ret, i;
> > +	int ret = -ENODEV;
> > +	int i;
> >  
> >  	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD &&
> >  	    boot_cpu_data.x86_vendor != X86_VENDOR_HYGON)
> 
> 
> Thanks for catching this. I see that 'ret' remains uninitialized for cases
> where the hotplug callback registration fails and was thinking if the
> following is a better fix for this as the reason might not be ENODEV.

Yeah, passing through the real error codes is usually better.

Here's it's probably a bit academic, as I don't think we are even using the 
init return code in the init sequence iterator, see how the return code by 
do_one_initcall() gets ignored by do_initcall_level() & do_pre_smp_initcalls() ...

Nevertheless, mind submitting this as a separate patch?

Thanks,

	Ingo

Re: [PATCH] perf/x86/amd/uncore: fix error codes in amd_uncore_init()

[Sandipan Das]

On 10/13/2023 2:33 PM, Ingo Molnar wrote:
> 
> * Sandipan Das <sandipan.das@amd.com> wrote:
> 
>> On 10/13/2023 12:48 PM, Dan Carpenter wrote:
>>> Some of the error paths in this function return don't initialize the
>>> error code.  Return -ENODEV.
>>>
>>> Fixes: d6389d3ccc13 ("perf/x86/amd/uncore: Refactor uncore management")
>>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>>> ---
>>>  arch/x86/events/amd/uncore.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
>>> index 9b444ce24108..a389828f378c 100644
>>> --- a/arch/x86/events/amd/uncore.c
>>> +++ b/arch/x86/events/amd/uncore.c
>>> @@ -1009,7 +1009,8 @@ static struct amd_uncore uncores[UNCORE_TYPE_MAX] = {
>>>  static int __init amd_uncore_init(void)
>>>  {
>>>  	struct amd_uncore *uncore;
>>> -	int ret, i;
>>> +	int ret = -ENODEV;
>>> +	int i;
>>>  
>>>  	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD &&
>>>  	    boot_cpu_data.x86_vendor != X86_VENDOR_HYGON)
>>
>>
>> Thanks for catching this. I see that 'ret' remains uninitialized for cases
>> where the hotplug callback registration fails and was thinking if the
>> following is a better fix for this as the reason might not be ENODEV.
> 
> Yeah, passing through the real error codes is usually better.
> 
> Here's it's probably a bit academic, as I don't think we are even using the 
> init return code in the init sequence iterator, see how the return code by 
> do_one_initcall() gets ignored by do_initcall_level() & do_pre_smp_initcalls() ...
> 
> Nevertheless, mind submitting this as a separate patch?
> 

Sure. Will do.