Hi,
On 11/8/2022 6:44 AM, Yonghong Song wrote:
>
>
> On 11/6/22 11:42 PM, Hou Tao wrote:
>> From: Hou Tao <houtao1@xxxxxxxxxx>
>>
>> The test closes both iterator link fd and cgroup fd, and removes the
>> cgroup file to make a dead cgroup before reading cgroup iterator fd. It
>> also uses kern_sync_rcu() and usleep() to wait for the release of
>> start cgroup. If the start cgroup is not pinned by cgroup iterator,
>> reading iterator fd will trigger use-after-free.
>>
>> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
>
> LGTM with a few nits below.
>
> Acked-by: Yonghong Song <yhs@xxxxxx>
SNIP
>
>> + cgrp_fd = create_and_get_cgroup(cgrp_name);
>> + if (!ASSERT_GE(cgrp_fd, 0, "create cgrp"))
>> + return;
>> +
>> + /* The cgroup is already dead during iteration, so it only has epilogue
>> + * in the output.
>> + */
>
> Let us reword the comment like
> The cgroup will be dead during read() iteration, and it only has
> epilogue in the output.
Will do in v2.
>
>> + snprintf(expected_output, sizeof(expected_output), EPILOGUE);
>> +
>> + memset(&linfo, 0, sizeof(linfo));
>> + linfo.cgroup.cgroup_fd = cgrp_fd;
>> + linfo.cgroup.order = BPF_CGROUP_ITER_SELF_ONLY;
>> + opts.link_info = &linfo;
>> + opts.link_info_len = sizeof(linfo);
>> +
SNIP
>> void test_cgroup_iter(void)
>> {
>> struct cgroup_iter *skel = NULL;
>> @@ -217,6 +293,8 @@ void test_cgroup_iter(void)
>> test_early_termination(skel);
>> if (test__start_subtest("cgroup_iter__self_only"))
>> test_walk_self_only(skel);
>> + if (test__start_subtest("cgroup_iter_dead_self_only"))
>
> Let us follow the convention in this file with
> cgroup_iter__dead_self_only
My bad. Will fixes in v2.
>
>> + test_walk_dead_self_only(skel);
>> out:
>> cgroup_iter__destroy(skel);
>> cleanup_cgroups();
