On Fri, 2022-12-16 at 12:05 -0800, Yonghong Song wrote:
>
>
> On 12/15/22 5:59 PM, Kui-Feng Lee wrote:
> > According to a report, the system may crash when a task iterator
>
> There is no context about this 'a report'. You can just remove it
> and say:
> When a task iterator traverses vma(s), it is possible task->mm
> might become invalid in the middle of traversal and this may
> cause kernel misbehave (e.g., crash).
>
> > travels vma(s). The investigation shows it takes place if the
> > visiting task dies during the visit. >
> > This test case creates iterators repeatedly and forks short-lived
> > processes in the background to detect this bug. The test will last
> > for 3 seconds to get the chance to trigger the issue.
>
> The subject is not precise. The test is not about
> "create new processes repeatedly in the background."
> It is about
> "Add a test for iter/task_vma with shortlived processes"
>
> >
> > Signed-off-by: Kui-Feng Lee <kuifeng@xxxxxxxx>
>
> Ack with a few nits.
>
> Acked-by: Yonghong Song <yhs@xxxxxx>
>
Thank you for the review.
> > ---
> > .../selftests/bpf/prog_tests/bpf_iter.c | 79
> > +++++++++++++++++++
> > 1 file changed, 79 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> > b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> > index 6f8ed61fc4b4..df13350d615a 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> > @@ -1465,6 +1465,83 @@ static void test_task_vma_common(struct
> > bpf_iter_attach_opts *opts)
> > bpf_iter_task_vma__destroy(skel);
> > }
> >
> > +static void test_task_vma_dead_task(void)
> > +{
> > + int err, iter_fd = -1;
> > + struct bpf_iter_task_vma *skel;
> > + int wstatus, child_pid = -1;
> > + time_t start_tm, cur_tm;
> > + int wait_sec = 3;
>
> Since it is new code, maybe reverse Christmas tree coding style.
Got it!
>
> > +
> > + skel = bpf_iter_task_vma__open();
> > + if (!ASSERT_OK_PTR(skel, "bpf_iter_task_vma__open"))
> > + return;
> > +
> > + skel->bss->pid = getpid();
> > +
> > + err = bpf_iter_task_vma__load(skel);
> > + if (!ASSERT_OK(err, "bpf_iter_task_vma__load"))
> > + goto out;
> > +
> > + skel->links.proc_maps = bpf_program__attach_iter(
> > + skel->progs.proc_maps, NULL);
> > +
> > + if (!ASSERT_OK_PTR(skel->links.proc_maps,
> > "bpf_program__attach_iter")) {
> > + skel->links.proc_maps = NULL;
> > + goto out;
> > + }
> > +
> > + start_tm = time(NULL);
> > + if (start_tm < 0)
> > + goto out;
>
> From the man page, start_tm should not fail. Note that you didn't
> put
> an ASSERT* either. So I think you can remove it. The same for a few
> instances below.
The only reason that mentioned in the man page to fail is passing an
invalid pointer. But, in our case, passing a NULL pointer, you are
right.
>
> > + cur_tm = start_tm;
> > +
> > + child_pid = fork();
> > + if (child_pid == 0) {
> > + /* Fork short-lived processes in the background. */
> > + while (cur_tm < start_tm + wait_sec) {
> > + system("echo > /dev/null");
> > + cur_tm = time(NULL);
> > + if (cur_tm < 0)
> > + exit(1);
> > + }
> > + exit(0);
> > + }
> > +
> > + if (!ASSERT_GE(child_pid, 0, "fork_child"))
> > + goto out;
> > +
> > + while (cur_tm < start_tm + wait_sec) {
> > + iter_fd = bpf_iter_create(bpf_link__fd(skel-
> > >links.proc_maps));
> > + if (!ASSERT_GE(iter_fd, 0, "create_iter"))
> > + goto out;
> > +
> > + /* Drain all data from iter_fd. */
> > + while (cur_tm < start_tm + wait_sec) {
> > + err = read_fd_into_buffer(iter_fd,
> > task_vma_output, CMP_BUFFER_SIZE);
> > + if (!ASSERT_GE(err, 0, "read_iter_fd"))
> > + goto out;
> > +
> > + cur_tm = time(NULL);
> > + if (cur_tm < 0)
> > + goto out;
> > +
> > + if (err == 0)
> > + break;
> > + }
> > +
> > + close(iter_fd);
> > + iter_fd = -1;
> > + }
> > +
> > + check_bpf_link_info(skel->progs.proc_maps);
> > +
> > +out:
> > + waitpid(child_pid, &wstatus, 0);
> > + close(iter_fd);
> > + bpf_iter_task_vma__destroy(skel);
> > +}
> > +
> > void test_bpf_sockmap_map_iter_fd(void)
> > {
> > struct bpf_iter_sockmap *skel;
> > @@ -1586,6 +1663,8 @@ void test_bpf_iter(void)
> > test_task_file();
> > if (test__start_subtest("task_vma"))
> > test_task_vma();
> > + if (test__start_subtest("task_vma_dead_task"))
> > + test_task_vma_dead_task();
> > if (test__start_subtest("task_btf"))
> > test_task_btf();
> > if (test__start_subtest("tcp4"))
