| Message ID | 20240322-hid-bpf-sleepable-v5-2-179c7b59eaaa@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | sleepable bpf_timer (was: allow HID-BPF to do device IOs) | expand |
On Fri, Mar 22, 2024 at 9:31 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Fri, 22 Mar 2024 at 15:57, Benjamin Tissoires <bentiss@kernel.org> wrote: > > > > 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 v5: > > - also check for the reg offset > > > > 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 | 23 +++++++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 63749ad5ac6b..24a604e26ec7 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,16 @@ 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; > > + } > > + if (reg->off) { > > + verbose(env, "arg#%d offset can not be greater than 0\n", i); > > + return -EINVAL; > > + } > > This won't be correct. You don't really check whether the timer exists > at reg->off (and if you did, this would still restrict it to 0 offset, > and not check the variable offset which would be non-zero). What I > would suggest is calling process_timer_func (see how dynptr calls the > same underlying process_dynptr_func to enforce type invariants). This > would allow sharing the same checks and avoid bugs from creeping in. > It does all checks wrt constant/variable offset and looking up the > timer field offset and matching it against the one in the pointer. Observation is correct. The patch is buggy, but the suggestion to follow process_dynptr_func() will lead to unnecessary complexity. dynptr-s are on stack with plenty of extra checks. In this case bpf_timer is in map_value. Much simpler is to follow KF_ARG_PTR_TO_MAP approach. Set ref_id, ref_t, ref_tname to bpf_timer and process_kf_arg_ptr_to_btf_id() should work as-is. You still need +BTF_ID(struct, bpf_timer_kern) to recognize that argument in a kfunc, but the selftests will be using 'struct bpf_timer t', so KF_ARG_PTR_TO_TIMER would need to match against 'struct bpf_timer' and not against 'struct bpf_timer_kern'.
On Sun, 24 Mar 2024 at 04:53, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Fri, Mar 22, 2024 at 9:31 AM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > On Fri, 22 Mar 2024 at 15:57, Benjamin Tissoires <bentiss@kernel.org> wrote: > > > > > > 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 v5: > > > - also check for the reg offset > > > > > > 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 | 23 +++++++++++++++++++++++ > > > 1 file changed, 23 insertions(+) > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > index 63749ad5ac6b..24a604e26ec7 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,16 @@ 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; > > > + } > > > + if (reg->off) { > > > + verbose(env, "arg#%d offset can not be greater than 0\n", i); > > > + return -EINVAL; > > > + } > > > > This won't be correct. You don't really check whether the timer exists > > at reg->off (and if you did, this would still restrict it to 0 offset, > > and not check the variable offset which would be non-zero). What I > > would suggest is calling process_timer_func (see how dynptr calls the > > same underlying process_dynptr_func to enforce type invariants). This > > would allow sharing the same checks and avoid bugs from creeping in. > > It does all checks wrt constant/variable offset and looking up the > > timer field offset and matching it against the one in the pointer. > > Observation is correct. The patch is buggy, > but the suggestion to follow process_dynptr_func() will lead > to unnecessary complexity. > dynptr-s are on stack with plenty of extra checks. The suggestion was to call process_timer_func, not process_dynptr_func. > In this case bpf_timer is in map_value. > Much simpler is to follow KF_ARG_PTR_TO_MAP approach. What I meant by the example was that dynptr handling does the same thing for kfuncs and helpers (using the same function), so timer arguments should do the same (i.e. use process_timer_func), which will do all checks for constant offset (ensuring var_off is tnum_is_const) and match it against btf_record->timer_off. > [...]
On Sat, Mar 23, 2024 at 9:01 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Sun, 24 Mar 2024 at 04:53, Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Fri, Mar 22, 2024 at 9:31 AM Kumar Kartikeya Dwivedi > > <memxor@gmail.com> wrote: > > > > > > On Fri, 22 Mar 2024 at 15:57, Benjamin Tissoires <bentiss@kernel.org> wrote: > > > > > > > > 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 v5: > > > > - also check for the reg offset > > > > > > > > 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 | 23 +++++++++++++++++++++++ > > > > 1 file changed, 23 insertions(+) > > > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > > index 63749ad5ac6b..24a604e26ec7 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,16 @@ 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; > > > > + } > > > > + if (reg->off) { > > > > + verbose(env, "arg#%d offset can not be greater than 0\n", i); > > > > + return -EINVAL; > > > > + } > > > > > > This won't be correct. You don't really check whether the timer exists > > > at reg->off (and if you did, this would still restrict it to 0 offset, > > > and not check the variable offset which would be non-zero). What I > > > would suggest is calling process_timer_func (see how dynptr calls the > > > same underlying process_dynptr_func to enforce type invariants). This > > > would allow sharing the same checks and avoid bugs from creeping in. > > > It does all checks wrt constant/variable offset and looking up the > > > timer field offset and matching it against the one in the pointer. > > > > Observation is correct. The patch is buggy, > > but the suggestion to follow process_dynptr_func() will lead > > to unnecessary complexity. > > dynptr-s are on stack with plenty of extra checks. > > The suggestion was to call process_timer_func, not process_dynptr_func. > > > In this case bpf_timer is in map_value. > > Much simpler is to follow KF_ARG_PTR_TO_MAP approach. > > What I meant by the example was that dynptr handling does the same > thing for kfuncs and helpers (using the same function), so timer > arguments should do the same (i.e. use process_timer_func), which will > do all checks for constant offset (ensuring var_off is tnum_is_const) > and match it against btf_record->timer_off. I don't follow. Please elaborate with a patch. The var_off and off is a part of the bug, but it's not the biggest part of it.
On Sun, 24 Mar 2024 at 05:38, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Sat, Mar 23, 2024 at 9:01 PM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > On Sun, 24 Mar 2024 at 04:53, Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Fri, Mar 22, 2024 at 9:31 AM Kumar Kartikeya Dwivedi > > > <memxor@gmail.com> wrote: > > > > > > > > On Fri, 22 Mar 2024 at 15:57, Benjamin Tissoires <bentiss@kernel.org> wrote: > > > > > > > > > > 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 v5: > > > > > - also check for the reg offset > > > > > > > > > > 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 | 23 +++++++++++++++++++++++ > > > > > 1 file changed, 23 insertions(+) > > > > > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > > > index 63749ad5ac6b..24a604e26ec7 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,16 @@ 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; > > > > > + } > > > > > + if (reg->off) { > > > > > + verbose(env, "arg#%d offset can not be greater than 0\n", i); > > > > > + return -EINVAL; > > > > > + } > > > > > > > > This won't be correct. You don't really check whether the timer exists > > > > at reg->off (and if you did, this would still restrict it to 0 offset, > > > > and not check the variable offset which would be non-zero). What I > > > > would suggest is calling process_timer_func (see how dynptr calls the > > > > same underlying process_dynptr_func to enforce type invariants). This > > > > would allow sharing the same checks and avoid bugs from creeping in. > > > > It does all checks wrt constant/variable offset and looking up the > > > > timer field offset and matching it against the one in the pointer. > > > > > > Observation is correct. The patch is buggy, > > > but the suggestion to follow process_dynptr_func() will lead > > > to unnecessary complexity. > > > dynptr-s are on stack with plenty of extra checks. > > > > The suggestion was to call process_timer_func, not process_dynptr_func. > > > > > In this case bpf_timer is in map_value. > > > Much simpler is to follow KF_ARG_PTR_TO_MAP approach. > > > > What I meant by the example was that dynptr handling does the same > > thing for kfuncs and helpers (using the same function), so timer > > arguments should do the same (i.e. use process_timer_func), which will > > do all checks for constant offset (ensuring var_off is tnum_is_const) > > and match it against btf_record->timer_off. > > I don't follow. Please elaborate with a patch. > The var_off and off is a part of the bug, but it's not the biggest part of it. Not compile tested.
On Sat, Mar 23, 2024 at 9:57 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > > > > > Observation is correct. The patch is buggy, > > > > but the suggestion to follow process_dynptr_func() will lead > > > > to unnecessary complexity. > > > > dynptr-s are on stack with plenty of extra checks. > > > > > > The suggestion was to call process_timer_func, not process_dynptr_func. > > > > > > > In this case bpf_timer is in map_value. > > > > Much simpler is to follow KF_ARG_PTR_TO_MAP approach. > > > > > > What I meant by the example was that dynptr handling does the same > > > thing for kfuncs and helpers (using the same function), so timer > > > arguments should do the same (i.e. use process_timer_func), which will > > > do all checks for constant offset (ensuring var_off is tnum_is_const) > > > and match it against btf_record->timer_off. > > > > I don't follow. Please elaborate with a patch. > > The var_off and off is a part of the bug, but it's not the biggest part of it. > > Not compile tested. I see. All makes sense to me. Benjamin, pls incorporate it in your set.
On Mar 24 2024, Alexei Starovoitov wrote: > On Sat, Mar 23, 2024 at 9:57 PM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > > > > > > > > Observation is correct. The patch is buggy, > > > > > but the suggestion to follow process_dynptr_func() will lead > > > > > to unnecessary complexity. > > > > > dynptr-s are on stack with plenty of extra checks. > > > > > > > > The suggestion was to call process_timer_func, not process_dynptr_func. > > > > > > > > > In this case bpf_timer is in map_value. > > > > > Much simpler is to follow KF_ARG_PTR_TO_MAP approach. > > > > > > > > What I meant by the example was that dynptr handling does the same > > > > thing for kfuncs and helpers (using the same function), so timer > > > > arguments should do the same (i.e. use process_timer_func), which will > > > > do all checks for constant offset (ensuring var_off is tnum_is_const) > > > > and match it against btf_record->timer_off. > > > > > > I don't follow. Please elaborate with a patch. > > > The var_off and off is a part of the bug, but it's not the biggest part of it. > > > > Not compile tested. Compiles just fine :) > > I see. All makes sense to me. > > Benjamin, > pls incorporate it in your set. > OK, done! I just had to revert to the following or KF_ARG_TIMER_ID was not recognized by the verifier: --- diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 7ee20e9d14bd..a5e147468ac8 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -10848,7 +10848,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) +BTF_ID(struct, bpf_timer_kern) static bool __is_kfunc_ptr_arg_type(const struct btf *btf, const struct btf_param *arg, int type) --- Cheers, Benjamin
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 63749ad5ac6b..24a604e26ec7 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,16 @@ 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; + } + if (reg->off) { + verbose(env, "arg#%d offset can not be greater than 0\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 v5: - also check for the reg offset 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 | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)