[PATCHv4 bpf-next 0/4] bpf: Add bpf_vma_build_id_parse kfunc

[PATCHv4 bpf-next 0/4] bpf: Add bpf_vma_build_id_parse kfunc

[Jiri Olsa]

hi,
first version of this patchset added helper for this functionality,
but based Alexei's feedback [1], changing it to kfunc.

With the current build_id_parse function as kfunc we can't effectively
check buffer size provided by user. Therefore adding new function as
bpf kfunc:

  int bpf_vma_build_id_parse(struct vm_area_struct *vma,
                             unsigned char *build_id,
                             size_t build_id__sz);

that triggers kfunc's verifier check for build_id/build_id__sz buffer
size and calls build_id_parse.

v4 changes:
  - vma object is now passed as trusted pointer argument [Alexei]
    - marked bpf_vma_build_id_parse with KF_TRUSTED_ARGS flag
      so it can be called only with PTR_TRUSTED args
    - marked vma objects from task_vma iter and find_vma callback
      as PTR_TRUSTED
  - added test for task_vma iterator

v3 changes:
  - restrict bpf_vma_build_id_parse to bpf_find_vma callback
  - move bpf_vma_build_id_parse to kernel/trace/bpf_trace.c
    and add new tracing_kfunc_set

thanks,
jirka


[1] https://lore.kernel.org/bpf/CAADnVQKyT4Mm4EdTCYK8c070E-BwPZS_FOkWKLJC80riSGmLTg@mail.gmail.com/
---
Jiri Olsa (4):
      bpf: Mark vma objects as trusted for task_vma iter and find_vma callback
      bpf: Add bpf_vma_build_id_parse function and kfunc
      selftests/bpf: Add bpf_vma_build_id_parse find_vma callback test
      selftests/bpf: Add bpf_vma_build_id_parse task vma iterator test

 include/linux/bpf.h                                             |  4 ++++
 kernel/bpf/task_iter.c                                          |  2 +-
 kernel/bpf/verifier.c                                           |  2 +-
 kernel/trace/bpf_trace.c                                        | 31 +++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/prog_tests/bpf_iter.c               | 44 ++++++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/prog_tests/bpf_vma_build_id_parse.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/bpf_iter_build_id.c           | 41 +++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/bpf_vma_build_id_parse.c      | 40 ++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/trace_helpers.c                     | 40 ++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/trace_helpers.h                     |  1 +
 10 files changed, 258 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/bpf_vma_build_id_parse.c
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_build_id.c
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_vma_build_id_parse.c

[PATCHv4 bpf-next 1/4] bpf: Mark vma objects as trusted for task_vma iter and find_vma callback

[Jiri Olsa]

Marking following vma objects as trusted so they can be used
as arguments for kfunc function added in following changes:

  - vma object argument in find_vma callback function
  - vma object in context of task_vma iterator program

Both places lock vma object so it can't go away while running
the bpf program.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/task_iter.c | 2 +-
 kernel/bpf/verifier.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index c2a2182ce570..cd67b3cadd91 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -755,7 +755,7 @@ static struct bpf_iter_reg task_vma_reg_info = {
 		{ offsetof(struct bpf_iter__task_vma, task),
 		  PTR_TO_BTF_ID_OR_NULL },
 		{ offsetof(struct bpf_iter__task_vma, vma),
-		  PTR_TO_BTF_ID_OR_NULL },
+		  PTR_TO_BTF_ID_OR_NULL | PTR_TRUSTED },
 	},
 	.seq_info		= &task_vma_seq_info,
 	.fill_link_info		= bpf_iter_fill_link_info,
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 6599d25dae38..2f04cab023be 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7206,7 +7206,7 @@ static int set_find_vma_callback_state(struct bpf_verifier_env *env,
 	 */
 	callee->regs[BPF_REG_1] = caller->regs[BPF_REG_1];
 
-	callee->regs[BPF_REG_2].type = PTR_TO_BTF_ID;
+	callee->regs[BPF_REG_2].type = PTR_TO_BTF_ID | PTR_TRUSTED;
 	__mark_reg_known_zero(&callee->regs[BPF_REG_2]);
 	callee->regs[BPF_REG_2].btf =  btf_vmlinux;
 	callee->regs[BPF_REG_2].btf_id = btf_tracing_ids[BTF_TRACING_TYPE_VMA],
-- 
2.38.1

[PATCHv4 bpf-next 2/4] bpf: Add bpf_vma_build_id_parse function and kfunc

[Jiri Olsa]

Adding bpf_vma_build_id_parse function to retrieve build id from
passed vma object and making it available as bpf kfunc.

We can't use build_id_parse directly as kfunc, because we would
not have control over the build id buffer size provided by user.

Instead we are adding new bpf_vma_build_id_parse function with
'build_id__sz' argument that instructs verifier to check for the
available space in build_id buffer.

This way we check that there's always available memory space
behind build_id pointer. We also check that the build_id__sz is
at least BUILD_ID_SIZE_MAX so we can place any buildid in.

The bpf_vma_build_id_parse kfunc is marked as KF_TRUSTED_ARGS,
so it can be only called with trusted vma objects. These are
currently provided only by find_vma callback function and
task_vma iterator program.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/bpf.h      |  4 ++++
 kernel/trace/bpf_trace.c | 31 +++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index c6aa6912ea16..359c8fe11779 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2839,4 +2839,8 @@ static inline bool type_is_alloc(u32 type)
 	return type & MEM_ALLOC;
 }
 
+int bpf_vma_build_id_parse(struct vm_area_struct *vma,
+			   unsigned char *build_id,
+			   size_t build_id__sz);
+
 #endif /* _LINUX_BPF_H */
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 3bbd3f0c810c..7340de74531a 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -23,6 +23,7 @@
 #include <linux/sort.h>
 #include <linux/key.h>
 #include <linux/verification.h>
+#include <linux/buildid.h>
 
 #include <net/bpf_sk_storage.h>
 
@@ -1383,6 +1384,36 @@ static int __init bpf_key_sig_kfuncs_init(void)
 late_initcall(bpf_key_sig_kfuncs_init);
 #endif /* CONFIG_KEYS */
 
