Messages in this thread Patch in this message | | | Date | Sun, 1 Oct 2023 16:30:59 +0800 | Subject | Re: [PATCH bpf-next v3 6/7] bpf: Let bpf_iter_task_new accept null task ptr | From | Chuyi Zhou <> |
| |
Hello,
在 2023/9/28 07:37, Andrii Nakryiko 写道: > On Mon, Sep 25, 2023 at 3:56 AM Chuyi Zhou <zhouchuyi@bytedance.com> wrote: >> >> When using task_iter to iterate all threads of a specific task, we enforce >> that the user must pass a valid task pointer to ensure safety. However, >> when iterating all threads/process in the system, BPF verifier still >> require a valid ptr instead of "nullable" pointer, even though it's >> pointless, which is a kind of surprising from usability standpoint. It >> would be nice if we could let that kfunc accept a explicit null pointer >> when we are using BPF_TASK_ITER_ALL/BPF_TASK_ITER_PROC and a valid pointer >> when using BPF_TASK_ITER_THREAD. >> >> Given a trival kfunc: >> __bpf_kfunc void FN(struct TYPE_A *obj) >> >> BPF Prog would reject a nullptr for obj. The error info is: >> "arg#x pointer type xx xx must point to scalar, or struct with scalar" >> reported by get_kfunc_ptr_arg_type(). The reg->type is SCALAR_VALUE and >> the btf type of ref_t is not scalar or scalar_struct which leads to the >> rejection of get_kfunc_ptr_arg_type. >> >> This patch reuse the __opt annotation which was used to indicate that >> the buffer associated with an __sz or __szk argument may be null: >> __bpf_kfunc void FN(struct TYPE_A *obj__opt) >> Here __opt indicates obj can be optional, user can pass a explicit nullptr >> or a normal TYPE_A pointer. In get_kfunc_ptr_arg_type(), we will detect >> whether the current arg is optional and register is null, If so, return >> a new kfunc_ptr_arg_type KF_ARG_PTR_TO_NULL and skip to the next arg in >> check_kfunc_args(). >> >> Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com> >> --- >> kernel/bpf/task_iter.c | 7 +++++-- >> kernel/bpf/verifier.c | 13 ++++++++++++- >> 2 files changed, 17 insertions(+), 3 deletions(-) >> >> diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c >> index 9bcd3f9922b1..7ac007f161cc 100644 >> --- a/kernel/bpf/task_iter.c >> +++ b/kernel/bpf/task_iter.c >> @@ -867,7 +867,7 @@ struct bpf_iter_task_kern { >> unsigned int type; >> } __attribute__((aligned(8))); >> >> -__bpf_kfunc int bpf_iter_task_new(struct bpf_iter_task *it, struct task_struct *task, unsigned int type) >> +__bpf_kfunc int bpf_iter_task_new(struct bpf_iter_task *it, struct task_struct *task__opt, unsigned int type) >> { >> struct bpf_iter_task_kern *kit = (void *)it; >> BUILD_BUG_ON(sizeof(struct bpf_iter_task_kern) != sizeof(struct bpf_iter_task)); >> @@ -877,14 +877,17 @@ __bpf_kfunc int bpf_iter_task_new(struct bpf_iter_task *it, struct task_struct * >> switch (type) { >> case BPF_TASK_ITER_ALL: >> case BPF_TASK_ITER_PROC: >> + break; >> case BPF_TASK_ITER_THREAD: >> + if (!task__opt) >> + return -EINVAL; >> break; >> default: >> return -EINVAL; >> } >> >> if (type == BPF_TASK_ITER_THREAD) >> - kit->task = task; >> + kit->task = task__opt; >> else >> kit->task = &init_task; >> kit->pos = kit->task; >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index a065e18a0b3a..a79204c75a90 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -10331,6 +10331,7 @@ enum kfunc_ptr_arg_type { >> KF_ARG_PTR_TO_CALLBACK, >> KF_ARG_PTR_TO_RB_ROOT, >> KF_ARG_PTR_TO_RB_NODE, >> + KF_ARG_PTR_TO_NULL, >> }; >> >> enum special_kfunc_type { >> @@ -10425,6 +10426,12 @@ static bool is_kfunc_bpf_rcu_read_unlock(struct bpf_kfunc_call_arg_meta *meta) >> return meta->func_id == special_kfunc_list[KF_bpf_rcu_read_unlock]; >> } >> >> +static inline bool is_kfunc_arg_optional_null(struct bpf_reg_state *reg, >> + const struct btf *btf, const struct btf_param *arg) >> +{ >> + return register_is_null(reg) && is_kfunc_arg_optional(btf, arg); >> +} >> + >> static enum kfunc_ptr_arg_type >> get_kfunc_ptr_arg_type(struct bpf_verifier_env *env, >> struct bpf_kfunc_call_arg_meta *meta, >> @@ -10497,6 +10504,8 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env, >> */ >> if (!btf_type_is_scalar(ref_t) && !__btf_type_is_scalar_struct(env, meta->btf, ref_t, 0) && >> (arg_mem_size ? !btf_type_is_void(ref_t) : 1)) { >> + if (is_kfunc_arg_optional_null(reg, meta->btf, &args[argno])) >> + return KF_ARG_PTR_TO_NULL; > > This nested check seems misplaced. Maybe we shouldn't reuse __opt > suffix which already has a different meaning (coupled with __sz). Why > not add "__nullable" convention and just check it separately? >
IIUC, do you mean:
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index dbba2b806017..05d197365fcb 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -10458,6 +10458,8 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env, if (is_kfunc_arg_callback(env, meta->btf, &args[argno])) return KF_ARG_PTR_TO_CALLBACK;
+ if (is_kfunc_arg_nullable(meta->btf, &args[argno]) && register_is_null(reg)) + return KF_ARG_PTR_TO_NULL;
if (argno + 1 < nargs && (is_kfunc_arg_mem_size(meta->btf, &args[argno + 1], ®s[regno + 1]) ||
OK, I would change in next version.
Thanks.
| |