[PATCH bpf-next v2 0/2] Relax allowlist for open-coded css_task iter

[PATCH bpf-next v2 0/2] Relax allowlist for open-coded css_task iter

[Chuyi Zhou]

Hi,
The patchset aims to relax the allowlist for open-coded css_task iter
suggested by Alexei[1].

Please see individual patches for more details. And comments are always
welcome.

Patch summary:
 * Patch #1: Relax the allowlist and let css_task iter can be used in
   bpf iters and any sleepable progs.
 * Patch #2: Add a test in cgroup_iters.c which demonstrates how
   css_task iters can be combined with cgroup iter.

link[1]:https://lore.kernel.org/lkml/CAADnVQKafk_junRyE=-FVAik4hjTRDtThymYGEL8hGTuYoOGpA@mail.gmail.com/

---

Changes in v2:
 * Fix the incorrect logic in check_css_task_iter_allowlist. Use
   expected_attach_type to check whether we are using bpf_iters.
 * Link to v1:https://lore.kernel.org/bpf/20231022154527.229117-1-zhouchuyi@bytedance.com/T/#m946f9cde86b44a13265d9a44c5738a711eb578fd

---

Chuyi Zhou (2):
  bpf: Relax allowlist for css_task iter
  selftests/bpf: Add test for css_task iter combining with cgroup iter

 kernel/bpf/verifier.c                         | 21 ++++++----
 .../selftests/bpf/prog_tests/cgroup_iter.c    | 33 +++++++++++++++
 .../selftests/bpf/progs/iters_css_task.c      | 41 +++++++++++++++++++
 .../selftests/bpf/progs/iters_task_failure.c  |  4 +-
 4 files changed, 89 insertions(+), 10 deletions(-)

-- 
2.20.1

[PATCH bpf-next v2 1/2] bpf: Relax allowlist for css_task iter

[Chuyi Zhou]

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                         | 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;
+
+	return env->prog->aux->sleepable;
 }
 
 static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_arg_meta *meta,
@@ -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();
-- 
2.20.1

[PATCH bpf-next v2 2/2] selftests/bpf: Add test for css_task iter combining with cgroup iter

[Chuyi Zhou]

This patch adds a test which demonstrates how css_task iter can be combined
with cgroup iter and it won't cause deadlock, though cgroup iter is not
sleepable.

Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com>
---
 .../selftests/bpf/prog_tests/cgroup_iter.c    | 33 +++++++++++++++
 .../selftests/bpf/progs/iters_css_task.c      | 41 +++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c b/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c
index e02feb5fae97..3679687a6927 100644
--- a/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c
@@ -4,6 +4,7 @@
 #include <test_progs.h>
 #include <bpf/libbpf.h>
 #include <bpf/btf.h>
+#include "iters_css_task.skel.h"
 #include "cgroup_iter.skel.h"
 #include "cgroup_helpers.h"
 
@@ -263,6 +264,35 @@ static void test_walk_dead_self_only(struct cgroup_iter *skel)
 	close(cgrp_fd);
 }
 
