Re: [PATCH v2 20/20] KVM: selftests: Add Xen evtchn vs mmu_notifier race test for HVA caches

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] KVM: selftests: commit message claims 20000 sends but TEST_ITERATIONS is 5000
- [Low] KVM: selftests: xen_evtchn_hva_notify_race_test duplicates memslot race test
- [Low] KVM: selftests: unchecked madvise() and raw mmap() in HVA race test
--

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

KVM: selftests: Add Xen evtchn vs mmu_notifier race test for HVA caches

This commit adds a KVM selftest racing a guest EVTCHNOP_send loop against a
host thread invoking MADV_DONTNEED on the userspace page backing the
HVA-based shared_info and vcpu_info caches.

>     KVM: selftests: Add Xen evtchn vs mmu_notifier race test for HVA caches
>     
>     Race a guest EVTCHNOP_send loop against a host thread hammering
>     MADV_DONTNEED on the userspace page backing HVA-based shared_info and
>     vcpu_info caches. Every madvise zaps the PTE and invalidates both
>     gfn_to_pfn_caches; every delivery must refresh through the window
>     (hva_to_pfn_retry() retries until the invalidation has finished, then
>     faults the page back in) and must succeed. The caches are HVA-based,
>     so no memslot semantics excuse a drop: unlike
>     xen_evtchn_memslot_race_test, which only pins the no-deadlock claim,
>     this test asserts every one of the 20000 sends delivers, and any
>     hypercall bounce to userspace fails the test.
>     
>     This exercises the pfncache invalidation protocol itself — the
>     invalidate_start walk, the reader draining for a cache mid-teardown,
>     and the handshake between a refresh publishing a new mapping and a
>     concurrent invalidation vetoing it — which is where a lifetime bug
>     in the cache would live; on a debug kernel one is expected to
>     surface as a splat rather than a dropped event.
>     
>     Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

[Severity: Low]
The commit message states that this test asserts every one of the 20000 sends
delivers, but in tools/testing/selftests/kvm/x86/xen_evtchn_hva_notify_race_test.c
TEST_ITERATIONS is defined as 5000 and guest_code() runs TEST_ITERATIONS
hypercalls:

#define TEST_ITERATIONS		5000
...
	for (i = 0; i < TEST_ITERATIONS; i++) {
		rc = __xen_hypercall(__HYPERVISOR_event_channel_op,
				     EVTCHNOP_send, &s);

Should the commit message say 5000 sends to match TEST_ITERATIONS and the
runtime estimate comment in main()?

> diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
> index c17408f8d61f5..19dc0744f25f8 100644
> --- a/tools/testing/selftests/kvm/Makefile.kvm
> +++ b/tools/testing/selftests/kvm/Makefile.kvm
> @@ -150,6 +150,7 @@ TEST_GEN_PROGS_x86 += x86/debug_regs
>  TEST_GEN_PROGS_x86 += x86/tsc_msrs_test
>  TEST_GEN_PROGS_x86 += x86/vmx_pmu_caps_test
>  TEST_GEN_PROGS_x86 += x86/xen_evtchn_memslot_race_test
> +TEST_GEN_PROGS_x86 += x86/xen_evtchn_hva_notify_race_test

[Severity: Low]
This isn't a bug, but tools/testing/selftests/kvm/x86/xen_evtchn_hva_notify_race_test.c
duplicates most of tools/testing/selftests/kvm/x86/xen_evtchn_memslot_race_test.c,
including the hypercall definitions, struct evtchn_send, VM and vCPU creation,
KVM_XEN_HVM_CONFIG setup, KVM_XEN_ATTR_TYPE_LONG_MODE setup,
KVM_XEN_ATTR_TYPE_EVTCHN IPI loopback routing, worker thread lifecycle,
alarm() watchdog setup, and the vcpu_run() / get_ucall() loop in main().

Since only the backing memory setup (HVA vs GPA/memslot), the churn thread
body (madvise MADV_DONTNEED vs memslot update), and whether KVM_EXIT_XEN
hypercall bounces are tolerated differ between the two tests, could this HVA
mmu_notifier variant be folded into xen_evtchn_memslot_race_test.c as a test
mode or parameter rather than adding a separate standalone binary?

> diff --git a/tools/testing/selftests/kvm/x86/xen_evtchn_hva_notify_race_test.c b/tools/testing/selftests/kvm/x86/xen_evtchn_hva_notify_race_test.c
> new file mode 100644
> index 0000000000000..c105435f63577
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/xen_evtchn_hva_notify_race_test.c

[ ... ]

> +static void *backing_churn(void *arg)
> +{
> +	void *backing = arg;
> +
> +	while (!atomic_load(&churn_stop))
> +		madvise(backing, PAGE_SIZE, MADV_DONTNEED);
> +
> +	return NULL;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm;
> +	pthread_t thread;
> +	struct ucall uc;
> +	void *backing;
> +	int done = 0;
> +
> +	int xen_caps = kvm_check_cap(KVM_CAP_XEN_HVM);
> +
> +	TEST_REQUIRE(xen_caps & KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA);
> +	TEST_REQUIRE(xen_caps & KVM_XEN_HVM_CONFIG_EVTCHN_SEND);
> +
> +	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> +
> +	/*
> +	 * The shared_info page and the vcpu_info within it live in plain
> +	 * anonymous VMM memory, not in any memslot: nothing but the
> +	 * mmu_notifier churn below can perturb the caches.
> +	 */
> +	backing = mmap(NULL, PAGE_SIZE, PROT_READ | PROT_WRITE,
> +		       MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +	TEST_ASSERT(backing != MAP_FAILED, "mmap of backing page failed");

[ ... ]

> +	atomic_store(&churn_stop, true);
> +	kvm_pthread_join(thread, NULL);
> +	alarm(0);
> +
> +	kvm_vm_free(vm);
> +	return 0;
> +}

[Severity: Low]
In backing_churn(), madvise(backing, PAGE_SIZE, MADV_DONTNEED) does not check
for errors; if madvise() fails, the churn loop would spin without zapping PTEs
or triggering mmu_notifier invalidations, allowing the test to pass without
exercising the race.

In addition, main() open-codes mmap() + TEST_ASSERT() and exits after
kvm_vm_free(vm) without unmapping backing.

Should backing_churn() and main() use the kvm_madvise(), kvm_mmap(), and
kvm_munmap() helpers from kvm_syscalls.h instead?

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