Re: [PATCH + QUESTION] bpf: use cond_resched_tasks_rcu_qs in bpf_fd_array_map_clear() loop

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] The commit message incorrectly claims that `bpf_fd_array_map_clear()` executes 'under RCU read lock'.

Pre-existing issues:
- [High] perf_event_fd_array_release() iterates over the entire map under a single rcu_read_lock() region, causing Classic RCU stalls for large maps.
- [High] The patch only fixes the RCU-tasks stall in bpf_fd_array_map_clear(), leaving identical vulnerabilities in other BPF map teardown functions.
- [High] A Time-Of-Check to Time-Of-Use race in perf_event_fd_array_release can maliciously delete concurrently updated perf events.
--

--- Patch [1]: [PATCH + QUESTION] bpf: use cond_resched_tasks_rcu_qs in bpf_fd_array_map_clear() loop ---
commit c19063bda5ebcdaccc0217e960d2910817786a73
Author: Rik van Riel <riel@surriel.com>

bpf: use cond_resched_tasks_rcu_qs in bpf_fd_array_map_clear() loop

syzkaller creates a PROG_ARRAY with huge max_entries and triggers perf
tracepoint open close in parallel. The hung task detector reports
"INFO: task hung in perf_tp_event_init" with event_mutex held waiting
for synchronize_rcu_tasks().

> bpf_fd_array_map_clear walks max_entries under RCU read lock on a
> workqueue kworker. Commit 4406942e65ca added cond_resched for classic
> RCU, but under PREEMPT it is a no-op and a preempted kworker never
> reports RCU-tasks QS. The loop then blocks the grace period while
> event_mutex is held in ftrace_shutdown, stalling perf_event_open().

[Severity: Medium]
This isn't a bug introduced by this patch, but the commit message incorrectly
claims that bpf_fd_array_map_clear() executes 'under RCU read lock'.

bpf_fd_array_map_clear() is executed via map destruction callbacks (e.g.,
prog_array_map_clear_deferred) on a sleepable workqueue context without holding
rcu_read_lock(). If a classic RCU read lock were actually held, calling
cond_resched_tasks_rcu_qs() (which invokes cond_resched()) would be an illegal
sleeping operation and trigger might_sleep() lockdep splats.

Could the commit message be updated to reflect the actual context?

[ ... ]

> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index 248b4818178cd..55fd97375130b 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -1015,7 +1015,8 @@ static void bpf_fd_array_map_clear(struct bpf_map *map, bool need_defer)
>  
>  	for (i = 0; i < array->map.max_entries; i++) {
>  		__fd_array_map_delete_elem(map, &i, need_defer);
> -		cond_resched();
> +		/* cond_resched() is a noop with preempt; unblock RCU */
> +		cond_resched_tasks_rcu_qs();
>  	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but the fix only addresses the
RCU-tasks stall in bpf_fd_array_map_clear(), leaving similar issues in other
BPF map teardown functions.

For instance, fd_htab_map_free() in kernel/bpf/hashtab.c loops over all
buckets without any yield point:

	for (i = 0; i < htab->n_buckets; i++) {
		head = select_bucket(htab, i);
		...
	}

Also, bpf_array_free_percpu() in kernel/bpf/arraymap.c loops over max_entries
using only cond_resched():

	for (i = 0; i < array->map.max_entries; i++) {
		free_percpu(array->pptrs[i]);
		cond_resched();
	}

Since cond_resched() is a noop under CONFIG_PREEMPT, wouldn't these loops
similarly stall RCU Tasks when freeing huge hash or per-cpu array maps?

[Severity: High]
This is another pre-existing issue, but while we are looking at
arraymap teardown, perf_event_fd_array_release() also has a potential issue.

It iterates over the entire map under a single rcu_read_lock() region:

	rcu_read_lock();
	for (i = 0; i < array->map.max_entries; i++) {
		ee = READ_ONCE(array->ptrs[i]);
		if (ee && ee->map_file == map_file)
			__fd_array_map_delete_elem(map, &i, true);
	}
	rcu_read_unlock();

Does this cause Classic RCU stalls for large maps, since it prevents grace
periods from completing without any yield points?

[Severity: High]
Additionally, there seems to be a pre-existing Time-Of-Check to Time-Of-Use
race in perf_event_fd_array_release() calling __fd_array_map_delete_elem().

If Thread A closes a map file, it triggers perf_event_fd_array_release(), which
verifies ee->map_file == map_file.

If Thread B concurrently calls bpf_map_update_elem to replace the element,
Thread A then calls __fd_array_map_delete_elem(), which unconditionally
executes:

	old_ptr = xchg(array->ptrs + index, NULL);

Does this mean Thread A could unconditionally nullify Thread B's newly inserted
element, incorrectly deleting it from the map?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715215314.44423f47@fangorn?part=1