From: Mykyta Yatsenko Move bpf_wq map-field validation into the common helper by adding a BPF_WORKQUEUE case that maps to record->wq_off, and switch process_wq_func() to use it instead of doing its own offset math. Fix handling maps with no BTF and non-constant offsets for the bpf_wq. This de-duplicates logic with other internal structs (task_work, timer), keeps error reporting consistent, and makes future changes to the layout handling centralized. Fixes: d940c9b94d7e ("bpf: add support for KF_ARG_PTR_TO_WORKQUEUE") Signed-off-by: Mykyta Yatsenko Acked-by: Andrii Nakryiko Acked-by: Eduard Zingerman --- kernel/bpf/verifier.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index e892df386eed..b2d8847b25cf 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -8464,6 +8464,9 @@ static int check_map_field_pointer(struct bpf_verifier_env *env, u32 regno, case BPF_TASK_WORK: field_off = map->record->task_work_off; break; + case BPF_WORKQUEUE: + field_off = map->record->wq_off; + break; default: verifier_bug(env, "unsupported BTF field type: %s\n", struct_name); return -EINVAL; @@ -8505,13 +8508,17 @@ static int process_wq_func(struct bpf_verifier_env *env, int regno, { struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; struct bpf_map *map = reg->map_ptr; - u64 val = reg->var_off.value; + int err; - if (map->record->wq_off != val + reg->off) { - verbose(env, "off %lld doesn't point to 'struct bpf_wq' that is at %d\n", - val + reg->off, map->record->wq_off); - return -EINVAL; + err = check_map_field_pointer(env, regno, BPF_WORKQUEUE); + if (err) + return err; + + if (meta->map.ptr) { + verifier_bug(env, "Two map pointers in a bpf_wq helper"); + return -EFAULT; } + meta->map.uid = reg->map_uid; meta->map.ptr = map; return 0; -- 2.51.0 From: Mykyta Yatsenko Add bpf_wq selftests to verify: * BPF program using non-constant offset of struct bpf_wq is rejected * BPF program using map with no BTF for storing struct bpf_wq is rejected Signed-off-by: Mykyta Yatsenko --- tools/testing/selftests/bpf/prog_tests/wq.c | 48 +++++++++++++++++++ .../testing/selftests/bpf/progs/wq_failures.c | 23 +++++++++ 2 files changed, 71 insertions(+) diff --git a/tools/testing/selftests/bpf/prog_tests/wq.c b/tools/testing/selftests/bpf/prog_tests/wq.c index 99e438fe12ac..13c124fff365 100644 --- a/tools/testing/selftests/bpf/prog_tests/wq.c +++ b/tools/testing/selftests/bpf/prog_tests/wq.c @@ -1,9 +1,12 @@ // SPDX-License-Identifier: GPL-2.0 /* Copyright (c) 2024 Benjamin Tissoires */ #include +#include #include "wq.skel.h" #include "wq_failures.skel.h" +static void test_failure_map_no_btf(void); + void serial_test_wq(void) { struct wq *wq_skel = NULL; @@ -11,6 +14,9 @@ void serial_test_wq(void) LIBBPF_OPTS(bpf_test_run_opts, topts); + if (test__start_subtest("test_failure_map_no_btf")) + test_failure_map_no_btf(); + RUN_TESTS(wq); /* re-run the success test to check if the timer was actually executed */ @@ -38,3 +44,45 @@ void serial_test_failures_wq(void) { RUN_TESTS(wq_failures); } + +static void test_failure_map_no_btf(void) +{ + char log[8192]; + struct btf *vmlinux_btf = libbpf_find_kernel_btf(); + int kfunc_id = btf__find_by_name_kind(vmlinux_btf, "bpf_wq_init", BTF_KIND_FUNC); + int map_fd = bpf_map_create(BPF_MAP_TYPE_ARRAY, "map_no_btf", sizeof(__u32), sizeof(__u64), + 100, NULL); + struct bpf_insn prog[] = { + /* key = 42 on stack at [fp-4] */ + BPF_MOV64_IMM(BPF_REG_0, 42), /* r0 = 42 */ + BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_0, -4), /* *(u32 *)(fp-4) = 42 */ + + /* r1 = &map (patched from map_fd), r2 = &key */ + BPF_LD_MAP_FD(BPF_REG_1, map_fd), /* r1 = map */ + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), /* r2 = fp */ + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -4), /* r2 = fp-4 (key addr) */ + + /* map_val = bpf_map_lookup_elem(map, &key) */ + BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem), /* r0 = map_val or NULL */ + + /* if (!map_val) goto out; */ + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4), /* if (r0 == NULL) skip next 4 insns */ + + /* wq = (void *)(map_val + 0); -> use r0 as arg1 directly */ + BPF_MOV64_REG(BPF_REG_1, BPF_REG_0), /* r1 = wq (= val ptr) */ + + /* bpf_wq_init(wq, &map, 0) */ + BPF_LD_MAP_FD(BPF_REG_2, map_fd), /* r2 = map */ + BPF_MOV64_IMM(BPF_REG_3, 0), /* r3 = flags (0) */ + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, + kfunc_id), /* r0 = bpf_wq_init(wq, &map, 0) */ + BPF_EXIT_INSN(), /* return -3 */ + }; + LIBBPF_OPTS(bpf_prog_load_opts, opts, .log_size = sizeof(log), .log_buf = log, + .log_level = 2); + int r = bpf_prog_load(BPF_PROG_TYPE_TRACEPOINT, NULL, "GPL", prog, ARRAY_SIZE(prog), &opts); + + ASSERT_NEQ(r, 0, "prog load failed"); + ASSERT_HAS_SUBSTR(log, "map 'map_no_btf' has to have BTF in order to use bpf_wq", + "log complains no map BTF"); +} diff --git a/tools/testing/selftests/bpf/progs/wq_failures.c b/tools/testing/selftests/bpf/progs/wq_failures.c index 4240211a1900..d06f6d40594a 100644 --- a/tools/testing/selftests/bpf/progs/wq_failures.c +++ b/tools/testing/selftests/bpf/progs/wq_failures.c @@ -142,3 +142,26 @@ long test_wrong_wq_pointer_offset(void *ctx) return -22; } + +SEC("tc") +__log_level(2) +__failure +__msg(": (85) call bpf_wq_init#") +__msg("R1 doesn't have constant offset. bpf_wq has to be at the constant offset") +long test_bad_wq_off(void *ctx) +{ + struct elem *val; + struct bpf_wq *wq; + int key = 42; + u64 unknown; + + val = bpf_map_lookup_elem(&array, &key); + if (!val) + return -2; + + unknown = bpf_get_prandom_u32(); + wq = &val->w + unknown; + if (bpf_wq_init(wq, &array, 0) != 0) + return -3; + return 0; +} -- 2.51.0 From: Mykyta Yatsenko arraymap and hashtab duplicate the logic that checks for and frees internal structs (timer, workqueue, task_work) based on BTF record flags. Centralize this by introducing two helpers: * bpf_map_has_internal_structs(map) Returns true if the map value contains any of internal structs: BPF_TIMER | BPF_WORKQUEUE | BPF_TASK_WORK. * bpf_map_free_internal_structs(map, obj) Frees the internal structs for a single value object. Convert arraymap and both the prealloc/malloc hashtab paths to use the new generic functions. This keeps the functionality for when/how to free these special fields in one place and makes it easier to add support for new internal structs in the future without touching every map implementation. Signed-off-by: Mykyta Yatsenko --- include/linux/bpf.h | 7 +++++++ kernel/bpf/arraymap.c | 19 ++++++------------- kernel/bpf/hashtab.c | 36 +++++++++++++----------------------- kernel/bpf/helpers.c | 10 ++++++++++ 4 files changed, 36 insertions(+), 36 deletions(-) diff --git a/include/linux/bpf.h b/include/linux/bpf.h index a98c83346134..f87fb203aaae 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -663,6 +663,13 @@ int map_check_no_btf(const struct bpf_map *map, bool bpf_map_meta_equal(const struct bpf_map *meta0, const struct bpf_map *meta1); +static inline bool bpf_map_has_internal_structs(struct bpf_map *map) +{ + return btf_record_has_field(map->record, BPF_TIMER | BPF_WORKQUEUE | BPF_TASK_WORK); +} + +void bpf_map_free_internal_structs(struct bpf_map *map, void *obj); + extern const struct bpf_map_ops bpf_map_offload_ops; /* bpf_type_flag contains a set of flags that are applicable to the values of diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c index 80b1765a3159..0ba790c2d2e5 100644 --- a/kernel/bpf/arraymap.c +++ b/kernel/bpf/arraymap.c @@ -448,19 +448,12 @@ static void array_map_free_internal_structs(struct bpf_map *map) struct bpf_array *array = container_of(map, struct bpf_array, map); int i; - /* We don't reset or free fields other than timer and workqueue - * on uref dropping to zero. - */ - if (btf_record_has_field(map->record, BPF_TIMER | BPF_WORKQUEUE | BPF_TASK_WORK)) { - for (i = 0; i < array->map.max_entries; i++) { - if (btf_record_has_field(map->record, BPF_TIMER)) - bpf_obj_free_timer(map->record, array_map_elem_ptr(array, i)); - if (btf_record_has_field(map->record, BPF_WORKQUEUE)) - bpf_obj_free_workqueue(map->record, array_map_elem_ptr(array, i)); - if (btf_record_has_field(map->record, BPF_TASK_WORK)) - bpf_obj_free_task_work(map->record, array_map_elem_ptr(array, i)); - } - } + /* We only free internal structs on uref dropping to zero */ + if (!bpf_map_has_internal_structs(map)) + return; + + for (i = 0; i < array->map.max_entries; i++) + bpf_map_free_internal_structs(map, array_map_elem_ptr(array, i)); } /* Called when map->refcnt goes to zero, either from workqueue or from syscall */ diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c index c2fcd0cd51e5..e7a6ba04dc82 100644 --- a/kernel/bpf/hashtab.c +++ b/kernel/bpf/hashtab.c @@ -215,19 +215,6 @@ static bool htab_has_extra_elems(struct bpf_htab *htab) return !htab_is_percpu(htab) && !htab_is_lru(htab) && !is_fd_htab(htab); } -static void htab_free_internal_structs(struct bpf_htab *htab, struct htab_elem *elem) -{ - if (btf_record_has_field(htab->map.record, BPF_TIMER)) - bpf_obj_free_timer(htab->map.record, - htab_elem_value(elem, htab->map.key_size)); - if (btf_record_has_field(htab->map.record, BPF_WORKQUEUE)) - bpf_obj_free_workqueue(htab->map.record, - htab_elem_value(elem, htab->map.key_size)); - if (btf_record_has_field(htab->map.record, BPF_TASK_WORK)) - bpf_obj_free_task_work(htab->map.record, - htab_elem_value(elem, htab->map.key_size)); -} - static void htab_free_prealloced_internal_structs(struct bpf_htab *htab) { u32 num_entries = htab->map.max_entries; @@ -240,7 +227,8 @@ static void htab_free_prealloced_internal_structs(struct bpf_htab *htab) struct htab_elem *elem; elem = get_htab_elem(htab, i); - htab_free_internal_structs(htab, elem); + bpf_map_free_internal_structs(&htab->map, + htab_elem_value(elem, htab->map.key_size)); cond_resched(); } } @@ -1509,8 +1497,9 @@ static void htab_free_malloced_internal_structs(struct bpf_htab *htab) struct htab_elem *l; hlist_nulls_for_each_entry(l, n, head, hash_node) { - /* We only free timer on uref dropping to zero */ - htab_free_internal_structs(htab, l); + /* We only free internal structs on uref dropping to zero */ + bpf_map_free_internal_structs(&htab->map, + htab_elem_value(l, htab->map.key_size)); } cond_resched_rcu(); } @@ -1521,13 +1510,14 @@ static void htab_map_free_internal_structs(struct bpf_map *map) { struct bpf_htab *htab = container_of(map, struct bpf_htab, map); - /* We only free timer and workqueue on uref dropping to zero */ - if (btf_record_has_field(htab->map.record, BPF_TIMER | BPF_WORKQUEUE | BPF_TASK_WORK)) { - if (!htab_is_prealloc(htab)) - htab_free_malloced_internal_structs(htab); - else - htab_free_prealloced_internal_structs(htab); - } + /* We only free internal structs on uref dropping to zero */ + if (!bpf_map_has_internal_structs(map)) + return; + + if (htab_is_prealloc(htab)) + htab_free_prealloced_internal_structs(htab); + else + htab_free_malloced_internal_structs(htab); } /* Called when map->refcnt goes to zero, either from workqueue or from syscall */ diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index c9fab9a356df..22fbff8310f6 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -4431,3 +4431,13 @@ void *__bpf_dynptr_data_rw(const struct bpf_dynptr_kern *ptr, u32 len) return NULL; return (void *)__bpf_dynptr_data(ptr, len); } + +void bpf_map_free_internal_structs(struct bpf_map *map, void *val) +{ + if (btf_record_has_field(map->record, BPF_TIMER)) + bpf_obj_free_timer(map->record, val); + if (btf_record_has_field(map->record, BPF_WORKQUEUE)) + bpf_obj_free_workqueue(map->record, val); + if (btf_record_has_field(map->record, BPF_TASK_WORK)) + bpf_obj_free_task_work(map->record, val); +} -- 2.51.0