Re: [PATCH bpf-next] selftests/bpf: Add missing trampoline program type to trampoline_count test

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Sat, May 14, 2022 at 11:31 PM Yuntao Wang <ytcoode@gmail.com> wrote:
>
> Currently the trampoline_count test doesn't include any fmod_ret bpf
> programs, fix it to make the test cover all possible trampoline program
> types.
>
> Since fmod_ret bpf programs can't be attached to __set_task_comm function,
> as it's neither whitelisted for error injection nor a security hook, change
> it to bpf_modify_return_test.
>
> This patch also does some other cleanups such as removing duplicate code,
> dropping inconsistent comments, etc.
>
> Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
> ---
>  include/linux/bpf.h                           |   2 +-
>  .../bpf/prog_tests/trampoline_count.c         | 121 ++++++------------
>  .../bpf/progs/test_trampoline_count.c         |  16 ++-
>  3 files changed, 47 insertions(+), 92 deletions(-)
>

[...]

>
>         /* with E2BIG error */
> -       ASSERT_EQ(err, -E2BIG, "proper error check");
> +       ASSERT_EQ(libbpf_get_error(link), -E2BIG, "E2BIG");
>         ASSERT_EQ(link, NULL, "ptr_is_null");
>
> -       /* and finaly execute the probe */
> -       if (CHECK_FAIL(prctl(PR_GET_NAME, comm, 0L, 0L, 0L)))
> -               goto cleanup_extra;
> -       CHECK_FAIL(test_task_rename());

we stopped testing that kernel function actually can be called
properly, why don't you do bpf_prog_test_run() here to trigger
bpf_modify_return_test in kernel?

> -       CHECK_FAIL(prctl(PR_SET_NAME, comm, 0L, 0L, 0L));
> -
> -cleanup_extra:
> -       bpf_object__close(obj);
>  cleanup:
> -       if (i >= MAX_TRAMP_PROGS)
> -               i = MAX_TRAMP_PROGS - 1;
>         for (; i >= 0; i--) {
> -               bpf_link__destroy(inst[i].link_fentry);
> -               bpf_link__destroy(inst[i].link_fexit);
> +               bpf_link__destroy(inst[i].link);
>                 bpf_object__close(inst[i].obj);
>         }
>  }

[...]