[PATCH] selftests/kvm: check fopen() return in get_run_delay()

[PATCH] selftests/kvm: check fopen() return in get_run_delay()

[zhangchujun]

get_run_delay() passes the FILE pointer returned by fopen() to
fscanf() and fclose() without checking it for NULL. If the
schedstat file cannot be opened, the program dereferences NULL
and crashes.

The comment above the fscanf() call already says to return
MIN_RUN_DELAY_NS upon failure just to be safe, so do exactly
that when fopen() fails.

Signed-off-by: zhangchujun <zhangchujun@cmss.chinamobile.com>

diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
index f5f7189f96a8..2827f7d7d905 100644
--- a/tools/testing/selftests/kvm/lib/test_util.c
+++ b/tools/testing/selftests/kvm/lib/test_util.c
@@ -421,6 +421,8 @@ long get_run_delay(void)
 	sprintf(path, "/proc/%ld/schedstat", (long)kvm_gettid());
 	fp = fopen(path, "r");
 	/* Return MIN_RUN_DELAY_NS upon failure just to be safe */
+	if (!fp)
+		return MIN_RUN_DELAY_NS;
 	if (fscanf(fp, "%ld %ld ", &val[0], &val[1]) < 2)
 		val[1] = MIN_RUN_DELAY_NS;
 	fclose(fp);
-- 
2.50.1.windows.1

Re: [PATCH] selftests/kvm: check fopen() return in get_run_delay()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/kvm: infinite loop when fopen() fails in get_run_delay()
--

commit fed2657e5d999eb78e5e508f23ea617f2787e14a
Author: zhangchujun <zhangchujun@cmss.chinamobile.com>

selftests/kvm: check fopen() return in get_run_delay()

This commit adds a NULL check on the FILE pointer returned by fopen() in
get_run_delay() so that it returns MIN_RUN_DELAY_NS instead of passing NULL to
fscanf() and fclose() when /proc/<tid>/schedstat cannot be opened.

> diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
> index f5f7189f96a84..2827f7d7d905e 100644
> --- a/tools/testing/selftests/kvm/lib/test_util.c
> +++ b/tools/testing/selftests/kvm/lib/test_util.c
> @@ -421,6 +421,8 @@ long get_run_delay(void)
>  	sprintf(path, "/proc/%ld/schedstat", (long)kvm_gettid());
>  	fp = fopen(path, "r");
>  	/* Return MIN_RUN_DELAY_NS upon failure just to be safe */
> +	if (!fp)
> +		return MIN_RUN_DELAY_NS;

[Severity: Medium]
If fopen() fails to open /proc/<tid>/schedstat (for instance, when
CONFIG_SCHED_INFO is disabled in the host kernel), will returning the constant
MIN_RUN_DELAY_NS here cause the callers of get_run_delay() to hang in an
infinite loop?

get_run_delay() reads the calling thread's cumulative scheduler run_delay
counter, and both callers in the tree wait for subsequent readings to advance
past an initial baseline.

In tools/testing/selftests/kvm/steal_time.c:main():

		run_delay = get_run_delay();
		kvm_pthread_create(&thread, NULL, do_steal_time, NULL);
		kvm_pthread_getaffinity(thread, &cpuset);
		TEST_ASSERT(CPU_COUNT(&cpuset) == 1 && CPU_ISSET(cpu, &cpuset),
			    "Worker failed to inherit parent's CPU affinity");

		do
			sched_yield();
		while (get_run_delay() - run_delay < MIN_RUN_DELAY_NS);

When get_run_delay() returns MIN_RUN_DELAY_NS on every call,
get_run_delay() - run_delay evaluates to 0, which is always less than
MIN_RUN_DELAY_NS, causing this loop to spin forever.

Similarly, in tools/testing/selftests/kvm/x86/xen_shinfo_test.c:main():

				rundelay = get_run_delay() + MIN_STEAL_TIME;
				do {
					sched_yield();
				} while (get_run_delay() < rundelay);

Here, rundelay becomes MIN_RUN_DELAY_NS + MIN_STEAL_TIME while get_run_delay()
continues returning MIN_RUN_DELAY_NS, which also spins forever.

Should get_run_delay() instead fail or skip via TEST_ASSERT() or
TEST_REQUIRE() when fopen() fails, similar to other file-opening helpers in
tools/testing/selftests/kvm/lib/test_util.c such as get_sysfs_val() and
get_def_hugetlb_pagesz()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104080721.2169-1-zhangchujun@cmss.chinamobile.com?part=1

Re: [PATCH] selftests/kvm: check fopen() return in get_run_delay()

