| Message ID | 20230925105552.817513-7-zhouchuyi@bytedance.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Add Open-coded task, css_task and css iters | expand |
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? > verbose(env, "arg#%d pointer type %s %s must point to %sscalar, or struct with scalar\n", > argno, btf_type_str(ref_t), ref_tname, arg_mem_size ? "void, " : ""); > return -EINVAL; > @@ -11028,7 +11037,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ > } > > if ((is_kfunc_trusted_args(meta) || is_kfunc_rcu(meta)) && > - (register_is_null(reg) || type_may_be_null(reg->type))) { > + (register_is_null(reg) || type_may_be_null(reg->type)) && !is_kfunc_arg_optional(meta->btf, &args[i])) { nit: looks like a very long line, probably wrap to the next line? > verbose(env, "Possibly NULL pointer passed to trusted arg%d\n", i); > return -EACCES; > } > @@ -11053,6 +11062,8 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ > return kf_arg_type; > > switch (kf_arg_type) { > + case KF_ARG_PTR_TO_NULL: > + continue; > 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)) > -- > 2.20.1 >
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.
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; verbose(env, "arg#%d pointer type %s %s must point to %sscalar, or struct with scalar\n", argno, btf_type_str(ref_t), ref_tname, arg_mem_size ? "void, " : ""); return -EINVAL; @@ -11028,7 +11037,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ } if ((is_kfunc_trusted_args(meta) || is_kfunc_rcu(meta)) && - (register_is_null(reg) || type_may_be_null(reg->type))) { + (register_is_null(reg) || type_may_be_null(reg->type)) && !is_kfunc_arg_optional(meta->btf, &args[i])) { verbose(env, "Possibly NULL pointer passed to trusted arg%d\n", i); return -EACCES; } @@ -11053,6 +11062,8 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ return kf_arg_type; switch (kf_arg_type) { + case KF_ARG_PTR_TO_NULL: + continue; 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))
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(-)