[PATCH bpf-next v3 0/3] Relax allowlist for open-coded css_task iter

[PATCH bpf-next v3 0/3] 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
Changes in v3:
 * Add a testcase to prove css_task can be used in fentry.s
 * Link to v2:https://lore.kernel.org/bpf/20231024024240.42790-1-zhouchuyi@bytedance.com/T/#m14a97041ff56c2df21bc0149449abd275b73f6a3

---

Chuyi Zhou (3):
  bpf: Relax allowlist for css_task iter
  selftests/bpf: Add tests for css_task iter combining with cgroup iter
  selftests/bpf: Add test for using css_task iter in sleepable progs

 kernel/bpf/verifier.c                         | 16 ++++++--
 .../selftests/bpf/prog_tests/cgroup_iter.c    | 33 +++++++++++++++
 .../selftests/bpf/progs/iters_css_task.c      | 41 +++++++++++++++++++
 .../selftests/bpf/progs/iters_task_failure.c  | 23 ++++++++++-
 4 files changed, 107 insertions(+), 6 deletions(-)

-- 
2.20.1

[PATCH bpf-next v3 1/3] 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                            | 16 ++++++++++++----
 .../selftests/bpf/progs/iters_task_failure.c     |  4 ++--
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index e9bc5d4a25a1..9243b6ca4854 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11088,6 +11088,12 @@ 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);
@@ -11095,10 +11101,12 @@ static bool check_css_task_iter_allowlist(struct bpf_verifier_env *env)
 	switch (prog_type) {
 	case BPF_PROG_TYPE_LSM:
 		return true;
-	case BPF_TRACE_ITER:
-		return env->prog->aux->sleepable;
+	case BPF_PROG_TYPE_TRACING:
+		if (env->prog->expected_attach_type == BPF_TRACE_ITER)
+			return true;
+		fallthrough;
 	default:
-		return false;
+		return env->prog->aux->sleepable;
 	}
 }
 
@@ -11357,7 +11365,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 v3 2/3] selftests/bpf: Add tests 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

[PATCH bpf-next v3 3/3] selftests/bpf: Add test for using css_task iter in sleepable progs

[Chuyi Zhou]

This Patch add a test to prove css_task iter can be used in normal
sleepable progs.

Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com>
---
 .../selftests/bpf/progs/iters_task_failure.c  | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/iters_task_failure.c b/tools/testing/selftests/bpf/progs/iters_task_failure.c
index 6b1588d70652..fe0b19e545d0 100644
--- a/tools/testing/selftests/bpf/progs/iters_task_failure.c
+++ b/tools/testing/selftests/bpf/progs/iters_task_failure.c
@@ -103,3 +103,22 @@ int BPF_PROG(iter_css_task_for_each)
 	bpf_cgroup_release(cgrp);
 	return 0;
 }
+
+SEC("?fentry.s/" SYS_PREFIX "sys_getpgid")
+int BPF_PROG(iter_css_task_for_each_sleep)
+{
+	u64 cg_id = bpf_get_current_cgroup_id();
+	struct cgroup *cgrp = bpf_cgroup_from_id(cg_id);
+	struct cgroup_subsys_state *css;
+	struct task_struct *task;
+
+	if (cgrp == NULL)
+		return 0;
+	css = &cgrp->self;
+
+	bpf_for_each(css_task, task, css, CSS_TASK_ITER_PROCS) {
+
+	}
+	bpf_cgroup_release(cgrp);
+	return 0;
+}
-- 
2.20.1

Re: [PATCH bpf-next v3 1/3] bpf: Relax allowlist for css_task iter

[Yonghong Song]

On 10/25/23 12:59 AM, Chuyi Zhou 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>

Acked-by: Yonghong Song <yonghong.song@linux.dev>

Re: [PATCH bpf-next v3 2/3] selftests/bpf: Add tests for css_task iter combining with cgroup iter

[Yonghong Song]

On 10/25/23 12:59 AM, Chuyi Zhou wrote:
> 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>

Ack with a few nits below:

Acked-by: Yonghong Song <yonghong.song@linux.dev>


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

'= NULL' is unnecessary.

> +	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));

cgroup_id(cgrp) needs only one call.

> +	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;
> +}

