[PATCH v2 bpf-next 0/2] bpf: Fix fill_link_info and add selftest

[PATCH v2 bpf-next 0/2] bpf: Fix fill_link_info and add selftest

[Yafang Shao]

Patch #1: Fix an error in fill_link_info reported by Dan
Patch #2: Add selftest for #1

Yafang Shao (2):
  bpf: Fix uninitialized symbol in bpf_perf_link_fill_kprobe()
  selftests/bpf: Add selftest for fill_link_info

v1->v2:
  - Fix BPF CI failure due to the enabled ENDBDR

 kernel/bpf/syscall.c                               |  10 +-
 .../selftests/bpf/prog_tests/fill_link_info.c      | 238 +++++++++++++++++++++
 .../selftests/bpf/progs/test_fill_link_info.c      |  25 +++
 tools/testing/selftests/bpf/test_progs.h           |  19 ++
 4 files changed, 287 insertions(+), 5 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/fill_link_info.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_fill_link_info.c

-- 
1.8.3.1

[PATCH v2 bpf-next 1/2] bpf: Fix uninitialized symbol in bpf_perf_link_fill_kprobe()

[Yafang Shao]

The patch 1b715e1b0ec5: "bpf: Support ->fill_link_info for
perf_event" from Jul 9, 2023, leads to the following Smatch static
checker warning:

    kernel/bpf/syscall.c:3416 bpf_perf_link_fill_kprobe()
    error: uninitialized symbol 'type'.

That can happens when uname is NULL. So fix it by verifying the uname
when we really need to fill it.

Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/bpf/85697a7e-f897-4f74-8b43-82721bebc462@kili.mountain/
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 kernel/bpf/syscall.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 7f4e8c3..ad9360d 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3376,16 +3376,16 @@ static int bpf_perf_link_fill_common(const struct perf_event *event,
 	size_t len;
 	int err;
 
-	if (!ulen ^ !uname)
-		return -EINVAL;
-	if (!uname)
-		return 0;
-
 	err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf,
 				      probe_offset, probe_addr);
 	if (err)
 		return err;
 
