[PATCH bpf-next] selftests/bpf: Wait for the struct_ops map to be freed in assoc_in_timer

[PATCH bpf-next] selftests/bpf: Wait for the struct_ops map to be freed in assoc_in_timer

[Qiliang Yuan]

st_ops_assoc_in_timer_no_uref runs .test_1, which starts a timer to
call it again 500ms later, drops every reference to the struct_ops map,
and expects the timer callback to find no struct_ops associated with
its program anymore.

The association goes away only when the map is freed, which happens in
a work item after an RCU grace period. With other tests running in
parallel, that can take longer than 500ms, and the callback still
reaches the struct_ops and gets MAP_MAGIC instead of -1.

Restart the timer from its callback until the map is gone.

Fixes: 0e841d19263a ("selftests/bpf: Test getting associated struct_ops in timer callback")
Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/test_struct_ops_assoc.c | 4 +++-
 tools/testing/selftests/bpf/progs/struct_ops_assoc_in_timer.c  | 7 +++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_assoc.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_assoc.c
index 461ded7223515..84da2c8651bec 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_assoc.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_assoc.c
@@ -158,9 +158,11 @@ static void test_st_ops_assoc_in_timer_no_uref(void)
 	 * Run .test_1 by calling kfunc bpf_kfunc_multi_st_ops_test_1_prog_arg() and checks
 	 * the return value. .test_1 will also schedule timer_cb that runs .test_1 again.
 	 * timer_cb will run 500ms after syscall_prog runs, when the user space no longer
-	 * holds a reference to st_ops_map.
+	 * holds a reference to st_ops_map, and every 500ms after that until the map is
+	 * freed.
 	 */
 	skel->bss->timer_ns = 500000000;
+	skel->bss->wait_map_free = 1;
 	err = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.syscall_prog), NULL);
 	ASSERT_OK(err, "bpf_prog_test_run_opts");
 
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
index 0bed49e9f2170..9f3124acd9b04 100644
--- a/tools/testing/selftests/bpf/progs/struct_ops_assoc_in_timer.c
+++ b/tools/testing/selftests/bpf/progs/struct_ops_assoc_in_timer.c
@@ -25,6 +25,7 @@ int test_err;
 int timer_ns;
 int timer_test_1_ret;
 int timer_cb_run;
+int wait_map_free;
 
 __noinline static int timer_cb(void *map, int *key, struct bpf_timer *timer)
 {
@@ -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;

---
base-commit: e1d84a37cba984388988d2f1ddc84561413f0db2
change-id: 20261009-selftests-bpf-struct-ops-assoc-timer-5eb0295ec300

Best regards,
-- 
Qiliang Yuan <odys.yuan@gmail.com>

Re: [PATCH bpf-next] selftests/bpf: Wait for the struct_ops map to be freed in assoc_in_timer

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 4570 bytes --]

> 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