Re: [RFC PATCH 14/14] KVM: selftests: Test guest_memfd mapped by iommufd

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] KVM: selftests: host_access() reimplements TEST_EXPECT_SIGBUS() poorly
--

commit 039c70671ebb1232a61b8053400cfbcf04938b2b
Author: Fred Griffoul <fgriffo@amazon.co.uk>

KVM: selftests: Test guest_memfd mapped by iommufd

This commit adds gmem_conversion_iommufd_test to verify that iommufd mock
domain device mappings of a guest_memfd stay synchronized across conversions,
allocations, hole punching, memory failure, and PFN provider operations.

> diff --git a/tools/testing/selftests/kvm/x86/gmem_conversion_iommufd_test.c b/tools/testing/selftests/kvm/x86/gmem_conversion_iommufd_test.c
> new file mode 100644
> index 0000000000000..5d17b0e2dd438
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/gmem_conversion_iommufd_test.c
> @@ -0,0 +1,956 @@

[ ... ]

> +/*
> + * Read @addr, or write @val to it, through a host mapping.  Return the
> + * signal that the access raised, or 0.
> + */
> +static int host_access(volatile uint64_t *addr, bool write, uint64_t val)
> +{
> +	struct sigaction sa = { .sa_handler = host_sig }, old_bus, old_segv;
> +	int sig;
> +
> +	sigaction(SIGBUS, &sa, &old_bus);
> +	sigaction(SIGSEGV, &sa, &old_segv);
> +	sig = sigsetjmp(host_jmp, 1);
> +	if (!sig) {
> +		if (write)
> +			*addr = val;
> +		else
> +			(void)*addr;
> +	}
> +	sigaction(SIGBUS, &old_bus, NULL);
> +	sigaction(SIGSEGV, &old_segv, NULL);
> +	return sig;
> +}

[ ... ]

> +#define host_read_faults(p)	(host_access(p, false, 0) == SIGBUS)
> +#define host_write_faults(p, v)	(host_access(p, true, v) == SIGBUS)

[ ... ]

> +static void scenario_readonly(void)
> +{
> +	struct vmm v = { .name = "pfn" };
> +	volatile uint64_t *page;
> +
> +	pr_info("   read-only frames on a PFN mode provider\n");
> +	vmm_setup(&v, BACKING_PFN);
> +	page = (volatile uint64_t *)(v.hva + RO_OFF);
> +
> +	*page = MAGIC;
> +	vmm_run(&v, 0, 0, RO_GPA);
> +	TEST_ASSERT(expect_sync_then_done(&v) == MAGIC,
> +		    "guest does not read the host's write");
> +
> +	mps_set(v.child, MPS_SET_READONLY, RO_OFF, PAGE, true);
> +	TEST_ASSERT(vmm_iova_mapped(&v, IOVA + RO_OFF, PAGE, true),
> +		    "read-only page not mapped for the device");
> +	vmm_run(&v, 0, 0, RO_GPA);
> +	TEST_ASSERT(expect_sync_then_done(&v) == MAGIC,
> +		    "guest cannot read a read-only page");
> +	vmm_run(&v, RO_GPA, ~MAGIC, 0);
> +	expect_memory_fault(&v, RO_GPA);
> +	TEST_ASSERT(host_write_faults(page, ~MAGIC),
> +		    "host write to a read-only page did not fault");
> +	TEST_ASSERT(*page == MAGIC, "write to a read-only page landed");
> +
> +	/* Writable again: the guest retries the same write. */
> +	mps_set(v.child, MPS_SET_READONLY, RO_OFF, PAGE, false);
> +	vmm_resume(&v);
> +	expect_done(&v);
> +	TEST_ASSERT(*page == ~MAGIC, "write after clearing read only lost");
> +	TEST_ASSERT(!host_write_faults(page, MAGIC),
> +		    "host write after clearing read only faulted");

[Severity: Low]
Could this test use TEST_EXPECT_SIGBUS() from test_util.h instead of
reimplementing signal-catching accesses in host_access()?

In host_access(), both SIGBUS and SIGSEGV are caught, while
host_write_faults() only checks whether the return value equals SIGBUS.  In
scenario_readonly(), if the write in !host_write_faults(page, MAGIC) raises
SIGSEGV instead of succeeding, host_access() catches it and returns SIGSEGV,
causing !host_write_faults() to evaluate to true without checking that
*page == MAGIC afterward.

Also, in scenario_readonly(), mps_set(..., MPS_SET_READONLY, ..., true)
revokes the host mapping before host_write_faults(page, ~MAGIC) runs.
Because the write happens before *page == MAGIC faults in a present read-only
PTE, it only exercises kvm_gmem_pfn_fault() on an unmapped PTE and leaves
kvm_gmem_pfn_mkwrite() on a present read-only PTE untested.  Should the host
read happen before the write attempt (or should another write attempt follow
the read), and should a direct store *page = MAGIC be used once the page is
writable again?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027154649.91571-1-griffoul@gmail.com?part=14