From: Sean Christopherson <seanjc@google.com> Date: 2021-08-27 00:57:35
This started out as a small series[1] to fix a KVM bug related to Intel PT
interrupt handling and snowballed horribly.
The main problem being addressed is that the perf_guest_cbs are shared by
all CPUs, can be nullified by KVM during module unload, and are not
protected against concurrent access from NMI context.
The bug has escaped notice because all dereferences of perf_guest_cbs
follow the same "perf_guest_cbs && perf_guest_cbs->is_in_guest()" pattern,
and AFAICT the compiler never reloads perf_guest_cbs in this sequence.
The compiler does reload perf_guest_cbs for any future dereferences, but
the ->is_in_guest() guard all but 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 unloaded.
But with help, e.g. READ_ONCE(perf_guest_cbs), unloading kvm_intel can
trigger a NULL pointer derference (see below). Manual intervention aside,
the bug is a bit of a time bomb, e.g. my patch 3 from the original PT
handling series would have omitted the ->is_in_guest() guard.
This series fixes the problem by making the callbacks per-CPU, and
registering/unregistering the callbacks only with preemption disabled
(except for the Xen case, which doesn't unregister).
This approach also allows for several nice cleanups in this series.
KVM x86 and arm64 can share callbacks, KVM x86 drops its somewhat
redundant current_vcpu, and the retpoline that is currently hit when KVM
is loaded (due to always checking ->is_in_guest()) goes away (it's still
there when running as Xen Dom0).
Changing to per-CPU callbacks also provides a good excuse to excise
copy+paste code from architectures that can't possibly have guest
callbacks.
This series conflicts horribly with a proposed patch[2] to use static
calls for perf_guest_cbs. But that patch is broken as it completely
fails to handle unregister, and it's not clear to me whether or not
it can correctly handle unregister without fixing the underlying race
(I don't know enough about the code patching for static calls).
This tweak
From: Sean Christopherson <seanjc@google.com> Date: 2021-08-27 00:57:38
Wait to register perf callbacks until after doing vendor hardaware setup.
VMX's hardware_setup() configures Intel Processor Trace (PT) mode, and a
future fix to register the Intel PT guest interrupt hook if and only if
Intel PT is exposed to the guest will consume the configured PT mode.
Delaying registration to hardware setup is effectively a nop as KVM's perf
hooks all pivot on the per-CPU current_vcpu, which is non-NULL only when
KVM is handling an IRQ/NMI in a VM-Exit path. I.e. current_vcpu will be
NULL throughout both kvm_arch_init() and kvm_arch_hardware_setup().
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Artem Kashkanov <redacted>
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/x86.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
From: Sean Christopherson <seanjc@google.com> Date: 2021-08-27 00:57:41
Override the Processor Trace (PT) interrupt handler for guest mode if and
only if PT is configured for host+guest mode, i.e. is being used
independently by both host and guest. If PT is configured for system
mode, the host fully controls PT and must handle all events.
Fixes: 8479e04e7d6b ("KVM: x86: Inject PMI for KVM guest")
Cc: stable@vger.kernel.org
Cc: Like Xu <redacted>
Reported-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Reported-by: Artem Kashkanov <redacted>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/include/asm/kvm_host.h | 1 +
arch/x86/kvm/pmu.h | 1 +
arch/x86/kvm/vmx/vmx.c | 1 +
arch/x86/kvm/x86.c | 5 ++++-
4 files changed, 7 insertions(+), 1 deletion(-)
From: Sean Christopherson <seanjc@google.com> Date: 2021-08-27 00:57:46
Introduce HAVE_GUEST_PERF_EVENTS and require architectures to select it
to allow register guest callbacks in perf. Future patches will convert
the callbacks to per-CPU definitions. Rather than churn a bunch of arch
code (that was presumably copy+pasted from x86), remove it wholesale as
it's useless and at best wasting cycles.
Wrap even the stubs with an #ifdef to avoid an arch sneaking in a bogus
registration with CONFIG_PERF_EVENTS=n.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/arm/kernel/perf_callchain.c | 28 ++++------------------------
arch/arm64/Kconfig | 1 +
arch/csky/kernel/perf_callchain.c | 10 ----------
arch/nds32/kernel/perf_event_cpu.c | 29 ++++-------------------------
arch/riscv/kernel/perf_callchain.c | 10 ----------
arch/x86/Kconfig | 1 +
include/linux/perf_event.h | 4 ++++
init/Kconfig | 3 +++
kernel/events/core.c | 2 ++
9 files changed, 19 insertions(+), 69 deletions(-)
@@ -64,11 +64,6 @@ perf_callchain_user(struct perf_callchain_entry_ctx *entry, struct pt_regs *regs{structframe_tail__user*tail;-if(perf_guest_cbs&&perf_guest_cbs->is_in_guest()){-/* We don't support guest os callchain now */-return;-}-perf_callchain_store(entry,regs->ARM_pc);if(!current->mm)
@@ -100,20 +95,12 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *re{structstackframefr;-if(perf_guest_cbs&&perf_guest_cbs->is_in_guest()){-/* We don't support guest os callchain now */-return;-}-arm_get_current_stackframe(regs,&fr);walk_stackframe(&fr,callchain_trace,entry);}unsignedlongperf_instruction_pointer(structpt_regs*regs){-if(perf_guest_cbs&&perf_guest_cbs->is_in_guest())-returnperf_guest_cbs->get_guest_ip();-returninstruction_pointer(regs);}
@@ -121,17 +108,10 @@ unsigned long perf_misc_flags(struct pt_regs *regs){intmisc=0;-if(perf_guest_cbs&&perf_guest_cbs->is_in_guest()){-if(perf_guest_cbs->is_user_mode())-misc|=PERF_RECORD_MISC_GUEST_USER;-else-misc|=PERF_RECORD_MISC_GUEST_KERNEL;-}else{-if(user_mode(regs))-misc|=PERF_RECORD_MISC_USER;-else-misc|=PERF_RECORD_MISC_KERNEL;-}+if(user_mode(regs))+misc|=PERF_RECORD_MISC_USER;+else+misc|=PERF_RECORD_MISC_KERNEL;returnmisc;}
@@ -88,10 +88,6 @@ void perf_callchain_user(struct perf_callchain_entry_ctx *entry,{unsignedlongfp=0;-/* C-SKY does not support virtualization. */-if(perf_guest_cbs&&perf_guest_cbs->is_in_guest())-return;-fp=regs->regs[4];perf_callchain_store(entry,regs->pc);
@@ -112,12 +108,6 @@ void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,{structstackframefr;-/* C-SKY does not support virtualization. */-if(perf_guest_cbs&&perf_guest_cbs->is_in_guest()){-pr_warn("C-SKY does not support perf in guest mode!");-return;-}-fr.fp=regs->regs[4];fr.lr=regs->lr;walk_stackframe(&fr,entry);
@@ -1371,11 +1371,6 @@ perf_callchain_user(struct perf_callchain_entry_ctx *entry,leaf_fp=0;-if(perf_guest_cbs&&perf_guest_cbs->is_in_guest()){-/* We don't support guest os callchain now */-return;-}-perf_callchain_store(entry,regs->ipc);fp=regs->fp;gp=regs->gp;
@@ -1481,10 +1476,6 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,{structstackframefr;-if(perf_guest_cbs&&perf_guest_cbs->is_in_guest()){-/* We don't support guest os callchain now */-return;-}fr.fp=regs->fp;fr.lp=regs->lp;fr.sp=regs->sp;
@@ -1493,10 +1484,6 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,unsignedlongperf_instruction_pointer(structpt_regs*regs){-/* However, NDS32 does not support virtualization */-if(perf_guest_cbs&&perf_guest_cbs->is_in_guest())-returnperf_guest_cbs->get_guest_ip();-returninstruction_pointer(regs);}
@@ -1504,18 +1491,10 @@ unsigned long perf_misc_flags(struct pt_regs *regs){intmisc=0;-/* However, NDS32 does not support virtualization */-if(perf_guest_cbs&&perf_guest_cbs->is_in_guest()){-if(perf_guest_cbs->is_user_mode())-misc|=PERF_RECORD_MISC_GUEST_USER;-else-misc|=PERF_RECORD_MISC_GUEST_KERNEL;-}else{-if(user_mode(regs))-misc|=PERF_RECORD_MISC_USER;-else-misc|=PERF_RECORD_MISC_KERNEL;-}+if(user_mode(regs))+misc|=PERF_RECORD_MISC_USER;+else+misc|=PERF_RECORD_MISC_KERNEL;returnmisc;}
@@ -58,10 +58,6 @@ void perf_callchain_user(struct perf_callchain_entry_ctx *entry,{unsignedlongfp=0;-/* RISC-V does not support perf in guest mode. */-if(perf_guest_cbs&&perf_guest_cbs->is_in_guest())-return;-fp=regs->s0;perf_callchain_store(entry,regs->epc);
@@ -78,11 +74,5 @@ static bool fill_callchain(void *entry, unsigned long pc)voidperf_callchain_kernel(structperf_callchain_entry_ctx*entry,structpt_regs*regs){-/* RISC-V does not support perf in guest mode. */-if(perf_guest_cbs&&perf_guest_cbs->is_in_guest()){-pr_warn("RISC-V does not support perf in guest mode!");-return;-}-walk_stackframe(NULL,regs,fill_callchain,entry);}
From: Sean Christopherson <seanjc@google.com> Date: 2021-08-27 00:57:48
Drop the 'int' return value from the perf (un)register callbacks helpers
and stop pretending perf can support multiple callbacks. The 'int'
returns are not future proofing anything as none of the callers take
action on an error. It's also not obvious that there will ever be
cotenant hypervisors, and if there are, that allowing multiple callbacks
to be registered is desirable or even correct.
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/arm64/include/asm/kvm_host.h | 4 ++--
arch/arm64/kvm/perf.c | 8 ++++----
arch/x86/kvm/x86.c | 2 +-
include/linux/perf_event.h | 11 +++++------
kernel/events/core.c | 11 ++---------
5 files changed, 14 insertions(+), 22 deletions(-)
From: Sean Christopherson <seanjc@google.com> Date: 2021-08-27 00:57:51
Use a per-CPU pointer to track perf's guest callbacks so that KVM can set
the callbacks more precisely and avoid a lurking NULL pointer dereference.
On x86, KVM supports being built as a module and thus can be unloaded.
And because the shared callbacks are referenced from IRQ/NMI context,
unloading KVM can run concurrently with perf, and thus all of perf's
checks for a NULL perf_guest_cbs are flawed as perf_guest_cbs could be
nullified between the check and dereference.
In practice, this has not been problematic because the callbacks are
always guarded with a "perf_guest_cbs && perf_guest_cbs->is_in_guest()"
pattern, and it's extremely unlikely the compiler will choost to reload
perf_guest_cbs in that particular sequence. Because is_in_guest() is
obviously true only when KVM is running a guest, perf always wins the
race to the guarded code (which does often reload perf_guest_cbs) as KVM
has to stop running all guests and do a heavy teardown before unloading.
Cc: Zhu Lingshan <redacted>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/arm64/kernel/perf_callchain.c | 18 ++++++++++++------
arch/x86/events/core.c | 17 +++++++++++------
arch/x86/events/intel/core.c | 8 +++++---
include/linux/perf_event.h | 2 +-
kernel/events/core.c | 12 +++++++++---
5 files changed, 38 insertions(+), 19 deletions(-)
@@ -102,7 +102,9 @@ compat_user_backtrace(struct compat_frame_tail __user *tail,voidperf_callchain_user(structperf_callchain_entry_ctx*entry,structpt_regs*regs){-if(perf_guest_cbs&&perf_guest_cbs->is_in_guest()){+structperf_guest_info_callbacks*guest_cbs=this_cpu_read(perf_guest_cbs);++if(guest_cbs&&guest_cbs->is_in_guest()){/* We don't support guest os callchain now */return;}
@@ -147,9 +149,10 @@ static bool callchain_trace(void *data, unsigned long pc)voidperf_callchain_kernel(structperf_callchain_entry_ctx*entry,structpt_regs*regs){+structperf_guest_info_callbacks*guest_cbs=this_cpu_read(perf_guest_cbs);structstackframeframe;-if(perf_guest_cbs&&perf_guest_cbs->is_in_guest()){+if(guest_cbs&&guest_cbs->is_in_guest()){/* We don't support guest os callchain now */return;}
@@ -2761,10 +2761,11 @@ static bool perf_hw_regs(struct pt_regs *regs)voidperf_callchain_kernel(structperf_callchain_entry_ctx*entry,structpt_regs*regs){+structperf_guest_info_callbacks*guest_cbs=this_cpu_read(perf_guest_cbs);structunwind_statestate;unsignedlongaddr;-if(perf_guest_cbs&&perf_guest_cbs->is_in_guest()){+if(guest_cbs&&guest_cbs->is_in_guest()){/* TODO: We don't support guest os callchain now */return;}
@@ -2864,10 +2865,11 @@ perf_callchain_user32(struct pt_regs *regs, struct perf_callchain_entry_ctx *entvoidperf_callchain_user(structperf_callchain_entry_ctx*entry,structpt_regs*regs){+structperf_guest_info_callbacks*guest_cbs=this_cpu_read(perf_guest_cbs);structstack_frameframe;conststructstack_frame__user*fp;-if(perf_guest_cbs&&perf_guest_cbs->is_in_guest()){+if(guest_cbs&&guest_cbs->is_in_guest()){/* TODO: We don't support guest os callchain now */return;}
@@ -2944,18 +2946,21 @@ static unsigned long code_segment_base(struct pt_regs *regs)unsignedlongperf_instruction_pointer(structpt_regs*regs){-if(perf_guest_cbs&&perf_guest_cbs->is_in_guest())-returnperf_guest_cbs->get_guest_ip();+structperf_guest_info_callbacks*guest_cbs=this_cpu_read(perf_guest_cbs);++if(guest_cbs&&guest_cbs->is_in_guest())+returnguest_cbs->get_guest_ip();returnregs->ip+code_segment_base(regs);}unsignedlongperf_misc_flags(structpt_regs*regs){+structperf_guest_info_callbacks*guest_cbs=this_cpu_read(perf_guest_cbs);intmisc=0;-if(perf_guest_cbs&&perf_guest_cbs->is_in_guest()){-if(perf_guest_cbs->is_user_mode())+if(guest_cbs&&guest_cbs->is_in_guest()){+if(guest_cbs->is_user_mode())misc|=PERF_RECORD_MISC_GUEST_USER;elsemisc|=PERF_RECORD_MISC_GUEST_KERNEL;
From: Sean Christopherson <seanjc@google.com> Date: 2021-08-27 00:57:54
Register KVM's perf callback only when handling an interrupt that may be
a PMI (sadly this includes IRQs), and unregister the callback immediately
after handling the interrupt (or closing the window). Registering the
callback on a per-CPU basis (with preemption disabled!), fixes a mostly
theoretical bug where perf could dereference a NULL pointer due to KVM
unloading and unregistering the callbacks in between perf queries of the
callback functions. The precise registration will also allow for future
cleanups and optimizations, e.g. the existence of the callbacks can serve
as the "in guest" check.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/x86.c | 27 +++++++++++++++++----------
arch/x86/kvm/x86.h | 10 ++++++++++
include/linux/perf_event.h | 2 ++
kernel/events/core.c | 12 ++++++++++++
4 files changed, 41 insertions(+), 10 deletions(-)
From: Sean Christopherson <seanjc@google.com> Date: 2021-08-27 00:57:58
Add a dedicated flag to detect the case where KVM's PMC overflow
callback was originally invoked in response to an NMI that arrived while
the guest was running. Using current_vcpu is less precise as IRQs also
set current_vcpu (though presumably KVM's callback should not be reached
in that case), and more importantly, this will allow dropping
current_vcpu as the perf callbacks can switch to kvm_running_vcpu now
that the perf callbacks are precisely registered, i.e. kvm_running_vcpu
doesn't need to be used to detect if a PMI arrived in the guest.
Fixes: dd60d217062f ("KVM: x86: Fix perf timer mode IP reporting")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/include/asm/kvm_host.h | 3 +--
arch/x86/kvm/pmu.c | 2 +-
arch/x86/kvm/svm/svm.c | 2 +-
arch/x86/kvm/vmx/vmx.c | 2 +-
arch/x86/kvm/x86.c | 4 ++--
arch/x86/kvm/x86.h | 4 +++-
6 files changed, 9 insertions(+), 8 deletions(-)
@@ -763,6 +763,7 @@ struct kvm_vcpu_arch {unsignednmi_pending;/* NMI queued after currently running handler */boolnmi_injected;/* Trying to inject an NMI this entry */boolsmi_pending;/* SMI queued after currently running handler */+boolhandling_nmi_from_guest;structkvm_mtrrmtrr_state;u64pat;
@@ -1874,8 +1875,6 @@ int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu);intkvm_complete_insn_gp(structkvm_vcpu*vcpu,interr);void__kvm_request_immediate_exit(structkvm_vcpu*vcpu);-intkvm_is_in_guest(void);-void__user*__x86_set_memory_region(structkvm*kvm,intid,gpa_tgpa,u32size);boolkvm_vcpu_is_reset_bsp(structkvm_vcpu*vcpu);
From: Sean Christopherson <seanjc@google.com> Date: 2021-08-27 00:58:01
Now that KVM registers perf callbacks only when the CPU is "in guest",
use kvm_running_vcpu instead of current_vcpu to retrieve the associated
vCPU and drop current_vcpu.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/x86.c | 12 +++++-------
arch/x86/kvm/x86.h | 4 ----
2 files changed, 5 insertions(+), 11 deletions(-)
@@ -8264,17 +8264,15 @@ static void kvm_timer_init(void)kvmclock_cpu_online,kvmclock_cpu_down_prep);}-DEFINE_PER_CPU(structkvm_vcpu*,current_vcpu);-EXPORT_PER_CPU_SYMBOL_GPL(current_vcpu);-staticintkvm_is_in_guest(void){-return__this_cpu_read(current_vcpu)!=NULL;+/* x86's callbacks are registered only when handling a guest NMI. */+returntrue;}staticintkvm_is_user_mode(void){-structkvm_vcpu*vcpu=__this_cpu_read(current_vcpu);+structkvm_vcpu*vcpu=kvm_get_running_vcpu();if(WARN_ON_ONCE(!vcpu))return0;
@@ -8284,7 +8282,7 @@ static int kvm_is_user_mode(void)staticunsignedlongkvm_get_guest_ip(void){-structkvm_vcpu*vcpu=__this_cpu_read(current_vcpu);+structkvm_vcpu*vcpu=kvm_get_running_vcpu();if(WARN_ON_ONCE(!vcpu))return0;
@@ -8294,7 +8292,7 @@ static unsigned long kvm_get_guest_ip(void)staticvoidkvm_handle_intel_pt_intr(void){-structkvm_vcpu*vcpu=__this_cpu_read(current_vcpu);+structkvm_vcpu*vcpu=kvm_get_running_vcpu();if(WARN_ON_ONCE(!vcpu))return;
From: Sean Christopherson <seanjc@google.com> Date: 2021-08-27 00:58:19
Register/unregister perf callbacks at vcpu_load()/vcpu_put() instead of
keeping the callbacks registered for all eternity after loading KVM.
This will allow future cleanups and optimizations as the registration
of the callbacks signifies "in guest". This will also allow moving the
callbacks into common KVM as they arm64 and x86 now have semantically
identical callback implementations.
Note, KVM could likely be more precise in its registration, but that's a
cleanup for the future.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/arm64/include/asm/kvm_host.h | 12 ++++++++++-
arch/arm64/kvm/arm.c | 5 ++++-
arch/arm64/kvm/perf.c | 36 ++++++++++++++-----------------
3 files changed, 31 insertions(+), 22 deletions(-)
@@ -429,10 +429,13 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)if(vcpu_has_ptrauth(vcpu))vcpu_ptrauth_disable(vcpu);kvm_arch_vcpu_load_debug_state_flags(vcpu);++kvm_register_perf_callbacks();}voidkvm_arch_vcpu_put(structkvm_vcpu*vcpu){+kvm_unregister_perf_callbacks();kvm_arch_vcpu_put_debug_state_flags(vcpu);kvm_arch_vcpu_put_fp(vcpu);if(has_vhe())
@@ -2155,7 +2158,7 @@ int kvm_arch_init(void *opaque)/* NOP: Compiling as a module not supported */voidkvm_arch_exit(void){-kvm_perf_teardown();+}staticint__initearly_kvm_mode_cfg(char*arg)
From: Sean Christopherson <seanjc@google.com> Date: 2021-08-27 00:58:27
Move x86's perf guest callbacks into common KVM, as they are semantically
identical to arm64's callbacks (the only other such KVM callbacks).
arm64 will convert to the common versions in a future patch.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/include/asm/kvm_host.h | 1 +
arch/x86/kvm/x86.c | 48 +++++----------------------------
arch/x86/kvm/x86.h | 6 -----
include/linux/kvm_host.h | 12 +++++++++
virt/kvm/kvm_main.c | 46 +++++++++++++++++++++++++++++++
5 files changed, 66 insertions(+), 47 deletions(-)
@@ -8264,32 +8264,6 @@ static void kvm_timer_init(void)kvmclock_cpu_online,kvmclock_cpu_down_prep);}-staticintkvm_is_in_guest(void)-{-/* x86's callbacks are registered only when handling a guest NMI. */-returntrue;-}--staticintkvm_is_user_mode(void)-{-structkvm_vcpu*vcpu=kvm_get_running_vcpu();--if(WARN_ON_ONCE(!vcpu))-return0;--returnstatic_call(kvm_x86_get_cpl)(vcpu)!=0;-}--staticunsignedlongkvm_get_guest_ip(void)-{-structkvm_vcpu*vcpu=kvm_get_running_vcpu();--if(WARN_ON_ONCE(!vcpu))-return0;--returnkvm_rip_read(vcpu);-}-staticvoidkvm_handle_intel_pt_intr(void){structkvm_vcpu*vcpu=kvm_get_running_vcpu();
From: Sean Christopherson <seanjc@google.com> Date: 2021-08-27 00:58:29
Now that all state needed for VMX's PT interrupt handler is exposed to
vmx.c (specifically the currently running vCPU), move the handler into
vmx.c where it belongs.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/include/asm/kvm_host.h | 1 -
arch/x86/kvm/vmx/vmx.c | 24 +++++++++++++++++++++---
arch/x86/kvm/x86.c | 17 -----------------
virt/kvm/kvm_main.c | 1 +
4 files changed, 22 insertions(+), 21 deletions(-)
From: Sean Christopherson <seanjc@google.com> Date: 2021-08-27 00:58:53
Drop arm64's version of the callbacks in favor of the callbacks provided
by generic KVM, which are semantically identical. Implement the "get ip"
hook as needed.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/arm64/include/asm/kvm_host.h | 6 +----
arch/arm64/kvm/arm.c | 5 ++++
arch/arm64/kvm/perf.c | 38 -------------------------------
3 files changed, 6 insertions(+), 43 deletions(-)
@@ -503,6 +503,11 @@ bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu)returnvcpu_mode_priv(vcpu);}+unsignedlongkvm_arch_vcpu_get_ip(structkvm_vcpu*vcpu)+{+return*vcpu_pc(vcpu);+}+/* Just ensure a guest exit from a particular CPU */staticvoidexit_vm_noop(void*info){
From: Sean Christopherson <seanjc@google.com> Date: 2021-08-27 00:59:32
Drop the helper that allows bulk unregistering of the per-CPU callbacks
now that KVM, the only entity that actually unregisters callbacks, uses
the per-CPU helpers. Bulk unregistering is inherently unsafe as there
are no protections against nullifying a pointer for a CPU that is using
said pointer in a PMI handler.
Opportunistically tweak names to better reflect reality.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/xen/pmu.c | 2 +-
include/linux/kvm_host.h | 2 +-
include/linux/perf_event.h | 9 +++------
kernel/events/core.c | 31 +++++++++++--------------------
virt/kvm/kvm_main.c | 2 +-
5 files changed, 17 insertions(+), 29 deletions(-)
From: Sean Christopherson <seanjc@google.com> Date: 2021-08-27 00:59:36
Interpret a null ->is_in_guest callback as meaning "in guest" and use
the new semantics in KVM, which currently returns 'true' unconditionally
in its implementation of ->is_in_guest(). This avoids a retpoline on
the indirect call for PMIs that arrive in a KVM guest, and also provides
a handy excuse for a wrapper around retrieval of perf_get_guest_cbs,
e.g. to reduce the probability of an errant direct read of perf_guest_cbs.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/events/core.c | 16 ++++++++--------
arch/x86/events/intel/core.c | 5 ++---
include/linux/perf_event.h | 17 +++++++++++++++++
virt/kvm/kvm_main.c | 9 ++-------
4 files changed, 29 insertions(+), 18 deletions(-)
@@ -2761,11 +2761,11 @@ static bool perf_hw_regs(struct pt_regs *regs)voidperf_callchain_kernel(structperf_callchain_entry_ctx*entry,structpt_regs*regs){-structperf_guest_info_callbacks*guest_cbs=this_cpu_read(perf_guest_cbs);+structperf_guest_info_callbacks*guest_cbs=perf_get_guest_cbs();structunwind_statestate;unsignedlongaddr;-if(guest_cbs&&guest_cbs->is_in_guest()){+if(guest_cbs){/* TODO: We don't support guest os callchain now */return;}
@@ -2865,11 +2865,11 @@ perf_callchain_user32(struct pt_regs *regs, struct perf_callchain_entry_ctx *entvoidperf_callchain_user(structperf_callchain_entry_ctx*entry,structpt_regs*regs){-structperf_guest_info_callbacks*guest_cbs=this_cpu_read(perf_guest_cbs);+structperf_guest_info_callbacks*guest_cbs=perf_get_guest_cbs();structstack_frameframe;conststructstack_frame__user*fp;-if(guest_cbs&&guest_cbs->is_in_guest()){+if(guest_cbs){/* TODO: We don't support guest os callchain now */return;}
@@ -2946,9 +2946,9 @@ static unsigned long code_segment_base(struct pt_regs *regs)unsignedlongperf_instruction_pointer(structpt_regs*regs){-structperf_guest_info_callbacks*guest_cbs=this_cpu_read(perf_guest_cbs);+structperf_guest_info_callbacks*guest_cbs=perf_get_guest_cbs();-if(guest_cbs&&guest_cbs->is_in_guest())+if(guest_cbs)returnguest_cbs->get_guest_ip();returnregs->ip+code_segment_base(regs);
@@ -2956,10 +2956,10 @@ unsigned long perf_instruction_pointer(struct pt_regs *regs)unsignedlongperf_misc_flags(structpt_regs*regs){-structperf_guest_info_callbacks*guest_cbs=this_cpu_read(perf_guest_cbs);+structperf_guest_info_callbacks*guest_cbs=perf_get_guest_cbs();intmisc=0;-if(guest_cbs&&guest_cbs->is_in_guest()){+if(guest_cbs){if(guest_cbs->is_user_mode())misc|=PERF_RECORD_MISC_GUEST_USER;else
+ STATIC BRANCH/CALL friends.
On 27/8/2021 8:57 am, Sean Christopherson wrote:
This started out as a small series[1] to fix a KVM bug related to Intel PT
interrupt handling and snowballed horribly.
The main problem being addressed is that the perf_guest_cbs are shared by
all CPUs, can be nullified by KVM during module unload, and are not
protected against concurrent access from NMI context.
Shouldn't this be a generic issue of the static_call() usage ?
At the beginning, we set up the static entry assuming perf_guest_cbs != NULL:
if (perf_guest_cbs && perf_guest_cbs->handle_intel_pt_intr) {
static_call_update(x86_guest_handle_intel_pt_intr,
perf_guest_cbs->handle_intel_pt_intr);
}
and then we unset the perf_guest_cbs and do the static function call like this:
DECLARE_STATIC_CALL(x86_guest_handle_intel_pt_intr,
*(perf_guest_cbs->handle_intel_pt_intr));
static int handle_pmi_common(struct pt_regs *regs, u64 status)
{
...
if (!static_call(x86_guest_handle_intel_pt_intr)())
intel_pt_interrupt();
...
}
Can we make static_call() back to the original "(void *)&__static_call_return0"
in this case ?
quoted hunk
The bug has escaped notice because all dereferences of perf_guest_cbs
follow the same "perf_guest_cbs && perf_guest_cbs->is_in_guest()" pattern,
and AFAICT the compiler never reloads perf_guest_cbs in this sequence.
The compiler does reload perf_guest_cbs for any future dereferences, but
the ->is_in_guest() guard all but 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 unloaded.
But with help, e.g. READ_ONCE(perf_guest_cbs), unloading kvm_intel can
trigger a NULL pointer derference (see below). Manual intervention aside,
the bug is a bit of a time bomb, e.g. my patch 3 from the original PT
handling series would have omitted the ->is_in_guest() guard.
This series fixes the problem by making the callbacks per-CPU, and
registering/unregistering the callbacks only with preemption disabled
(except for the Xen case, which doesn't unregister).
This approach also allows for several nice cleanups in this series.
KVM x86 and arm64 can share callbacks, KVM x86 drops its somewhat
redundant current_vcpu, and the retpoline that is currently hit when KVM
is loaded (due to always checking ->is_in_guest()) goes away (it's still
there when running as Xen Dom0).
Changing to per-CPU callbacks also provides a good excuse to excise
copy+paste code from architectures that can't possibly have guest
callbacks.
This series conflicts horribly with a proposed patch[2] to use static
calls for perf_guest_cbs. But that patch is broken as it completely
fails to handle unregister, and it's not clear to me whether or not
it can correctly handle unregister without fixing the underlying race
(I don't know enough about the code patching for static calls).
This tweak
From: Peter Zijlstra <peterz@infradead.org> Date: 2021-08-27 07:19:37
On Thu, Aug 26, 2021 at 05:57:08PM -0700, Sean Christopherson wrote:
Use a per-CPU pointer to track perf's guest callbacks so that KVM can set
the callbacks more precisely and avoid a lurking NULL pointer dereference.
I'm completely failing to see how per-cpu helps anything here...
On x86, KVM supports being built as a module and thus can be unloaded.
And because the shared callbacks are referenced from IRQ/NMI context,
unloading KVM can run concurrently with perf, and thus all of perf's
checks for a NULL perf_guest_cbs are flawed as perf_guest_cbs could be
nullified between the check and dereference.
No longer allowing KVM to be a module would be *AWESOME*. I detest how
much we have to export for KVM :/
Still, what stops KVM from doing a coherent unreg? Even the
static_call() proposed in the other patch, unreg can do
static_call_update() + synchronize_rcu() to ensure everybody sees the
updated pointer (would require a quick audit to see all users are with
preempt disabled, but I think your using per-cpu here already imposes
the same).
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
This is 100% broken, and a prime example of why I hate modules.
It provides an interface for all modules, and completely fails to
validate even the most basic usage.
By using __this_cpu*() it omits the preemption checks, so you can call
this with preemption enabled, no problem.
By not checking the previous state, multiple modules can call this
interleaved without issue.
Basically assume any EXPORTed function is hostile, binary modules and
out-of-tree modules *are* just that. It's a cesspit out there.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Does this rely on kvm_{,un}register_perf_callback() being a function
call and thus implying a sequence point to order the stores?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Peter Zijlstra <peterz@infradead.org> Date: 2021-08-27 07:49:19
On Fri, Aug 27, 2021 at 02:52:25PM +0800, Like Xu wrote:
+ STATIC BRANCH/CALL friends.
On 27/8/2021 8:57 am, Sean Christopherson wrote:
quoted
This started out as a small series[1] to fix a KVM bug related to Intel PT
interrupt handling and snowballed horribly.
The main problem being addressed is that the perf_guest_cbs are shared by
all CPUs, can be nullified by KVM during module unload, and are not
protected against concurrent access from NMI context.
Shouldn't this be a generic issue of the static_call() usage ?
At the beginning, we set up the static entry assuming perf_guest_cbs != NULL:
if (perf_guest_cbs && perf_guest_cbs->handle_intel_pt_intr) {
static_call_update(x86_guest_handle_intel_pt_intr,
perf_guest_cbs->handle_intel_pt_intr);
}
and then we unset the perf_guest_cbs and do the static function call like this:
DECLARE_STATIC_CALL(x86_guest_handle_intel_pt_intr,
*(perf_guest_cbs->handle_intel_pt_intr));
static int handle_pmi_common(struct pt_regs *regs, u64 status)
{
...
if (!static_call(x86_guest_handle_intel_pt_intr)())
intel_pt_interrupt();
...
}
You just have to make sure all static_call() invocations that started
before unreg are finished before continuing with the unload.
synchronize_rcu() can help with that.
This is module unload 101. Nothing specific to static_call().
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Aug 27, 2021 at 02:52:25PM +0800, Like Xu wrote:
quoted
+ STATIC BRANCH/CALL friends.
On 27/8/2021 8:57 am, Sean Christopherson wrote:
quoted
This started out as a small series[1] to fix a KVM bug related to Intel PT
interrupt handling and snowballed horribly.
The main problem being addressed is that the perf_guest_cbs are shared by
all CPUs, can be nullified by KVM during module unload, and are not
protected against concurrent access from NMI context.
Shouldn't this be a generic issue of the static_call() usage ?
At the beginning, we set up the static entry assuming perf_guest_cbs != NULL:
if (perf_guest_cbs && perf_guest_cbs->handle_intel_pt_intr) {
static_call_update(x86_guest_handle_intel_pt_intr,
perf_guest_cbs->handle_intel_pt_intr);
}
and then we unset the perf_guest_cbs and do the static function call like this:
DECLARE_STATIC_CALL(x86_guest_handle_intel_pt_intr,
*(perf_guest_cbs->handle_intel_pt_intr));
static int handle_pmi_common(struct pt_regs *regs, u64 status)
{
...
if (!static_call(x86_guest_handle_intel_pt_intr)())
intel_pt_interrupt();
...
}
You just have to make sure all static_call() invocations that started
before unreg are finished before continuing with the unload.
synchronize_rcu() can help with that.
From: Peter Zijlstra <peterz@infradead.org> Date: 2021-08-27 10:52:52
On Fri, Aug 27, 2021 at 04:01:45PM +0800, Like Xu wrote:
On 27/8/2021 3:44 pm, Peter Zijlstra wrote:
quoted hunk
quoted
You just have to make sure all static_call() invocations that started
before unreg are finished before continuing with the unload.
synchronize_rcu() can help with that.
I'm thinking the synchronize_rcu() should go here, and access to
perf_guest_cbs should be wrapped to yell when called with preemption
enabled.
But yes..
From: Sean Christopherson <seanjc@google.com> Date: 2021-08-27 14:49:57
On Fri, Aug 27, 2021, Peter Zijlstra wrote:
On Thu, Aug 26, 2021 at 05:57:08PM -0700, Sean Christopherson wrote:
quoted
Use a per-CPU pointer to track perf's guest callbacks so that KVM can set
the callbacks more precisely and avoid a lurking NULL pointer dereference.
I'm completely failing to see how per-cpu helps anything here...
It doesn't help until KVM is converted to set the per-cpu pointer in flows that
are protected against preemption, and more specifically when KVM only writes to
the pointer from the owning CPU.
quoted
On x86, KVM supports being built as a module and thus can be unloaded.
And because the shared callbacks are referenced from IRQ/NMI context,
unloading KVM can run concurrently with perf, and thus all of perf's
checks for a NULL perf_guest_cbs are flawed as perf_guest_cbs could be
nullified between the check and dereference.
No longer allowing KVM to be a module would be *AWESOME*. I detest how
much we have to export for KVM :/
Still, what stops KVM from doing a coherent unreg? Even the
static_call() proposed in the other patch, unreg can do
static_call_update() + synchronize_rcu() to ensure everybody sees the
updated pointer (would require a quick audit to see all users are with
preempt disabled, but I think your using per-cpu here already imposes
the same).
Ignoring static call for the moment, I don't see how the unreg side can be safe
using a bare single global pointer. There is no way for KVM to prevent an NMI
from running in parallel on a different CPU. If there's a more elegant solution,
especially something that can be backported, e.g. an rcu-protected pointer, I'm
all for it. I went down the per-cpu path because it allowed for cleanups in KVM,
but similar cleanups can be done without per-cpu perf callbacks.
As for static calls, I certainly have no objection to employing static calls for
the callbacks, but IMO we should not be relying on static call for correctness,
i.e. the existing bug needs to be fixed first.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Peter Zijlstra <peterz@infradead.org> Date: 2021-08-27 14:57:41
On Fri, Aug 27, 2021 at 02:49:50PM +0000, Sean Christopherson wrote:
On Fri, Aug 27, 2021, Peter Zijlstra wrote:
quoted
On Thu, Aug 26, 2021 at 05:57:08PM -0700, Sean Christopherson wrote:
quoted
Use a per-CPU pointer to track perf's guest callbacks so that KVM can set
the callbacks more precisely and avoid a lurking NULL pointer dereference.
I'm completely failing to see how per-cpu helps anything here...
It doesn't help until KVM is converted to set the per-cpu pointer in flows that
are protected against preemption, and more specifically when KVM only writes to
the pointer from the owning CPU.
So the 'problem' I have with this is that sane (!KVM using) people, will
still have to suffer that load, whereas with the static_call() we patch
in an 'xor %rax,%rax' and only have immediate code flow.
Ignoring static call for the moment, I don't see how the unreg side can be safe
using a bare single global pointer. There is no way for KVM to prevent an NMI
from running in parallel on a different CPU. If there's a more elegant solution,
especially something that can be backported, e.g. an rcu-protected pointer, I'm
all for it. I went down the per-cpu path because it allowed for cleanups in KVM,
but similar cleanups can be done without per-cpu perf callbacks.
If all the perf_guest_cbs dereferences are with preemption disabled
(IRQs disabled, IRQ context, NMI context included), then the sequence:
WRITE_ONCE(perf_guest_cbs, NULL);
synchronize_rcu();
Ensures that all prior observers of perf_guest_csb will have completed
and future observes must observe the NULL value.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Does this rely on kvm_{,un}register_perf_callback() being a function
call and thus implying a sequence point to order the stores?
No, I'm just terrible at remembering which macros provide what ordering guarantees,
i.e. I was thinking WRITE_ONCE provided guarantees against compiler reordering.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Sean Christopherson <seanjc@google.com> Date: 2021-08-27 15:22:42
On Fri, Aug 27, 2021, Peter Zijlstra wrote:
On Fri, Aug 27, 2021 at 02:49:50PM +0000, Sean Christopherson wrote:
quoted
On Fri, Aug 27, 2021, Peter Zijlstra wrote:
quoted
On Thu, Aug 26, 2021 at 05:57:08PM -0700, Sean Christopherson wrote:
quoted
Use a per-CPU pointer to track perf's guest callbacks so that KVM can set
the callbacks more precisely and avoid a lurking NULL pointer dereference.
I'm completely failing to see how per-cpu helps anything here...
It doesn't help until KVM is converted to set the per-cpu pointer in flows that
are protected against preemption, and more specifically when KVM only writes to
the pointer from the owning CPU.
So the 'problem' I have with this is that sane (!KVM using) people, will
still have to suffer that load, whereas with the static_call() we patch
in an 'xor %rax,%rax' and only have immediate code flow.
Again, I've no objection to the static_call() approach. I didn't even see the
patch until I had finished testing my series :-/
quoted
Ignoring static call for the moment, I don't see how the unreg side can be safe
using a bare single global pointer. There is no way for KVM to prevent an NMI
from running in parallel on a different CPU. If there's a more elegant solution,
especially something that can be backported, e.g. an rcu-protected pointer, I'm
all for it. I went down the per-cpu path because it allowed for cleanups in KVM,
but similar cleanups can be done without per-cpu perf callbacks.
If all the perf_guest_cbs dereferences are with preemption disabled
(IRQs disabled, IRQ context, NMI context included), then the sequence:
WRITE_ONCE(perf_guest_cbs, NULL);
synchronize_rcu();
Ensures that all prior observers of perf_guest_csb will have completed
and future observes must observe the NULL value.
That alone won't be sufficient, as the read side also needs to ensure it doesn't
reload perf_guest_cbs between NULL checks and dereferences. But that's easy
enough to solve with a READ_ONCE and maybe a helper to make it more cumbersome
to use perf_guest_cbs directly.
How about this for a series?
1. Use READ_ONCE/WRITE_ONCE + synchronize_rcu() to fix the underlying bug
2. Fix KVM PT interrupt handler bug
3. Kill off perf_guest_cbs usage in architectures that don't need the callbacks
4. Replace ->is_in_guest()/->is_user_mode() with ->state(), and s/get_guest_ip/get_ip
5. Implement static_call() support
6. Cleanups, if there are any
6..N KVM cleanups, e.g. to eliminate current_vcpu and share x86+arm64 callbacks
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel