Re: [PATCH v2 07/16] KVM: Store gfn_to_pfn_cache length as an immutable property

[David Woodhouse]

On Thu, 2022-10-13 at 21:12 +0000, Sean Christopherson wrote:
> From: Michal Luczaj <mhal@xxxxxxx>
> 
> Make the length of a gfn=>pfn cache an immutable property of the cache
> to cleanup the APIs and avoid potential bugs, e.g calling check() with a
> larger size than refresh() could put KVM into an infinite loop.

Hm, that's a strange hypothetical bug to be worried about, given the
pattern is usually to have the check() and refresh() within a few lines
of each other with just atomicity/locking stuff in between them.

I won't fight for it, but I quite liked the idea that each user of a
GPC would know how much space *it* is going to access, and provide that
length as a required parameter. I do note you've added a WARN_ON to one
such user, and that's great — but overall, this patch makes that
checking *optional* instead of mandatory.

> All current (and anticipated future) users access the cache with a
> predetermined size, which isn't a coincidence as using a dedicated cache
> really only make sense when the access pattern is "fixed".

In fixing up the runstate area, I've made that not true. Not only does
the runstate area change size at runtime if the guest changes between
32-bit and 64-bit mode, but it also now uses *two* GPCs to cope with a
region that crosses a page boundary, and the size of the first
therefore changes according to how much fits on the tail of the page.

> Add a WARN in kvm_setup_guest_pvclock() to assert that the offset+size
> matches the length of the cache, both to make it more obvious that the
> length really is immutable in that case, and to detect future bugs.
...
> @@ -3031,13 +3030,13 @@ static void kvm_setup_guest_pvclock(struct kvm_vcpu *v,
>  	struct pvclock_vcpu_time_info *guest_hv_clock;
>  	unsigned long flags;
>  
> +	WARN_ON_ONCE(gpc->len != offset + sizeof(*guest_hv_clock));
> +

That ought to be 'gpc->len < offset + sizeof(*guest_hv_clock)' I think?

In the case where we are writing a clock *within* a mapped Xen
vcpu_info structure, it doesn't have to be at the *end* of that
structure. I think the xen_shinfo_test should have caught that?

Attachment: smime.p7s
Description: S/MIME cryptographic signature