Re: [RFC PATCH 09/11] selftests/kvm: gmem_provider KVM-only tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The test does not check for partial completion of KVM_SEV_SNP_LAUNCH_UPDATE, leading to potentially silent incomplete memory initialization.
- [Medium] The test fails to assert KVM_EXIT_MEMORY_FAULT on the revoke path because the validation is placed in an unreachable 'else' branch.
--

--- Patch [9]: [RFC PATCH 09/11] selftests/kvm: gmem_provider KVM-only tests ---
commit 93ae3986a0340a4d14084b20d7839ed583b0c01b
Author: David Woodhouse <dwmw@amazon.co.uk>

selftests/kvm: gmem_provider KVM-only tests

Three selftests that exercise the guest_memfd provider ABI purely from
the KVM side (no iommufd, no assigned devices). These tests verify the
live-update survivability, hugepage mappings, and memory revocation.

> diff --git a/tools/testing/selftests/kvm/x86/gmem_provider_revoke_test.c b/tools/testing/selftests/kvm/x86/gmem_provider_revoke_test.c
> index 0000000000000..7e711ea526748 100644
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/gmem_provider_revoke_test.c
[ ... ]
> +	/* 2) Revoke: mark absent and zap the guest NPT (provider->KVM). */
> +	req = (struct gmem_provider_present){ .offset = 0, .len = 4096, .present = 0 };
> +	r = ioctl(gmem_fd, GMEM_PROVIDER_SET_PRESENT, &req);
> +	TEST_ASSERT(!r, "revoke ioctl failed, errno %d", errno);
> +
> +	/* 3) Guest re-reads -> re-fault into absent get_pfn -> must NOT see MAGIC. */
> +	r = _vcpu_run(vcpu);
> +	if (r) {
> +		pr_info("revoked: vcpu_run failed (errno %d) -- guest faulted, as expected\n",
> +			errno);
> +	} else {
> +		TEST_ASSERT(vcpu->run->exit_reason == KVM_EXIT_MEMORY_FAULT,
> +			    "revoked: expected fault or KVM_EXIT_MEMORY_FAULT, got %u (%s)",
> +			    vcpu->run->exit_reason,
> +			    exit_reason_str(vcpu->run->exit_reason));
> +		pr_info("revoked: KVM_EXIT_MEMORY_FAULT as expected\n");
> +	}

[Severity: Medium]
Since KVM_EXIT_MEMORY_FAULT always causes the KVM_RUN ioctl to return -1
with errno EFAULT, won't _vcpu_run(vcpu) return a non-zero value here?

If so, this execution path would take the if (r) branch, completely bypassing
the exit reason validation in the else block. Should this check be moved or
adjusted to properly assert that the exit reason is indeed
KVM_EXIT_MEMORY_FAULT?

> +
> +	/* 4) Restore: mark present again. */
> +	req.present = 1;
> +	r = ioctl(gmem_fd, GMEM_PROVIDER_SET_PRESENT, &req);
> +	TEST_ASSERT(!r, "restore ioctl failed, errno %d", errno);
> +

[ ... ]
> diff --git a/tools/testing/selftests/kvm/x86/gmem_provider_test.c b/tools/testing/selftests/kvm/x86/gmem_provider_test.c
> index 0000000000000..d7caa11616dfe 100644
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/gmem_provider_test.c
[ ... ]
> +	struct kvm_sev_snp_launch_start start = { .policy = 0x30000 };
> +	ret = sev_ioctl(vm_fd, sev_fd, KVM_SEV_SNP_LAUNCH_START, &start);
> +	if (ret) { perror("SNP_LAUNCH_START"); goto out; }
> +
> +	printf("[VM%d] SNP_LAUNCH_UPDATE (code + zero, %luMB)\n",
> +	       vm_num, GUEST_MEM_SIZE >> 20);
> +	struct kvm_sev_snp_launch_update update = {
> +		.gfn_start = 0,
> +		.uaddr = (uint64_t)(unsigned long)src,
> +		.len = PAGE_SIZE_4K,
> +		.type = KVM_SEV_SNP_PAGE_TYPE_NORMAL,
> +	};
> +	ret = sev_ioctl(vm_fd, sev_fd, KVM_SEV_SNP_LAUNCH_UPDATE, &update);
> +	if (ret) { perror("SNP_LAUNCH_UPDATE code"); goto out; }

[Severity: Medium]
If a signal interrupts the KVM_SEV_SNP_LAUNCH_UPDATE ioctl, KVM might
return 0 but leave update.len > 0 indicating a partial completion.

Does this code need a loop to check update.len and retry the remaining
length to prevent silent partial initialization?

> +
> +	struct kvm_sev_snp_launch_update update_zero = {
> +		.gfn_start = 1,
> +		.uaddr = (uint64_t)(unsigned long)(src + PAGE_SIZE_4K),
> +		.len = GUEST_MEM_SIZE - PAGE_SIZE_4K,
> +		.type = KVM_SEV_SNP_PAGE_TYPE_ZERO,
> +	};
> +	ret = sev_ioctl(vm_fd, sev_fd, KVM_SEV_SNP_LAUNCH_UPDATE, &update_zero);
> +	if (ret) { perror("SNP_LAUNCH_UPDATE zero"); goto out; }

[Severity: Medium]
Similarly, could a signal interrupting this update_zero ioctl cause a partial
completion here as well? If the return value is 0 but update_zero.len is
still greater than 0, the rest of the 16MB guest memory would be left
uninitialized, potentially causing flaky test failures later.

> +
> +	struct kvm_sev_snp_launch_finish finish = { 0 };
> +	ret = sev_ioctl(vm_fd, sev_fd, KVM_SEV_SNP_LAUNCH_FINISH, &finish);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/f09647f427565c1789a6979470045aab78661563.1784194290.git.dwmw@amazon.co.uk?part=9