Re: [PATCH v3 07/13] KVM: x86/xen: Use 32-bit locked bts for vcpu_info evtchn_pending_sel

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing dirty page logging for event channel injection breaks live migration.
--

commit 71bb4c7ed046d44bbf5b561e00bbc1a82294213c
Author: David Woodhouse <dwmw@amazon.co.uk>
Subject: KVM: x86/xen: Use 32-bit locked bts for vcpu_info evtchn_pending_sel

This commit replaces a 64-bit locked test-and-set with a 32-bit equivalent
for updating evtchn_pending_sel. This prevents a split-lock exception when
accessing a guest-controlled structure that is only 32-bit aligned.

Closes: https://lore.kernel.org/all/20260604193554.1BA311F00893@smtp.kernel.org

> 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
[ ... ]
> @@ -1879,19 +1888,29 @@ int kvm_xen_set_evtchn_fast(struct kvm_xen_evtchn *xe, struct kvm *kvm)
>  			goto out_rcu;
>  		}
>  
> -		if (has_64bit_shinfo) {
[ ... ]
> -		} else {
[ ... ]
> -		}
> +		/*
> +		 * Explicitly use a 32-bit btsl instead of test_and_set_bit(),
> +		 * which would use btsq on x86-64. The vcpu_info is guest-
> +		 * controlled and only required to be 32-bit aligned, so a
> +		 * 64-bit access could generate a split-lock #AC.
> +		 *
> +		 * Note, this does not apply to the test_and_set_bit() on
> +		 * pending_bits above: that is in the per-VM shared_info, which
> +		 * is page aligned, so the access is guaranteed to be 64-bit
> +		 * aligned.
> +		 */
> +		old = GEN_BINARY_RMWcc(LOCK_PREFIX "btsl",
> +				       *(u32 *)(gpc->khva + vi_pending_sel_ofs),
> +				       c, "Ir", port_word_bit);
> +		if (!old) {
> +			struct vcpu_info *vi = gpc->khva;
> +
> +			/* No need for compat handling */
> +			BUILD_BUG_ON(offsetof(struct vcpu_info, evtchn_upcall_pending) !=
> +				     offsetof(struct compat_vcpu_info, evtchn_upcall_pending));
> +
> +			WRITE_ONCE(vi->evtchn_upcall_pending, 1);
> +			kick_vcpu = true;
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but is it possible that modifying
the guest memory here without dirtying the page breaks live migration?

Looking at kvm_xen_set_evtchn_fast(), it modifies both the shared_info
page (via test_and_set_bit on pending_bits earlier in the function) and
the vcpu_info page (via the new btsl inline asm on gpc->khva here).

Because gpc->khva uses the cached kernel address and bypasses guest page
tables, hardware A/D bits are not set.

If we don't call kvm_gpc_mark_dirty_in_slot(gpc) after modifying these
pages, KVM's dirty page tracking mechanism won't record the changes.

During the pre-copy phase of live migration, could this cause userspace to
skip re-transmitting the page, resulting in the destination VM resuming
with an outdated copy of the shared_info and vcpu_info pages where the
pending interrupt bits are missing?

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