Re: [PATCH v8 39/46] KVM: selftests: Test conversion with elevated page refcount

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Test assertion evasion due to `error_offset` zero-initialization masking kernel bugs.
--

commit b2da0256dee5eac26d6a8400d7fa310b8e255837
Author: Ackerley Tng <ackerleytng@google.com>

KVM: selftests: Test conversion with elevated page refcount

This commit adds a selftest to ensure that converting a shared
guest_memfd page to private fails if the page has an elevated reference
count.

> diff --git a/tools/testing/selftests/kvm/x86/guest_memfd_conversions_test.c b/tools/testing/selftests/kvm/x86/guest_memfd_conversions_test.c
> index 99b0023609670..4ebbd29029526 100644
> --- a/tools/testing/selftests/kvm/x86/guest_memfd_conversions_test.c
> +++ b/tools/testing/selftests/kvm/x86/guest_memfd_conversions_test.c
[ ... ]
> @@ -441,6 +441,62 @@ GMEM_CONVERSION_TEST_INIT_SHARED(forked_accesses)
>  #undef TEST_STATE_AWAIT
>  }
>  
> +static void test_convert_to_private_fails(test_data_t *t, u64 pgoff,
> +					  size_t nr_pages,
> +					  u64 expected_error_offset)
> +{
> +	/* +1 to make it anything but expected_error_offset. */
> +	u64 error_offset = expected_error_offset + 1;
> +	u64 offset = pgoff * page_size;
> +	int ret;
> +
> +	do {
> +		ret = __gmem_set_private(t->gmem_fd, offset,
> +					 nr_pages * page_size, &error_offset);
> +	} while (ret == -1 && errno == EINTR);
> +	TEST_ASSERT(ret == -1 && errno == EAGAIN,
> +		    "Wanted EAGAIN on page %lu, got %d (ret = %d)", pgoff,
> +		    errno, ret);
> +	TEST_ASSERT_EQ(error_offset, expected_error_offset);

[Severity: Medium]
Will this poison mechanism work as intended when expected_error_offset is 0?

Looking at the underlying call chain, __gmem_set_private() calls
__gmem_set_memory_attributes(), which unconditionally zero-initializes the
error_offset field before issuing the ioctl:

	struct kvm_memory_attributes2 attr = {
		.attributes = attributes,
		.offset = offset,
		.size = size,
		.flags = 0,
		.error_offset = 0,
	};
	int r;

	r = __gmem_ioctl(fd, KVM_SET_MEMORY_ATTRIBUTES2, &attr);

	/* Copy error_offset regardless of r so caller can check. */
	if (error_offset)
		*error_offset = attr.error_offset;

Since the wrapper ignores the poison value we pass in and overwrites it with 0,
if expected_error_offset happens to be 0 (e.g. for test_page == 0) and a kernel
bug causes KVM to fail to populate error_offset, TEST_ASSERT_EQ(0, 0) will
still pass, incorrectly hiding the bug.

Should the test's poison value be passed down to the ioctl structure, or
should we use a non-zero expected_error_offset to reliably detect if KVM
updated it?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-gmem-inplace-conversion-v8-0-9d2959357853@google.com?part=39