linux-kernel - Re: [PATCH 4/5] KVM: Check for empty mask of harvested dirty ring entries in caller

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: <Z4TrQedpUgNrW2OB@yzhao56-desk.sh.intel.com>
Date: Mon, 13 Jan 2025 18:30:25 +0800
From: Yan Zhao <yan.y.zhao@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: Paolo Bonzini <pbonzini@...hat.com>, <kvm@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, Peter Xu <peterx@...hat.com>, Maxim Levitsky
	<mlevitsk@...hat.com>
Subject: Re: [PATCH 4/5] KVM: Check for empty mask of harvested dirty ring
 entries in caller

On Fri, Jan 10, 2025 at 05:04:08PM -0800, Sean Christopherson wrote:
> When resetting a dirty ring, explicitly check that there is work to be
> done before calling kvm_reset_dirty_gfn(), e.g. if no harvested entries
> are found and/or on the loop's first iteration, and delete the extremely
> misleading comment "This is only needed to make compilers happy".  KVM
> absolutely relies on mask to be zero-initialized, i.e. the comment is an
> outright lie.  Furthermore, the compiler is right to complain that KVM is
> calling a function with uninitialized data, as there are no guarantees
> the implementation details of kvm_reset_dirty_gfn() will be visible to
> kvm_dirty_ring_reset().
> 
> While the flaw could be fixed by simply deleting (or rewording) the
> comment, and duplicating the check is unfortunate, checking mask in the
> caller will allow for additional cleanups.
> 
> Opportunisticaly drop the zero-initialization of cur_slot and cur_offset.
> If a bug were introduced where either the slot or offset was consumed
> before mask is set to a non-zero value, then it is highly desirable for
> the compiler (or some other sanitizer) to yell.
> 
> Cc: Peter Xu <peterx@...hat.com>
> Cc: Yan Zhao <yan.y.zhao@...el.com>
> Cc: Maxim Levitsky <mlevitsk@...hat.com>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  virt/kvm/dirty_ring.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/virt/kvm/dirty_ring.c b/virt/kvm/dirty_ring.c
> index 37eb2b7142bd..95ab0e3cf9da 100644
> --- a/virt/kvm/dirty_ring.c
> +++ b/virt/kvm/dirty_ring.c
> @@ -55,9 +55,6 @@ static void kvm_reset_dirty_gfn(struct kvm *kvm, u32 slot, u64 offset, u64 mask)
>  	struct kvm_memory_slot *memslot;
>  	int as_id, id;
>  
> -	if (!mask)
> -		return;
> -
>  	as_id = slot >> 16;
>  	id = (u16)slot;
>  
> @@ -109,13 +106,10 @@ int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring,
>  {
>  	u32 cur_slot, next_slot;
>  	u64 cur_offset, next_offset;
> -	unsigned long mask;
> +	unsigned long mask = 0;
>  	struct kvm_dirty_gfn *entry;
>  	bool first_round = true;
>  
> -	/* This is only needed to make compilers happy */
> -	cur_slot = cur_offset = mask = 0;
> -
>  	while (likely((*nr_entries_reset) < INT_MAX)) {
>  		if (signal_pending(current))
>  			return -EINTR;
> @@ -163,14 +157,31 @@ int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring,
>  				continue;
>  			}
>  		}
> -		kvm_reset_dirty_gfn(kvm, cur_slot, cur_offset, mask);
> +
> +		/*
> +		 * Reset the slot for all the harvested entries that have been
> +		 * gathered, but not yet fully processed.
> +		 */
I really like the logs as it took me quite a while figuring out how this part of
the code works :)

Does "processed" mean the entries have been reset, and "gathered" means they've
been read from the ring?

I'm not sure, but do you like this version? e.g.
"Combined reset of the harvested entries that can be identified by curr_slot
plus cur_offset+mask" ?


> +		if (mask)
> +			kvm_reset_dirty_gfn(kvm, cur_slot, cur_offset, mask);
> +
> +		/*
> +		 * The current slot was reset or this is the first harvested
> +		 * entry, (re)initialize the metadata.
> +		 */
What about
"Save the current slot and cur_offset (with mask initialized to 1) to check if
any future entries can be found for a combined reset." ?

>  		cur_slot = next_slot;
>  		cur_offset = next_offset;
>  		mask = 1;
>  		first_round = false;
>  	}
>  
> -	kvm_reset_dirty_gfn(kvm, cur_slot, cur_offset, mask);
> +	/*
> +	 * Perform a final reset if there are harvested entries that haven't
> +	 * been processed. The loop only performs a reset when an entry can't
> +	 * be coalesced, i.e. always leaves at least one entry pending.
The loop only performs a reset when an entry can be coalesced?

> +	 */
> +	if (mask)
> +		kvm_reset_dirty_gfn(kvm, cur_slot, cur_offset, mask);
>  
>  	/*
>  	 * The request KVM_REQ_DIRTY_RING_SOFT_FULL will be cleared
> -- 
> 2.47.1.613.gc27f4b7a9f-goog
>