Re: [PATCH bpf-next 4/6] selftests/bpf: Use bpf_link__destroy in fill_link_info tests

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Nov 01, 2023 at 03:24:36PM -0700, Andrii Nakryiko wrote:
> On Wed, Oct 25, 2023 at 1:25 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > The fill_link_info test keeps skeleton open and just creates
> > various links. We are wrongly calling bpf_link__detach after
> > each test to close them, we need to call bpf_link__destroy.
> >
> > Also we need to set the link NULL so the skeleton destroy
> > won't try to destroy them again.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  .../selftests/bpf/prog_tests/fill_link_info.c       | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> > index 97142a4db374..0379872c445a 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> > @@ -22,6 +22,11 @@ static __u64 kmulti_addrs[KMULTI_CNT];
> >  #define KPROBE_FUNC "bpf_fentry_test1"
> >  static __u64 kprobe_addr;
> >
> > +#define LINK_DESTROY(__link) ({                \
> > +       bpf_link__destroy(__link);      \
> > +       __link = NULL;                  \
> > +})
> > +
> >  #define UPROBE_FILE "/proc/self/exe"
> >  static ssize_t uprobe_offset;
> >  /* uprobe attach point */
> > @@ -157,7 +162,7 @@ static void test_kprobe_fill_link_info(struct test_fill_link_info *skel,
> >         } else {
> >                 kprobe_fill_invalid_user_buffer(link_fd);
> >         }
> > -       bpf_link__detach(skel->links.kprobe_run);
> > +       LINK_DESTROY(skel->links.kprobe_run);
> >  }
> >
> >  static void test_tp_fill_link_info(struct test_fill_link_info *skel)
> > @@ -171,7 +176,7 @@ static void test_tp_fill_link_info(struct test_fill_link_info *skel)
> >         link_fd = bpf_link__fd(skel->links.tp_run);
> >         err = verify_perf_link_info(link_fd, BPF_PERF_EVENT_TRACEPOINT, 0, 0, 0);
> >         ASSERT_OK(err, "verify_perf_link_info");
> > -       bpf_link__detach(skel->links.tp_run);
> > +       LINK_DESTROY(skel->links.tp_run);
> >  }
> >
> >  static void test_uprobe_fill_link_info(struct test_fill_link_info *skel,
> > @@ -189,7 +194,7 @@ static void test_uprobe_fill_link_info(struct test_fill_link_info *skel,
> >         link_fd = bpf_link__fd(skel->links.uprobe_run);
> >         err = verify_perf_link_info(link_fd, type, 0, uprobe_offset, 0);
> >         ASSERT_OK(err, "verify_perf_link_info");
> > -       bpf_link__detach(skel->links.uprobe_run);
> > +       LINK_DESTROY(skel->links.uprobe_run);
> >  }
> >
> >  static int verify_kmulti_link_info(int fd, bool retprobe)
> > @@ -295,7 +300,7 @@ static void test_kprobe_multi_fill_link_info(struct test_fill_link_info *skel,
> >         } else {
> >                 verify_kmulti_invalid_user_buffer(link_fd);
> >         }
> > -       bpf_link__detach(skel->links.kmulti_run);
> > +       LINK_DESTROY(skel->links.kmulti_run);
> 
> if we don't want skeleton to take care of these links, we shouldn't
> assign them into skel->links region, IMO
> 
> so perhaps the proper fix is to have local bpf_link variable in these tests?

ok, that looks cleaner

jirka