Re: [PATCH 1/3] selftests/sched_ext: Add a test for ops.dequeue() on remote local DSQ moves

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/sched_ext: leaked bpf_link FD on error path blocks subsequent tests
--

commit 1ccbec9969985b2fbfb40e59807388cd3896b3e6
Author: Kuba Piecuch <jpiecuch@google.com>
Subject: selftests/sched_ext: Add a test for ops.dequeue() on remote local DSQ moves

When a BPF scheduler moves a task that is in its custody to the local DSQ
of a CPU other than the one whose rq the task is on, sched_ext migrates the
task with move_remote_task_to_local_dsq(). The task leaves the BPF
scheduler's custody when it is inserted into the destination local DSQ and
ops.dequeue() should be called at that point, just like for same-rq
dispatches.

> diff --git a/tools/testing/selftests/sched_ext/dequeue_remote.c b/tools/testing/selftests/sched_ext/dequeue_remote.c
> new file mode 100644
> index 0000000000000..ab42ac694ac6b
> --- /dev/null
> +++ b/tools/testing/selftests/sched_ext/dequeue_remote.c
[ ... ]
> +static enum scx_test_status run_scenario(struct dequeue_remote *skel,
> +					 bool use_move_to_local,
> +					 const char *name)
> +{
> +	struct bpf_link *link;
> +	pid_t pids[MAX_WORKERS];
> +	int nr_workers, i, status;
> +	u64 enq, deq, dsp_deq, chg_deq, remote_dsp, remote_run;
[ ... ]
> +	link = bpf_map__attach_struct_ops(skel->maps.dequeue_remote_ops);
> +	SCX_FAIL_IF(!link, "Failed to attach struct_ops for %s", name);
> +
> +	fflush(stdout);
> +	fflush(stderr);
> +
> +	for (i = 0; i < nr_workers; i++) {
> +		pids[i] = fork();
> +		SCX_FAIL_IF(pids[i] < 0, "Failed to fork worker %d", i);

[Severity: Medium]
If fork() fails here, SCX_FAIL_IF() will return immediately. Does this leak
the bpf_link in the runner process?

Since link is stored in a local variable, it won't be freed by the test
framework's cleanup() callback. If leaked, this would leave the scheduler
attached, causing subsequent tests to fail with -EBUSY because only one
sched_ext scheduler can be active at a time.

> +		if (pids[i] == 0)
> +			worker_fn(i);
> +	}
> +
> +	for (i = 0; i < nr_workers; i++) {
> +		SCX_FAIL_IF(waitpid(pids[i], &status, 0) != pids[i],
> +			    "Failed to wait for worker %d", i);
> +		SCX_FAIL_IF(status != 0, "Worker %d exited with status %d", i, status);

[Severity: Medium]
Similarly, if waitpid() fails or a worker exits with a non-zero status, the
early return will skip bpf_link__destroy(link).

Could the test be updated to use a goto cleanup path, or perhaps assign the
link to the skeleton so it is destroyed automatically, ensuring it doesn't
block subsequent tests on error?

> +	}
> +
> +	bpf_link__destroy(link);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929161730.185271-1-jpiecuch@google.com?part=1