Re: [PATCH bpf-next v3 3/3] selftests/bpf: Add test for using css_task iter in sleepable progs

[Yonghong Song]

On 10/25/23 12:59 AM, Chuyi Zhou wrote:
> This Patch add a test to prove css_task iter can be used in normal
> sleepable progs.
>
> Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com>
> ---
>   .../selftests/bpf/progs/iters_task_failure.c  | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/progs/iters_task_failure.c b/tools/testing/selftests/bpf/progs/iters_task_failure.c
> index 6b1588d70652..fe0b19e545d0 100644
> --- a/tools/testing/selftests/bpf/progs/iters_task_failure.c
> +++ b/tools/testing/selftests/bpf/progs/iters_task_failure.c
> @@ -103,3 +103,22 @@ int BPF_PROG(iter_css_task_for_each)
>   	bpf_cgroup_release(cgrp);
>   	return 0;
>   }
> +
> +SEC("?fentry.s/" SYS_PREFIX "sys_getpgid")
> +int BPF_PROG(iter_css_task_for_each_sleep)
> +{
> +	u64 cg_id = bpf_get_current_cgroup_id();
> +	struct cgroup *cgrp = bpf_cgroup_from_id(cg_id);
> +	struct cgroup_subsys_state *css;
> +	struct task_struct *task;
> +
> +	if (cgrp == NULL)
> +		return 0;
> +	css = &cgrp->self;
> +
> +	bpf_for_each(css_task, task, css, CSS_TASK_ITER_PROCS) {
> +
> +	}
> +	bpf_cgroup_release(cgrp);
> +	return 0;
> +}

Could you move this prog toiters_css_task.c and add a subtest in cgroup_iter.c? The file 
iters_task_failure.c intends for negative tests. This prog succeeds with 
loading.

Re: [PATCH bpf-next v3 3/3] selftests/bpf: Add test for using css_task iter in sleepable progs

[Chuyi Zhou]

Hello,

在 2023/10/31 08:20, Yonghong Song 写道:
> 
> On 10/25/23 12:59 AM, Chuyi Zhou wrote:
>> This Patch add a test to prove css_task iter can be used in normal
>> sleepable progs.
>>
>> Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com>
>> ---
>>   .../selftests/bpf/progs/iters_task_failure.c  | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/tools/testing/selftests/bpf/progs/iters_task_failure.c 
>> b/tools/testing/selftests/bpf/progs/iters_task_failure.c
>> index 6b1588d70652..fe0b19e545d0 100644
>> --- a/tools/testing/selftests/bpf/progs/iters_task_failure.c
>> +++ b/tools/testing/selftests/bpf/progs/iters_task_failure.c
>> @@ -103,3 +103,22 @@ int BPF_PROG(iter_css_task_for_each)
>>       bpf_cgroup_release(cgrp);
>>       return 0;
>>   }
>> +
>> +SEC("?fentry.s/" SYS_PREFIX "sys_getpgid")
>> +int BPF_PROG(iter_css_task_for_each_sleep)
>> +{
>> +    u64 cg_id = bpf_get_current_cgroup_id();
>> +    struct cgroup *cgrp = bpf_cgroup_from_id(cg_id);
>> +    struct cgroup_subsys_state *css;
>> +    struct task_struct *task;
>> +
>> +    if (cgrp == NULL)
>> +        return 0;
>> +    css = &cgrp->self;
>> +
>> +    bpf_for_each(css_task, task, css, CSS_TASK_ITER_PROCS) {
>> +
>> +    }
>> +    bpf_cgroup_release(cgrp);
>> +    return 0;
>> +}
> 
> Could you move this prog toiters_css_task.c and add a subtest in 
> cgroup_iter.c? The file iters_task_failure.c intends for negative tests. 
> This prog succeeds with loading.
> 

Thanks for the review. I will change in next version.

But do we need a extra subtest like subtest_css_task_iters() in iters.c 
or just use RUN_TESTS(iters_css_task) to prove it can be loaded?

If we do need a extra subtest, maybe we can reuse 
subtest_css_task_iters() in iters.c? cgroup_iter.c is used to test 
SEC("iter/cgroup") and iters.c is used to test open-coded iters.

We can let this prog outo-loaded, and use 'syscall(SYS_getpgid)' after 
'stack_mprotect()' to trigger the prog.

