linux-kernel - Re: [PATCH] KVM: x86/mmu: Ensure NX huge page recovery thread is alive before waking

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: <Z5Q1SQ-JI6PdmVbi@google.com>
Date: Fri, 24 Jan 2025 16:50:17 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Keith Busch <kbusch@...nel.org>
Subject: Re: [PATCH] KVM: x86/mmu: Ensure NX huge page recovery thread is
 alive before waking

On Fri, Jan 24, 2025, Sean Christopherson wrote:
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index a45ae60e84ab..74c20dbb92da 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -7120,6 +7120,19 @@ static void mmu_destroy_caches(void)
>  	kmem_cache_destroy(mmu_page_header_cache);
>  }
>  
> +static void kvm_wake_nx_recovery_thread(struct kvm *kvm)
> +{
> +	/*
> +	 * The NX recovery thread is spawned on-demand at the first KVM_RUN and
> +	 * may not be valid even though the VM is globally visible.  Do nothing,
> +	 * as such a VM can't have any possible NX huge pages.
> +	 */
> +	struct vhost_task *nx_thread = READ_ONCE(kvm->arch.nx_huge_page_recovery_thread);
> +
> +	if (nx_thread)
> +		vhost_task_wake(nx_thread);

As mentioned in the original thread[*], I belatedly realized there's a race with
this approach.  If vhost_task_start() completes and kvm_nx_huge_page_recovery_worker()
runs before a parameter change, but the parameter change runs before the WRITE_ONCE(),
then the worker will run with stale params and could end up sleeping for far longer
than userspace wants.

I assume we could address that by taking kvm->arch.nx_once.mutex in this helper
instead of using the lockless approach.  I don't think that would lead to any
deadlocks?

[*] https://lore.kernel.org/all/Z5QsBXJ7rkJFDtmK@google.com