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.