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

Re: [PATCH V10 01/18] perf/core: Use static_call to optimize perf_guest_info_callbacks

On 8/27/2021 3:59 AM, Sean Christopherson wrote:
TL;DR: Please don't merge this patch, it's broken and is also built on a shoddy
        foundation that I would like to fix.
Hi Sean,Peter, Paolo

I will send out an V11 which drops this patch since it's buggy, and Sean is working on fix this.
Does this sound good?

Zhu Lingshan

On Fri, Aug 06, 2021, Zhu Lingshan wrote:
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 464917096e73..e466fc8176e1 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6489,9 +6489,18 @@ static void perf_pending_event(struct irq_work *entry)
  struct perf_guest_info_callbacks *perf_guest_cbs;
+/* explicitly use __weak to fix duplicate symbol error */
+void __weak arch_perf_update_guest_cbs(void)
  int perf_register_guest_info_callbacks(struct perf_guest_info_callbacks *cbs)
+       if (WARN_ON_ONCE(perf_guest_cbs))
+               return -EBUSY;
        perf_guest_cbs = cbs;
+       arch_perf_update_guest_cbs();
This is horribly broken, it fails to cleanup the static calls when KVM 
the callbacks, which happens when the vendor module, e.g. kvm_intel, is 
The explosion doesn't happen until 'kvm' is unloaded because the functions are
implemented in 'kvm', i.e. the use-after-free is deferred a bit.

   BUG: unable to handle page fault for address: ffffffffa011bb90
   #PF: supervisor instruction fetch in kernel mode
   #PF: error_code(0x0010) - not-present page
   PGD 6211067 P4D 6211067 PUD 6212063 PMD 102b99067 PTE 0
   Oops: 0010 [#1] PREEMPT SMP
   CPU: 0 PID: 1047 Comm: rmmod Not tainted 5.14.0-rc2+ #460
   Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
   RIP: 0010:0xffffffffa011bb90
   Code: Unable to access opcode bytes at RIP 0xffffffffa011bb66.
   Call Trace:
    ? perf_misc_flags+0xe/0x50
    ? perf_prepare_sample+0x53/0x6b0
    ? perf_event_output_forward+0x67/0x160
    ? kvm_clock_read+0x14/0x30
    ? kvm_sched_clock_read+0x5/0x10
    ? sched_clock_cpu+0xd/0xd0
    ? __perf_event_overflow+0x52/0xf0
    ? handle_pmi_common+0x1f2/0x2d0
    ? __flush_tlb_all+0x30/0x30
    ? intel_pmu_handle_irq+0xcf/0x410
    ? nmi_handle+0x5/0x260
    ? perf_event_nmi_handler+0x28/0x50
    ? nmi_handle+0xc7/0x260
    ? lock_release+0x2b0/0x2b0
    ? default_do_nmi+0x6b/0x170
    ? exc_nmi+0x103/0x130
    ? end_repeat_nmi+0x16/0x1f
    ? lock_release+0x2b0/0x2b0
    ? lock_release+0x2b0/0x2b0
    ? lock_release+0x2b0/0x2b0
   Modules linked in: irqbypass [last unloaded: kvm]

Even more fun, the existing perf_guest_cbs framework is also broken, though it's
much harder to get it to fail, and probably impossible to get it to fail without
some help.  The issue is that perf_guest_cbs is global, which means that it can
be nullified by KVM (during module unload) while the callbacks are being 
by a PMI handler on a different CPU.

The bug has escaped notice because all dererfences of perf_guest_cbs follow the
same "perf_guest_cbs && perf_guest_cbs->is_in_guest()" pattern, and AFAICT the
compiler never reload perf_guest_cbs in this sequence.  The compiler does reload
perf_guest_cbs for any future dereferences, but the ->is_in_guest() guard all 
guarantees the PMI handler will win the race, e.g. to nullify perf_guest_cbs,
KVM has to completely exit the guest and teardown down all VMs before it can be

But with a help, e.g. RAED_ONCE(perf_guest_cbs), unloading kvm_intel can trigger
a NULL pointer derference, e.g. this tweak

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 1eb45139fcc6..202e5ad97f82 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -2954,7 +2954,7 @@ unsigned long perf_misc_flags(struct pt_regs *regs)
         int misc = 0;

-       if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
+       if (READ_ONCE(perf_guest_cbs) && 
READ_ONCE(perf_guest_cbs)->is_in_guest()) {
                 if (perf_guest_cbs->is_user_mode())
                         misc |= PERF_RECORD_MISC_GUEST_USER;

while spamming module load/unload leads to:

   BUG: kernel NULL pointer dereference, address: 0000000000000000
   #PF: supervisor read access in kernel mode
   #PF: error_code(0x0000) - not-present page
   PGD 0 P4D 0
   Oops: 0000 [#1] PREEMPT SMP
   CPU: 6 PID: 1825 Comm: stress Not tainted 5.14.0-rc2+ #459
   Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
   RIP: 0010:perf_misc_flags+0x1c/0x70
   Call Trace:

The good news is that I have a series that should fix both the existing NULL 
bug and mostly obviate the need for static calls.  The bad news is that my 
making perf_guest_cbs per-CPU, likely complicates turning these into static 
though I'm guessing it's still a solvable problem.

Tangentially related, IMO we should make architectures opt-in to getting
perf_guest_cbs and nuke all of the code in the below files.  Except for arm,
which recently lost KVM support, it's all a bunch of useless copy-paste code 
serves no purpose and just complicates cleanups like this.

  arch/arm/kernel/perf_callchain.c   | 16 +++++++-----
  arch/csky/kernel/perf_callchain.c  |  4 +--
  arch/nds32/kernel/perf_event_cpu.c | 16 +++++++-----
  arch/riscv/kernel/perf_callchain.c |  4 +--



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.