Re: [PATCHv3 bpf-next 5/6] selftests/bpf: Add link_info test for uprobe_multi link

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, Nov 20, 2023 at 10:22:26AM -0800, Yonghong Song wrote:
> 
> On 11/20/23 9:56 AM, Jiri Olsa wrote:
> > Adding fill_link_info test for uprobe_multi link.
> > 
> > Setting up uprobes with bogus ref_ctr_offsets and cookie values
> > to test all the bpf_link_info::uprobe_multi fields.
> > 
> > Acked-by: Song Liu <song@kernel.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >   .../selftests/bpf/prog_tests/fill_link_info.c | 191 ++++++++++++++++++
> >   .../selftests/bpf/progs/test_fill_link_info.c |   6 +
> >   2 files changed, 197 insertions(+)
> > 
> > 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 9294cb8d7743..fdf2c6b8c0cf 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> > @@ -7,6 +7,7 @@
> >   #include <test_progs.h>
> >   #include "trace_helpers.h"
> >   #include "test_fill_link_info.skel.h"
> > +#include "bpf/libbpf_internal.h"
> >   #define TP_CAT "sched"
> >   #define TP_NAME "sched_switch"
> > @@ -300,6 +301,189 @@ static void test_kprobe_multi_fill_link_info(struct test_fill_link_info *skel,
> >   	bpf_link__destroy(link);
> >   }
> > +/* Initialize semaphore variables so they don't end up in bss
> > + * section and we could get retrieve their offsets.
> > + */
> > +static short uprobe_link_info_sema_1 = 1;
> > +static short uprobe_link_info_sema_2 = 1;
> > +static short uprobe_link_info_sema_3 = 1;
> 
> I guess The typical sema value starting value should be 0, right?
> If this is the case, the above is not a good example.
> So the issue is that current libbpf does not support
> retrieving offset from .bss section? Do you know why?

hum, I can't recall why it was the problem, because it seems to work
with .bss now when I try it ... anyway I think your suggestion below
is better

> 
> In selftest udst.c, we have semaphore defined as
> usdt.c:unsigned short test_usdt0_semaphore SEC(".probes");
> usdt.c:unsigned short test_usdt3_semaphore SEC(".probes");
> usdt.c:unsigned short test_usdt12_semaphore SEC(".probes");
> 
> Will the following work?
> static short uprobe_link_info_sema_1 SEC(".probes");

yes, that will work and it's better

> ...
> 
> > +
> > +noinline void uprobe_link_info_func_1(void)
> > +{
> > +	uprobe_link_info_sema_1++;
> > +	asm volatile ("");
> 
> The 'asm volatile' above intends to prevent compiler from
> doing 'implicit' inlining. So as a convention let us
> switch statement order to
> 
> 	asm volatile ("");
> 	uprobe_link_info_sema_1++;
> 
> Similarly for below.

ok

SNIP

> > +static void test_uprobe_multi_fill_link_info(struct test_fill_link_info *skel,
> > +					     bool retprobe, bool invalid)
> > +{
> > +	LIBBPF_OPTS(bpf_uprobe_multi_opts, opts,
> > +		.retprobe = retprobe,
> > +	);
> > +	const char *syms[3] = {
> > +		"uprobe_link_info_func_1",
> > +		"uprobe_link_info_func_2",
> > +		"uprobe_link_info_func_3",
> > +	};
> > +	__u64 cookies[3] = {
> > +		0xdead,
> > +		0xbeef,
> > +		0xcafe,
> > +	};
> > +	const char *sema[3] = {
> > +		"uprobe_link_info_sema_1",
> > +		"uprobe_link_info_sema_2",
> > +		"uprobe_link_info_sema_3",
> > +	};
> > +	__u64 *offsets, *ref_ctr_offsets;
> > +	struct bpf_link *link;
> > +	int link_fd, err;
> > +
> > +	err = elf_resolve_syms_offsets("/proc/self/exe", 3, sema,
> > +				       (unsigned long **) &ref_ctr_offsets, STT_OBJECT);
> > +	if (!ASSERT_OK(err, "elf_resolve_syms_offsets_object"))
> > +		return;
> > +
> > +	err = elf_resolve_syms_offsets("/proc/self/exe", 3, syms,
> > +				       (unsigned long **) &offsets, STT_FUNC);
> > +	if (!ASSERT_OK(err, "elf_resolve_syms_offsets_func"))
> > +		return;
> 
> potential leak of ref_ctr_offsets?

ugh yep, will fix

> 
> > +
> > +	opts.syms = syms;
> > +	opts.cookies = &cookies[0];
> > +	opts.ref_ctr_offsets = (unsigned long *) &ref_ctr_offsets[0];
> > +	opts.cnt = ARRAY_SIZE(syms);
> > +
> > +	link = bpf_program__attach_uprobe_multi(skel->progs.umulti_run, 0,
> > +						"/proc/self/exe", NULL, &opts);
> > +	if (!ASSERT_OK_PTR(link, "bpf_program__attach_uprobe_multi"))
> > +		goto out;
> > +
> > +	link_fd = bpf_link__fd(link);
> > +	if (invalid)
> > +		verify_umulti_invalid_user_buffer(link_fd);
> > +	else
> > +		verify_umulti_link_info(link_fd, retprobe, offsets, cookies, ref_ctr_offsets);
> > +
> > +	bpf_link__destroy(link);
> > +out:
> > +	free(offsets);
> 
> Should we free ref_ctr_offsets here?

yes, thanks

jirka