Re: [PATCH RFC v2 30/50] KVM: selftests: Add basic test for sample memory provider

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KVM: selftests: open_provider_dev() reimplements __open_path_or_exit()
--

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

KVM: selftests: Add basic test for sample memory provider

This commit adds a selftest for /dev/mem_provider_sample to verify basic file
creation across multiple sizes, stats reporting via MPS_GET_STATS, and page
presence toggling via MPS_SET_PRESENT.

> diff --git a/tools/testing/selftests/kvm/mem_provider_sample_test.c b/tools/testing/selftests/kvm/mem_provider_sample_test.c
> new file mode 100644
> index 0000000000000..932ea3f30f494
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/mem_provider_sample_test.c
> @@ -0,0 +1,147 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Tests for the sample memory provider (/dev/mem_provider_sample).
> + */
> +#include <fcntl.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/ioctl.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> +
> +#include "test_util.h"
> +#include "../../../../samples/kvm/mem_provider_sample.h"
> +
> +#define DEV_PATH "/dev/mem_provider_sample"
> +
> +static int open_provider_dev(void)
> +{
> +	int fd = open(DEV_PATH, O_RDWR);
> +
> +	if (fd < 0) {
> +		TEST_REQUIRE(errno != ENOENT && errno != ENODEV);
> +		TEST_FAIL("Failed to open %s: %d (%s)", DEV_PATH, errno,
> +			  strerror(errno));
> +	}
> +	return fd;
> +}

[Severity: Medium]
Could open_provider_dev() use __open_path_or_exit() from
tools/testing/selftests/kvm/lib/kvm_util.c instead of hand-rolling the
open() error handling?

tools/testing/selftests/kvm/lib/kvm_util.c:__open_path_or_exit() {
	fd = open(path, flags);
	if (fd < 0)
		goto error;

	return fd;

error:
	if (errno == EACCES || errno == ENOENT)
		ksft_exit_skip("- Cannot open '%s': %s.  %s\n",
			       path, strerror(errno),
			       errno == EACCES ? "Root required?" : enoent_help);
	TEST_FAIL("Failed to open '%s'", path);
}

Because samples/kvm/mem_provider_sample.c registers /dev/mem_provider_sample
via misc_register() with default 0600 permissions, running the test as an
unprivileged user causes open() in main()->open_provider_dev() to fail with
EACCES. That bypasses TEST_REQUIRE(errno != ENOENT && errno != ENODEV) and
calls TEST_FAIL() -> test_assert() in
tools/testing/selftests/kvm/lib/assert.c:

tools/testing/selftests/kvm/lib/assert.c:test_assert() {
	if (!(exp)) {
		va_start(ap, fmt);

		fprintf(stderr, "\n==== Test Assertion Failure ====\n"
			"  %s:%u: %s\n"
			"  pid=%d tid=%d errno=%d - %s\n",
			file, line, exp_str, getpid(), kvm_gettid(),
			errno, strerror(errno));
		test_dump_stack();
		...
		if (errno == EACCES) {
			print_skip("Access denied - Exiting");
			exit(KSFT_SKIP);
		}
		exit(254);
	}
}

In test_assert(), fprintf() and test_dump_stack() run before the
errno == EACCES check, printing an assertion failure stack trace and
clobbering errno so the test exits with 254 instead of skipping cleanly. In
addition, when the sample module is not loaded,
TEST_REQUIRE(errno != ENOENT && errno != ENODEV) only prints the raw
expression without the missing device path or Kconfig hint.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-gmem-alternative-providers-v2-0-5dd1dc6c660a@google.com?part=30