+int bpf_vma_build_id_parse(struct vm_area_struct *vma,
+			   unsigned char *build_id,
+			   size_t build_id__sz)
+{
+	__u32 size;
+	int err;
+
+	if (build_id__sz < BUILD_ID_SIZE_MAX)
+		return -EINVAL;
+
+	err = build_id_parse(vma, build_id, &size);
+	return err ?: (int) size;
+}
+
+BTF_SET8_START(tracing_btf_ids)
+BTF_ID_FLAGS(func, bpf_vma_build_id_parse, KF_TRUSTED_ARGS)
+BTF_SET8_END(tracing_btf_ids)
+
+static const struct btf_kfunc_id_set tracing_kfunc_set = {
+	.owner = THIS_MODULE,
+	.set   = &tracing_btf_ids,
+};
+
+static int __init kfunc_tracing_init(void)
+{
+	return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &tracing_kfunc_set);
+}
+
+late_initcall(kfunc_tracing_init);
+
 static const struct bpf_func_proto *
 bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
-- 
2.38.1

[PATCHv4 bpf-next 3/4] selftests/bpf: Add bpf_vma_build_id_parse find_vma callback test

[Jiri Olsa]

Adding tests for using new bpf_vma_build_id_parse kfunc in find_vma
callback function.

On bpf side the test finds the vma of the test_progs text through the
test function pointer and reads its build id with the new kfunc.

On user side the test uses readelf to get test_progs build id and
compares it with the one from bpf side.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../bpf/prog_tests/bpf_vma_build_id_parse.c   | 55 +++++++++++++++++++
 .../bpf/progs/bpf_vma_build_id_parse.c        | 40 ++++++++++++++
 tools/testing/selftests/bpf/trace_helpers.c   | 40 ++++++++++++++
 tools/testing/selftests/bpf/trace_helpers.h   |  1 +
 4 files changed, 136 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/bpf_vma_build_id_parse.c
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_vma_build_id_parse.c

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_vma_build_id_parse.c b/tools/testing/selftests/bpf/prog_tests/bpf_vma_build_id_parse.c
new file mode 100644
index 000000000000..895a5ba47f47
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_vma_build_id_parse.c
@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include <sys/types.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include "bpf_vma_build_id_parse.skel.h"
+#include "trace_helpers.h"
+
+#define BUILDID_STR_SIZE (BPF_BUILD_ID_SIZE*2 + 1)
+
+void test_bpf_vma_build_id_parse(void)
+{
+	char bpf_build_id[BUILDID_STR_SIZE] = {}, *build_id;
+	LIBBPF_OPTS(bpf_test_run_opts, topts);
+	struct bpf_vma_build_id_parse *skel;
+	int i, err, prog_fd;
+
+	skel = bpf_vma_build_id_parse__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "bpf_vma_build_id_parse__open_and_load"))
+		return;
+
+	skel->bss->target_pid = getpid();
+	skel->bss->addr = (__u64)(uintptr_t)test_bpf_vma_build_id_parse;
+
+	err = bpf_vma_build_id_parse__attach(skel);
+	if (!ASSERT_OK(err, "bpf_vma_build_id_parse__attach"))
+		goto out;
+
+	prog_fd = bpf_program__fd(skel->progs.test1);
+	err = bpf_prog_test_run_opts(prog_fd, &topts);
+	ASSERT_OK(err, "test_run_err");
+	ASSERT_EQ(topts.retval, 0, "test_run_retval");
+
+	ASSERT_EQ(skel->data->ret, 0, "ret");
+
+	ASSERT_GT(skel->data->size_pass, 0, "size_pass");
+	ASSERT_EQ(skel->data->size_fail, -EINVAL, "size_fail");
+
+	/* Read build id via readelf to compare with build_id. */
+	if (!ASSERT_OK(read_self_buildid(&build_id), "read_buildid"))
+		goto out;
+
+	ASSERT_EQ(skel->data->size_pass, strlen(build_id)/2, "build_id_size");
+
+	/* Convert bpf build id to string, so we can compare it later. */
+	for (i = 0; i < skel->data->size_pass; i++) {
+		sprintf(bpf_build_id + i*2, "%02x",
+			(unsigned char) skel->bss->build_id[i]);
+	}
+	ASSERT_STREQ(bpf_build_id, build_id, "build_id_match");
+
+	free(build_id);
+out:
+	bpf_vma_build_id_parse__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/bpf_vma_build_id_parse.c b/tools/testing/selftests/bpf/progs/bpf_vma_build_id_parse.c
new file mode 100644
index 000000000000..8937212207db
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/bpf_vma_build_id_parse.c
@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+#define BPF_BUILD_ID_SIZE 20
+
+extern int bpf_vma_build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
+				  size_t build_id__sz) __ksym;
+
+pid_t target_pid = 0;
+__u64 addr = 0;
+
+int ret = -1;
+int size_pass = -1;
+int size_fail = -1;
+
+unsigned char build_id[BPF_BUILD_ID_SIZE];
+
+static long check_vma(struct task_struct *task, struct vm_area_struct *vma,
+		      void *data)
+{
+	size_fail = bpf_vma_build_id_parse(vma, build_id, sizeof(build_id)/2);
+	size_pass = bpf_vma_build_id_parse(vma, build_id, sizeof(build_id));
+	return 0;
+}
+
+SEC("fentry/bpf_fentry_test1")
+int BPF_PROG(test1, int a)
+{
+	struct task_struct *task = bpf_get_current_task_btf();
+
+	if (task->pid != target_pid)
+		return 0;
+
+	ret = bpf_find_vma(task, addr, check_vma, NULL, 0);
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
index 09a16a77bae4..bdd486de17ee 100644
--- a/tools/testing/selftests/bpf/trace_helpers.c
+++ b/tools/testing/selftests/bpf/trace_helpers.c
@@ -9,6 +9,7 @@
 #include <poll.h>
 #include <unistd.h>
 #include <linux/perf_event.h>
+#include <linux/limits.h>
 #include <sys/mman.h>
 #include "trace_helpers.h"
 
@@ -230,3 +231,42 @@ ssize_t get_rel_offset(uintptr_t addr)
 	fclose(f);
 	return -EINVAL;
 }