+static void test_walk_self_only_css_task(void)
+{
+	struct iters_css_task *skel = NULL;
+	int err;
+
+	skel = iters_css_task__open();
+	if (!ASSERT_OK_PTR(skel, "skel_open"))
+		return;
+
+	bpf_program__set_autoload(skel->progs.cgroup_id_printer, true);
+
+	err = iters_css_task__load(skel);
+	if (!ASSERT_OK(err, "skel_load"))
+		goto cleanup;
+
+	err = join_cgroup(cg_path[CHILD2]);
+	if (!ASSERT_OK(err, "join_cgroup"))
+		goto cleanup;
+
+	skel->bss->target_pid = getpid();
+	snprintf(expected_output, sizeof(expected_output),
+		PROLOGUE "%8llu\n" EPILOGUE, cg_id[CHILD2]);
+	read_from_cgroup_iter(skel->progs.cgroup_id_printer, cg_fd[CHILD2],
+		BPF_CGROUP_ITER_SELF_ONLY, "test_walk_self_only_css_task");
+	ASSERT_EQ(skel->bss->css_task_cnt, 1, "css_task_cnt");
+cleanup:
+	iters_css_task__destroy(skel);
+}
+
 void test_cgroup_iter(void)
 {
 	struct cgroup_iter *skel = NULL;
@@ -293,6 +323,9 @@ void test_cgroup_iter(void)
 		test_walk_self_only(skel);
 	if (test__start_subtest("cgroup_iter__dead_self_only"))
 		test_walk_dead_self_only(skel);
+	if (test__start_subtest("cgroup_iter__self_only_css_task"))
+		test_walk_self_only_css_task();
+
 out:
 	cgroup_iter__destroy(skel);
 	cleanup_cgroups();
diff --git a/tools/testing/selftests/bpf/progs/iters_css_task.c b/tools/testing/selftests/bpf/progs/iters_css_task.c
index 5089ce384a1c..0974e6f44328 100644
--- a/tools/testing/selftests/bpf/progs/iters_css_task.c
+++ b/tools/testing/selftests/bpf/progs/iters_css_task.c
@@ -10,6 +10,7 @@
 
 char _license[] SEC("license") = "GPL";
 
+struct cgroup *bpf_cgroup_acquire(struct cgroup *p) __ksym;
 struct cgroup *bpf_cgroup_from_id(u64 cgid) __ksym;
 void bpf_cgroup_release(struct cgroup *p) __ksym;
 
@@ -45,3 +46,43 @@ int BPF_PROG(iter_css_task_for_each, struct vm_area_struct *vma,
 
 	return -EPERM;
 }
+
+static inline u64 cgroup_id(struct cgroup *cgrp)
+{
+	return cgrp->kn->id;
+}
+
+SEC("?iter/cgroup")
+int cgroup_id_printer(struct bpf_iter__cgroup *ctx)
+{
+	struct seq_file *seq = ctx->meta->seq;
+	struct cgroup *cgrp, *acquired;
+	struct cgroup_subsys_state *css;
+	struct task_struct *task;
+
+	cgrp = ctx->cgroup;
+
+	/* epilogue */
+	if (cgrp == NULL) {
+		BPF_SEQ_PRINTF(seq, "epilogue\n");
+		return 0;
+	}
+
+	/* prologue */
+	if (ctx->meta->seq_num == 0)
+		BPF_SEQ_PRINTF(seq, "prologue\n");
+
+	BPF_SEQ_PRINTF(seq, "%8llu\n", cgroup_id(cgrp));
+
+	acquired = bpf_cgroup_from_id(cgroup_id(cgrp));
+	if (!acquired)
+		return 0;
+	css = &acquired->self;
+	css_task_cnt = 0;
+	bpf_for_each(css_task, task, css, CSS_TASK_ITER_PROCS) {
+		if (task->pid == target_pid)
+			css_task_cnt++;
+	}
+	bpf_cgroup_release(acquired);
+	return 0;
+}
-- 
2.20.1

Re: [PATCH bpf-next v2 1/2] bpf: Relax allowlist for css_task iter

[Alexei Starovoitov]

On Mon, Oct 23, 2023 at 7:42 PM 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                         | 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.

Re: [PATCH bpf-next v2 1/2] bpf: Relax allowlist for css_task iter

[Chuyi Zhou]

Hello,

在 2023/10/24 12:57, Alexei Starovoitov 写道:
> On Mon, Oct 23, 2023 at 7:42 PM 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                         | 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.
> 

IIUC, do you mean:

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_PROG_TYPE_TRACING:
		if (env->prog->expected_attach_type == BPF_TRACE_ITER)
			return true;
		return env->prog->aux->sleepable;
  	default:
		return env->prog->aux->sleepable;
  	}
}

>> -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.

Sure.

Re: [PATCH bpf-next v2 1/2] bpf: Relax allowlist for css_task iter

[Yonghong Song]

On 10/23/23 10:52 PM, Chuyi Zhou wrote:
> Hello,
>
> 在 2023/10/24 12:57, Alexei Starovoitov 写道:
>> On Mon, Oct 23, 2023 at 7:42 PM 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                         | 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.
>>
>
> IIUC, do you mean:
>
> 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_PROG_TYPE_TRACING:
>         if (env->prog->expected_attach_type == BPF_TRACE_ITER)
>             return true;
>         return env->prog->aux->sleepable;


