| Message ID | 20240416-bpf_wq-v1-8-c9e66092f842@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce bpf_wq | expand |
On Tue, Apr 16, 2024 at 04:08:21PM +0200, Benjamin Tissoires wrote: > Introduce support for KF_ARG_PTR_TO_WORKQUEUE. The kfuncs will use bpf_wq > as argument and that will be recognized as workqueue argument by verifier. > bpf_wq_kern casting can happen inside kfunc, but using bpf_wq in > argument makes life easier for users who work with non-kern type in BPF > progs. > > Duplicate process_timer_func into process_wq_func. > meta argument is only needed to ensure bpf_wq_init's workqueue and map > arguments are coming from the same map (map_uid logic is necessary for > correct inner-map handling), so also amend check_kfunc_args() to > match what helpers functions check is doing. > > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org> > --- > kernel/bpf/verifier.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 86 insertions(+) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index deaf2e1ab690..112faf2cd7e9 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -332,6 +332,10 @@ struct bpf_kfunc_call_arg_meta { > u8 spi; > u8 frameno; > } iter; > + struct { > + struct bpf_map *ptr; > + int uid; > + } map; > u64 mem_size; > }; > > @@ -7598,6 +7602,43 @@ static int process_timer_func(struct bpf_verifier_env *env, int regno, > return 0; > } > > +static int process_wq_func(struct bpf_verifier_env *env, int regno, > + struct bpf_kfunc_call_arg_meta *meta) > +{ > + struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; > + bool is_const = tnum_is_const(reg->var_off); > + struct bpf_map *map = reg->map_ptr; > + u64 val = reg->var_off.value; > + > + if (!is_const) { > + verbose(env, > + "R%d doesn't have constant offset. bpf_wq has to be at the constant offset\n", > + regno); > + return -EINVAL; > + } This check is unnecessary. Before calling into this function it goes as: switch (kf_arg_type) { case KF_ARG_PTR_TO_MAP: ... your new checks ... fallthrough; case KF_ARG_PTR_TO_CTX: arg_type |= OBJ_RELEASE; break; check_func_arg_reg_off() if (arg_type_is_release(arg_type) return __check_ptr_off_reg() // that does offset checks > + if (!map->btf) { > + verbose(env, "map '%s' has to have BTF in order to use bpf_wq\n", > + map->name); > + return -EINVAL; > + } > + if (!btf_record_has_field(map->record, BPF_WORKQUEUE)) { > + verbose(env, "map '%s' has no valid bpf_wq\n", map->name); > + return -EINVAL; > + } above two are also unnecessary. wq_off will be negative otherwise. > + 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; > + } only this one is needed, but I need to study the rest of the patches. > + if (meta->map.ptr) { > + verbose(env, "verifier bug. Two map pointers in a workqueue helper\n"); > + return -EFAULT; > + } this was checked already as well. > + meta->map.uid = reg->map_uid; > + meta->map.ptr = map; > + return 0; > +} > + > static int process_kptr_func(struct bpf_verifier_env *env, int regno, > struct bpf_call_arg_meta *meta) > { > @@ -10843,6 +10884,7 @@ enum { > KF_ARG_LIST_NODE_ID, > KF_ARG_RB_ROOT_ID, > KF_ARG_RB_NODE_ID, > + KF_ARG_WORKQUEUE_ID, > }; > > BTF_ID_LIST(kf_arg_btf_ids) > @@ -10851,6 +10893,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_wq) > > static bool __is_kfunc_ptr_arg_type(const struct btf *btf, > const struct btf_param *arg, int type) > @@ -10894,6 +10937,11 @@ 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_wq(const struct btf *btf, const struct btf_param *arg) > +{ > + return __is_kfunc_ptr_arg_type(btf, arg, KF_ARG_WORKQUEUE_ID); > +} > + > static bool is_kfunc_arg_callback(struct bpf_verifier_env *env, const struct btf *btf, > const struct btf_param *arg) > { > @@ -10963,6 +11011,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_WORKQUEUE, > }; > > enum special_kfunc_type { > @@ -11119,6 +11168,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_wq(meta->btf, &args[argno])) > + return KF_ARG_PTR_TO_WORKQUEUE; > + > 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", > @@ -11720,6 +11772,30 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ > case KF_ARG_PTR_TO_NULL: > continue; > case KF_ARG_PTR_TO_MAP: > + if (meta->map.ptr) { > + /* Use map_uid (which is unique id of inner map) to reject: > + * inner_map1 = bpf_map_lookup_elem(outer_map, key1) > + * inner_map2 = bpf_map_lookup_elem(outer_map, key2) > + * if (inner_map1 && inner_map2) { > + * timer = bpf_map_lookup_elem(inner_map1); > + * if (timer) > + * // mismatch would have been allowed > + * bpf_timer_init(timer, inner_map2); adjust the comment too. Here it can only be wq. > + * } > + * > + * Comparing map_ptr is enough to distinguish normal and outer maps. > + */ > + if (meta->map.ptr != reg->map_ptr || > + meta->map.uid != reg->map_uid) { let's also gate this check with map->record->wq_off. Otherwise it will trigger for maps that don't have wq field. Copy paste is a bit unfortunate, but fine. We can clean it up later. > + verbose(env, > + "workqueue pointer in R1 map_uid=%d doesn't match map pointer in R2 map_uid=%d\n", > + meta->map.uid, reg->map_uid); > + return -EINVAL; > + } > + } > + meta->map.ptr = reg->map_ptr; > + meta->map.uid = reg->map_uid; > + fallthrough; > case KF_ARG_PTR_TO_ALLOC_BTF_ID: > case KF_ARG_PTR_TO_BTF_ID: > if (!is_kfunc_trusted_args(meta) && !is_kfunc_rcu(meta)) > @@ -11752,6 +11828,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_WORKQUEUE: > /* Trusted by default */ > break; > default: > @@ -12038,6 +12115,15 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ > if (ret) > return ret; > break; > + case KF_ARG_PTR_TO_WORKQUEUE: > + if (reg->type != PTR_TO_MAP_VALUE) { > + verbose(env, "arg#%d doesn't point to a map value\n", i); > + return -EINVAL; > + } > + ret = process_wq_func(env, regno, meta); > + if (ret < 0) > + return ret; > + break; > } > } > > > -- > 2.44.0 >
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index deaf2e1ab690..112faf2cd7e9 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -332,6 +332,10 @@ struct bpf_kfunc_call_arg_meta { u8 spi; u8 frameno; } iter; + struct { + struct bpf_map *ptr; + int uid; + } map; u64 mem_size; }; @@ -7598,6 +7602,43 @@ static int process_timer_func(struct bpf_verifier_env *env, int regno, return 0; } +static int process_wq_func(struct bpf_verifier_env *env, int regno, + struct bpf_kfunc_call_arg_meta *meta) +{ + struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; + bool is_const = tnum_is_const(reg->var_off); + struct bpf_map *map = reg->map_ptr; + u64 val = reg->var_off.value; + + if (!is_const) { + verbose(env, + "R%d doesn't have constant offset. bpf_wq has to be at the constant offset\n", + regno); + return -EINVAL; + } + if (!map->btf) { + verbose(env, "map '%s' has to have BTF in order to use bpf_wq\n", + map->name); + return -EINVAL; + } + if (!btf_record_has_field(map->record, BPF_WORKQUEUE)) { + verbose(env, "map '%s' has no valid bpf_wq\n", map->name); + return -EINVAL; + } + 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; + } + if (meta->map.ptr) { + verbose(env, "verifier bug. Two map pointers in a workqueue helper\n"); + return -EFAULT; + } + meta->map.uid = reg->map_uid; + meta->map.ptr = map; + return 0; +} + static int process_kptr_func(struct bpf_verifier_env *env, int regno, struct bpf_call_arg_meta *meta) { @@ -10843,6 +10884,7 @@ enum { KF_ARG_LIST_NODE_ID, KF_ARG_RB_ROOT_ID, KF_ARG_RB_NODE_ID, + KF_ARG_WORKQUEUE_ID, }; BTF_ID_LIST(kf_arg_btf_ids) @@ -10851,6 +10893,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_wq) static bool __is_kfunc_ptr_arg_type(const struct btf *btf, const struct btf_param *arg, int type) @@ -10894,6 +10937,11 @@ 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_wq(const struct btf *btf, const struct btf_param *arg) +{ + return __is_kfunc_ptr_arg_type(btf, arg, KF_ARG_WORKQUEUE_ID); +} + static bool is_kfunc_arg_callback(struct bpf_verifier_env *env, const struct btf *btf, const struct btf_param *arg) { @@ -10963,6 +11011,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_WORKQUEUE, }; enum special_kfunc_type { @@ -11119,6 +11168,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_wq(meta->btf, &args[argno])) + return KF_ARG_PTR_TO_WORKQUEUE; + 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", @@ -11720,6 +11772,30 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ case KF_ARG_PTR_TO_NULL: continue; case KF_ARG_PTR_TO_MAP: + if (meta->map.ptr) { + /* Use map_uid (which is unique id of inner map) to reject: + * inner_map1 = bpf_map_lookup_elem(outer_map, key1) + * inner_map2 = bpf_map_lookup_elem(outer_map, key2) + * if (inner_map1 && inner_map2) { + * timer = bpf_map_lookup_elem(inner_map1); + * if (timer) + * // mismatch would have been allowed + * bpf_timer_init(timer, inner_map2); + * } + * + * Comparing map_ptr is enough to distinguish normal and outer maps. + */ + if (meta->map.ptr != reg->map_ptr || + meta->map.uid != reg->map_uid) { + verbose(env, + "workqueue pointer in R1 map_uid=%d doesn't match map pointer in R2 map_uid=%d\n", + meta->map.uid, reg->map_uid); + return -EINVAL; + } + } + meta->map.ptr = reg->map_ptr; + meta->map.uid = reg->map_uid; + fallthrough; case KF_ARG_PTR_TO_ALLOC_BTF_ID: case KF_ARG_PTR_TO_BTF_ID: if (!is_kfunc_trusted_args(meta) && !is_kfunc_rcu(meta)) @@ -11752,6 +11828,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_WORKQUEUE: /* Trusted by default */ break; default: @@ -12038,6 +12115,15 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ if (ret) return ret; break; + case KF_ARG_PTR_TO_WORKQUEUE: + if (reg->type != PTR_TO_MAP_VALUE) { + verbose(env, "arg#%d doesn't point to a map value\n", i); + return -EINVAL; + } + ret = process_wq_func(env, regno, meta); + if (ret < 0) + return ret; + break; } }
Introduce support for KF_ARG_PTR_TO_WORKQUEUE. The kfuncs will use bpf_wq as argument and that will be recognized as workqueue argument by verifier. bpf_wq_kern casting can happen inside kfunc, but using bpf_wq in argument makes life easier for users who work with non-kern type in BPF progs. Duplicate process_timer_func into process_wq_func. meta argument is only needed to ensure bpf_wq_init's workqueue and map arguments are coming from the same map (map_uid logic is necessary for correct inner-map handling), so also amend check_kfunc_args() to match what helpers functions check is doing. Signed-off-by: Benjamin Tissoires <bentiss@kernel.org> --- kernel/bpf/verifier.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+)