linux-kernel - Re: [PATCH v10 08/11] KVM: x86/pmu: Add LBR feature emulation via guest LBR event

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 8 May 2020 15:09:31 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Like Xu <like.xu@...ux.intel.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, wei.w.wang@...el.com,
        ak@...ux.intel.com
Subject: Re: [PATCH v10 08/11] KVM: x86/pmu: Add LBR feature emulation via
 guest LBR event

On Mon, Apr 27, 2020 at 11:16:40AM +0800, Like Xu wrote:
> On 2020/4/24 20:16, Peter Zijlstra wrote:

> > And I suppose that is why you need that horrible:
> > needs_guest_lbr_without_counter() thing to begin with.
> 
> Do you suggest to use event->attr.config check to replace
> "needs_branch_stack(event) && is_kernel_event(event) &&
> event->attr.exclude_host" check for guest LBR event ?

That's what the BTS thing does.

> > Please allocate yourself an event from the pseudo event range:
> > event==0x00. Currently we only have umask==3 for Fixed2 and umask==4
> > for Fixed3, given you claim 58, which is effectively Fixed25,
> > umask==0x1a might be appropriate.
> 
> OK, I assume that adding one more field ".config = 0x1a00" is
> efficient enough for perf_event_attr to allocate guest LBR events.

Uh what? The code is already setting .config. You just have to change it
do another value.

> > Also, I suppose we need to claim 0x0000 as an error, so that other
> > people won't try this again.
> 
> Does the following fix address your concern on this ?
> 
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 2405926e2dba..32d2a3f8c51f 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -498,6 +498,9 @@ int x86_pmu_max_precise(void)
> 
>  int x86_pmu_hw_config(struct perf_event *event)
>  {
> +       if (!unlikely(event->attr.config & X86_ARCH_EVENT_MASK))
> +               return -EINVAL;
> +
>         if (event->attr.precise_ip) {
>                 int precise = x86_pmu_max_precise();

That wouldn't work right for AMD. But yes, something like that.

> > Also, what happens if you fail programming due to a conflicting cpu
> > event? That pinned doesn't guarantee you'll get the event, it just means
> > you'll error instead of getting RR.
> > 
> > I didn't find any code checking the event state.
> > 
> 
> Error instead of RR is expected.
> 
> If the KVM fails programming due to a conflicting cpu event
> the LBR registers will not be passthrough to the guest,
> and KVM would return zero for any guest LBR records accesses
> until the next attempt to program the guest LBR event.
> 
> Every time before cpu enters the non-root mode where irq is
> disabled, the "event-> oncpu! =-1" check will be applied.
> (more details in the comment around intel_pmu_availability_check())
> 
> The guests administer is supposed to know the result of guest
> LBR records is inaccurate if someone is using LBR to record
> guest or hypervisor on the host side.
> 
> Is this acceptable to you？
> 
> If there is anything needs to be improved, please let me know.

It might be nice to emit a pr_warn() or something on the host when this
happens. Then at least the host admin can know he wrecked things for
which guest.