linux-kernel - Re: [PATCH v2 10/10] KVM: x86: deprecate KVM_ASYNC_PF_SEND

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]
Date:   Thu, 28 May 2020 13:03:56 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>, kvm@...r.kernel.org,
        x86@...nel.org
Cc:     Andy Lutomirski <luto@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Jim Mattson <jmattson@...gle.com>,
        Vivek Goyal <vgoyal@...hat.com>, Gavin Shan <gshan@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 10/10] KVM: x86: deprecate KVM_ASYNC_PF_SEND_ALWAYS

On 25/05/20 16:41, Vitaly Kuznetsov wrote:
> Concerns were expressed around APF events delivery when CPU is not
> in user mode (KVM_ASYNC_PF_SEND_ALWAYS), e.g.
> https://lore.kernel.org/kvm/ed71d0967113a35f670a9625a058b8e6e0b2f104.1583547991.git.luto@kernel.org/
> 
> 'Page ready' events are already free from '#PF abuse' but 'page not ready'
> notifications still go through #PF (to be changed in the future). Make the
> probability of running into issues when APF collides with regular #PF lower
> by deprecating KVM_ASYNC_PF_SEND_ALWAYS. The feature doesn't seem to be
> important enough for any particular workload to notice the difference.

This has been disabled already in guest code, but I don't see a
particular reason to deprecate it in the host too.  Supporting it on the
host is just one line of code; if it's a problem *for the guest*, you
just don't use KVM_ASYNC_PF_SEND_ALWAYS.

Also, note that #VE will always be delivered to the guest even at CPL0;
the decision on whether to do sync or async page fault at CPL0 will move
to the guest, but enabling #VE will probably _require_ the
KVM_ASYNC_PF_SEND_ALWAYS bit to be set (and KVM_ASYNC_PF_DELIVERY_AS_INT
as well).

Thanks,

Paolo

> Suggested-by: Andy Lutomirski <luto@...nel.org>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@...hat.com>
> ---
>  arch/x86/include/asm/kvm_host.h      |  1 -
>  arch/x86/include/uapi/asm/kvm_para.h |  2 +-
>  arch/x86/kernel/kvm.c                |  3 ---
>  arch/x86/kvm/x86.c                   | 13 +++++++++----
>  4 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 356c02bfa587..f491214b7667 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -769,7 +769,6 @@ struct kvm_vcpu_arch {
>  		u64 msr_int_val; /* MSR_KVM_ASYNC_PF_INT */
>  		u16 vec;
>  		u32 id;
> -		bool send_user_only;
>  		u32 host_apf_flags;
>  		unsigned long nested_apf_token;
>  		bool delivery_as_pf_vmexit;
> diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
> index 812e9b4c1114..3cae0faac2b8 100644
> --- a/arch/x86/include/uapi/asm/kvm_para.h
> +++ b/arch/x86/include/uapi/asm/kvm_para.h
> @@ -82,7 +82,7 @@ struct kvm_clock_pairing {
>  #define KVM_MAX_MMU_OP_BATCH           32
>  
>  #define KVM_ASYNC_PF_ENABLED			(1 << 0)
> -#define KVM_ASYNC_PF_SEND_ALWAYS		(1 << 1)
> +#define KVM_ASYNC_PF_SEND_ALWAYS		(1 << 1) /* deprecated */
>  #define KVM_ASYNC_PF_DELIVERY_AS_PF_VMEXIT	(1 << 2)
>  #define KVM_ASYNC_PF_DELIVERY_AS_INT		(1 << 3)
>  
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 79730eaef1e1..add123302122 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -324,9 +324,6 @@ static void kvm_guest_cpu_init(void)
>  	if (kvm_para_has_feature(KVM_FEATURE_ASYNC_PF_INT) && kvmapf) {
>  		u64 pa = slow_virt_to_phys(this_cpu_ptr(&apf_reason));
>  
> -#ifdef CONFIG_PREEMPTION
> -		pa |= KVM_ASYNC_PF_SEND_ALWAYS;
> -#endif
>  		pa |= KVM_ASYNC_PF_ENABLED | KVM_ASYNC_PF_DELIVERY_AS_INT;
>  
>  		if (kvm_para_has_feature(KVM_FEATURE_ASYNC_PF_VMEXIT))
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index cc1bf6cfc5e0..8222133bf5be 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2648,7 +2648,10 @@ static int kvm_pv_enable_async_pf(struct kvm_vcpu *vcpu, u64 data)
>  {
>  	gpa_t gpa = data & ~0x3f;
>  
> -	/* Bits 4:5 are reserved, Should be zero */
> +	/*
> +	 * Bits 4:5 are reserved and should be zero. Bit 1
> +	 * (KVM_ASYNC_PF_SEND_ALWAYS) was deprecated and is being ignored.
> +	 */
>  	if (data & 0x30)
>  		return 1;
>  
> @@ -2664,7 +2667,6 @@ static int kvm_pv_enable_async_pf(struct kvm_vcpu *vcpu, u64 data)
>  					sizeof(u64)))
>  		return 1;
>  
> -	vcpu->arch.apf.send_user_only = !(data & KVM_ASYNC_PF_SEND_ALWAYS);
>  	vcpu->arch.apf.delivery_as_pf_vmexit = data & KVM_ASYNC_PF_DELIVERY_AS_PF_VMEXIT;
>  
>  	kvm_async_pf_wakeup_all(vcpu);
> @@ -10433,8 +10435,11 @@ static bool kvm_can_deliver_async_pf(struct kvm_vcpu *vcpu)
>  	if (!vcpu->arch.apf.delivery_as_pf_vmexit && is_guest_mode(vcpu))
>  		return false;
>  
> -	if (!kvm_pv_async_pf_enabled(vcpu) ||
> -	    (vcpu->arch.apf.send_user_only && kvm_x86_ops.get_cpl(vcpu) == 0))
> +	/*
> +	 * 'Page not present' APF events are only delivered when CPU is in
> +	 * user mode and APF mechanism is enabled.
> +	 */
> +	if (!kvm_pv_async_pf_enabled(vcpu) || kvm_x86_ops.get_cpl(vcpu) == 0)
>  		return false;
>  
>  	return true;
>