+
+int read_self_buildid(char **build_id)
+{
+	char path[PATH_MAX], buf[PATH_MAX + 200];
+	char tmp[] = "/tmp/dataXXXXXX";
+	int err, fd;
+	FILE *f;
+
+	fd = mkstemp(tmp);
+	if (fd == -1)
+		return -1;
+	close(fd);
+
+	err = readlink("/proc/self/exe", path, sizeof(path));
+	if (err == -1)
+		goto out;
+	path[err] = 0;
+
+	snprintf(buf, sizeof(buf),
+		"readelf -n %s 2>/dev/null | grep 'Build ID' | awk '{print $3}' > %s",
+		path, tmp);
+
+	err = system(buf);
+	if (err)
+		goto out;
+
+	f = fopen(tmp, "r");
+	if (f) {
+		if (fscanf(f, "%ms$*\n", build_id) != 1) {
+			*build_id = NULL;
+			err = -1;
+		}
+		fclose(f);
+	}
+
+out:
+	unlink(tmp);
+	return err;
+}
diff --git a/tools/testing/selftests/bpf/trace_helpers.h b/tools/testing/selftests/bpf/trace_helpers.h
index 53efde0e2998..c19bd06231d7 100644
--- a/tools/testing/selftests/bpf/trace_helpers.h
+++ b/tools/testing/selftests/bpf/trace_helpers.h
@@ -23,4 +23,5 @@ void read_trace_pipe(void);
 ssize_t get_uprobe_offset(const void *addr);
 ssize_t get_rel_offset(uintptr_t addr);
 
+int read_self_buildid(char **build_id);
 #endif
-- 
2.38.1

[PATCHv4 bpf-next 4/4] selftests/bpf: Add bpf_vma_build_id_parse task vma iterator test

[Jiri Olsa]

Adding tests for using new bpf_vma_build_id_parse kfunc in task_vma
iterator program.

On bpf program side the iterator filters test proccess and proper
vma by provided function pointer and reads its build id with the
new kfunc.

On user side the test uses readelf to get test_progs build id and
compares it with the one read from iterator.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../selftests/bpf/prog_tests/bpf_iter.c       | 44 +++++++++++++++++++
 .../selftests/bpf/progs/bpf_iter_build_id.c   | 41 +++++++++++++++++
 2 files changed, 85 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_build_id.c

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
index 6f8ed61fc4b4..b2cad9f70b32 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
@@ -33,6 +33,9 @@
 #include "bpf_iter_bpf_link.skel.h"
 #include "bpf_iter_ksym.skel.h"
 #include "bpf_iter_sockmap.skel.h"
+#include "bpf_iter_build_id.skel.h"
+
+#define BUILDID_STR_SIZE (BPF_BUILD_ID_SIZE*2 + 1)
 
 static int duration;
 
@@ -1560,6 +1563,45 @@ static void test_task_vma_offset(void)
 	test_task_vma_offset_common(NULL, false);
 }
 
+static void test_task_vma_build_id(void)
+{
+	struct bpf_iter_build_id *skel;
+	char buf[BUILDID_STR_SIZE] = {};
+	int iter_fd, len;
+	char *build_id;
+
+	skel = bpf_iter_build_id__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "bpf_iter_vma_offset__open_and_load"))
+		return;
+
+	skel->bss->pid = getpid();
+	skel->bss->address = (uintptr_t)trigger_func;
+
+	skel->links.vma_build_id = bpf_program__attach_iter(skel->progs.vma_build_id, NULL);
+	if (!ASSERT_OK_PTR(skel->links.vma_build_id, "attach_iter"))
+		goto exit;
+
+	iter_fd = bpf_iter_create(bpf_link__fd(skel->links.vma_build_id));
+	if (!ASSERT_GT(iter_fd, 0, "create_iter"))
+		goto exit;
+
+	while ((len = read(iter_fd, buf, sizeof(buf))) > 0)
+		;
+	buf[BUILDID_STR_SIZE] = 0;
+
+	/* Read build_id via readelf to compare with iterator buf. */
+	if (!ASSERT_OK(read_self_buildid(&build_id), "read_buildid"))
+		goto exit;
+
+	ASSERT_STREQ(buf, build_id, "build_id_match");
+	ASSERT_GT(skel->data->size, 0, "size");
+
+	free(build_id);
+	close(iter_fd);
+exit:
+	bpf_iter_build_id__destroy(skel);
+}
+
 void test_bpf_iter(void)
 {
 	ASSERT_OK(pthread_mutex_init(&do_nothing_mutex, NULL), "pthread_mutex_init");
@@ -1640,4 +1682,6 @@ void test_bpf_iter(void)
 		test_bpf_sockmap_map_iter_fd();
 	if (test__start_subtest("vma_offset"))
 		test_task_vma_offset();
+	if (test__start_subtest("vma_build_id"))
+		test_task_vma_build_id();
 }
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_build_id.c b/tools/testing/selftests/bpf/progs/bpf_iter_build_id.c
new file mode 100644
index 000000000000..86694ce6a194
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_build_id.c
@@ -0,0 +1,41 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "bpf_iter.h"
+#include <bpf/bpf_helpers.h>
+
+#define BPF_BUILD_ID_SIZE 20
+
+extern int bpf_vma_build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
+				  size_t build_id__sz) __ksym;
+
+char _license[] SEC("license") = "GPL";
+
+uintptr_t address = 0;
+__u32 pid = 0;
+int size = -1;
+
+static unsigned char build_id[BPF_BUILD_ID_SIZE];
+
+SEC("iter/task_vma")
+int vma_build_id(struct bpf_iter__task_vma *ctx)
+{
+	struct vm_area_struct *vma = ctx->vma;
+	struct seq_file *seq = ctx->meta->seq;
+	struct task_struct *task = ctx->task;
+	int i;
+
+	if (task == NULL || vma == NULL)
+		return 0;
+
+	if (task->tgid != pid)
+		return 0;
+
+	if (address < vma->vm_start || vma->vm_end < address)
+		return 0;
+
+	size = bpf_vma_build_id_parse(vma, build_id, sizeof(build_id));
+
+	for (i = 0; i < BPF_BUILD_ID_SIZE; i++)
+		BPF_SEQ_PRINTF(seq, "%02x", build_id[i]);
+	return 0;
+}
-- 
2.38.1

Re: [PATCHv4 bpf-next 1/4] bpf: Mark vma objects as trusted for task_vma iter and find_vma callback

[Alexei Starovoitov]

On Mon, Nov 28, 2022 at 5:29 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Marking following vma objects as trusted so they can be used
> as arguments for kfunc function added in following changes:
>
>   - vma object argument in find_vma callback function
>   - vma object in context of task_vma iterator program
>
> Both places lock vma object so it can't go away while running
> the bpf program.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/bpf/task_iter.c | 2 +-
>  kernel/bpf/verifier.c  | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
> index c2a2182ce570..cd67b3cadd91 100644
> --- a/kernel/bpf/task_iter.c
> +++ b/kernel/bpf/task_iter.c
> @@ -755,7 +755,7 @@ static struct bpf_iter_reg task_vma_reg_info = {
>                 { offsetof(struct bpf_iter__task_vma, task),
>                   PTR_TO_BTF_ID_OR_NULL },
>                 { offsetof(struct bpf_iter__task_vma, vma),
> -                 PTR_TO_BTF_ID_OR_NULL },
> +                 PTR_TO_BTF_ID_OR_NULL | PTR_TRUSTED },

