Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add selftest for fill_link_info

[Jiri Olsa <olsajiri@gmail.com>]

On Thu, Jul 27, 2023 at 11:43:09AM +0000, Yafang Shao wrote:

SNIP

> +static int verify_link_info(int fd, enum bpf_perf_event_type type, long addr, ssize_t offset)
> +{
> +	struct bpf_link_info info;
> +	__u32 len = sizeof(info);
> +	char buf[PATH_MAX];
> +	int err = 0;
> +
> +	memset(&info, 0, sizeof(info));
> +	buf[0] = '\0';
> +
> +again:
> +	err = bpf_link_get_info_by_fd(fd, &info, &len);
> +	if (!ASSERT_OK(err, "get_link_info"))
> +		return -1;
> +
> +	switch (info.type) {
> +	case BPF_LINK_TYPE_PERF_EVENT:
> +		if (!ASSERT_EQ(info.perf_event.type, type, "perf_type_match"))
> +			return -1;
> +
> +		switch (info.perf_event.type) {
> +		case BPF_PERF_EVENT_KPROBE:
> +		case BPF_PERF_EVENT_KRETPROBE:
> +			ASSERT_EQ(info.perf_event.kprobe.offset, offset, "kprobe_offset");
> +
> +			/* In case kptr setting is not permitted or MAX_SYMS is reached */
> +			if (addr) {
> +				long addrs[2] = {
> +					addr + offset,
> +					addr + 0x4, /* For ENDBDR */
> +				};
> +
> +				ASSERT_IN_ARRAY(info.perf_event.kprobe.addr, addrs, "kprobe_addr");

we have check for IBT in get_func_ip_test, it might be easier
to use the same in here as well and do the exact check

we wouldn't need the ASSERT_IN_ARRAY then and would be correct
wrt other archs


SNIP

> +static void test_uprobe_fill_link_info(struct test_fill_link_info *skel,
> +				       enum bpf_perf_event_type type, ssize_t offset,
> +				       bool retprobe)
> +{
> +	int link_fd, err;
> +
> +	skel->links.uprobe_run = bpf_program__attach_uprobe(skel->progs.uprobe_run, retprobe,
> +							    0, /* self pid */
> +							    UPROBE_FILE, offset);
> +	if (!ASSERT_OK_PTR(skel->links.uprobe_run, "attach_uprobe"))
> +		return;
> +
> +	link_fd = bpf_link__fd(skel->links.uprobe_run);
> +	if (!ASSERT_GE(link_fd, 0, "link_fd"))
> +		return;
> +
> +	err = verify_link_info(link_fd, type, 0, offset);
> +	ASSERT_OK(err, "verify_link_info");
> +	bpf_link__detach(skel->links.uprobe_run);
> +}
> +
> +void serial_test_fill_link_info(void)

why does it need to be serial?

> +{
> +	struct test_fill_link_info *skel;
> +	ssize_t offset;
> +
> +	skel = test_fill_link_info__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "skel_open"))
> +		goto cleanup;
> +
> +	/* load kallsyms to compare the addr */
> +	if (!ASSERT_OK(load_kallsyms_refresh(), "load_kallsyms_refresh"))
> +		return;
> +	if (test__start_subtest("kprobe_link_info"))
> +		test_kprobe_fill_link_info(skel, BPF_PERF_EVENT_KPROBE, false, false);
> +	if (test__start_subtest("kretprobe_link_info"))
> +		test_kprobe_fill_link_info(skel, BPF_PERF_EVENT_KRETPROBE, true, false);
> +	if (test__start_subtest("fill_invalid_user_buff"))
> +		test_kprobe_fill_link_info(skel, BPF_PERF_EVENT_KPROBE, false, true);
> +	if (test__start_subtest("tracepoint_link_info"))
> +		test_tp_fill_link_info(skel);
> +
> +	offset = get_uprobe_offset(&uprobe_func);
> +	if (test__start_subtest("uprobe_link_info"))
> +		test_uprobe_fill_link_info(skel, BPF_PERF_EVENT_UPROBE, offset, false);
> +	if (test__start_subtest("uretprobe_link_info"))
> +		test_uprobe_fill_link_info(skel, BPF_PERF_EVENT_URETPROBE, offset, true);

do you plan to add kprobe_multi link test as well?

thanks,
jirka


> +
> +cleanup:
> +	test_fill_link_info__destroy(skel);
> +}

SNIP