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

Commit Message

Chuyi Zhou Oct. 25, 2023, 7:59 a.m. UTC

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

Signed-off-by: Chuyi Zhou <[e-mail]>
---
 .../selftests/bpf/progs/iters_task_failure.c  | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Yonghong Song Oct. 31, 2023, 12:20 a.m. UTC | #1

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 <[e-mail]>
> ---
>   .../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.

Chuyi Zhou Oct. 31, 2023, 2:28 a.m. UTC | #2

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 <[e-mail]>
>> ---
>>   .../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.

Yonghong Song Oct. 31, 2023, 2:41 a.m. UTC | #3

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 <[e-mail]>
>>> ---
>>>   .../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.
>
>
>

Patch

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