Re: [PATCH bpf-next 2/2] selftests/bpf: create new processes repeatedly in the background.

[Kui-Feng Lee <kuifeng@meta.com>]

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@meta.com>
> 
> Ack with a few nits.
> 
> Acked-by: Yonghong Song <yhs@fb.com>
> 

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"))