+	if (!ulen ^ !uname)
+		return -EINVAL;
+	if (!uname)
+		return 0;
+
 	if (buf) {
 		len = strlen(buf);
 		err = bpf_copy_to_user(uname, buf, ulen, len);
-- 
1.8.3.1

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

[Yafang Shao]

Add selftest for the fill_link_info of uprobe, kprobe and tracepoint.
The result:

  #78/1    fill_link_info/kprobe_link_info:OK
  #78/2    fill_link_info/kretprobe_link_info:OK
  #78/3    fill_link_info/fill_invalid_user_buff:OK
  #78/4    fill_link_info/tracepoint_link_info:OK
  #78/5    fill_link_info/uprobe_link_info:OK
  #78/6    fill_link_info/uretprobe_link_info:OK
  #78      fill_link_info:OK
  Summary: 1/6 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 .../selftests/bpf/prog_tests/fill_link_info.c      | 238 +++++++++++++++++++++
 .../selftests/bpf/progs/test_fill_link_info.c      |  25 +++
 tools/testing/selftests/bpf/test_progs.h           |  19 ++
 3 files changed, 282 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/fill_link_info.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_fill_link_info.c

diff --git a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
new file mode 100644
index 0000000..acb16b5
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
@@ -0,0 +1,238 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2023 Yafang Shao <laoar.shao@gmail.com> */
+
+#include <string.h>
+#include <linux/bpf.h>
+#include <linux/limits.h>
+#include <test_progs.h>
+#include "trace_helpers.h"
+#include "test_fill_link_info.skel.h"
+
+#define TP_CAT "sched"
+#define TP_NAME "sched_switch"
+#define KPROBE_FUNC "tcp_rcv_established"
+#define UPROBE_FILE "/proc/self/exe"
+
+/* uprobe attach point */
+static noinline void uprobe_func(void)
+{
+	asm volatile ("");
+}
+
+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");
+			}
+
+			if (!info.perf_event.kprobe.func_name) {
+				ASSERT_EQ(info.perf_event.kprobe.name_len, 0, "name_len");
+				info.perf_event.kprobe.func_name = ptr_to_u64(&buf);
+				info.perf_event.kprobe.name_len = sizeof(buf);
+				goto again;
+			}
+
+			err = strncmp(u64_to_ptr(info.perf_event.kprobe.func_name), KPROBE_FUNC,
+				      strlen(KPROBE_FUNC));
+			ASSERT_EQ(err, 0, "cmp_kprobe_func_name");
+			break;
+		case BPF_PERF_EVENT_TRACEPOINT:
+			if (!info.perf_event.tracepoint.tp_name) {
+				ASSERT_EQ(info.perf_event.tracepoint.name_len, 0, "name_len");
+				info.perf_event.tracepoint.tp_name = ptr_to_u64(&buf);
+				info.perf_event.tracepoint.name_len = sizeof(buf);
+				goto again;
+			}
+
+			err = strncmp(u64_to_ptr(info.perf_event.tracepoint.tp_name), TP_NAME,
+				      strlen(TP_NAME));
+			ASSERT_EQ(err, 0, "cmp_tp_name");
+			break;
+		case BPF_PERF_EVENT_UPROBE:
+		case BPF_PERF_EVENT_URETPROBE:
+			ASSERT_EQ(info.perf_event.uprobe.offset, offset, "uprobe_offset");
+
+			if (!info.perf_event.uprobe.file_name) {
+				ASSERT_EQ(info.perf_event.uprobe.name_len, 0, "name_len");
+				info.perf_event.uprobe.file_name = ptr_to_u64(&buf);
+				info.perf_event.uprobe.name_len = sizeof(buf);
+				goto again;
+			}
+
+			err = strncmp(u64_to_ptr(info.perf_event.uprobe.file_name), UPROBE_FILE,
+				      strlen(UPROBE_FILE));
+			ASSERT_EQ(err, 0, "cmp_file_name");
+			break;
+		default:
+			break;
+		}
+		break;
+	default:
+		switch (type) {
+		case BPF_PERF_EVENT_KPROBE:
+		case BPF_PERF_EVENT_KRETPROBE:
+		case BPF_PERF_EVENT_TRACEPOINT:
+		case BPF_PERF_EVENT_UPROBE:
+		case BPF_PERF_EVENT_URETPROBE:
+			err = -1;
+			break;
+		default:
+			break;
+		}
+		break;
+	}
+	return err;
+}
+
+static void kprobe_fill_invalid_user_buffer(int fd)
+{
+	struct bpf_link_info info;
+	__u32 len = sizeof(info);
+	int err = 0;
+
+	memset(&info, 0, sizeof(info));
+
+	info.perf_event.kprobe.func_name = 0x1; /* invalid address */
+	err = bpf_link_get_info_by_fd(fd, &info, &len);
+	ASSERT_EQ(err, -EINVAL, "invalid_buff_and_len");
+
+	info.perf_event.kprobe.name_len = 64;
+	err = bpf_link_get_info_by_fd(fd, &info, &len);
+	ASSERT_EQ(err, -EFAULT, "invalid_buff");
+
+	info.perf_event.kprobe.func_name = 0;
+	err = bpf_link_get_info_by_fd(fd, &info, &len);
+	ASSERT_EQ(err, -EINVAL, "invalid_len");
+
+	ASSERT_EQ(info.perf_event.kprobe.addr, 0, "func_addr");
+	ASSERT_EQ(info.perf_event.kprobe.offset, 0, "func_offset");
+	ASSERT_EQ(info.perf_event.type, 0, "type");
+}
+
+static void test_kprobe_fill_link_info(struct test_fill_link_info *skel,
+				       enum bpf_perf_event_type type,
+				       bool retprobe, bool invalid)
+{
+	DECLARE_LIBBPF_OPTS(bpf_kprobe_opts, opts,
+		.attach_mode = PROBE_ATTACH_MODE_LINK,
+		.retprobe = retprobe,
+	);
+	int link_fd, err;
+	long addr;
+
+	skel->links.kprobe_run = bpf_program__attach_kprobe_opts(skel->progs.kprobe_run,
+								 KPROBE_FUNC, &opts);
+	if (!ASSERT_OK_PTR(skel->links.kprobe_run, "attach_kprobe"))
+		return;
+
+	link_fd = bpf_link__fd(skel->links.kprobe_run);
+	if (!ASSERT_GE(link_fd, 0, "link_fd"))
+		return;
+
+	addr = ksym_get_addr(KPROBE_FUNC);
+	if (!invalid) {
+		err = verify_link_info(link_fd, type, addr, 0);
+		ASSERT_OK(err, "verify_link_info");
+	} else {
+		kprobe_fill_invalid_user_buffer(link_fd);
+	}
+	bpf_link__detach(skel->links.kprobe_run);
+}
+
+static void test_tp_fill_link_info(struct test_fill_link_info *skel)
+{
+	int link_fd, err;
+
+	skel->links.tp_run = bpf_program__attach_tracepoint(skel->progs.tp_run, TP_CAT, TP_NAME);
+	if (!ASSERT_OK_PTR(skel->links.tp_run, "attach_tp"))
+		return;
+
+	link_fd = bpf_link__fd(skel->links.tp_run);
+	if (!ASSERT_GE(link_fd, 0, "link_fd"))
+		return;
+
+	err = verify_link_info(link_fd, BPF_PERF_EVENT_TRACEPOINT, 0, 0);
+	ASSERT_OK(err, "verify_link_info");
+	bpf_link__detach(skel->links.tp_run);
+}
+
+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)
+{
+	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);
+
+cleanup:
+	test_fill_link_info__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_fill_link_info.c b/tools/testing/selftests/bpf/progs/test_fill_link_info.c
new file mode 100644
index 0000000..f776134
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_fill_link_info.c
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2023 Yafang Shao <laoar.shao@gmail.com> */
+
+#include "vmlinux.h"
+#include <bpf/bpf_tracing.h>
+
+SEC("kprobe")
+int BPF_PROG(kprobe_run)
+{
+	return 0;
+}
+
+SEC("uprobe")
+int BPF_PROG(uprobe_run)
+{
+	return 0;
+}
+
+SEC("tracepoint")
+int BPF_PROG(tp_run)
+{
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index 77bd492..b51c822 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -300,6 +300,25 @@ struct msg {
 	___ok;								\
 })
 
