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 <[email protected]>
---
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
* Dan Carpenter <[email protected]> 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 <[email protected]>
> ---
> 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
The following commit has been merged into the perf/core branch of tip:
Commit-ID: 7543365739a4ff61d40ad53ab68c17d2e7dfb0c9
Gitweb: https://git.kernel.org/tip/7543365739a4ff61d40ad53ab68c17d2e7dfb0c9
Author: Dan Carpenter <[email protected]>
AuthorDate: Fri, 13 Oct 2023 10:18:12 +03:00
Committer: Ingo Molnar <[email protected]>
CommitterDate: Fri, 13 Oct 2023 09:32:50 +02:00
perf/x86/amd/uncore: Fix uninitialized return value in amd_uncore_init()
Some of the error paths in this function return don't initialize the
error code. Return -ENODEV by default.
Fixes: d6389d3ccc13 ("perf/x86/amd/uncore: Refactor uncore management")
Signed-off-by: Dan Carpenter <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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 9b444ce..a389828 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)
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 <[email protected]>
> ---
> 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:
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
* Sandipan Das <[email protected]> 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 <[email protected]>
> > ---
> > 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
* Dan Carpenter <[email protected]> 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
On 10/13/2023 2:33 PM, Ingo Molnar wrote:
>
> * Sandipan Das <[email protected]> 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 <[email protected]>
>>> ---
>>> 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.
On Fri, Oct 13, 2023 at 11:06 AM Ingo Molnar <[email protected]> wrote:
>
>
> * Dan Carpenter <[email protected]> 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.
* Uros Bizjak <[email protected]> wrote:
> On Fri, Oct 13, 2023 at 11:06 AM Ingo Molnar <[email protected]> wrote:
> >
> >
> > * Dan Carpenter <[email protected]> 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
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
* Dan Carpenter <[email protected]> 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