Re: [PATCH 5/5] selftests: KVM: guest_memfd: test GUEST_MEMFD_FLAG_BIND_NODE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The test `test_bind_node` assumes that NUMA nodes 0 and 1 always exist and are online, which will crash the test on systems with sparse NUMA topologies.
- [Medium] Skipping GUEST_MEMFD_FLAG_BIND_NODE unconditionally in test_guest_memfd_flags() omits testing its rejection on unsupported kernels.
--

commit bc5b1f010d9045d5cca04643d1e37516ee53b8b5
Author: Gregory Price <gourry@gourry.net>

selftests: KVM: guest_memfd: test GUEST_MEMFD_FLAG_BIND_NODE

This commit adds tests for the GUEST_MEMFD_FLAG_BIND_NODE flag in the KVM
guest_memfd selftest. It verifies behavior on NUMA setups, including
faulting onto remote nodes, and checks for invalid arguments. The flag
is skipped in the general test_guest_memfd_flags() loop as it requires
a valid node field.

> diff --git a/tools/testing/selftests/kvm/guest_memfd_test.c b/tools/testing/selftests/kvm/guest_memfd_test.c
> index 1818e0fea5690..b333cb42fab29 100644
> --- a/tools/testing/selftests/kvm/guest_memfd_test.c
> +++ b/tools/testing/selftests/kvm/guest_memfd_test.c
[ ... ]
> @@ -196,6 +196,83 @@ static void test_numa_allocation(int fd, size_t total_size)
>  	kvm_munmap(mem, total_size);
>  }
>  
> +static bool has_bind_node(struct kvm_vm *vm)
> +{
> +	return vm_check_cap(vm, KVM_CAP_GUEST_MEMFD_FLAGS) &
> +	       GUEST_MEMFD_FLAG_BIND_NODE;
> +}
> +
[ ... ]
> +static void test_bind_node(int fd, size_t total_size, int node)
> +{
> +	const unsigned long other_mask = 1UL << (node ? 0 : 1);

[Severity: Medium]
Does this code assume NUMA nodes 0 and 1 always exist and are online, which
could fail the test on systems with sparse NUMA topologies?

By hardcoding the "other" node as 1 (or 0) using
other_mask = 1UL << (node ? 0 : 1), the test assumes these nodes are always
present. On a multi-node system where node 0 is memoryless or only nodes 0
and 2 exist, wouldn't the kvm_set_mempolicy() or guest_memfd creation fail
with EINVAL?

> +	const unsigned long maxnode = BITS_PER_TYPE(other_mask);
> +	bool steer_away = is_multi_numa_node_system();
[ ... ]
> @@ -429,6 +506,10 @@ static void test_guest_memfd_flags(struct kvm_vm *vm)
>  	int fd;
>  
>  	for (flag = BIT(0); flag; flag <<= 1) {
> +		/* BIND_NODE depends on a valid node field, test separately */
> +		if (flag == GUEST_MEMFD_FLAG_BIND_NODE)
> +			continue;
> +

[Severity: Medium]
Does skipping this unconditionally omit testing its rejection on
unsupported kernels?

The loop in test_guest_memfd_flags() is designed to test that all
unsupported flags are correctly rejected with -EINVAL. By bypassing it
entirely with this continue statement, wouldn't this fail to verify
that older kernels without BIND_NODE actually reject the flag? Should
this instead only skip if (flag & valid_flags)?

>  		fd = __vm_create_guest_memfd(vm, page_size, flag);
>  		if (flag & valid_flags) {
>  			TEST_ASSERT(fd >= 0,
> @@ -486,6 +568,8 @@ static void __test_guest_memfd(struct kvm_vm *vm, u64 flags)
>  			gmem_test(mmap_supported, vm, flags);
>  			gmem_test(fault_overflow, vm, flags);
>  			gmem_test(numa_allocation, vm, flags);
> +			if (has_bind_node(vm))
> +				gmem_test_node(bind_node, vm, flags, 0);

[Severity: Medium]
Does hardcoding node 0 here cause the same sparse NUMA topology issue?

Similar to the logic in test_bind_node(), passing 0 as the node assumes
that NUMA node 0 always exists and is online. Could this crash the test on
systems where node 0 does not exist?

>  			__gmem_test(collapse, vm, flags, pmd_size);
>  		} else {
>  			gmem_test(fault_private, vm, flags);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902194657.79075-1-gourry@gourry.net?part=5