| Message ID | 20240315-hid-bpf-sleepable-v4-2-5658f2540564@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | sleepable bpf_timer (was: allow HID-BPF to do device IOs) | expand |
On Fri, 2024-03-15 at 15:29 +0100, Benjamin Tissoires wrote: [...] > @@ -12021,6 +12034,12 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ > if (ret) > return ret; > break; > + case KF_ARG_PTR_TO_TIMER: > + if (reg->type != PTR_TO_MAP_VALUE) { > + verbose(env, "arg#%d doesn't point to a map value\n", i); > + return -EINVAL; > + } > + break; I think that pointer offset has to be checked as well, otherwise the following program verifies w/o error: --- 8< ---------------------------- #include <linux/bpf.h> #include <time.h> #include <errno.h> #include <bpf/bpf_helpers.h> #include "bpf_tcp_helpers.h" extern int bpf_timer_set_sleepable_cb_impl(struct bpf_timer *timer, int (callback_fn)(void *map, int *key, struct bpf_timer *timer), void *aux__ign) __ksym; #define bpf_timer_set_sleepable_cb(timer, cb) \ bpf_timer_set_sleepable_cb_impl(timer, cb, NULL) struct elem { struct bpf_timer t; }; struct { __uint(type, BPF_MAP_TYPE_ARRAY); __uint(max_entries, 2); __type(key, int); __type(value, struct elem); } array SEC(".maps"); static int cb_sleepable(void *map, int *key, struct bpf_timer *timer) { return 0; } SEC("fentry/bpf_fentry_test5") int BPF_PROG2(test_sleepable, int, a) { struct bpf_timer *arr_timer; int array_key = 1; arr_timer = bpf_map_lookup_elem(&array, &array_key); if (!arr_timer) return 0; bpf_timer_init(arr_timer, &array, CLOCK_MONOTONIC); bpf_timer_set_sleepable_cb((void *)arr_timer + 1, // <-- note incorrect offset cb_sleepable); bpf_timer_start(arr_timer, 0, 0); return 0; } char _license[] SEC("license") = "GPL"; ---------------------------- >8 ---
On Mon, Mar 18, 2024 at 10:59 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Fri, 2024-03-15 at 15:29 +0100, Benjamin Tissoires wrote: > [...] > > > @@ -12021,6 +12034,12 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ > > if (ret) > > return ret; > > break; > > + case KF_ARG_PTR_TO_TIMER: > > + if (reg->type != PTR_TO_MAP_VALUE) { > > + verbose(env, "arg#%d doesn't point to a map value\n", i); > > + return -EINVAL; > > + } > > + break; > > I think that pointer offset has to be checked as well, > otherwise the following program verifies w/o error: I initially thought it would be harder than it actually was. Fixed (with the new test below) in v5. Cheers, Benjamin > > --- 8< ---------------------------- > > #include <linux/bpf.h> > #include <time.h> > #include <errno.h> > #include <bpf/bpf_helpers.h> > #include "bpf_tcp_helpers.h" > > extern int bpf_timer_set_sleepable_cb_impl(struct bpf_timer *timer, > int (callback_fn)(void *map, int *key, struct bpf_timer *timer), void *aux__ign) __ksym; > > #define bpf_timer_set_sleepable_cb(timer, cb) \ > bpf_timer_set_sleepable_cb_impl(timer, cb, NULL) > > struct elem { > struct bpf_timer t; > }; > > struct { > __uint(type, BPF_MAP_TYPE_ARRAY); > __uint(max_entries, 2); > __type(key, int); > __type(value, struct elem); > } array SEC(".maps"); > > static int cb_sleepable(void *map, int *key, struct bpf_timer *timer) > { > return 0; > } > > SEC("fentry/bpf_fentry_test5") > int BPF_PROG2(test_sleepable, int, a) > { > struct bpf_timer *arr_timer; > int array_key = 1; > > arr_timer = bpf_map_lookup_elem(&array, &array_key); > if (!arr_timer) > return 0; > bpf_timer_init(arr_timer, &array, CLOCK_MONOTONIC); > bpf_timer_set_sleepable_cb((void *)arr_timer + 1, // <-- note incorrect offset > cb_sleepable); > bpf_timer_start(arr_timer, 0, 0); > return 0; > } > > char _license[] SEC("license") = "GPL"; > > ---------------------------- >8 --- >
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 63749ad5ac6b..1483ebc0ee73 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -10826,6 +10826,7 @@ enum { KF_ARG_LIST_NODE_ID, KF_ARG_RB_ROOT_ID, KF_ARG_RB_NODE_ID, + KF_ARG_TIMER_ID, }; BTF_ID_LIST(kf_arg_btf_ids) @@ -10834,6 +10835,7 @@ BTF_ID(struct, bpf_list_head) BTF_ID(struct, bpf_list_node) BTF_ID(struct, bpf_rb_root) BTF_ID(struct, bpf_rb_node) +BTF_ID(struct, bpf_timer_kern) static bool __is_kfunc_ptr_arg_type(const struct btf *btf, const struct btf_param *arg, int type) @@ -10877,6 +10879,12 @@ static bool is_kfunc_arg_rbtree_node(const struct btf *btf, const struct btf_par return __is_kfunc_ptr_arg_type(btf, arg, KF_ARG_RB_NODE_ID); } +static bool is_kfunc_arg_timer(const struct btf *btf, const struct btf_param *arg) +{ + bool ret = __is_kfunc_ptr_arg_type(btf, arg, KF_ARG_TIMER_ID); + return ret; +} + static bool is_kfunc_arg_callback(struct bpf_verifier_env *env, const struct btf *btf, const struct btf_param *arg) { @@ -10946,6 +10954,7 @@ enum kfunc_ptr_arg_type { KF_ARG_PTR_TO_NULL, KF_ARG_PTR_TO_CONST_STR, KF_ARG_PTR_TO_MAP, + KF_ARG_PTR_TO_TIMER, }; enum special_kfunc_type { @@ -11102,6 +11111,9 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env, if (is_kfunc_arg_map(meta->btf, &args[argno])) return KF_ARG_PTR_TO_MAP; + if (is_kfunc_arg_timer(meta->btf, &args[argno])) + return KF_ARG_PTR_TO_TIMER; + if ((base_type(reg->type) == PTR_TO_BTF_ID || reg2btf_ids[base_type(reg->type)])) { if (!btf_type_is_struct(ref_t)) { verbose(env, "kernel function %s args#%d pointer type %s %s is not supported\n", @@ -11735,6 +11747,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ case KF_ARG_PTR_TO_CALLBACK: case KF_ARG_PTR_TO_REFCOUNTED_KPTR: case KF_ARG_PTR_TO_CONST_STR: + case KF_ARG_PTR_TO_TIMER: /* Trusted by default */ break; default: @@ -12021,6 +12034,12 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ if (ret) return ret; break; + case KF_ARG_PTR_TO_TIMER: + if (reg->type != PTR_TO_MAP_VALUE) { + verbose(env, "arg#%d doesn't point to a map value\n", i); + return -EINVAL; + } + break; } }
We need to extend the bpf_timer API, but the way forward relies on kfuncs. So make bpf_timer known for kfuncs from the verifier PoV Signed-off-by: Benjamin Tissoires <bentiss@kernel.org> --- changes in v4: - enforce KF_ARG_PTR_TO_TIMER to be of type PTR_TO_MAP_VALUE new in v3 (split from v2 02/10) --- kernel/bpf/verifier.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)