Yonghong, Song,

Do you remember when task or vma is NULL here?
Maybe we can do: if (!task || !vma) skip prog run
in __task_vma_seq_show()
and make both pointers as PTR_TO_BTF_ID | PTR_TRUSTED?

Re: [PATCHv4 bpf-next 1/4] bpf: Mark vma objects as trusted for task_vma iter and find_vma callback

[Yonghong Song]

On 11/28/22 10:43 AM, Alexei Starovoitov wrote:
> On Mon, Nov 28, 2022 at 5:29 AM Jiri Olsa <jolsa@kernel.org> wrote:
>>
>> Marking following vma objects as trusted so they can be used
>> as arguments for kfunc function added in following changes:
>>
>>    - vma object argument in find_vma callback function
>>    - vma object in context of task_vma iterator program
>>
>> Both places lock vma object so it can't go away while running
>> the bpf program.
>>
>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>> ---
>>   kernel/bpf/task_iter.c | 2 +-
>>   kernel/bpf/verifier.c  | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
>> index c2a2182ce570..cd67b3cadd91 100644
>> --- a/kernel/bpf/task_iter.c
>> +++ b/kernel/bpf/task_iter.c
>> @@ -755,7 +755,7 @@ static struct bpf_iter_reg task_vma_reg_info = {
>>                  { offsetof(struct bpf_iter__task_vma, task),
>>                    PTR_TO_BTF_ID_OR_NULL },
>>                  { offsetof(struct bpf_iter__task_vma, vma),
>> -                 PTR_TO_BTF_ID_OR_NULL },
>> +                 PTR_TO_BTF_ID_OR_NULL | PTR_TRUSTED },
> 
> Yonghong, Song,
> 
> Do you remember when task or vma is NULL here?
> Maybe we can do: if (!task || !vma) skip prog run
> in __task_vma_seq_show()
> and make both pointers as PTR_TO_BTF_ID | PTR_TRUSTED?

The 'NULL' is to indicate the last bpf prog run before iteration
ends. It is to provide an opportunity for bpf program to know
all regular iterations are done and the bpf program can do
end aggregation or print a footer if the prog link is cat'able.

Re: [PATCHv4 bpf-next 4/4] selftests/bpf: Add bpf_vma_build_id_parse task vma iterator test

[Hao Luo]

On Mon, Nov 28, 2022 at 5:30 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding tests for using new bpf_vma_build_id_parse kfunc in task_vma
> iterator program.
>
> On bpf program side the iterator filters test proccess and proper
> vma by provided function pointer and reads its build id with the
> new kfunc.
>
> On user side the test uses readelf to get test_progs build id and
> compares it with the one read from iterator.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  .../selftests/bpf/prog_tests/bpf_iter.c       | 44 +++++++++++++++++++
>  .../selftests/bpf/progs/bpf_iter_build_id.c   | 41 +++++++++++++++++
>  2 files changed, 85 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_build_id.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> index 6f8ed61fc4b4..b2cad9f70b32 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> @@ -33,6 +33,9 @@
<...>
>
> +static void test_task_vma_build_id(void)
> +{
> +       struct bpf_iter_build_id *skel;
> +       char buf[BUILDID_STR_SIZE] = {};

Jiri, do you mean buf[BUILDID_STR_SIZE + 1]? I see you have
buf[BUILDID_STR_SIZE] = 0; below.

> +       int iter_fd, len;
> +       char *build_id;
<...>
> +
> +       while ((len = read(iter_fd, buf, sizeof(buf))) > 0)
> +               ;

I think you need to pass 'buf + len' to read(), otherwise the last
iteration will overwrite the content read from the previous
iterations.

> +       buf[BUILDID_STR_SIZE] = 0;
> +
> +       /* Read build_id via readelf to compare with iterator buf. */
> +       if (!ASSERT_OK(read_self_buildid(&build_id), "read_buildid"))
> +               goto exit;

We need to close iter_fd before going to exit.

> +
> +       ASSERT_STREQ(buf, build_id, "build_id_match");
> +       ASSERT_GT(skel->data->size, 0, "size");
> +
> +       free(build_id);
> +       close(iter_fd);
> +exit:
> +       bpf_iter_build_id__destroy(skel);
> +}
> +
<...>
> --
> 2.38.1
>

Re: [PATCHv4 bpf-next 3/4] selftests/bpf: Add bpf_vma_build_id_parse find_vma callback test

[Hao Luo]

On Mon, Nov 28, 2022 at 5:30 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding tests for using new bpf_vma_build_id_parse kfunc in find_vma
> callback function.
>
> On bpf side the test finds the vma of the test_progs text through the
> test function pointer and reads its build id with the new kfunc.
>
> On user side the test uses readelf to get test_progs build id and
> compares it with the one from bpf side.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
<...>
> diff --git a/tools/testing/selftests/bpf/progs/bpf_vma_build_id_parse.c b/tools/testing/selftests/bpf/progs/bpf_vma_build_id_parse.c
> new file mode 100644
> index 000000000000..8937212207db
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/bpf_vma_build_id_parse.c
<...>
> +
> +SEC("fentry/bpf_fentry_test1")
> +int BPF_PROG(test1, int a)
> +{
> +       struct task_struct *task = bpf_get_current_task_btf();
> +
> +       if (task->pid != target_pid)
> +               return 0;

I think here we should use task->tgid. IIRC, task->pid corresponds to
thread id in the userspace. task->tgid is the process id.

> +
> +       ret = bpf_find_vma(task, addr, check_vma, NULL, 0);
> +       return 0;
> +}
<...>
> --
> 2.38.1
>

Re: [PATCHv4 bpf-next 1/4] bpf: Mark vma objects as trusted for task_vma iter and find_vma callback

[Alexei Starovoitov]

