A struct_ops prog is associated with its map without a reference on the
map, and finds the struct_ops through bpf_prog_get_assoc_struct_ops()
under RCU until bpf_struct_ops_map_free() clears the association. For a
struct_ops map, bpf_map_put() waits for an RCU grace period before it
calls bpf_struct_ops_map_free(), which clears the association and then
frees the trampoline and the kdata right away.
Nothing waits for a prog that reads the association after that grace
period started and before the association is cleared. A timer callback
of a struct_ops prog that calls test_1 of bpf_testmod's multi_st_ops
through the association while the map is freed returns into the freed
trampoline, whose image is filled with int3. RAX holds MAP_MAGIC (1234),
which test_1 has just returned:
Oops: int3: 0000 [#1] SMP KASAN NOPTI
CPU: 2 UID: 0 PID: 31 Comm: ksoftirqd/2 ...
RIP: 0010:0xffffffffc043114d
Code: cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc <cc> cc cc cc
RAX: 00000000000004d2 ...
Call Trace:
bpf_prog_ec591f4f9d13f364_timer_cb+0x96/0x1e2
bpf_timer_cb+0x15d/0x240
__hrtimer_run_queues+0x264/0x5a0
hrtimer_run_softirq+0x1a7/0x3b0
handle_softirqs+0x18c/0x5b0
Free the map after a grace period that starts once the association is
cleared, a tasks trace one if the progs of the map may sleep. Clearing
the association before the grace period of bpf_map_put() doesn't work,
as it takes a mutex while bpf_map_put() may run in softirq, e.g. from
bpf_struct_ops_put() of a tcp congestion control.
The st_ops_assoc_in_timer_free subtest of test_progs reproduces it. Its
timer callback keeps calling test_1 through the association while the
map is freed, and test_1 spins in bpf_loop() when called from there:
$ cd tools/testing/selftests/bpf
$ make test_progs
$ ./test_progs -t struct_ops_assoc/st_ops_assoc_in_timer_free
On kernels of the same bpf-next commit with KASAN enabled, x86_64 VM
with 32 vCPUs, before and after this patch:
before after
outcome Oops (int3) on run 1 100 of 100 runs passed, no Oops
Fixes: 5db69b0fbdc8 ("bpf: Make struct_ops tasks_rcu grace period optional")
Signed-off-by: Qiliang Yuan <[email protected]>
---
kernel/bpf/bpf_struct_ops.c | 30 +++++++++++++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 1178acd72296e..cd082339589b3 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -42,6 +42,9 @@ struct bpf_struct_ops_map {
void *image_pages[MAX_TRAMP_IMAGE_PAGES];
/* The owner moduler's btf. */
struct btf *btf;
+ /* free the map once the progs that used it have finished */
+ struct rcu_head free_rcu;
+ struct work_struct free_work;
/* uvalue->data stores the kernel struct
* (e.g. tcp_congestion_ops) that is more useful
* to userspace than the kvalue. For example,
@@ -1032,6 +1035,23 @@ static void bpf_struct_ops_map_free_pre_rcu(struct
bpf_map *map)
bpf_struct_ops_map_del_ksyms(st_map);
}
+static void bpf_struct_ops_map_free_deferred(struct work_struct *work)
+{
+ struct bpf_struct_ops_map *st_map;
+
+ st_map = container_of(work, struct bpf_struct_ops_map, free_work);
+ __bpf_struct_ops_map_free(&st_map->map);
+}
+
+static void bpf_struct_ops_map_free_rcu_gp(struct rcu_head *rcu)
+{
+ struct bpf_struct_ops_map *st_map;
+
+ st_map = container_of(rcu, struct bpf_struct_ops_map, free_rcu);
+ INIT_WORK(&st_map->free_work, bpf_struct_ops_map_free_deferred);
+ queue_work(system_dfl_wq, &st_map->free_work);
+}
+
static void bpf_struct_ops_map_free(struct bpf_map *map)
{
struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map;
@@ -1050,7 +1070,15 @@ static void bpf_struct_ops_map_free(struct bpf_map *map)
if (tasks_rcu && IS_ENABLED(CONFIG_TASKS_RCU))
synchronize_rcu_tasks();
- __bpf_struct_ops_map_free(map);
+ /*
+ * A prog may have read the association before it was cleared above,
+ * e.g. from a timer callback, and may still be running the struct_ops.
+ * Free the map only after a grace period that waits for such a prog.
+ */
+ if (map->free_after_mult_rcu_gp)
+ call_rcu_tasks_trace(&st_map->free_rcu,
bpf_struct_ops_map_free_rcu_gp);
+ else
+ call_rcu(&st_map->free_rcu, bpf_struct_ops_map_free_rcu_gp);
}
static int bpf_struct_ops_map_alloc_check(union bpf_attr *attr)
--
2.43.0