[Sean Christopherson]

On Wed, Nov 04, 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] selftests/kvm: infinite loop when fopen() fails in get_run_delay()
> --
> 
> commit fed2657e5d999eb78e5e508f23ea617f2787e14a
> Author: zhangchujun <zhangchujun@cmss.chinamobile.com>
> 
> selftests/kvm: check fopen() return in get_run_delay()
> 
> This commit adds a NULL check on the FILE pointer returned by fopen() in
> get_run_delay() so that it returns MIN_RUN_DELAY_NS instead of passing NULL to
> fscanf() and fclose() when /proc/<tid>/schedstat cannot be opened.
> 
> > diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
> > index f5f7189f96a84..2827f7d7d905e 100644
> > --- a/tools/testing/selftests/kvm/lib/test_util.c
> > +++ b/tools/testing/selftests/kvm/lib/test_util.c
> > @@ -421,6 +421,8 @@ long get_run_delay(void)
> >  	sprintf(path, "/proc/%ld/schedstat", (long)kvm_gettid());
> >  	fp = fopen(path, "r");
> >  	/* Return MIN_RUN_DELAY_NS upon failure just to be safe */
> > +	if (!fp)
> > +		return MIN_RUN_DELAY_NS;
> 
> [Severity: Medium]
> If fopen() fails to open /proc/<tid>/schedstat (for instance, when
> CONFIG_SCHED_INFO is disabled in the host kernel), will returning the constant
> MIN_RUN_DELAY_NS here cause the callers of get_run_delay() to hang in an
> infinite loop?
> 
> get_run_delay() reads the calling thread's cumulative scheduler run_delay
> counter, and both callers in the tree wait for subsequent readings to advance
> past an initial baseline.
> 
> In tools/testing/selftests/kvm/steal_time.c:main():
> 
> 		run_delay = get_run_delay();
> 		kvm_pthread_create(&thread, NULL, do_steal_time, NULL);
> 		kvm_pthread_getaffinity(thread, &cpuset);
> 		TEST_ASSERT(CPU_COUNT(&cpuset) == 1 && CPU_ISSET(cpu, &cpuset),
> 			    "Worker failed to inherit parent's CPU affinity");
> 
> 		do
> 			sched_yield();
> 		while (get_run_delay() - run_delay < MIN_RUN_DELAY_NS);
> 
> When get_run_delay() returns MIN_RUN_DELAY_NS on every call,
> get_run_delay() - run_delay evaluates to 0, which is always less than
> MIN_RUN_DELAY_NS, causing this loop to spin forever.
> 
> Similarly, in tools/testing/selftests/kvm/x86/xen_shinfo_test.c:main():
> 
> 				rundelay = get_run_delay() + MIN_STEAL_TIME;
> 				do {
> 					sched_yield();
> 				} while (get_run_delay() < rundelay);
> 
> Here, rundelay becomes MIN_RUN_DELAY_NS + MIN_STEAL_TIME while get_run_delay()
> continues returning MIN_RUN_DELAY_NS, which also spins forever.
> 
> Should get_run_delay() instead fail or skip via TEST_ASSERT() or
> TEST_REQUIRE() when fopen() fails, similar to other file-opening helpers in
> tools/testing/selftests/kvm/lib/test_util.c such as get_sysfs_val() and
> get_def_hugetlb_pagesz()?

Ya, I agree with Sashiko.  Not only could returning MIN_RUN_DELAY_NS lead to
failure, it could result in false passes, which is arguably even worse.  E.g.
running the steal time test with CONFIG_SCHEDSTATS=n seems pointless.

I don't think we want to simply do TEST_REQUIRE() if fopen() fails, because that
would result in weird skips if fopen() initially succeeds but then randomly fails
partway through the test.

I think the best way to tackle this would be to provide separate APIs to open and
read the file, and then have the open API do TEST_REQUIRE() and the read API do
TEST_ASSERT().

E.g.

FILE *sys_open_schedstat(void)
{
	char path[64];
	FILE *f;
	int fd;

	sprintf(path, "/proc/%ld/schedstat", (long)kvm_gettid());
	fd = open_path_or_exit(path, O_RDONLY);

	f = fdopen(fd, "r");
	TEST_ASSERT(f, __KVM_SYSCALL_ERROR("fdopen()", -1));
	return f;
}

long get_run_delay(FILE *f)
{
	long val[2];
	int r;

	r = fscanf(f, "%ld %ld ", &val[0], &val[1]);
	TEST_ASSERT(r == 2, __KVM_SYSCALL_ERROR("fscanf()", r));

	return val[1];
}