On Mon, Nov 28, 2022 at 11:04 AM Yonghong Song <yhs@meta.com> wrote:
>
>
>
> On 11/28/22 10:43 AM, Alexei Starovoitov wrote:
> > On Mon, Nov 28, 2022 at 5:29 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >>
> >> Marking following vma objects as trusted so they can be used
> >> as arguments for kfunc function added in following changes:
> >>
> >>    - vma object argument in find_vma callback function
> >>    - vma object in context of task_vma iterator program
> >>
> >> Both places lock vma object so it can't go away while running
> >> the bpf program.
> >>
> >> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> >> ---
> >>   kernel/bpf/task_iter.c | 2 +-
> >>   kernel/bpf/verifier.c  | 2 +-
> >>   2 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
> >> index c2a2182ce570..cd67b3cadd91 100644
> >> --- a/kernel/bpf/task_iter.c
> >> +++ b/kernel/bpf/task_iter.c
> >> @@ -755,7 +755,7 @@ static struct bpf_iter_reg task_vma_reg_info = {
> >>                  { offsetof(struct bpf_iter__task_vma, task),
> >>                    PTR_TO_BTF_ID_OR_NULL },
> >>                  { offsetof(struct bpf_iter__task_vma, vma),
> >> -                 PTR_TO_BTF_ID_OR_NULL },
> >> +                 PTR_TO_BTF_ID_OR_NULL | PTR_TRUSTED },
> >
> > Yonghong, Song,
> >
> > Do you remember when task or vma is NULL here?
> > Maybe we can do: if (!task || !vma) skip prog run
> > in __task_vma_seq_show()
> > and make both pointers as PTR_TO_BTF_ID | PTR_TRUSTED?
>
> The 'NULL' is to indicate the last bpf prog run before iteration
> ends. It is to provide an opportunity for bpf program to know
> all regular iterations are done and the bpf program can do
> end aggregation or print a footer if the prog link is cat'able.

Ahh. Right. Now I remember :)
I think we're fine with PTR_TRUSTED here.
The pointer still has to be checked for != NULL before
being dereferenced or passed into kfunc.

Re: [PATCHv4 bpf-next 2/4] bpf: Add bpf_vma_build_id_parse function and kfunc

[Alexei Starovoitov]

On Mon, Nov 28, 2022 at 5:29 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding bpf_vma_build_id_parse function to retrieve build id from
> passed vma object and making it available as bpf kfunc.
>
> We can't use build_id_parse directly as kfunc, because we would
> not have control over the build id buffer size provided by user.
>
> Instead we are adding new bpf_vma_build_id_parse function with
> 'build_id__sz' argument that instructs verifier to check for the
> available space in build_id buffer.
>
> This way we check that there's always available memory space
> behind build_id pointer. We also check that the build_id__sz is
> at least BUILD_ID_SIZE_MAX so we can place any buildid in.
>
> The bpf_vma_build_id_parse kfunc is marked as KF_TRUSTED_ARGS,
> so it can be only called with trusted vma objects. These are
> currently provided only by find_vma callback function and
> task_vma iterator program.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/linux/bpf.h      |  4 ++++
>  kernel/trace/bpf_trace.c | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index c6aa6912ea16..359c8fe11779 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2839,4 +2839,8 @@ static inline bool type_is_alloc(u32 type)
>         return type & MEM_ALLOC;
>  }
>
> +int bpf_vma_build_id_parse(struct vm_area_struct *vma,
> +                          unsigned char *build_id,
> +                          size_t build_id__sz);
> +
>  #endif /* _LINUX_BPF_H */
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 3bbd3f0c810c..7340de74531a 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -23,6 +23,7 @@
>  #include <linux/sort.h>
>  #include <linux/key.h>
>  #include <linux/verification.h>
> +#include <linux/buildid.h>
>
>  #include <net/bpf_sk_storage.h>
>
> @@ -1383,6 +1384,36 @@ static int __init bpf_key_sig_kfuncs_init(void)
>  late_initcall(bpf_key_sig_kfuncs_init);
>  #endif /* CONFIG_KEYS */
>
> +int bpf_vma_build_id_parse(struct vm_area_struct *vma,
> +                          unsigned char *build_id,
> +                          size_t build_id__sz)
> +{
> +       __u32 size;
> +       int err;
> +
> +       if (build_id__sz < BUILD_ID_SIZE_MAX)
> +               return -EINVAL;
> +
> +       err = build_id_parse(vma, build_id, &size);
> +       return err ?: (int) size;

if err is positive the caller won't be able
to distinguish it vs size.

> +}
> +
> +BTF_SET8_START(tracing_btf_ids)
> +BTF_ID_FLAGS(func, bpf_vma_build_id_parse, KF_TRUSTED_ARGS)
> +BTF_SET8_END(tracing_btf_ids)
> +
> +static const struct btf_kfunc_id_set tracing_kfunc_set = {
> +       .owner = THIS_MODULE,
> +       .set   = &tracing_btf_ids,
> +};
> +
> +static int __init kfunc_tracing_init(void)
> +{
> +       return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &tracing_kfunc_set);
> +}
> +
> +late_initcall(kfunc_tracing_init);

Its own btf_id set and its own late_initcall just for one kfunc?
Please reduce this boilerplate code.
Move it to kernel/bpf/helpers.c ?

Re: [PATCHv4 bpf-next 2/4] bpf: Add bpf_vma_build_id_parse function and kfunc

[Andrii Nakryiko]

On Mon, Nov 28, 2022 at 5:29 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding bpf_vma_build_id_parse function to retrieve build id from
> passed vma object and making it available as bpf kfunc.

As a completely different way of solving this problem of retrieving
build_id for tracing needs, can we teach kernel itself to parse and
store build_id (probably gated behind Kconfig option) in struct file
(ideally)? On exec() and when mmap()'ing with executable permissions,
Linux kernel will try to fetch build_id from ELF file and if
successful store it in struct file. Given build_id can be up to 20
bytes (currently) and not each struct file points to executable, we
might want to only add a pointer field to `struct file` itself, which,
if build_id is present, will point to

struct build_id {
    char sz;
    char data[];
};

This way we don't increase `struct file` by much.

And then any BPF program would be able to easily probe_read_kernel
such build_id from vma_area_struct->vm_file->build_id?

I'm sure I'm oversimplifying, but this seems like a good solution for
all kinds of profiling BPF programs without the need to maintain all
these allowlists and adding new helpers/kfuncs?

I know Hao was looking at the problem of getting build_id, I'm curious
if something like this would work for their use cases as well?


>
> We can't use build_id_parse directly as kfunc, because we would
> not have control over the build id buffer size provided by user.
>
> Instead we are adding new bpf_vma_build_id_parse function with
> 'build_id__sz' argument that instructs verifier to check for the
> available space in build_id buffer.
>
> This way we check that there's always available memory space
> behind build_id pointer. We also check that the build_id__sz is
> at least BUILD_ID_SIZE_MAX so we can place any buildid in.
>
> The bpf_vma_build_id_parse kfunc is marked as KF_TRUSTED_ARGS,
> so it can be only called with trusted vma objects. These are
> currently provided only by find_vma callback function and
> task_vma iterator program.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/linux/bpf.h      |  4 ++++
>  kernel/trace/bpf_trace.c | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
>

