| Message ID | 20231022154527.229117-2-zhouchuyi@bytedance.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Relax allowlist for open-coded css_task iter | expand |
On Sun, Oct 22, 2023 at 8:45 AM Chuyi Zhou <zhouchuyi@bytedance.com> wrote: > > The newly added open-coded css_task iter would try to hold the global > css_set_lock in bpf_iter_css_task_new, so the bpf side has to be careful in > where it allows to use this iter. The mainly concern is dead locking on > css_set_lock. check_css_task_iter_allowlist() in verifier enforced css_task > can only be used in bpf_lsm hooks and sleepable bpf_iter. > > This patch relax the allowlist for css_task iter. Any lsm and any iter > (even non-sleepable) and any sleepable are safe since they would not hold > the css_set_lock before entering BPF progs context. > > This patch also fixes the misused BPF_TRACE_ITER in > check_css_task_iter_allowlist which compared bpf_prog_type with > bpf_attach_type. > > Fixes: 9c66dc94b62ae ("bpf: Introduce css_task open-coded iterator kfuncs") > Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com> > --- > kernel/bpf/verifier.c | 15 ++++++++++----- > .../selftests/bpf/progs/iters_task_failure.c | 4 ++-- > 2 files changed, 12 insertions(+), 7 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index e9bc5d4a25a1..cc79cd555337 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -11088,17 +11088,22 @@ static int process_kf_arg_ptr_to_rbtree_node(struct bpf_verifier_env *env, > &meta->arg_rbtree_root.field); > } > > +/* > + * css_task iter allowlist is needed to avoid dead locking on css_set_lock. > + * LSM hooks and iters (both sleepable and non-sleepable) are safe. > + * Any sleepable progs are also safe since bpf_check_attach_target() enforce > + * them can only be attached to some specific hook points. > + */ > static bool check_css_task_iter_allowlist(struct bpf_verifier_env *env) > { > enum bpf_prog_type prog_type = resolve_prog_type(env->prog); > - > switch (prog_type) { > case BPF_PROG_TYPE_LSM: > return true; > - case BPF_TRACE_ITER: > - return env->prog->aux->sleepable; > + case BPF_PROG_TYPE_TRACING: > + return env->prog->expected_attach_type == BPF_TRACE_ITER; I think it needs to be if (env->prog->expected_attach_type == BPF_TRACE_ITER) return true; /* else: fall through to check sleepable */ > default: > - return false; > + return env->prog->aux->sleepable; > } > } > > @@ -11357,7 +11362,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ > case KF_ARG_PTR_TO_ITER: > if (meta->func_id == special_kfunc_list[KF_bpf_iter_css_task_new]) { > if (!check_css_task_iter_allowlist(env)) { > - verbose(env, "css_task_iter is only allowed in bpf_lsm and bpf iter-s\n"); > + verbose(env, "css_task_iter is only allowed in bpf_lsm, bpf_iter and sleepable progs\n"); > return -EINVAL; > } > } > diff --git a/tools/testing/selftests/bpf/progs/iters_task_failure.c b/tools/testing/selftests/bpf/progs/iters_task_failure.c > index c3bf96a67dba..6b1588d70652 100644 > --- a/tools/testing/selftests/bpf/progs/iters_task_failure.c > +++ b/tools/testing/selftests/bpf/progs/iters_task_failure.c > @@ -84,8 +84,8 @@ int BPF_PROG(iter_css_lock_and_unlock) > return 0; > } > > -SEC("?fentry.s/" SYS_PREFIX "sys_getpgid") > -__failure __msg("css_task_iter is only allowed in bpf_lsm and bpf iter-s") > +SEC("?fentry/" SYS_PREFIX "sys_getpgid") so that fentry/sys_foo is rejected, but fentry.s/sys_foo loads ok. > +__failure __msg("css_task_iter is only allowed in bpf_lsm, bpf_iter and sleepable progs") > int BPF_PROG(iter_css_task_for_each) > { > u64 cg_id = bpf_get_current_cgroup_id(); > -- > 2.20.1 >
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index e9bc5d4a25a1..cc79cd555337 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -11088,17 +11088,22 @@ static int process_kf_arg_ptr_to_rbtree_node(struct bpf_verifier_env *env, &meta->arg_rbtree_root.field); } +/* + * css_task iter allowlist is needed to avoid dead locking on css_set_lock. + * LSM hooks and iters (both sleepable and non-sleepable) are safe. + * Any sleepable progs are also safe since bpf_check_attach_target() enforce + * them can only be attached to some specific hook points. + */ static bool check_css_task_iter_allowlist(struct bpf_verifier_env *env) { enum bpf_prog_type prog_type = resolve_prog_type(env->prog); - switch (prog_type) { case BPF_PROG_TYPE_LSM: return true; - case BPF_TRACE_ITER: - return env->prog->aux->sleepable; + case BPF_PROG_TYPE_TRACING: + return env->prog->expected_attach_type == BPF_TRACE_ITER; default: - return false; + return env->prog->aux->sleepable; } } @@ -11357,7 +11362,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ case KF_ARG_PTR_TO_ITER: if (meta->func_id == special_kfunc_list[KF_bpf_iter_css_task_new]) { if (!check_css_task_iter_allowlist(env)) { - verbose(env, "css_task_iter is only allowed in bpf_lsm and bpf iter-s\n"); + verbose(env, "css_task_iter is only allowed in bpf_lsm, bpf_iter and sleepable progs\n"); return -EINVAL; } } diff --git a/tools/testing/selftests/bpf/progs/iters_task_failure.c b/tools/testing/selftests/bpf/progs/iters_task_failure.c index c3bf96a67dba..6b1588d70652 100644 --- a/tools/testing/selftests/bpf/progs/iters_task_failure.c +++ b/tools/testing/selftests/bpf/progs/iters_task_failure.c @@ -84,8 +84,8 @@ int BPF_PROG(iter_css_lock_and_unlock) return 0; } -SEC("?fentry.s/" SYS_PREFIX "sys_getpgid") -__failure __msg("css_task_iter is only allowed in bpf_lsm and bpf iter-s") +SEC("?fentry/" SYS_PREFIX "sys_getpgid") +__failure __msg("css_task_iter is only allowed in bpf_lsm, bpf_iter and sleepable progs") int BPF_PROG(iter_css_task_for_each) { u64 cg_id = bpf_get_current_cgroup_id();
The newly added open-coded css_task iter would try to hold the global css_set_lock in bpf_iter_css_task_new, so the bpf side has to be careful in where it allows to use this iter. The mainly concern is dead locking on css_set_lock. check_css_task_iter_allowlist() in verifier enforced css_task can only be used in bpf_lsm hooks and sleepable bpf_iter. This patch relax the allowlist for css_task iter. Any lsm and any iter (even non-sleepable) and any sleepable are safe since they would not hold the css_set_lock before entering BPF progs context. This patch also fixes the misused BPF_TRACE_ITER in check_css_task_iter_allowlist which compared bpf_prog_type with bpf_attach_type. Fixes: 9c66dc94b62ae ("bpf: Introduce css_task open-coded iterator kfuncs") Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com> --- kernel/bpf/verifier.c | 15 ++++++++++----- .../selftests/bpf/progs/iters_task_failure.c | 4 ++-- 2 files changed, 12 insertions(+), 7 deletions(-)