Re: [PATCH v3 03/11] KVM: x86/xen: Wait out memslot updates in the Xen attribute ioctls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] kvm: x86/xen: torn guest wallclock read due to concurrent writers
--

commit dd1c2ed3d111f222bc79a42aa91b7852c3e338f8
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: x86/xen: Wait out memslot updates in the Xen attribute ioctls

This commit updates Xen attribute ioctls to wait out memslot updates. To
prevent deadlocks, the waiting is performed without holding the xen_lock,
explicitly allowing some operations to run concurrently.

> This also means two kvm_xen_shared_info_init() calls can publish the
> wallclock concurrently, no longer excluded by xen_lock. That is
> harmless: concurrent writers compute the same epoch, modulo
> nanoseconds of jitter, and each re-reads long_mode to pick the
> destination, so the location for the final mode is always freshly
> written.

[Severity: Medium]
While the computed epochs are nearly identical, could concurrent writers
cause a torn read for the guest if the clock readings straddle a second
boundary?

If thread A and thread B compute the time concurrently, they might obtain
readings on opposite sides of a second boundary:
  Thread A: sec = 100, nsec = 999999999
  Thread B: sec = 101, nsec = 0

Because they both write to the shared structure non-atomically, their
updates can interleave:
  Thread A writes wc->sec = 100
  Thread B writes wc->nsec = 0

This would produce a time shifted by a full second (sec = 100, nsec = 0).

Because both threads increment the version counter using the same
read-modify-write pattern, the seqlock ends up in a valid, even state.
Would the guest then accept this torn time since the writers are not
serialized?

> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -44,17 +44,24 @@ static int kvm_xen_shared_info_init(struct kvm *kvm)
>  	u32 wc_version;
>  	u64 wall_nsec;
>  	int ret = 0;
> -	int idx = srcu_read_lock(&kvm->srcu);
> +	int idx;
>  
> -	read_lock_irq(&gpc->lock);
> -	while (!kvm_gpc_check(gpc, PAGE_SIZE)) {
> +	/*
> +	 * kvm_gpc_check() consults the memslots, so take kvm->srcu around
> +	 * each check and the write through the mapping — but not across
> +	 * the refresh, which may wait for a memslot update to complete.
> +	 */
> +	for (;;) {
> +		idx = srcu_read_lock(&kvm->srcu);
> +		read_lock_irq(&gpc->lock);

[Severity: Medium]
Is using an rwlock for reading here what directly enables the concurrent
writes to the guest wallclock? Since multiple CPUs can acquire the read lock
simultaneously, the seqlock in the guest is no longer protected by serialized
writers.

[ ... ]

> @@ -98,9 +105,9 @@ static int kvm_xen_shared_info_init(struct kvm *kvm)
>  
>  	wc->version = wc_version + 1;
>  	read_unlock_irq(&gpc->lock);
> +	srcu_read_unlock(&kvm->srcu, idx);
>  
>  out:
> -	srcu_read_unlock(&kvm->srcu, idx);
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016145646.579369-1-dwmw2@infradead.org?part=3