[...]

Re: [PATCHv4 bpf-next 2/4] bpf: Add bpf_vma_build_id_parse function and kfunc

[Hao Luo]

On Mon, Nov 28, 2022 at 5:15 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Nov 28, 2022 at 5:29 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding bpf_vma_build_id_parse function to retrieve build id from
> > passed vma object and making it available as bpf kfunc.
>
> As a completely different way of solving this problem of retrieving
> build_id for tracing needs, can we teach kernel itself to parse and
> store build_id (probably gated behind Kconfig option) in struct file
> (ideally)? On exec() and when mmap()'ing with executable permissions,
> Linux kernel will try to fetch build_id from ELF file and if
> successful store it in struct file. Given build_id can be up to 20
> bytes (currently) and not each struct file points to executable, we
> might want to only add a pointer field to `struct file` itself, which,
> if build_id is present, will point to
>
> struct build_id {
>     char sz;
>     char data[];
> };
>
> This way we don't increase `struct file` by much.
>
> And then any BPF program would be able to easily probe_read_kernel
> such build_id from vma_area_struct->vm_file->build_id?
>
> I'm sure I'm oversimplifying, but this seems like a good solution for
> all kinds of profiling BPF programs without the need to maintain all
> these allowlists and adding new helpers/kfuncs?
>
> I know Hao was looking at the problem of getting build_id, I'm curious
> if something like this would work for their use cases as well?
>

This helps a little. We would like to get build_id reliably. There are
two problems we encountered.

First, sometimes we need to get build_id from an atomic context. We
fail to get build_id if the page that contains the build_id has been
evicted from pagecache. Storing the build_id in `struct file` or
`struct inode` is a good and natural solution. But, this problem can
also be solved by using mlock to pin the page in memory. We are using
mlock, it seems to be working well right now.

The other problem we encountered may be very specific to our own use
case. Sometimes we execute code that is mapped in an anonymous page
(not backed by file). In that case, we can't get build_id either. What
we are doing right now is writing the build_id into the
vm_area_struct->anon_name field and teach build_id_parse to try
parsing from there, when seeing an anonymous page. I can send this
with upstream if there are other users who have the same problem.

>
> >
> > We can't use build_id_parse directly as kfunc, because we would
> > not have control over the build id buffer size provided by user.
> >
> > Instead we are adding new bpf_vma_build_id_parse function with
> > 'build_id__sz' argument that instructs verifier to check for the
> > available space in build_id buffer.
> >
> > This way we check that there's always available memory space
> > behind build_id pointer. We also check that the build_id__sz is
> > at least BUILD_ID_SIZE_MAX so we can place any buildid in.
> >
> > The bpf_vma_build_id_parse kfunc is marked as KF_TRUSTED_ARGS,
> > so it can be only called with trusted vma objects. These are
> > currently provided only by find_vma callback function and
> > task_vma iterator program.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/linux/bpf.h      |  4 ++++
> >  kernel/trace/bpf_trace.c | 31 +++++++++++++++++++++++++++++++
> >  2 files changed, 35 insertions(+)
> >
>
> [...]

Re: [PATCHv4 bpf-next 4/4] selftests/bpf: Add bpf_vma_build_id_parse task vma iterator test

[Jiri Olsa]

On Mon, Nov 28, 2022 at 11:21:30AM -0800, Hao Luo wrote:
> On Mon, Nov 28, 2022 at 5:30 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding tests for using new bpf_vma_build_id_parse kfunc in task_vma
> > iterator program.
> >
> > On bpf program side the iterator filters test proccess and proper
> > vma by provided function pointer and reads its build id with the
> > new kfunc.
> >
> > On user side the test uses readelf to get test_progs build id and
> > compares it with the one read from iterator.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  .../selftests/bpf/prog_tests/bpf_iter.c       | 44 +++++++++++++++++++
> >  .../selftests/bpf/progs/bpf_iter_build_id.c   | 41 +++++++++++++++++
> >  2 files changed, 85 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_build_id.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> > index 6f8ed61fc4b4..b2cad9f70b32 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> > @@ -33,6 +33,9 @@
> <...>
> >
> > +static void test_task_vma_build_id(void)
> > +{
> > +       struct bpf_iter_build_id *skel;
> > +       char buf[BUILDID_STR_SIZE] = {};
> 
> Jiri, do you mean buf[BUILDID_STR_SIZE + 1]? I see you have
> buf[BUILDID_STR_SIZE] = 0; below.

ugh, that plus one ended up in the BUILDID_STR_SIZE define, will fix

> 
> > +       int iter_fd, len;
> > +       char *build_id;
> <...>
> > +
> > +       while ((len = read(iter_fd, buf, sizeof(buf))) > 0)
> > +               ;
> 
> I think you need to pass 'buf + len' to read(), otherwise the last
> iteration will overwrite the content read from the previous
> iterations.

ok

> 
> > +       buf[BUILDID_STR_SIZE] = 0;
> > +
> > +       /* Read build_id via readelf to compare with iterator buf. */
> > +       if (!ASSERT_OK(read_self_buildid(&build_id), "read_buildid"))
> > +               goto exit;
> 
> We need to close iter_fd before going to exit.

right, will fix

thanks,
jirka

> 
> > +
> > +       ASSERT_STREQ(buf, build_id, "build_id_match");
> > +       ASSERT_GT(skel->data->size, 0, "size");
> > +
> > +       free(build_id);
> > +       close(iter_fd);
> > +exit:
> > +       bpf_iter_build_id__destroy(skel);
> > +}
> > +
> <...>
> > --
> > 2.38.1
> >

Re: [PATCHv4 bpf-next 3/4] selftests/bpf: Add bpf_vma_build_id_parse find_vma callback test

[Jiri Olsa]

