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

[Yonghong Song <yonghong.song@linux.dev>]

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