Re: [PATCH v7 20/36] 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:
- [Medium] Cross-thread arithmetic on per-task `run_delay` leads to garbage `steal_ns` values and corrupts runstate accounting.
--

--- Patch [20]: [PATCH v7 20/36] KVM: x86/xen: Prevent runstate times from becoming negative ---
commit 1979f0260ec16f7c2af18f6f359a100b98ca0b72
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: x86/xen: Prevent runstate times from becoming negative

This commit handles situations where the KVM clock or run_delay goes backwards
by refraining from updating runstate times and clamping steal time. It adds
logic to prevent steal_ns from exceeding the total elapsed delta_ns.

> Also clamp steal_ns to delta_ns to prevent steal time from exceeding
> the total elapsed time, and handle negative steal_ns (which can happen
> if run_delay goes backwards across a scheduler update).

[Severity: Medium]
This is a pre-existing issue, but is the negative steal_ns actually caused by
run_delay going backwards, or is it caused by cross-thread arithmetic?

When kvm_xen_update_runstate() is called via ioctl (such as from
kvm_xen_vcpu_set_attr()), current evaluates to the ioctl/migration thread,
not the vCPU thread.

> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index 694b31c1fcc9a..0670d5e50b883 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -588,29 +588,45 @@ void kvm_xen_update_runstate(struct kvm_vcpu *v, int state)
>  {
>  	struct kvm_vcpu_xen *vx = &v->arch.xen;
>  	u64 now = get_kvmclock_ns(v->kvm);
> -	u64 delta_ns = now - vx->runstate_entry_time;
>  	u64 run_delay = current->sched_info.run_delay;
> +	s64 delta_ns = now - vx->runstate_entry_time;
> +	s64 steal_ns = run_delay - vx->last_steal;

[Severity: Medium]
This is a pre-existing issue, but does this cross-thread subtraction
corrupt the steal time calculation?

If the previous call was from the ioctl thread, vx->last_steal is seeded with
that thread's run_delay. When the vCPU thread later runs and executes this,
steal_ns becomes:

    steal_ns = VCPU_run_delay - monitor_run_delay

Since these are run_delay values from two completely different threads, the
result is garbage (often negative, or massively positive).

[ ... ]

> -	if (vx->current_runstate == RUNSTATE_running) {
> -		u64 steal_ns = run_delay - vx->last_steal;
> +	if (vx->current_runstate == RUNSTATE_running && steal_ns > 0) {
> +		if (steal_ns > delta_ns)
> +			steal_ns = delta_ns;

[Severity: Medium]
While this clamp prevents integer underflow/overflow, will a large positive
garbage steal_ns from the cross-thread subtraction above silently convert
100% of the elapsed delta_ns into stolen time?

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