Re: [PATCH v5 25/34] KVM: x86/xen: Prevent runstate times from becoming negative

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Double-counting and state corruption of KVM Xen runstate times due to reentrancy from preempt notifiers.
--

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

KVM: x86/xen: Handle retrograde KVM clock in runstate accounting

When kvm_xen_update_runstate() is invoked to set a vCPU's runstate, the
time spent in the previous runstate is accounted. This commit handles
cases where the KVM clock goes backwards by refraining from updating
runstate times until the clock catches up with runstate_entry_time.

> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index 82e34edbfdbd..b1d67ece5db3 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c

[ ... ]

> @@ -586,29 +586,45 @@ void kvm_xen_update_runstate(struct kvm_vcpu *v, int state)

[ ... ]

>  	vx->runstate_times[vx->current_runstate] += delta_ns;
> -	vx->current_runstate = state;
>  	vx->runstate_entry_time = now;
>  
> + update_guest:
> +	vx->current_runstate = state;

[Severity: High]
This isn't a bug introduced by this patch, but can this read-modify-write
sequence be safely executed with preemption enabled?

If record_steal_time() calls kvm_xen_update_runstate() with preemption
enabled, and the thread is preempted in the middle of these updates:

Original thread starts:
    vx->runstate_times[vx->current_runstate] += delta_ns;
    <preempted here>

Preempt notifier kvm_sched_out() triggers:
    kvm_xen_runstate_set_preempted() re-enters kvm_xen_update_runstate()
    updates vx->runstate_entry_time and vx->current_runstate

Original thread resumes:
    vx->runstate_entry_time = now;
    vx->current_runstate = state;

Would this cause the original execution to overwrite the state variables
with its old, pre-preemption timestamps and state? It seems this would
obliterate the preempt notifier's update and force runstate_entry_time
backwards, potentially causing KVM to double-count elapsed time on the
next accounting cycle.

Does this state tracking need to be protected by disabling preemption or
using a lock to avoid this reentrancy problem?

> +	vx->last_steal = run_delay;
>  	if (vx->runstate_cache.active)
>  		kvm_xen_update_runstate_guest(v, state == RUNSTATE_runnable);
>  }

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