linux-kernel - Re: [PATCH v8 02/11] KVM: Add lockless memslot walk to KVM

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]

Message-ID: <Z4GesYBOCNhoUKJx@google.com>
Date: Fri, 10 Jan 2025 14:26:57 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: James Houghton <jthoughton@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, David Matlack <dmatlack@...gle.com>, 
	David Rientjes <rientjes@...gle.com>, Marc Zyngier <maz@...nel.org>, 
	Oliver Upton <oliver.upton@...ux.dev>, Wei Xu <weixugc@...gle.com>, Yu Zhao <yuzhao@...gle.com>, 
	Axel Rasmussen <axelrasmussen@...gle.com>, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 02/11] KVM: Add lockless memslot walk to KVM

On Tue, Nov 05, 2024, James Houghton wrote:
> Provide flexibility to the architecture to synchronize as optimally as

Similar to my nit on "locking" below, "synchronize" is somewhat misleading.  There's
no requirement for architectures to synchronize during aging.  There is all but
guaranteed to be some form of "synchronization", e.g. to prevent walking freed
page tables, but the aging itself never synchronizes, and protecting a walk by
disabling IRQs (as x86's shadow MMU does in some flows) only "synchronizes" in a
loose sense of the word.

> they can instead of always taking the MMU lock for writing.
> 
> Architectures that do their own locking must select
> CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS.

This is backwards and could be misleading, and for the TDP MMU outright wrong.
If some hypothetical architecture took _additional_ locks, then it can do so
without needing to select CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS.

What you want to say is that architectures that select
CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS are responsible for ensuring correctness.
And it's specifically all about correctness.  Common KVM doesn't care if the
arch does it's own locking, e.g. taking mmu_lock for read, or has some completely
lock-free scheme for aging.

> The immediate application is to allow architectures to implement the

"immediate application" is pretty redundant.  There's really only one reason to
not take mmu_lock in this path.

> test/clear_young MMU notifiers more cheaply.

IMO, "more cheaply" is vague, and doesn't add much beyond the opening sentence
about "synchronize as optimally as they can".  I would just delete this last
sentence.

> Suggested-by: Yu Zhao <yuzhao@...gle.com>
> Signed-off-by: James Houghton <jthoughton@...gle.com>
> Reviewed-by: David Matlack <dmatlack@...gle.com>
> ---
> @@ -797,6 +805,8 @@ static int kvm_mmu_notifier_clear_flush_young(struct mmu_notifier *mn,
>  		.flush_on_ret	=
>  			!IS_ENABLED(CONFIG_KVM_ELIDE_TLB_FLUSH_IF_YOUNG),
>  		.may_block	= false,
> +		.lockless	=
> +			IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS),

		.lockess	= IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS),

Random thought, maybe CONFIG_KVM_MMU_NOTIFIER_AGING_LOCKLESS or
CONFIG_KVM_MMU_NOTIFIER_AGE_LOCKLESS instead of "YOUNG"?

>  	};
>  
>  	trace_kvm_age_hva(start, end);
> @@ -817,6 +827,8 @@ static int kvm_mmu_notifier_clear_young(struct mmu_notifier *mn,
>  		.on_lock	= (void *)kvm_null_fn,
>  		.flush_on_ret	= false,
>  		.may_block	= false,
> +		.lockless	=
> +			IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS),
>  	};
>  
>  	trace_kvm_age_hva(start, end);
> @@ -849,6 +861,8 @@ static int kvm_mmu_notifier_test_young(struct mmu_notifier *mn,
>  		.on_lock	= (void *)kvm_null_fn,
>  		.flush_on_ret	= false,
>  		.may_block	= false,
> +		.lockless	=
> +			IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS),
>  	};
>  
>  	trace_kvm_test_age_hva(address);
> -- 
> 2.47.0.199.ga7371fff76-goog
>