On Mon, Nov 28, 2022 at 11:25:02AM -0800, Hao Luo wrote:
> On Mon, Nov 28, 2022 at 5:30 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding tests for using new bpf_vma_build_id_parse kfunc in find_vma
> > callback function.
> >
> > On bpf side the test finds the vma of the test_progs text through the
> > test function pointer and reads its build id with the new kfunc.
> >
> > On user side the test uses readelf to get test_progs build id and
> > compares it with the one from bpf side.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> <...>
> > diff --git a/tools/testing/selftests/bpf/progs/bpf_vma_build_id_parse.c b/tools/testing/selftests/bpf/progs/bpf_vma_build_id_parse.c
> > new file mode 100644
> > index 000000000000..8937212207db
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/bpf_vma_build_id_parse.c
> <...>
> > +
> > +SEC("fentry/bpf_fentry_test1")
> > +int BPF_PROG(test1, int a)
> > +{
> > +       struct task_struct *task = bpf_get_current_task_btf();
> > +
> > +       if (task->pid != target_pid)
> > +               return 0;
> 
> I think here we should use task->tgid. IIRC, task->pid corresponds to
> thread id in the userspace. task->tgid is the process id.

right, will change

thanks,
jirka

> 
> > +
> > +       ret = bpf_find_vma(task, addr, check_vma, NULL, 0);
> > +       return 0;
> > +}
> <...>
> > --
> > 2.38.1
> >

Re: [PATCHv4 bpf-next 2/4] bpf: Add bpf_vma_build_id_parse function and kfunc

[Jiri Olsa]

On Mon, Nov 28, 2022 at 01:36:03PM -0800, Alexei Starovoitov wrote:
> On Mon, Nov 28, 2022 at 5:29 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding bpf_vma_build_id_parse function to retrieve build id from
> > passed vma object and making it available as bpf kfunc.
> >
> > We can't use build_id_parse directly as kfunc, because we would
> > not have control over the build id buffer size provided by user.
> >
> > Instead we are adding new bpf_vma_build_id_parse function with
> > 'build_id__sz' argument that instructs verifier to check for the
> > available space in build_id buffer.
> >
> > This way we check that there's always available memory space
> > behind build_id pointer. We also check that the build_id__sz is
> > at least BUILD_ID_SIZE_MAX so we can place any buildid in.
> >
> > The bpf_vma_build_id_parse kfunc is marked as KF_TRUSTED_ARGS,
> > so it can be only called with trusted vma objects. These are
> > currently provided only by find_vma callback function and
> > task_vma iterator program.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/linux/bpf.h      |  4 ++++
> >  kernel/trace/bpf_trace.c | 31 +++++++++++++++++++++++++++++++
> >  2 files changed, 35 insertions(+)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index c6aa6912ea16..359c8fe11779 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -2839,4 +2839,8 @@ static inline bool type_is_alloc(u32 type)
> >         return type & MEM_ALLOC;
> >  }
> >
> > +int bpf_vma_build_id_parse(struct vm_area_struct *vma,
> > +                          unsigned char *build_id,
> > +                          size_t build_id__sz);
> > +
> >  #endif /* _LINUX_BPF_H */
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 3bbd3f0c810c..7340de74531a 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -23,6 +23,7 @@
> >  #include <linux/sort.h>
> >  #include <linux/key.h>
> >  #include <linux/verification.h>
> > +#include <linux/buildid.h>
> >
> >  #include <net/bpf_sk_storage.h>
> >
> > @@ -1383,6 +1384,36 @@ static int __init bpf_key_sig_kfuncs_init(void)
> >  late_initcall(bpf_key_sig_kfuncs_init);
> >  #endif /* CONFIG_KEYS */
> >
> > +int bpf_vma_build_id_parse(struct vm_area_struct *vma,
> > +                          unsigned char *build_id,
> > +                          size_t build_id__sz)
> > +{
> > +       __u32 size;
> > +       int err;
> > +
> > +       if (build_id__sz < BUILD_ID_SIZE_MAX)
> > +               return -EINVAL;
> > +
> > +       err = build_id_parse(vma, build_id, &size);
> > +       return err ?: (int) size;
> 
> if err is positive the caller won't be able
> to distinguish it vs size.

that should not happen, build_id_parse returns either < 0 for error, or 0 for success

> 
> > +}
> > +
> > +BTF_SET8_START(tracing_btf_ids)
> > +BTF_ID_FLAGS(func, bpf_vma_build_id_parse, KF_TRUSTED_ARGS)
> > +BTF_SET8_END(tracing_btf_ids)
> > +
> > +static const struct btf_kfunc_id_set tracing_kfunc_set = {
> > +       .owner = THIS_MODULE,
> > +       .set   = &tracing_btf_ids,
> > +};
> > +
> > +static int __init kfunc_tracing_init(void)
> > +{
> > +       return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &tracing_kfunc_set);
> > +}
> > +
> > +late_initcall(kfunc_tracing_init);
> 
> Its own btf_id set and its own late_initcall just for one kfunc?
> Please reduce this boilerplate code.
> Move it to kernel/bpf/helpers.c ?

ok, will move it

thanks,
jirka

Re: [PATCHv4 bpf-next 2/4] bpf: Add bpf_vma_build_id_parse function and kfunc

[Jiri Olsa]

On Mon, Nov 28, 2022 at 05:15:44PM -0800, Andrii Nakryiko wrote:
> On Mon, Nov 28, 2022 at 5:29 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding bpf_vma_build_id_parse function to retrieve build id from
> > passed vma object and making it available as bpf kfunc.
> 
> As a completely different way of solving this problem of retrieving
> build_id for tracing needs, can we teach kernel itself to parse and
> store build_id (probably gated behind Kconfig option) in struct file
> (ideally)? On exec() and when mmap()'ing with executable permissions,
> Linux kernel will try to fetch build_id from ELF file and if
> successful store it in struct file. Given build_id can be up to 20
> bytes (currently) and not each struct file points to executable, we
> might want to only add a pointer field to `struct file` itself, which,
> if build_id is present, will point to
> 
> struct build_id {
>     char sz;
>     char data[];
> };
> 
> This way we don't increase `struct file` by much.
> 
> And then any BPF program would be able to easily probe_read_kernel
> such build_id from vma_area_struct->vm_file->build_id?
> 
> I'm sure I'm oversimplifying, but this seems like a good solution for
> all kinds of profiling BPF programs without the need to maintain all
> these allowlists and adding new helpers/kfuncs?

good idea, that would make it much easier, will check

jirka

> 
> I know Hao was looking at the problem of getting build_id, I'm curious
> if something like this would work for their use cases as well?
> 
> 
> >
> > We can't use build_id_parse directly as kfunc, because we would
> > not have control over the build id buffer size provided by user.
> >
> > Instead we are adding new bpf_vma_build_id_parse function with
> > 'build_id__sz' argument that instructs verifier to check for the
> > available space in build_id buffer.
> >
> > This way we check that there's always available memory space
> > behind build_id pointer. We also check that the build_id__sz is
> > at least BUILD_ID_SIZE_MAX so we can place any buildid in.
> >
> > The bpf_vma_build_id_parse kfunc is marked as KF_TRUSTED_ARGS,
> > so it can be only called with trusted vma objects. These are
> > currently provided only by find_vma callback function and
> > task_vma iterator program.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/linux/bpf.h      |  4 ++++
> >  kernel/trace/bpf_trace.c | 31 +++++++++++++++++++++++++++++++
> >  2 files changed, 35 insertions(+)
> >
> 
> [...]