+#define ASSERT_IN_ARRAY(actual, expected, name) ({			\
+	static int duration;						\
+	typeof(actual) ___act = (actual);				\
+	typeof((expected)[0]) * ___exp = (expected);			\
+	bool ___ok = false;						\
+	int i;								\
+									\
+	for (i = 0; i < ARRAY_SIZE(expected); i++) {			\
+		if (___act == ___exp[i]) {				\
+			___ok = true;					\
+			break;						\
+		}							\
+	}								\
+	CHECK(!___ok, (name),						\
+	      "unexpected %s: actual %lld not in array\n",		\
+	      (name), (long long)(___act));				\
+	___ok;								\
+})
+
 #define ASSERT_STREQ(actual, expected, name) ({				\
 	static int duration = 0;					\
 	const char *___act = actual;					\
-- 
1.8.3.1

Re: [PATCH v2 bpf-next 1/2] bpf: Fix uninitialized symbol in bpf_perf_link_fill_kprobe()

[Jiri Olsa]

On Thu, Jul 27, 2023 at 11:43:08AM +0000, Yafang Shao wrote:
> The patch 1b715e1b0ec5: "bpf: Support ->fill_link_info for
> perf_event" from Jul 9, 2023, leads to the following Smatch static
> checker warning:
> 
>     kernel/bpf/syscall.c:3416 bpf_perf_link_fill_kprobe()
>     error: uninitialized symbol 'type'.
> 
> That can happens when uname is NULL. So fix it by verifying the uname
> when we really need to fill it.
> 
> Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/bpf/85697a7e-f897-4f74-8b43-82721bebc462@kili.mountain/
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  kernel/bpf/syscall.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 7f4e8c3..ad9360d 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3376,16 +3376,16 @@ static int bpf_perf_link_fill_common(const struct perf_event *event,
>  	size_t len;
>  	int err;
>  
> -	if (!ulen ^ !uname)
> -		return -EINVAL;

would it make more sense to keep above check in place and move the
!uname change below? I'd think we want to return error in case of
wrong arguments as soon as possible

> -	if (!uname)
> -		return 0;
> -
>  	err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf,
>  				      probe_offset, probe_addr);
>  	if (err)
>  		return err;
>  
> +	if (!ulen ^ !uname)
> +		return -EINVAL;
> +	if (!uname)
> +		return 0;

and here we just return 0 if we do not store the name to provided buffer

thanks,
jirka

> +
>  	if (buf) {
>  		len = strlen(buf);
>  		err = bpf_copy_to_user(uname, buf, ulen, len);
> -- 
> 1.8.3.1
> 

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

[Jiri Olsa]

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

Re: [PATCH v2 bpf-next 1/2] bpf: Fix uninitialized symbol in bpf_perf_link_fill_kprobe()

[Yafang Shao]

On Thu, Jul 27, 2023 at 9:07 PM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Thu, Jul 27, 2023 at 11:43:08AM +0000, Yafang Shao wrote:
> > The patch 1b715e1b0ec5: "bpf: Support ->fill_link_info for
> > perf_event" from Jul 9, 2023, leads to the following Smatch static
> > checker warning:
> >
> >     kernel/bpf/syscall.c:3416 bpf_perf_link_fill_kprobe()
> >     error: uninitialized symbol 'type'.
> >
> > That can happens when uname is NULL. So fix it by verifying the uname
> > when we really need to fill it.
> >
> > Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event")
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/bpf/85697a7e-f897-4f74-8b43-82721bebc462@kili.mountain/
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >  kernel/bpf/syscall.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 7f4e8c3..ad9360d 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -3376,16 +3376,16 @@ static int bpf_perf_link_fill_common(const struct perf_event *event,
> >       size_t len;
> >       int err;
> >
> > -     if (!ulen ^ !uname)
> > -             return -EINVAL;
>
> would it make more sense to keep above check in place and move the
> !uname change below? I'd think we want to return error in case of
> wrong arguments as soon as possible

Good point. Will change it.

>
> > -     if (!uname)
> > -             return 0;
> > -
> >       err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf,
> >                                     probe_offset, probe_addr);
> >       if (err)
> >               return err;
> >
> > +     if (!ulen ^ !uname)
> > +             return -EINVAL;
> > +     if (!uname)
> > +             return 0;
>
> and here we just return 0 if we do not store the name to provided buffer

Will do it.

-- 
Regards
Yafang

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

[Yafang Shao]

On Thu, Jul 27, 2023 at 9:49 PM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> 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

Thanks for your information!
will change it.

>
> 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?

Ah, it can run in parallel. will change it.

>
> > +{
> > +     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?

will add it in the next version.

-- 
Regards
Yafang