linux-kernel - Re: [PATCH] KVM: x86: async_pf: check earlier if can deliver async pf

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: <87h684ctlg.fsf@redhat.com>
Date: Mon, 18 Nov 2024 18:58:03 +0100
From: Vitaly Kuznetsov <vkuznets@...hat.com>
To: Nikita Kalyazin <kalyazin@...zon.com>
Cc: david@...hat.com, peterx@...hat.com, oleg@...hat.com, gshan@...hat.com,
 graf@...zon.de, jgowans@...zon.com, roypat@...zon.co.uk,
 derekmn@...zon.com, nsaenz@...zon.es, xmarcalx@...zon.com,
 kalyazin@...zon.com
Subject: Re: [PATCH] KVM: x86: async_pf: check earlier if can deliver async pf

Nikita Kalyazin <kalyazin@...zon.com> writes:

> On x86, async pagefault events can only be delivered if the page fault
> was triggered by guest userspace, not kernel.  This is because
> the guest may be in non-sleepable context and will not be able
> to reschedule.

We used to set KVM_ASYNC_PF_SEND_ALWAYS for Linux guests before

commit 3a7c8fafd1b42adea229fd204132f6a2fb3cd2d9
Author: Thomas Gleixner <tglx@...utronix.de>
Date:   Fri Apr 24 09:57:56 2020 +0200

    x86/kvm: Restrict ASYNC_PF to user space

but KVM side of the feature is kind of still there, namely

kvm_pv_enable_async_pf() sets

    vcpu->arch.apf.send_user_only = !(data & KVM_ASYNC_PF_SEND_ALWAYS);

and then we check it in 

kvm_can_deliver_async_pf():

     if (vcpu->arch.apf.send_user_only &&
         kvm_x86_call(get_cpl)(vcpu) == 0)
             return false;

and this can still be used by some legacy guests I suppose. How about
we start with removing this completely? It does not matter if some
legacy guest wants to get an APF for CPL0, we are never obliged to
actually use the mechanism.

>
> However existing implementation pays the following overhead even for the
> kernel-originated faults, even though it is known in advance that they
> cannot be processed asynchronously:
>  - allocate async PF token
>  - create and schedule an async work
>
> This patch avoids the overhead above in case of kernel-originated faults
> by moving the `kvm_can_deliver_async_pf` check from
> `kvm_arch_async_page_not_present` to `__kvm_faultin_pfn`.
>
> Note that the existing check `kvm_can_do_async_pf` already calls
> `kvm_can_deliver_async_pf` internally, however it only does that if the
> `kvm_hlt_in_guest` check is true, ie userspace requested KVM not to exit
> on guest halts via `KVM_CAP_X86_DISABLE_EXITS`.  In that case the code
> proceeds with the async fault processing with the following
> justification in 1dfdb45ec510ba27e366878f97484e9c9e728902 ("KVM: x86:
> clean up conditions for asynchronous page fault handling"):
>
> "Even when asynchronous page fault is disabled, KVM does not want to pause
> the host if a guest triggers a page fault; instead it will put it into
> an artificial HLT state that allows running other host processes while
> allowing interrupt delivery into the guest."
>
> Signed-off-by: Nikita Kalyazin <kalyazin@...zon.com>
> ---
>  arch/x86/kvm/mmu/mmu.c | 3 ++-
>  arch/x86/kvm/x86.c     | 5 ++---
>  arch/x86/kvm/x86.h     | 2 ++
>  3 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 22e7ad235123..11d29d15b6cd 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -4369,7 +4369,8 @@ static int __kvm_mmu_faultin_pfn(struct kvm_vcpu *vcpu,
>  			trace_kvm_async_pf_repeated_fault(fault->addr, fault->gfn);
>  			kvm_make_request(KVM_REQ_APF_HALT, vcpu);
>  			return RET_PF_RETRY;
> -		} else if (kvm_arch_setup_async_pf(vcpu, fault)) {
> +		} else if (kvm_can_deliver_async_pf(vcpu) &&
> +			kvm_arch_setup_async_pf(vcpu, fault)) {
>  			return RET_PF_RETRY;
>  		}
>  	}
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2e713480933a..8edae75b39f7 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -13355,7 +13355,7 @@ static inline bool apf_pageready_slot_free(struct kvm_vcpu *vcpu)
>  	return !val;
>  }
>  
> -static bool kvm_can_deliver_async_pf(struct kvm_vcpu *vcpu)
> +bool kvm_can_deliver_async_pf(struct kvm_vcpu *vcpu)
>  {
>  
>  	if (!kvm_pv_async_pf_enabled(vcpu))
> @@ -13406,8 +13406,7 @@ bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu,
>  	trace_kvm_async_pf_not_present(work->arch.token, work->cr2_or_gpa);
>  	kvm_add_async_pf_gfn(vcpu, work->arch.gfn);
>  
> -	if (kvm_can_deliver_async_pf(vcpu) &&
> -	    !apf_put_user_notpresent(vcpu)) {
> +	if (!apf_put_user_notpresent(vcpu)) {
>  		fault.vector = PF_VECTOR;
>  		fault.error_code_valid = true;
>  		fault.error_code = 0;
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index ec623d23d13d..9647f41e5c49 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -387,6 +387,8 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
>  fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu);
>  fastpath_t handle_fastpath_hlt(struct kvm_vcpu *vcpu);
>  
> +bool kvm_can_deliver_async_pf(struct kvm_vcpu *vcpu);
> +
>  extern struct kvm_caps kvm_caps;
>  extern struct kvm_host_values kvm_host;
>  
>
> base-commit: d96c77bd4eeba469bddbbb14323d2191684da82a

-- 
Vitaly