Re: [PATCH] KVM: selftests: Test partial KVM_CLEAR_DIRTY_LOG requests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Hardcoded page offsets and lengths cause memslot bounds violations and guest memory aborts when host_page_size > guest_page_size.
- [Medium] Test falsely assumes KVM_DIRTY_LOG_INITIALLY_SET is always supported if KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2 is present.
--

commit 7a613b42770cbf4a2e9e3cd0b003c952b47213c3
Author: Tharit Tangkijwanichakul <tharitt97@gmail.com>

KVM: selftests: Test partial KVM_CLEAR_DIRTY_LOG requests

This commit adds a new test mode to verify that KVM_CLEAR_DIRTY_LOG correctly
applies targeted clearing. It creates a memory slot with
KVM_DIRTY_LOG_INITIALLY_SET, submits a 64-page clear request at offset 64,
and verifies that only the requested page is cleared and subsequently
re-dirtied.

> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index af5eb0334a74f..a12b538c3144e 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -815,6 +815,106 @@ static void run_test(enum vm_guest_mode mode, void *arg)
>  	kvm_vm_free(vm);
>  }
>  
> +/* Use the 2nd 64-page range to exercise a non-zero offset. */
> +#define TEST_CLEAR_LOG_FIRST_PAGE	64
> +#define TEST_CLEAR_LOG_NUM_PAGES	64
> +#define TEST_CLEAR_LOG_BIT		1
> +#define TEST_CLEAR_LOG_PAGE		(TEST_CLEAR_LOG_FIRST_PAGE + \
> +					 TEST_CLEAR_LOG_BIT)
> +static void guest_code_clear_log(void)
> +{
> +	u64 addr = guest_test_virt_mem + TEST_CLEAR_LOG_PAGE * host_page_size;
> +
> +	while (true) {
> +		vcpu_arch_put_guest(*(u64 *)addr, 1);

[Severity: Medium]
Will this result in an out-of-bounds guest memory access on architectures where
the host page size is larger than the guest page size?

When run_clear_log_partial_test() calculates the memory size using
guest_num_pages = vm_adjust_num_guest_pages(mode, 3 * BITS_PER_LONG), the
memslot may be severely undersized. For example, on an architecture with a
64K host page size and 4K guest page size, 192 guest pages is equivalent to
768KB, which is only 12 host pages.

Since TEST_CLEAR_LOG_PAGE is hardcoded to 65, calculating addr as
guest_test_virt_mem + TEST_CLEAR_LOG_PAGE * host_page_size places it far beyond
the mapped 768KB memslot mapping, which can cause a guest data abort.

> +		GUEST_SYNC(1);
> +	}
> +}
> +
> +/*
> + * Verify KVM_CLEAR_DIRTY_LOG only clears (and re-arms) the requested range:
> + * with KVM_DIRTY_LOG_INITIALLY_SET every page starts dirty; clearing a single
> + * page must leave its neighbours dirty and must write-protect the page so the
> + * next guest write re-dirties it.
> + */
> +static void run_clear_log_partial_test(enum vm_guest_mode mode, void *arg)
> +{
> +	unsigned long *bmap, *clear_bmap;
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm;
> +	u64 page;
> +
> +	guest_num_pages = vm_adjust_num_guest_pages(mode, 3 * BITS_PER_LONG);
> +
> +	vm = create_vm(mode, &vcpu, 2 * guest_num_pages, guest_code_clear_log);
> +
> +	guest_page_size = vm->page_size;
> +	host_page_size = getpagesize();
> +	host_num_pages = vm_num_host_pages(mode, guest_num_pages);
> +
> +	guest_test_virt_mem = DEFAULT_GUEST_TEST_MEM;
> +	guest_test_phys_mem = (vm->max_gfn - guest_num_pages) * guest_page_size;
> +	guest_test_phys_mem = align_down(guest_test_phys_mem, host_page_size);

[ ... ]

> +	kvm_vm_get_dirty_log(vm, TEST_MEM_SLOT_INDEX, bmap);
> +	for (page = 0; page < host_num_pages; page++)
> +		TEST_ASSERT(test_bit_le(page, bmap),
> +			    "Page %lu should be dirty after INITIALLY_SET", page);

[Severity: Medium]
Does this assertion assume that KVM_DIRTY_LOG_INITIALLY_SET is always supported
if KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2 is present?

The clear-log test mode checks for KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2, but it
does not explicitly check if KVM_DIRTY_LOG_INITIALLY_SET is supported before
running this test.

On older kernels that support manual protect but not INITIALLY_SET, 
kvm_vm_get_dirty_log() will return a clean bitmap, which could cause this
assertion to falsely fail instead of gracefully skipping.

> +
> +	__set_bit_le(TEST_CLEAR_LOG_BIT, clear_bmap);
> +	kvm_vm_clear_dirty_log(vm, TEST_MEM_SLOT_INDEX, clear_bmap,
> +			       TEST_CLEAR_LOG_FIRST_PAGE,
> +			       TEST_CLEAR_LOG_NUM_PAGES);

[Severity: Medium]
Can passing TEST_CLEAR_LOG_FIRST_PAGE (64) cause kvm_vm_clear_dirty_log() to
fail with -EINVAL on systems where the host page size exceeds the guest page
size?

As mentioned above, if the memslot only has 12 host pages (e.g., when using a
64K host and 4K guest), a first_page index of 64 exceeds the memslot bounds.

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