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

[Sean Christopherson <seanjc@google.com>]

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];
}