static void subtest_css_task_iters(void)
{
	...
	err = stack_mprotect();
	syscall(SYS_getpgid);
	if (!ASSERT_EQ(err, -1, "stack_mprotect") ||
	    !ASSERT_EQ(errno, EPERM, "stack_mprotect"))
		goto cleanup;
	iters_css_task__detach(skel);
	ASSERT_EQ(skel->bss->css_task_cnt_in_lsm, 1, "css_task_cnt_in_lsm");
	ASSERT_EQ(skel->bss->css_task_cnt_in_sleep, 1, "css_task_cnt_in_sleep");
	...
}

What do you think?

Thanks.

Re: [PATCH bpf-next v3 3/3] selftests/bpf: Add test for using css_task iter in sleepable progs

[Yonghong Song]

On 10/30/23 7:28 PM, Chuyi Zhou wrote:
> Hello,
>
> 在 2023/10/31 08:20, Yonghong Song 写道:
>>
>> On 10/25/23 12:59 AM, Chuyi Zhou wrote:
>>> This Patch add a test to prove css_task iter can be used in normal
>>> sleepable progs.
>>>
>>> Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com>
>>> ---
>>>   .../selftests/bpf/progs/iters_task_failure.c  | 19 
>>> +++++++++++++++++++
>>>   1 file changed, 19 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/bpf/progs/iters_task_failure.c 
>>> b/tools/testing/selftests/bpf/progs/iters_task_failure.c
>>> index 6b1588d70652..fe0b19e545d0 100644
>>> --- a/tools/testing/selftests/bpf/progs/iters_task_failure.c
>>> +++ b/tools/testing/selftests/bpf/progs/iters_task_failure.c
>>> @@ -103,3 +103,22 @@ int BPF_PROG(iter_css_task_for_each)
>>>       bpf_cgroup_release(cgrp);
>>>       return 0;
>>>   }
>>> +
>>> +SEC("?fentry.s/" SYS_PREFIX "sys_getpgid")
>>> +int BPF_PROG(iter_css_task_for_each_sleep)
>>> +{
>>> +    u64 cg_id = bpf_get_current_cgroup_id();
>>> +    struct cgroup *cgrp = bpf_cgroup_from_id(cg_id);
>>> +    struct cgroup_subsys_state *css;
>>> +    struct task_struct *task;
>>> +
>>> +    if (cgrp == NULL)
>>> +        return 0;
>>> +    css = &cgrp->self;
>>> +
>>> +    bpf_for_each(css_task, task, css, CSS_TASK_ITER_PROCS) {
>>> +
>>> +    }
>>> +    bpf_cgroup_release(cgrp);
>>> +    return 0;
>>> +}
>>
>> Could you move this prog toiters_css_task.c and add a subtest in 
>> cgroup_iter.c? The file iters_task_failure.c intends for negative 
>> tests. This prog succeeds with loading.
>>
>
> Thanks for the review. I will change in next version.
>
> But do we need a extra subtest like subtest_css_task_iters() in 
> iters.c or just use RUN_TESTS(iters_css_task) to prove it can be loaded?

Yes, you can do RUN_TESTS. We only need to confirm verification success.


>
> If we do need a extra subtest, maybe we can reuse 
> subtest_css_task_iters() in iters.c? cgroup_iter.c is used to test 
> SEC("iter/cgroup") and iters.c is used to test open-coded iters.
>
> We can let this prog outo-loaded, and use 'syscall(SYS_getpgid)' after 
> 'stack_mprotect()' to trigger the prog.
>
> static void subtest_css_task_iters(void)
> {
>     ...
>     err = stack_mprotect();
>     syscall(SYS_getpgid);
>     if (!ASSERT_EQ(err, -1, "stack_mprotect") ||
>         !ASSERT_EQ(errno, EPERM, "stack_mprotect"))
>         goto cleanup;
>     iters_css_task__detach(skel);
>     ASSERT_EQ(skel->bss->css_task_cnt_in_lsm, 1, "css_task_cnt_in_lsm");
>     ASSERT_EQ(skel->bss->css_task_cnt_in_sleep, 1, 
> "css_task_cnt_in_sleep");
>     ...
> }
>
> What do you think?
>
> Thanks.
>
>
>