On Sun, Oct 22, 2023 at 8:45 AM Chuyi Zhou <zhouchuyi@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> ---
> 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
>
