linux-kernel - Re: [PATCH 1/4] KVM: x86/pmu: Force reprogramming of all counters on PMU filter change

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <86d88222-a70f-49ef-71f3-a7d15ae17d7d@gmail.com>
Date:   Thu, 13 Oct 2022 20:01:33 +0800
From:   Like Xu <like.xu.linux@...il.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Aaron Lewis <aaronlewis@...gle.com>,
        Like Xu <likexu@...cent.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH 1/4] KVM: x86/pmu: Force reprogramming of all counters on
 PMU filter change

Firstly, thanks for your comments that spewed out around vpmu.

On 23/9/2022 8:13 am, Sean Christopherson wrote:
> Force vCPUs to reprogram all counters on a PMU filter change to provide
> a sane ABI for userspace.  Use the existing KVM_REQ_PMU to do the
> programming, and take advantage of the fact that the reprogram_pmi bitmap
> fits in a u64 to set all bits in a single atomic update.  Note, setting
> the bitmap and making the request needs to be done _after_ the SRCU
> synchronization to ensure that vCPUs will reprogram using the new filter.
> 
> KVM's current "lazy" approach is confusing and non-deterministic.  It's

The resolute lazy approach was introduced in patch 03, right after this change.

> confusing because, from a developer perspective, the code is buggy as it
> makes zero sense to let userspace modify the filter but then not actually
> enforce the new filter.  The lazy approach is non-deterministic because
> KVM enforces the filter whenever a counter is reprogrammed, not just on
> guest WRMSRs, i.e. a guest might gain/lose access to an event at random
> times depending on what is going on in the host.
> 
> Note, the resulting behavior is still non-determinstic while the filter
> is in flux.  If userspace wants to guarantee deterministic behavior, all
> vCPUs should be paused during the filter update.
> 
> Fixes: 66bb8a065f5a ("KVM: x86: PMU Event Filter")
> Cc: Aaron Lewis <aaronlewis@...gle.com>
> Jim Mattson <jmattson@...gle.com>

miss "Cc:" ?

> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>   arch/x86/include/asm/kvm_host.h | 11 ++++++++++-
>   arch/x86/kvm/pmu.c              | 15 +++++++++++++--
>   2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index b3ce723efb43..462f041ede9f 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -519,7 +519,16 @@ struct kvm_pmu {
>   	struct kvm_pmc gp_counters[INTEL_PMC_MAX_GENERIC];
>   	struct kvm_pmc fixed_counters[KVM_PMC_MAX_FIXED];
>   	struct irq_work irq_work;
> -	DECLARE_BITMAP(reprogram_pmi, X86_PMC_IDX_MAX);
> +
> +	/*
> +	 * Overlay the bitmap with a 64-bit atomic so that all bits can be
> +	 * set in a single access, e.g. to reprogram all counters when the PMU
> +	 * filter changes.
> +	 */
> +	union {
> +		DECLARE_BITMAP(reprogram_pmi, X86_PMC_IDX_MAX);
> +		atomic64_t __reprogram_pmi;
> +	};
>   	DECLARE_BITMAP(all_valid_pmc_idx, X86_PMC_IDX_MAX);
>   	DECLARE_BITMAP(pmc_in_use, X86_PMC_IDX_MAX);
>   
> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> index d9b9a0f0db17..4504987cbbe2 100644
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -577,6 +577,8 @@ EXPORT_SYMBOL_GPL(kvm_pmu_trigger_event);
>   int kvm_vm_ioctl_set_pmu_event_filter(struct kvm *kvm, void __user *argp)
>   {
>   	struct kvm_pmu_event_filter tmp, *filter;
> +	struct kvm_vcpu *vcpu;
> +	unsigned long i;
>   	size_t size;
>   	int r;
>   
> @@ -613,9 +615,18 @@ int kvm_vm_ioctl_set_pmu_event_filter(struct kvm *kvm, void __user *argp)
>   	mutex_lock(&kvm->lock);
>   	filter = rcu_replace_pointer(kvm->arch.pmu_event_filter, filter,
>   				     mutex_is_locked(&kvm->lock));
> -	mutex_unlock(&kvm->lock);
> -
>   	synchronize_srcu_expedited(&kvm->srcu);

The relative order of these two operations has been reversed
	mutex_unlock() and synchronize_srcu_expedited()
, extending the execution window of the critical area of "kvm->lock)".
The motivation is also not explicitly stated in the commit message.

> +
> +	BUILD_BUG_ON(sizeof(((struct kvm_pmu *)0)->reprogram_pmi) >
> +		     sizeof(((struct kvm_pmu *)0)->__reprogram_pmi));
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm)
> +		atomic64_set(&vcpu_to_pmu(vcpu)->__reprogram_pmi, -1ull);

How about:
	bitmap_copy(pmu->reprogram_pmi, pmu->all_valid_pmc_idx, X86_PMC_IDX_MAX);
to avoid further cycles on calls of 
"static_call(kvm_x86_pmu_pmc_idx_to_pmc)(pmu, bit)" ?

> +
> +	kvm_make_all_cpus_request(kvm, KVM_REQ_PMU);
> +
> +	mutex_unlock(&kvm->lock);
> +
>   	r = 0;
>   cleanup:
>   	kfree(filter);