* Ingo Molnar <[email protected]> wrote:
> So it's all highly inefficient and fragile.
>
> There's also another cost, the cost of finding the bugs themselves - for
> example
> here's a recent upstream kernel fix:
>
> commit e01d8718de4170373cd7fbf5cf6f9cb61cebb1e9
> Author: Peter Zijlstra <[email protected]>
> Date: Wed Jan 27 23:24:29 2016 +0100
>
> perf/x86: Fix uninitialized value usage
>
> When calling intel_alt_er() with .idx != EXTRA_REG_RSP_* we will not
> initialize alt_idx and then use this uninitialized value to index an
> array.
>
> When that is not fatal, it can result in an infinite loop in its
> caller __intel_shared_reg_get_constraints(), with IRQs disabled.
>
> Alternative error modes are random memory corruption due to the
> cpuc->shared_regs->regs[] array overrun, which manifest in either
> get_constraints or put_constraints doing weird stuff.
>
> Only took 6 hours of painful debugging to find this. Neither GCC nor
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Smatch warnings flagged this bug.
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> --- a/arch/x86/kernel/cpu/perf_event_intel.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel.c
> @@ -1960,7 +1960,8 @@ intel_bts_constraints(struct perf_event *event)
>
> static int intel_alt_er(int idx, u64 config)
> {
> - int alt_idx;
> + int alt_idx = idx;
> +
> if (!(x86_pmu.flags & PMU_FL_HAS_RSP_1))
> return idx;
>
> 6 hours of PeterZ time translates to quite a bit of code restructuring
> overhead to
> eliminate false positive warnings...
Btw., here's the wider context of that bug:
static int intel_alt_er(int idx, u64 config)
{
int alt_idx;
if (!(x86_pmu.flags & PMU_FL_HAS_RSP_1))
return idx;
if (idx == EXTRA_REG_RSP_0)
alt_idx = EXTRA_REG_RSP_1;
if (idx == EXTRA_REG_RSP_1)
alt_idx = EXTRA_REG_RSP_0;
if (config & ~x86_pmu.extra_regs[alt_idx].valid_mask)
return idx;
return alt_idx;
}
so it's a straightforward uninitialized variable bug.
I tried to distill a testcase out of it, and the following silly hack seems to
trigger it:
------------------------------->
#include <stdio.h>
#define PMU_FL_HAS_RSP_1 1
#define EXTRA_REG_RSP_1 2
#define EXTRA_REG_RSP_0 4
int global_flags = -1;
static int intel_alt_er(int idx, unsigned long long config)
{
int alt_idx;
int uninitialized = 1;
printf("idx: %d, config: %Ld\n", idx, config);
if (!(global_flags & PMU_FL_HAS_RSP_1))
return idx;
if (idx == EXTRA_REG_RSP_0) {
alt_idx = EXTRA_REG_RSP_1;
uninitialized = 0;
}
if (idx == EXTRA_REG_RSP_1) {
alt_idx = EXTRA_REG_RSP_0;
uninitialized = 0;
}
if (config & ~0xff)
return idx;
if (uninitialized)
printf("ugh, using uninitialized alt_idx (%d)!\n", alt_idx);
return alt_idx;
}
int main(int argc, char **argv)
{
argv++;
return intel_alt_er(argc, argc);
}
<-------------------------------
built with:
gcc -Wbad-function-cast -Wdeclaration-after-statement -Wformat-security
-Wformat-y2k \
-Winit-self -Wmissing-declarations -Wmissing-prototypes -Wnested-externs \
-Wno-system-headers -Wold-style-definition -Wpacked -Wredundant-decls \
-Wshadow -Wstrict-aliasing=3 -Wstrict-prototypes -Wswitch-default
-Wswitch-enum \
-Wundef -Wwrite-strings -Wformat \
-Werror -O6 -fno-omit-frame-pointer -ggdb3 -funwind-tables -Wall -Wextra
-std=gnu99 -fstack-protector-all -D_FORTIFY_SOURCE=2 \
-o uninitialized uninitialized.c
gives:
triton:~> ./uninitialized 1
idx: 2, config: 2
triton:~> ./uninitialized 0 0
idx: 3, config: 3
ugh, using uninitialized alt_idx (2)!
I.e. I cannot get GCC to warn about this seemingly trivial bug, using:
gcc version 5.2.1 20151010 (Ubuntu 5.2.1-22ubuntu2)
Thanks,
Ingo
