Re: [PATCH stable 4.9] perf/x86/intel/rapl: Make package handling more robust

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Sorry, this is a backport for stable 4.9, Greg please consider to
apply to 4.9, this fix NULL pointer deref during cpu hot-add in debian
9 VM.

[  270.876510] CPU10 has been hot-added
[  270.891832] smpboot: Booting Node 0 Processor 10 APIC 0xa
[  270.887997] kvm-clock: cpu 10, msr 1:3fff0281, secondary cpu clock
[  270.990904] TSC synchronization [CPU#4 -> CPU#10]:
[  270.990906] Measured 713133966959 cycles TSC warp between CPUs,
turning off TSC clock.
[  270.990910] tsc: Marking TSC unstable due to check_tsc_sync_source failed
[  270.990978] BUG: unable to handle kernel NULL pointer dereference
at 0000000000000008
[  270.991503] IP: [<ffffffffc050f6dd>] rapl_cpu_online+0x5d/0x69
[intel_rapl_perf]
[  270.992036] PGD 0
[  270.992047]
[  270.992366] Oops: 0002 [#1] SMP
[  270.992782] Modules linked in: qxl ttm drm_kms_helper drm kvm_intel
kvm irqbypass crct10dif_pclmul crc32_pclmul hid_generic joydev
ghash_clmulni_intel serio_raw pcspkr intel_rapl_perf ppdev evdev
parport_pc parport usbhid hid acpi_cpufreq button ip_tables x_tables
autofs4 ext4 crc16 jbd2 crc32c_generic fscrypto ecb mbcache
ata_generic virtio_net virtio_blk crc32c_intel aesni_intel aes_x86_64
glue_helper lrw gf128mul ablk_helper cryptd ata_piix libata uhci_hcd
ehci_hcd psmouse virtio_pci virtio_ring usbcore virtio i2c_piix4
scsi_mod usb_common floppy
[  270.995252] CPU: 10 PID: 628 Comm: cpuhp/10 Not tainted
4.9.0-3-amd64 #1 Debian 4.9.30-2+deb9u2
[  270.995650] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS ?-20160812_114322-buildbot 04/01/2014
[  270.996184] task: ffff9d5b73e6d140 task.stack: ffffb7f600f58000
[  270.996538] RIP: 0010:[<ffffffffc050f6dd>]  [<ffffffffc050f6dd>]
rapl_cpu_online+0x5d/0x69 [intel_rapl_perf]
[  270.997131] RSP: 0000:ffffb7f600f5bde8  EFLAGS: 00010206
[  270.997491] RAX: 0000000000000200 RBX: 000000000000000a RCX: 0000000000000000
[  270.998091] RDX: 0000000000000200 RSI: 0000000000000200 RDI: 0000000000000200
[  270.998459] RBP: 000000000000000a R08: fffffffffffffe00 R09: 0000000000000155
[  270.999064] R10: 00000000000001b6 R11: 000000000047f4d8 R12: 0000000000000000
[  270.999438] R13: ffffffffc050f680 R14: 0000000000000000 R15: 0000000000000000
[  271.000043] FS:  0000000000000000(0000) GS:ffff9d5b7a880000(0000)
knlGS:0000000000000000
[  271.000421] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  271.001052] CR2: 0000000000000008 CR3: 000000006fc07000 CR4: 00000000001406e0
[  271.001431] Stack:
[  271.002212]  000000000000000a 000000000000006a 0000000000000062
ffffffff95278008
[  271.002927]  ffff9d5b7a88dae0 ffffffff95e33ad0 000000017a618240
ffff9d5b73e6d140
[  271.003546]  ffff9d5b7a88dae0 000000000000000a 0000000000000062
0000000000000097
[  271.004393] Call Trace:
[  271.005200]  [<ffffffff95278008>] ? cpuhp_invoke_callback+0x78/0x3a0
[  271.006015]  [<ffffffff9527bb0a>] ? do_group_exit+0x3a/0xa0
[  271.006584]  [<ffffffff95278470>] ? cpuhp_up_callbacks+0x30/0xb0
[  271.007339]  [<ffffffff95299d30>] ? sort_range+0x20/0x20
[  271.008143]  [<ffffffff95278674>] ? cpuhp_thread_fun+0xf4/0x100
[  271.008937]  [<ffffffff95299e2e>] ? smpboot_thread_fn+0xfe/0x150
[  271.009742]  [<ffffffff952965d7>] ? kthread+0xd7/0xf0
[  271.010272]  [<ffffffff95296500>] ? kthread_park+0x60/0x60
[  271.011012]  [<ffffffff958064f5>] ? ret_from_fork+0x25/0x30
[  271.011496] Code: 24 00 00 4c 8b a4 d0 10 01 00 00 48 c7 c2 80 a0
00 00 48 01 ca e8 44 8b 01 d5 3b 05 92 40 a0 d5 7c 0e f0 48 0f ab 2d
c3 23 00 00 <41> 89 6c 24 08 5b 31 c0 5d 41 5c c3 0f 1f 44 00 00 53 31
db 48
[  271.012776] RIP  [<ffffffffc050f6dd>] rapl_cpu_online+0x5d/0x69
[intel_rapl_perf]
[  271.013301]  RSP <ffffb7f600f5bde8>
[  271.013794] CR2: 0000000000000008

similar bug was reported in :
https://lkml.org/lkml/2016/10/20/745
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=851577

On Tue, Aug 22, 2017 at 4:52 PM, Jack Wang <jinpu.wang@xxxxxxxxxxxxxxxx> wrote:
> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>
> commit dd86e373e09fb16b83e8adf5c48c421a4ca76468 upstream
>
> The package management code in RAPL relies on package mapping being
> available before a CPU is started. This changed with:
>
>   9d85eb9119f4 ("x86/smpboot: Make logical package management more robust")
>
> because the ACPI/BIOS information turned out to be unreliable, but that
> left RAPL in broken state. This was not noticed because on a regular boot
> all CPUs are online before RAPL is initialized.
>
> A possible fix would be to reintroduce the mess which allocates a package
> data structure in CPU prepare and when it turns out to already exist in
> starting throw it away later in the CPU online callback. But that's a
> horrible hack and not required at all because RAPL becomes functional for
> perf only in the CPU online callback. That's correct because user space is
> not yet informed about the CPU being onlined, so nothing caan rely on RAPL
> being available on that particular CPU.
>
> Move the allocation to the CPU online callback and simplify the hotplug
> handling. At this point the package mapping is established and correct.
>
> This also adds a missing check for available package data in the
> event_init() function.
>
> Reported-by: Yasuaki Ishimatsu <yasu.isimatu@xxxxxxxxx>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Sebastian Siewior <bigeasy@xxxxxxxxxxxxx>
> Cc: Stephane Eranian <eranian@xxxxxxxxxx>
> Cc: Vince Weaver <vincent.weaver@xxxxxxxxx>
> Fixes: 9d85eb9119f4 ("x86/smpboot: Make logical package management more robust")
> Link: http://lkml.kernel.org/r/20170131230141.212593966@xxxxxxxxxxxxx
> Signed-off-by: Ingo Molnar <mingo@xxxxxxxxxx>
> [ jwang: backport to 4.9 fix Null pointer deref during hotplug cpu.]
> Signed-off-by: Jack Wang <jinpu.wang@xxxxxxxxxxxxxxxx>
> ---
>  arch/x86/events/intel/rapl.c | 58 +++++++++++++++++++-------------------------
>  include/linux/cpuhotplug.h   |  1 -
>  2 files changed, 25 insertions(+), 34 deletions(-)
>
> diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c
> index 970c1de..4c1b7ea 100644
> --- a/arch/x86/events/intel/rapl.c
> +++ b/arch/x86/events/intel/rapl.c
> @@ -161,7 +161,13 @@ static u64 rapl_timer_ms;
>
>  static inline struct rapl_pmu *cpu_to_rapl_pmu(unsigned int cpu)
>  {
> -       return rapl_pmus->pmus[topology_logical_package_id(cpu)];
> +       unsigned int pkgid = topology_logical_package_id(cpu);
> +
> +       /*
> +        * The unsigned check also catches the '-1' return value for non
> +        * existent mappings in the topology map.
> +        */
> +       return pkgid < rapl_pmus->maxpkg ? rapl_pmus->pmus[pkgid] : NULL;
>  }
>
>  static inline u64 rapl_read_counter(struct perf_event *event)
> @@ -402,6 +408,8 @@ static int rapl_pmu_event_init(struct perf_event *event)
>
>         /* must be done before validate_group */
>         pmu = cpu_to_rapl_pmu(event->cpu);
> +       if (!pmu)
> +               return -EINVAL;
>         event->cpu = pmu->cpu;
>         event->pmu_private = pmu;
>         event->hw.event_base = msr;
> @@ -585,6 +593,19 @@ static int rapl_cpu_online(unsigned int cpu)
>         struct rapl_pmu *pmu = cpu_to_rapl_pmu(cpu);
>         int target;
>
> +       if (!pmu) {
> +               pmu = kzalloc_node(sizeof(*pmu), GFP_KERNEL, cpu_to_node(cpu));
> +               if (!pmu)
> +                       return -ENOMEM;
> +
> +               raw_spin_lock_init(&pmu->lock);
> +               INIT_LIST_HEAD(&pmu->active_list);
> +               pmu->pmu = &rapl_pmus->pmu;
> +               pmu->timer_interval = ms_to_ktime(rapl_timer_ms);
> +               rapl_hrtimer_init(pmu);
> +
> +               rapl_pmus->pmus[topology_logical_package_id(cpu)] = pmu;
> +       }
>         /*
>          * Check if there is an online cpu in the package which collects rapl
>          * events already.
> @@ -598,27 +619,6 @@ static int rapl_cpu_online(unsigned int cpu)
>         return 0;
>  }
>
> -static int rapl_cpu_prepare(unsigned int cpu)
> -{
> -       struct rapl_pmu *pmu = cpu_to_rapl_pmu(cpu);
> -
> -       if (pmu)
> -               return 0;
> -
> -       pmu = kzalloc_node(sizeof(*pmu), GFP_KERNEL, cpu_to_node(cpu));
> -       if (!pmu)
> -               return -ENOMEM;
> -
> -       raw_spin_lock_init(&pmu->lock);
> -       INIT_LIST_HEAD(&pmu->active_list);
> -       pmu->pmu = &rapl_pmus->pmu;
> -       pmu->timer_interval = ms_to_ktime(rapl_timer_ms);
> -       pmu->cpu = -1;
> -       rapl_hrtimer_init(pmu);
> -       rapl_pmus->pmus[topology_logical_package_id(cpu)] = pmu;
> -       return 0;
> -}
> -
>  static int rapl_check_hw_unit(bool apply_quirk)
>  {
>         u64 msr_rapl_power_unit_bits;
> @@ -804,28 +804,21 @@ static int __init rapl_pmu_init(void)
>          * Install callbacks. Core will call them for each online cpu.
>          */
>
> -       ret = cpuhp_setup_state(CPUHP_PERF_X86_RAPL_PREP, "PERF_X86_RAPL_PREP",
> -                               rapl_cpu_prepare, NULL);
> -       if (ret)
> -               goto out;
> -
>         ret = cpuhp_setup_state(CPUHP_AP_PERF_X86_RAPL_ONLINE,
>                                 "AP_PERF_X86_RAPL_ONLINE",
>                                 rapl_cpu_online, rapl_cpu_offline);
>         if (ret)
> -               goto out1;
> +               goto out;
>
>         ret = perf_pmu_register(&rapl_pmus->pmu, "power", -1);
>         if (ret)
> -               goto out2;
> +               goto out1;
>
>         rapl_advertise();
>         return 0;
>
> -out2:
> -       cpuhp_remove_state(CPUHP_AP_PERF_X86_RAPL_ONLINE);
>  out1:
> -       cpuhp_remove_state(CPUHP_PERF_X86_RAPL_PREP);
> +       cpuhp_remove_state(CPUHP_AP_PERF_X86_RAPL_ONLINE);
>  out:
>         pr_warn("Initialization failed (%d), disabled\n", ret);
>         cleanup_rapl_pmus();
> @@ -836,7 +829,6 @@ module_init(rapl_pmu_init);
>  static void __exit intel_rapl_exit(void)
>  {
>         cpuhp_remove_state_nocalls(CPUHP_AP_PERF_X86_RAPL_ONLINE);
> -       cpuhp_remove_state_nocalls(CPUHP_PERF_X86_RAPL_PREP);
>         perf_pmu_unregister(&rapl_pmus->pmu);
>         cleanup_rapl_pmus();
>  }
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index ba1cad7..965cc56 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -10,7 +10,6 @@ enum cpuhp_state {
>         CPUHP_PERF_X86_PREPARE,
>         CPUHP_PERF_X86_UNCORE_PREP,
>         CPUHP_PERF_X86_AMD_UNCORE_PREP,
> -       CPUHP_PERF_X86_RAPL_PREP,
>         CPUHP_PERF_BFIN,
>         CPUHP_PERF_POWER,
>         CPUHP_PERF_SUPERH,
> --
> 2.7.4
>



-- 
Jack Wang
Linux Kernel Developer

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin

Tel:       +49 30 577 008  042
Fax:      +49 30 577 008 299
Email:    jinpu.wang@xxxxxxxxxxxxxxxx
URL:      https://www.profitbricks.de

Sitz der Gesellschaft: Berlin
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B
Geschäftsführer: Achim Weiss




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]