Re: [PATCHv4 bpf-next 2/4] bpf: Add bpf_vma_build_id_parse function and kfunc

[Andrii Nakryiko]

On Mon, Nov 28, 2022 at 10:20 PM Hao Luo <haoluo@google.com> wrote:
>
> On Mon, Nov 28, 2022 at 5:15 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Nov 28, 2022 at 5:29 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Adding bpf_vma_build_id_parse function to retrieve build id from
> > > passed vma object and making it available as bpf kfunc.
> >
> > As a completely different way of solving this problem of retrieving
> > build_id for tracing needs, can we teach kernel itself to parse and
> > store build_id (probably gated behind Kconfig option) in struct file
> > (ideally)? On exec() and when mmap()'ing with executable permissions,
> > Linux kernel will try to fetch build_id from ELF file and if
> > successful store it in struct file. Given build_id can be up to 20
> > bytes (currently) and not each struct file points to executable, we
> > might want to only add a pointer field to `struct file` itself, which,
> > if build_id is present, will point to
> >
> > struct build_id {
> >     char sz;
> >     char data[];
> > };
> >
> > This way we don't increase `struct file` by much.
> >
> > And then any BPF program would be able to easily probe_read_kernel
> > such build_id from vma_area_struct->vm_file->build_id?
> >
> > I'm sure I'm oversimplifying, but this seems like a good solution for
> > all kinds of profiling BPF programs without the need to maintain all
> > these allowlists and adding new helpers/kfuncs?
> >
> > I know Hao was looking at the problem of getting build_id, I'm curious
> > if something like this would work for their use cases as well?
> >
>
> This helps a little. We would like to get build_id reliably. There are
> two problems we encountered.
>
> First, sometimes we need to get build_id from an atomic context. We
> fail to get build_id if the page that contains the build_id has been
> evicted from pagecache. Storing the build_id in `struct file` or
> `struct inode` is a good and natural solution. But, this problem can
> also be solved by using mlock to pin the page in memory. We are using
> mlock, it seems to be working well right now.

This is hardly a generic solution, as it requires instrumenting every
application to do this, right? So what I'm proposing is exactly to
avoid having each individual application do something special just to
allow profiling tools to capture build_id.

>
> The other problem we encountered may be very specific to our own use
> case. Sometimes we execute code that is mapped in an anonymous page
> (not backed by file). In that case, we can't get build_id either. What
> we are doing right now is writing the build_id into the
> vm_area_struct->anon_name field and teach build_id_parse to try
> parsing from there, when seeing an anonymous page. I can send this
> with upstream if there are other users who have the same problem.
>

Is this due to remapping some binary onto huge pages?

But regardless, your custom BPF applications can fetch this build_id
from vm_area_struct->anon_name in pure BPF code, can't it? Why do you
need to modify in-kernel build_id_parse implementation?

> >
> > >
> > > We can't use build_id_parse directly as kfunc, because we would
> > > not have control over the build id buffer size provided by user.
> > >
> > > Instead we are adding new bpf_vma_build_id_parse function with
> > > 'build_id__sz' argument that instructs verifier to check for the
> > > available space in build_id buffer.
> > >
> > > This way we check that there's always available memory space
> > > behind build_id pointer. We also check that the build_id__sz is
> > > at least BUILD_ID_SIZE_MAX so we can place any buildid in.
> > >
> > > The bpf_vma_build_id_parse kfunc is marked as KF_TRUSTED_ARGS,
> > > so it can be only called with trusted vma objects. These are
> > > currently provided only by find_vma callback function and
> > > task_vma iterator program.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  include/linux/bpf.h      |  4 ++++
> > >  kernel/trace/bpf_trace.c | 31 +++++++++++++++++++++++++++++++
> > >  2 files changed, 35 insertions(+)
> > >
> >
> > [...]

Re: [PATCHv4 bpf-next 2/4] bpf: Add bpf_vma_build_id_parse function and kfunc

[Hao Luo]

On Tue, Nov 29, 2022 at 4:35 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
> This is hardly a generic solution, as it requires instrumenting every
> application to do this, right? So what I'm proposing is exactly to
> avoid having each individual application do something special just to
> allow profiling tools to capture build_id.

I agree. Because the mlock approach is working, we didn't look further
or try improving it. But an upstreamable and generic solution would be
nice. I think Jiri has started looking at it, I am happy to help
there.

> Is this due to remapping some binary onto huge pages?

I think so, but I'm not sure.

> But regardless, your custom BPF applications can fetch this build_id
> from vm_area_struct->anon_name in pure BPF code, can't it? Why do you
> need to modify in-kernel build_id_parse implementation?

The user is using bpf_get_stack() to collect stack traces. They don't
implement walking the stack and fetching build_id from vma in their
BPF code.

Re: [PATCHv4 bpf-next 2/4] bpf: Add bpf_vma_build_id_parse function and kfunc

[Andrii Nakryiko]

On Tue, Nov 29, 2022 at 5:27 PM Hao Luo <haoluo@google.com> wrote:
>
> On Tue, Nov 29, 2022 at 4:35 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> > This is hardly a generic solution, as it requires instrumenting every
> > application to do this, right? So what I'm proposing is exactly to
> > avoid having each individual application do something special just to
> > allow profiling tools to capture build_id.
>
> I agree. Because the mlock approach is working, we didn't look further
> or try improving it. But an upstreamable and generic solution would be
> nice. I think Jiri has started looking at it, I am happy to help
> there.
>

Ok, cool, it would be great to have this work reliably and not rely on
user-space apps doing something special here.

> > Is this due to remapping some binary onto huge pages?
>
> I think so, but I'm not sure.
>

We used to have this problem, but then Song added some in-kernel
support that we now preserve the original file information. Song, do
you mind providing details?

> > But regardless, your custom BPF applications can fetch this build_id
> > from vm_area_struct->anon_name in pure BPF code, can't it? Why do you
> > need to modify in-kernel build_id_parse implementation?
>
> The user is using bpf_get_stack() to collect stack traces. They don't
> implement walking the stack and fetching build_id from vma in their
> BPF code.

Ah, I see. Let's figure out why Song's approach doesn't work in your
case, because this anon_name hack is just that -- hack.