Re: [PATCH bpf-next 2/2] selftests/bpf: Add task work round ownership race test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] bpf: spurious -ENOENT test failure due to unhandled early delete race
- [Medium] bpf: missing memory barriers cause stale error state reads in race test
- [Medium] bpf: TOCTOU race in task_work_race_wait_ready() causes false test failures
--

commit 6a4b075b19b6a65e88e8c41b961bca6ee7533e52
Author: Yun Lu <luyun@kylinos.cn>

selftests/bpf: Add task work round ownership race test

Exercise task-work scheduling, callback completion and map-value deletion
on separate execution contexts.  Rotate through ctx reuse, deletion
after the callback body, and deletion immediately before the scheduling
kfunc.

> diff --git a/tools/testing/selftests/bpf/prog_tests/test_task_work.c b/tools/testing/selftests/bpf/prog_tests/test_task_work.c
> index 774b31a5f6ca1..6d5e1e97aa3af 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_task_work.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_task_work.c
[ ... ]
> +static int task_work_race_wait_ready(struct task_work_race *skel, __u32 seq)
> +{
> +	int i;
> +
> +	for (i = 0; i < 2000000 / 100; i++) {
> +		__s64 value = 0;
> +
> +		if (!task_work_race_status(skel, RACE_READY_SEQ, &value) &&
> +		    value == seq)
> +			return 0;
> +		if (!task_work_race_status(skel, RACE_DONE_SEQ, &value) &&
> +		    value == seq)
> +			return -EIO;

[Severity: Medium]
Could this sequential checking lead to a false negative?

If the concurrent BPF program finishes execution exactly between these two
syscalls, the thread will see RACE_READY_SEQ as false from the first read
and RACE_DONE_SEQ as true from the second read, returning -EIO. 

If the BPF program had finished before the first check, it would have read
RACE_READY_SEQ == seq and correctly returned 0. Can this time-of-check to
time-of-use interleaving incorrectly penalize a fast BPF execution as an
error?

> +		usleep(100);
> +	}
> +	return -ETIMEDOUT;
> +}
[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/task_work_race.c b/tools/testing/selftests/bpf/progs/task_work_race.c
> new file mode 100644
> index 0000000000000..8d3f31fb075f8
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/task_work_race.c
[ ... ]
> +SEC("tracepoint/syscalls/sys_enter_getppid")
> +int race_sched_work(void *ctx)
> +{
[ ... ]
> +	key = 0;
> +	work = bpf_map_lookup_elem(&hmap, &key);
> +	if (!work) {
> +		err = -ENOENT;
> +		goto out_task;
> +	}
> +
> +	work->seq = seq;
> +	set_status(RACE_READY_SEQ, seq);
> +	err = bpf_task_work_schedule_signal(task, &work->tw, &hmap,
> +					    process_work);
> +	if (err == -EBUSY) {
> +		bpf_task_release(task);
> +		return 0;
> +	}

[Severity: Medium]
When variant 2 is tested, userspace deliberately races to delete the map
element early. If this early delete wins the race against the scheduling
kfunc above, bpf_task_work_schedule_signal() returns -EBUSY. 

This causes the BPF program to return 0, triggering a retry. On the next
tracepoint invocation, looking up the now-deleted element fails with -ENOENT. 

Because userspace expects a successful schedule (err == 0) in
task_work_race_check_done(), the test will fail when it observes -ENOENT.
Should the test be updated to handle this expected -ENOENT outcome, or
should the BPF program avoid retrying when the element is permanently
deleted?

> +
> +out_task:
> +	bpf_task_release(task);
> +out_done:
> +	set_status(RACE_SCHED_ERR, err);
> +	set_status(RACE_DONE_SEQ, seq);

[Severity: Medium]
Does this code sequence need a write memory barrier?

On weakly-ordered architectures like ARM64, the CPU can reorder these
independent stores because the BPF JIT does not emit barrier instructions
for array map stores.

If userspace polls RACE_DONE_SEQ and observes the update before
RACE_SCHED_ERR is globally visible, it might read a stale error code 
(e.g., 0 instead of the actual error) and potentially mask failures.

> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918080014.54012-1-luyun_611@163.com?part=2