On Mon, Oct 23, 2023 at 7:42 PM 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 | 21 ++++++++++++-------
> .../selftests/bpf/progs/iters_task_failure.c | 4 ++--
> 2 files changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index e9bc5d4a25a1..9f209adc4ccb 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11088,18 +11088,23 @@ 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:
> + if (prog_type == BPF_PROG_TYPE_LSM)
> return true;
> - case BPF_TRACE_ITER:
> - return env->prog->aux->sleepable;
> - default:
> - return false;
> - }
> +
> + if (env->prog->expected_attach_type == BPF_TRACE_ITER)
> + return true;
I think the switch by prog_type has to stay.
Checking attach_type == BPF_TRACE_ITER without considering prog_type
is fragile. It likely works, but we don't do it anywhere else.
Let's stick to what is known to work.
> -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")
Please add both. fentry that is rejected and fentry.s that is accepted.
