| Message ID | 20210305223331.4173565-1-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/perf: Fix guest_get_msrs static call if there is no PMU | expand |
On 2021/3/6 6:33, Sean Christopherson wrote: > Handle a NULL x86_pmu.guest_get_msrs at invocation instead of patching > in perf_guest_get_msrs_nop() during setup. If there is no PMU, setup "If there is no PMU" ... How to set up this kind of environment, and what changes are needed in .config or boot parameters ? > bails before updating the static calls, leaving x86_pmu.guest_get_msrs > NULL and thus a complete nop. > Ultimately, this causes VMX abort on > VM-Exit due to KVM putting random garbage from the stack into the MSR > load list. > > Fixes: abd562df94d1 ("x86/perf: Use static_call for x86_pmu.guest_get_msrs") > Cc: Like Xu <like.xu@linux.intel.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Jim Mattson <jmattson@google.com> > Cc: kvm@vger.kernel.org > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/events/core.c | 16 +++++----------- > 1 file changed, 5 insertions(+), 11 deletions(-) > > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c > index 6ddeed3cd2ac..ff874461f14c 100644 > --- a/arch/x86/events/core.c > +++ b/arch/x86/events/core.c > @@ -671,7 +671,11 @@ void x86_pmu_disable_all(void) > > struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr) > { > - return static_call(x86_pmu_guest_get_msrs)(nr); > + if (x86_pmu.guest_get_msrs) > + return static_call(x86_pmu_guest_get_msrs)(nr); How about using "static_call_cond" per commit "452cddbff7" ? > + > + *nr = 0; > + return NULL; > } > EXPORT_SYMBOL_GPL(perf_guest_get_msrs); > > @@ -1944,13 +1948,6 @@ static void _x86_pmu_read(struct perf_event *event) > x86_perf_event_update(event); > } > > -static inline struct perf_guest_switch_msr * > -perf_guest_get_msrs_nop(int *nr) > -{ > - *nr = 0; > - return NULL; > -} > - > static int __init init_hw_perf_events(void) > { > struct x86_pmu_quirk *quirk; > @@ -2024,9 +2021,6 @@ static int __init init_hw_perf_events(void) > if (!x86_pmu.read) > x86_pmu.read = _x86_pmu_read; > > - if (!x86_pmu.guest_get_msrs) > - x86_pmu.guest_get_msrs = perf_guest_get_msrs_nop; > - > x86_pmu_static_call_update(); > > /*
On Mon, Mar 8, 2021 at 3:26 AM Xu, Like <like.xu@intel.com> wrote: > > On 2021/3/6 6:33, Sean Christopherson wrote: > > Handle a NULL x86_pmu.guest_get_msrs at invocation instead of patching > > in perf_guest_get_msrs_nop() during setup. If there is no PMU, setup > > "If there is no PMU" ... > > How to set up this kind of environment, > and what changes are needed in .config or boot parameters ? Hi Xu, This can be reproduced in qemu with "-cpu max,-pmu" flag using this reproducer: https://groups.google.com/g/syzkaller-bugs/c/D8eHw3LIOd0/m/L2G0lVkVBAAJ > > bails before updating the static calls, leaving x86_pmu.guest_get_msrs > > NULL and thus a complete nop. > > > Ultimately, this causes VMX abort on > > VM-Exit due to KVM putting random garbage from the stack into the MSR > > load list. > > > > Fixes: abd562df94d1 ("x86/perf: Use static_call for x86_pmu.guest_get_msrs") > > Cc: Like Xu <like.xu@linux.intel.com> > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Cc: Jim Mattson <jmattson@google.com> > > Cc: kvm@vger.kernel.org > > Reported-by: Dmitry Vyukov <dvyukov@google.com> > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > arch/x86/events/core.c | 16 +++++----------- > > 1 file changed, 5 insertions(+), 11 deletions(-) > > > > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c > > index 6ddeed3cd2ac..ff874461f14c 100644 > > --- a/arch/x86/events/core.c > > +++ b/arch/x86/events/core.c > > @@ -671,7 +671,11 @@ void x86_pmu_disable_all(void) > > > > struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr) > > { > > - return static_call(x86_pmu_guest_get_msrs)(nr); > > + if (x86_pmu.guest_get_msrs) > > + return static_call(x86_pmu_guest_get_msrs)(nr); > > How about using "static_call_cond" per commit "452cddbff7" ? > > > + > > + *nr = 0; > > + return NULL; > > } > > EXPORT_SYMBOL_GPL(perf_guest_get_msrs); > > > > @@ -1944,13 +1948,6 @@ static void _x86_pmu_read(struct perf_event *event) > > x86_perf_event_update(event); > > } > > > > -static inline struct perf_guest_switch_msr * > > -perf_guest_get_msrs_nop(int *nr) > > -{ > > - *nr = 0; > > - return NULL; > > -} > > - > > static int __init init_hw_perf_events(void) > > { > > struct x86_pmu_quirk *quirk; > > @@ -2024,9 +2021,6 @@ static int __init init_hw_perf_events(void) > > if (!x86_pmu.read) > > x86_pmu.read = _x86_pmu_read; > > > > - if (!x86_pmu.guest_get_msrs) > > - x86_pmu.guest_get_msrs = perf_guest_get_msrs_nop; > > - > > x86_pmu_static_call_update(); > > > > /* >
On 2021/3/8 15:12, Dmitry Vyukov wrote: > On Mon, Mar 8, 2021 at 3:26 AM Xu, Like <like.xu@intel.com> wrote: >> >> On 2021/3/6 6:33, Sean Christopherson wrote: >>> Handle a NULL x86_pmu.guest_get_msrs at invocation instead of patching >>> in perf_guest_get_msrs_nop() during setup. If there is no PMU, setup >> >> "If there is no PMU" ... >> >> How to set up this kind of environment, >> and what changes are needed in .config or boot parameters ? > > Hi Xu, > > This can be reproduced in qemu with "-cpu max,-pmu" flag using this reproducer: > https://groups.google.com/g/syzkaller-bugs/c/D8eHw3LIOd0/m/L2G0lVkVBAAJ Sorry, I couldn't reproduce any VMX abort with "-cpu max,-pmu". Doe this patch fix this "unexpected kernel reboot" issue ? If so, you may add "Tested-by" for more attention. > >>> bails before updating the static calls, leaving x86_pmu.guest_get_msrs >>> NULL and thus a complete nop. >> >>> Ultimately, this causes VMX abort on >>> VM-Exit due to KVM putting random garbage from the stack into the MSR >>> load list. >>> >>> Fixes: abd562df94d1 ("x86/perf: Use static_call for x86_pmu.guest_get_msrs") >>> Cc: Like Xu <like.xu@linux.intel.com> >>> Cc: Paolo Bonzini <pbonzini@redhat.com> >>> Cc: Jim Mattson <jmattson@google.com> >>> Cc: kvm@vger.kernel.org >>> Reported-by: Dmitry Vyukov <dvyukov@google.com> >>> Signed-off-by: Sean Christopherson <seanjc@google.com> >>> --- >>> arch/x86/events/core.c | 16 +++++----------- >>> 1 file changed, 5 insertions(+), 11 deletions(-) >>> >>> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c >>> index 6ddeed3cd2ac..ff874461f14c 100644 >>> --- a/arch/x86/events/core.c >>> +++ b/arch/x86/events/core.c >>> @@ -671,7 +671,11 @@ void x86_pmu_disable_all(void) >>> >>> struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr) >>> { >>> - return static_call(x86_pmu_guest_get_msrs)(nr); >>> + if (x86_pmu.guest_get_msrs) >>> + return static_call(x86_pmu_guest_get_msrs)(nr); >> >> How about using "static_call_cond" per commit "452cddbff7" ? >> >>> + >>> + *nr = 0; >>> + return NULL; >>> } >>> EXPORT_SYMBOL_GPL(perf_guest_get_msrs); >>> >>> @@ -1944,13 +1948,6 @@ static void _x86_pmu_read(struct perf_event *event) >>> x86_perf_event_update(event); >>> } >>> >>> -static inline struct perf_guest_switch_msr * >>> -perf_guest_get_msrs_nop(int *nr) >>> -{ >>> - *nr = 0; >>> - return NULL; >>> -} >>> - >>> static int __init init_hw_perf_events(void) >>> { >>> struct x86_pmu_quirk *quirk; >>> @@ -2024,9 +2021,6 @@ static int __init init_hw_perf_events(void) >>> if (!x86_pmu.read) >>> x86_pmu.read = _x86_pmu_read; >>> >>> - if (!x86_pmu.guest_get_msrs) >>> - x86_pmu.guest_get_msrs = perf_guest_get_msrs_nop; >>> - >>> x86_pmu_static_call_update(); >>> >>> /* >>
On Mon, Mar 8, 2021 at 9:35 AM Like Xu <like.xu@linux.intel.com> wrote: > > On 2021/3/8 15:12, Dmitry Vyukov wrote: > > On Mon, Mar 8, 2021 at 3:26 AM Xu, Like <like.xu@intel.com> wrote: > >> > >> On 2021/3/6 6:33, Sean Christopherson wrote: > >>> Handle a NULL x86_pmu.guest_get_msrs at invocation instead of patching > >>> in perf_guest_get_msrs_nop() during setup. If there is no PMU, setup > >> > >> "If there is no PMU" ... > >> > >> How to set up this kind of environment, > >> and what changes are needed in .config or boot parameters ? > > > > Hi Xu, > > > > This can be reproduced in qemu with "-cpu max,-pmu" flag using this reproducer: > > https://groups.google.com/g/syzkaller-bugs/c/D8eHw3LIOd0/m/L2G0lVkVBAAJ > > Sorry, I couldn't reproduce any VMX abort with "-cpu max,-pmu". > Doe this patch fix this "unexpected kernel reboot" issue ? > > If so, you may add "Tested-by" for more attention. There is an uninit involved. For me it crashed reliably when kernel compiled with clang 11, but with gcc it worked most of the time. You may try to add something like: --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -6581,6 +6581,7 @@ static void atomic_switch_perf_msrs(struct vcpu_vmx *vmx) struct perf_guest_switch_msr *msrs; + nr_msrs = 12345678; msrs = perf_guest_get_msrs(&nr_msrs); + pr_err("atomic_switch_perf_msrs: msrs=%px nr_msrs=%d\n", msrs, nr_msrs); Then you will see surprising things. > >>> bails before updating the static calls, leaving x86_pmu.guest_get_msrs > >>> NULL and thus a complete nop. > >> > >>> Ultimately, this causes VMX abort on > >>> VM-Exit due to KVM putting random garbage from the stack into the MSR > >>> load list. > >>> > >>> Fixes: abd562df94d1 ("x86/perf: Use static_call for x86_pmu.guest_get_msrs") > >>> Cc: Like Xu <like.xu@linux.intel.com> > >>> Cc: Paolo Bonzini <pbonzini@redhat.com> > >>> Cc: Jim Mattson <jmattson@google.com> > >>> Cc: kvm@vger.kernel.org > >>> Reported-by: Dmitry Vyukov <dvyukov@google.com> > >>> Signed-off-by: Sean Christopherson <seanjc@google.com> > >>> --- > >>> arch/x86/events/core.c | 16 +++++----------- > >>> 1 file changed, 5 insertions(+), 11 deletions(-) > >>> > >>> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c > >>> index 6ddeed3cd2ac..ff874461f14c 100644 > >>> --- a/arch/x86/events/core.c > >>> +++ b/arch/x86/events/core.c > >>> @@ -671,7 +671,11 @@ void x86_pmu_disable_all(void) > >>> > >>> struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr) > >>> { > >>> - return static_call(x86_pmu_guest_get_msrs)(nr); > >>> + if (x86_pmu.guest_get_msrs) > >>> + return static_call(x86_pmu_guest_get_msrs)(nr); > >> > >> How about using "static_call_cond" per commit "452cddbff7" ? > >> > >>> + > >>> + *nr = 0; > >>> + return NULL; > >>> } > >>> EXPORT_SYMBOL_GPL(perf_guest_get_msrs); > >>> > >>> @@ -1944,13 +1948,6 @@ static void _x86_pmu_read(struct perf_event *event) > >>> x86_perf_event_update(event); > >>> } > >>> > >>> -static inline struct perf_guest_switch_msr * > >>> -perf_guest_get_msrs_nop(int *nr) > >>> -{ > >>> - *nr = 0; > >>> - return NULL; > >>> -} > >>> - > >>> static int __init init_hw_perf_events(void) > >>> { > >>> struct x86_pmu_quirk *quirk; > >>> @@ -2024,9 +2021,6 @@ static int __init init_hw_perf_events(void) > >>> if (!x86_pmu.read) > >>> x86_pmu.read = _x86_pmu_read; > >>> > >>> - if (!x86_pmu.guest_get_msrs) > >>> - x86_pmu.guest_get_msrs = perf_guest_get_msrs_nop; > >>> - > >>> x86_pmu_static_call_update(); > >>> > >>> /* > >> >
On Mon, Mar 08, 2021 at 10:25:59AM +0800, Xu, Like wrote: > On 2021/3/6 6:33, Sean Christopherson wrote: > > Handle a NULL x86_pmu.guest_get_msrs at invocation instead of patching > > in perf_guest_get_msrs_nop() during setup. If there is no PMU, setup > > "If there is no PMU" ... Then you shouldn't be calling this either ofcourse :-) > > @@ -671,7 +671,11 @@ void x86_pmu_disable_all(void) > > struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr) > > { > > - return static_call(x86_pmu_guest_get_msrs)(nr); > > + if (x86_pmu.guest_get_msrs) > > + return static_call(x86_pmu_guest_get_msrs)(nr); > > How about using "static_call_cond" per commit "452cddbff7" ? Given the one user in atomic_switch_perf_msrs() that should work because it doesn't seem to care about nr_msrs when !msrs. Still, it calling atomic_switch_perf_msrs() and intel_pmu_lbr_is_enabled() when there isn't a PMU at all is of course, a complete waste of cycles.
On 2021/3/8 16:53, Peter Zijlstra wrote: > Still, it calling atomic_switch_perf_msrs() and > intel_pmu_lbr_is_enabled() when there isn't a PMU at all is of course, a > complete waste of cycles. This suggestion is reminiscent of a sad regression of optimizing it: https://lore.kernel.org/kvm/20200619094046.654019-1-vkuznets@redhat.com/ https://lore.kernel.org/kvm/20210209225653.1393771-1-jmattson@google.com/
On Mon, Mar 08, 2021, Peter Zijlstra wrote: > On Mon, Mar 08, 2021 at 10:25:59AM +0800, Xu, Like wrote: > > On 2021/3/6 6:33, Sean Christopherson wrote: > > > Handle a NULL x86_pmu.guest_get_msrs at invocation instead of patching > > > in perf_guest_get_msrs_nop() during setup. If there is no PMU, setup > > > > "If there is no PMU" ... > > Then you shouldn't be calling this either ofcourse :-) This effectively is KVM's check to find out there is no PMU. I certainly don't want to replicate the switch statement in init_hw_perf_events(), plus whatever is buried in check_hw_exists(). The alternative would be to add X86_FEATURE_PMU so that KVM can easily check for PMU existence. I don't really see the point though, as bare metal KVM, where we really care about performance, is likely to have a functional PMU, and if it doesn't then I doubt whoever is running KVM cares much about performance. > > > @@ -671,7 +671,11 @@ void x86_pmu_disable_all(void) > > > struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr) > > > { > > > - return static_call(x86_pmu_guest_get_msrs)(nr); > > > + if (x86_pmu.guest_get_msrs) > > > + return static_call(x86_pmu_guest_get_msrs)(nr); > > > > How about using "static_call_cond" per commit "452cddbff7" ? > > Given the one user in atomic_switch_perf_msrs() that should work because > it doesn't seem to care about nr_msrs when !msrs. Uh, that commit quite cleary says: NOTE: this is 'obviously' limited to functions with a 'void' return type. Even if we somehow bypass the (void) cast, IIUC it will compile to a single 'ret', and return whatever happens to be in RAX, not NULL as is needed. > Still, it calling atomic_switch_perf_msrs() and > intel_pmu_lbr_is_enabled() when there isn't a PMU at all is of course, a
On Mon, Mar 08, 2021 at 12:40:44PM -0800, Sean Christopherson wrote: > On Mon, Mar 08, 2021, Peter Zijlstra wrote: > > Given the one user in atomic_switch_perf_msrs() that should work because > > it doesn't seem to care about nr_msrs when !msrs. > > Uh, that commit quite cleary says: D0h! I got static_call_cond() and __static_call_return0 mixed up. Anyway, let me see if I can make something work here.
On Tue, Mar 09, 2021 at 08:46:49AM +0100, Peter Zijlstra wrote: > On Mon, Mar 08, 2021 at 12:40:44PM -0800, Sean Christopherson wrote: > > On Mon, Mar 08, 2021, Peter Zijlstra wrote: > > > > Given the one user in atomic_switch_perf_msrs() that should work because > > > it doesn't seem to care about nr_msrs when !msrs. > > > > Uh, that commit quite cleary says: > > D0h! I got static_call_cond() and __static_call_return0 mixed up. > Anyway, let me see if I can make something work here. Does this work? I can never seem to start a VM, and if I do accidentally manage, then it never contains the things I need :/ --- diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c index 6ddeed3cd2ac..fadcecd73e1a 100644 --- a/arch/x86/events/core.c +++ b/arch/x86/events/core.c @@ -81,7 +81,11 @@ DEFINE_STATIC_CALL_NULL(x86_pmu_swap_task_ctx, *x86_pmu.swap_task_ctx); DEFINE_STATIC_CALL_NULL(x86_pmu_drain_pebs, *x86_pmu.drain_pebs); DEFINE_STATIC_CALL_NULL(x86_pmu_pebs_aliases, *x86_pmu.pebs_aliases); -DEFINE_STATIC_CALL_NULL(x86_pmu_guest_get_msrs, *x86_pmu.guest_get_msrs); +/* + * This one is magic, it will get called even when PMU init fails (because + * there is no PMU), in which case it should simply return NULL. + */ +__DEFINE_STATIC_CALL(x86_pmu_guest_get_msrs, *x86_pmu.guest_get_msrs, __static_call_return0); u64 __read_mostly hw_cache_event_ids [PERF_COUNT_HW_CACHE_MAX] @@ -1944,13 +1948,6 @@ static void _x86_pmu_read(struct perf_event *event) x86_perf_event_update(event); } -static inline struct perf_guest_switch_msr * -perf_guest_get_msrs_nop(int *nr) -{ - *nr = 0; - return NULL; -} - static int __init init_hw_perf_events(void) { struct x86_pmu_quirk *quirk; @@ -2024,9 +2021,6 @@ static int __init init_hw_perf_events(void) if (!x86_pmu.read) x86_pmu.read = _x86_pmu_read; - if (!x86_pmu.guest_get_msrs) - x86_pmu.guest_get_msrs = perf_guest_get_msrs_nop; - x86_pmu_static_call_update(); /*
On Tue, Mar 09, 2021, Peter Zijlstra wrote: > On Tue, Mar 09, 2021 at 08:46:49AM +0100, Peter Zijlstra wrote: > > On Mon, Mar 08, 2021 at 12:40:44PM -0800, Sean Christopherson wrote: > > > On Mon, Mar 08, 2021, Peter Zijlstra wrote: > > > > > > Given the one user in atomic_switch_perf_msrs() that should work because > > > > it doesn't seem to care about nr_msrs when !msrs. > > > > > > Uh, that commit quite cleary says: > > > > D0h! I got static_call_cond() and __static_call_return0 mixed up. > > Anyway, let me see if I can make something work here. > > Does this work? I can never seem to start a VM, and if I do accidentally > manage, then it never contains the things I need :/ Yep, once I found the dependencies in tip/sched/core (thank tip-bot!). I'll send v2 your way.
On Tue, Mar 9, 2021 at 6:05 PM Sean Christopherson <seanjc@google.com> wrote: > > On Tue, Mar 09, 2021, Peter Zijlstra wrote: > > On Tue, Mar 09, 2021 at 08:46:49AM +0100, Peter Zijlstra wrote: > > > On Mon, Mar 08, 2021 at 12:40:44PM -0800, Sean Christopherson wrote: > > > > On Mon, Mar 08, 2021, Peter Zijlstra wrote: > > > > > > > > Given the one user in atomic_switch_perf_msrs() that should work because > > > > > it doesn't seem to care about nr_msrs when !msrs. > > > > > > > > Uh, that commit quite cleary says: > > > > > > D0h! I got static_call_cond() and __static_call_return0 mixed up. > > > Anyway, let me see if I can make something work here. > > > > Does this work? I can never seem to start a VM, and if I do accidentally > > manage, then it never contains the things I need :/ > > Yep, once I found the dependencies in tip/sched/core (thank tip-bot!). I'll > send v2 your way. If you are resending, please also add the syzbot Reported-by tag. Thanks.
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c index 6ddeed3cd2ac..ff874461f14c 100644 --- a/arch/x86/events/core.c +++ b/arch/x86/events/core.c @@ -671,7 +671,11 @@ void x86_pmu_disable_all(void) struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr) { - return static_call(x86_pmu_guest_get_msrs)(nr); + if (x86_pmu.guest_get_msrs) + return static_call(x86_pmu_guest_get_msrs)(nr); + + *nr = 0; + return NULL; } EXPORT_SYMBOL_GPL(perf_guest_get_msrs); @@ -1944,13 +1948,6 @@ static void _x86_pmu_read(struct perf_event *event) x86_perf_event_update(event); } -static inline struct perf_guest_switch_msr * -perf_guest_get_msrs_nop(int *nr) -{ - *nr = 0; - return NULL; -} - static int __init init_hw_perf_events(void) { struct x86_pmu_quirk *quirk; @@ -2024,9 +2021,6 @@ static int __init init_hw_perf_events(void) if (!x86_pmu.read) x86_pmu.read = _x86_pmu_read; - if (!x86_pmu.guest_get_msrs) - x86_pmu.guest_get_msrs = perf_guest_get_msrs_nop; - x86_pmu_static_call_update(); /*
Handle a NULL x86_pmu.guest_get_msrs at invocation instead of patching in perf_guest_get_msrs_nop() during setup. If there is no PMU, setup bails before updating the static calls, leaving x86_pmu.guest_get_msrs NULL and thus a complete nop. Ultimately, this causes VMX abort on VM-Exit due to KVM putting random garbage from the stack into the MSR load list. Fixes: abd562df94d1 ("x86/perf: Use static_call for x86_pmu.guest_get_msrs") Cc: Like Xu <like.xu@linux.intel.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Jim Mattson <jmattson@google.com> Cc: kvm@vger.kernel.org Reported-by: Dmitry Vyukov <dvyukov@google.com> Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/events/core.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)