The above can be a fullthrough instead.


> default:
>         return env->prog->aux->sleepable;
>      }
> }
>
>>> -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.
>
> Sure.
>

Re: [PATCH bpf-next v2 1/2] bpf: Relax allowlist for css_task iter

[Chuyi Zhou]

Hello,

在 2023/10/24 14:08, Yonghong Song 写道:
> 
> On 10/23/23 10:52 PM, Chuyi Zhou wrote:
>> Hello,
>>
>> 在 2023/10/24 12:57, Alexei Starovoitov 写道:
>>> On Mon, Oct 23, 2023 at 7:42 PM 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                         | 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.
>>>
>>
>> IIUC, do you mean:
>>
>> 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_PROG_TYPE_TRACING:
>>         if (env->prog->expected_attach_type == BPF_TRACE_ITER)
>>             return true;
>>         return env->prog->aux->sleepable;
> 
> 
> The above can be a fullthrough instead.
> 

Sorry, what do you mean 'a fullthrough' ?
Do you mean we can check env->prog->aux->sleepable first and then fall 
back to check prog/attach type ?

Re: [PATCH bpf-next v2 1/2] bpf: Relax allowlist for css_task iter

[Chuyi Zhou]

在 2023/10/24 14:23, Chuyi Zhou 写道:
> Hello,
> 
> 在 2023/10/24 14:08, Yonghong Song 写道:
>>
>> On 10/23/23 10:52 PM, Chuyi Zhou wrote:
>>> Hello,
>>>
>>> 在 2023/10/24 12:57, Alexei Starovoitov 写道:
>>>> On Mon, Oct 23, 2023 at 7:42 PM 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                         | 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.
>>>>
>>>
>>> IIUC, do you mean:
>>>
>>> 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_PROG_TYPE_TRACING:
>>>         if (env->prog->expected_attach_type == BPF_TRACE_ITER)
>>>             return true;
>>>         return env->prog->aux->sleepable;
>>
>>
>> The above can be a fullthrough instead.
>>
> 
> Sorry, what do you mean 'a fullthrough' ?
> Do you mean we can check env->prog->aux->sleepable first and then fall 
> back to check prog/attach type ?
> 

I see...

Sorry for the above noise. I noticed verifier.c uses 'fallthrough' to 
avoid the build warning, so we can:

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_PROG_TYPE_TRACING:
		if (env->prog->expected_attach_type == BPF_TRACE_ITER)
			return true;
		fallthrough;
	default:
		return env->prog->aux->sleepable;
	}
}

Re: [PATCH bpf-next v2 1/2] bpf: Relax allowlist for css_task iter

[Yonghong Song]

On 10/24/23 4:43 AM, Chuyi Zhou wrote:
>
>
> 在 2023/10/24 14:23, Chuyi Zhou 写道:
>> Hello,
>>
>> 在 2023/10/24 14:08, Yonghong Song 写道:
>>>
>>> On 10/23/23 10:52 PM, Chuyi Zhou wrote:
>>>> Hello,
>>>>
>>>> 在 2023/10/24 12:57, Alexei Starovoitov 写道:
>>>>> On Mon, Oct 23, 2023 at 7:42 PM 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                         | 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.
>>>>>
>>>>
>>>> IIUC, do you mean:
>>>>
>>>> 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_PROG_TYPE_TRACING:
>>>>         if (env->prog->expected_attach_type == BPF_TRACE_ITER)
>>>>             return true;
>>>>         return env->prog->aux->sleepable;
>>>
>>>
>>> The above can be a fullthrough instead.
>>>
>>
>> Sorry, what do you mean 'a fullthrough' ?
>> Do you mean we can check env->prog->aux->sleepable first and then 
>> fall back to check prog/attach type ?
>>
>
> I see...
>
> Sorry for the above noise. I noticed verifier.c uses 'fallthrough' to 
> avoid the build warning, so we can:
>
> 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_PROG_TYPE_TRACING:
>         if (env->prog->expected_attach_type == BPF_TRACE_ITER)
>             return true;
>         fallthrough;
>     default:
>         return env->prog->aux->sleepable;
>     }
> }


The above LGTM.