> selftests/bpf: Wait for the struct_ops map to be freed in assoc_in_timer
TL;DR: Pre-existing race in bpf_struct_ops_map_free(): a timer callback may
still use a freed struct_ops or trampoline, since dissociation happens late
with no grace period. The test only waits longer; should the kernel be fixed?
Fixes tag questioned.
> diff --git a/tools/testing/selftests/bpf/progs/struct_ops_assoc_in_timer.c
> b/tools/testing/selftests/bpf/progs/struct_ops_assoc_in_timer.c
> --- a/tools/testing/selftests/bpf/progs/struct_ops_assoc_in_timer.c
> +++ b/tools/testing/selftests/bpf/progs/struct_ops_assoc_in_timer.c
[ ... ]
> @@ -34,6 +35,12 @@ __noinline static int timer_cb(void *map, int *key, struct
> bpf_timer *timer)
> timer_test_1_ret = bpf_kfunc_multi_st_ops_test_1_assoc(&args);
> recur--;
>
> + /* Try again later until the map is freed */
> + if (wait_map_free && timer_test_1_ret != -1) {
> + bpf_timer_start(timer, timer_ns, 0);
> + return 0;
> + }
> +
> timer_cb_run++;
>
> return 0;
This isn't introduced by this patch, but the commit message relies on it:
The association goes away only when the map is freed, which happens in
a work item after an RCU grace period.
A subsystem pattern flags this ordering as potentially concerning: the
removal happens inside the RCU callback path rather than before it. Could
this leave a window where a timer callback like this one uses a struct_ops
that is being freed?
The kernel side ordering is:
bpf_map_put()
map_free_pre_rcu() /* only deletes ksyms */
call_rcu(&map->rcu, bpf_map_free_rcu_gp)
... grace period ...
bpf_map_free_deferred()
bpf_struct_ops_map_free()
bpf_struct_ops_map_dissoc_progs(st_map);
/* RCU_INIT_POINTER(prog->aux->st_ops_assoc, NULL) */
if (tasks_rcu && IS_ENABLED(CONFIG_TASKS_RCU))
synchronize_rcu_tasks();
__bpf_struct_ops_map_free(map);
/* frees image pages and st_map (kdata) */
The reader is this callback:
bpf_timer_cb() /* hrtimer softirq */
timer_cb()
bpf_kfunc_multi_st_ops_test_1_assoc()
bpf_prog_get_assoc_struct_ops()
/* rcu_dereference_check(aux->st_ops_assoc), returns
* &st_map->kvalue.data */
st_ops->test_1() /* jumps into the trampoline image */
test_1 does not pin the map, and the timer keeps the prog alive through
cb->prog. So a callback can start after the grace period has ended and
before the NULL store in bpf_struct_ops_map_dissoc_progs(), and call_rcu()
does not wait for that reader.
testmod_multi_st_ops does not set free_after_tasks_rcu_gp, so there is no
grace period between RCU_INIT_POINTER(NULL) and
bpf_struct_ops_map_free_image()/bpf_map_area_free(st_map). The callback
can therefore still be reading kdata or running trampoline code that has
just been freed.
Before 5db69b0fbdc8 ("bpf: Make struct_ops tasks_rcu grace period
optional"), bpf_struct_ops_map_free() ran synchronize_rcu_mult(call_rcu,
call_rcu_tasks) after the dissociation and before freeing, which covered
this reader. That commit moved the grace period in front of map_free but
left the dissociation inside it. It is also why the association is now
cleared later, which is the delay this patch works around.
0e841d19263a added this subtest to make sure "the timer callback cannot get
a dangled pointer to the struct_ops when the map is freed". The new loop
keeps calling through the association every 500ms until it is cleared and
then only checks for -1, so it cannot detect this window.
For balance, the race is in kernel/bpf/bpf_struct_ops.c rather than in the
files this patch changes, and it predates the series since 5db69b0fbdc8 is
in the base. The window is narrow: the reader must load st_ops_assoc in the
gap between the end of the grace period and the dissociation in the work
item, and still be inside st_ops->test_1() when __bpf_struct_ops_map_free()
frees the image. It is still structurally possible because nothing orders
the reader against the free.
Should the late dissociation be fixed in bpf_struct_ops_map_free() instead
of only making the test wait longer, for example with a grace period after
bpf_struct_ops_map_dissoc_progs() or by dissociating before the call_rcu()?
If the flakiness started with 5db69b0fbdc8, is the Fixes: tag pointing at
the right commit?
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/37873469458
