Re: [PATCH v2 05/11] KVM: x86/xen: Don't lose the timer event when delivery fails transiently

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kvm: x86/xen: Stale hrtimer expiry used when re-arming past-due timer in GET_ATTR
--

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

KVM: x86/xen: Don't lose the timer event when delivery fails transiently

This commit modifies the KVM Xen timer injection logic to retry deferred
timer events if the event channel fails to set due to transient issues like
memslot updates. It clears the pending flag on unrecoverable errors while
preserving the expiration to allow migration and retry. It also updates the
GET_ATTR timer path to avoid re-arming the hrtimer when delivery is pending.

> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index cd1063d659cc5..f1be9022bc6fe 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c

[ ... ]

> @@ -1296,8 +1319,20 @@ int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
>  		 * That's fine, as the guest won't even have had a chance
>  		 * to run and handle the interrupt. Asserting an already
>  		 * pending event channel is idempotent.
> +		 *
> +		 * Don't re-arm if the flush above failed transiently and
> +		 * left the timer pending: delivery is owned by the retry
> +		 * on the guest entry path, and an immediate hrtimer fire
> +		 * would race it — the callback's timer_pending suppression
> +		 * check is not serialized against the entry path, so a
> +		 * late callback could re-assert a port the entry path had
> +		 * already delivered and the guest already consumed.
> +		 *
> +		 * An expiry already in the past fires immediately: the
> +		 * event is still owed.
>  		 */
> -		if (vcpu->arch.xen.timer_expires)
> +		if (vcpu->arch.xen.timer_expires &&
> +		    !atomic_read(&vcpu->arch.xen.timer_pending))
>  			hrtimer_start_expires(&vcpu->arch.xen.timer,
>  					      HRTIMER_MODE_ABS_HARD);

[Severity: High]
Does this re-arm the timer using a stale expiry time if the original timer was
set in the past?

In kvm_xen_start_timer(), if a guest sets a timer that is already in the past:

	if (delta <= 0)
		xen_timer_callback(&vcpu->arch.xen.timer);
	else
		hrtimer_start(&vcpu->arch.xen.timer,
			      ktime_add_ns(kernel_now, delta),
			      HRTIMER_MODE_ABS_HARD);

KVM directly calls xen_timer_callback() without calling hrtimer_start().
Bypassing hrtimer_start() means the internal expires field of the struct
hrtimer is not updated, retaining whatever previous timestamp it had.

If xen_timer_callback() fails to deliver the event, and then
kvm_xen_inject_timer_irqs() attempts delivery but fails with -EINVAL, this
patch clears timer_pending but keeps timer_expires > 0.

If userspace later calls KVM_XEN_VCPU_GET_ATTR, it sees timer_expires > 0 and
timer_pending == 0, and attempts to re-arm the timer here using
hrtimer_start_expires().

Because the internal hrtimer expiry was never updated, would
hrtimer_start_expires() arm the timer using the stale future expiry time,
effectively delaying the past-due timer and causing it to hang on the host?

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