[PATCHv3 bpf-next 0/2] bpf: Add bpf_vma_build_id_parse kfunc

[PATCHv3 bpf-next 0/2] 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.

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 (2):
      bpf: Add bpf_vma_build_id_parse function and kfunc
      selftests/bpf: Add bpf_vma_build_id_parse kfunc test

 include/linux/bpf.h                                             |  4 ++++
 kernel/bpf/verifier.c                                           | 26 ++++++++++++++++++++++++++
 kernel/trace/bpf_trace.c                                        | 31 +++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/prog_tests/bpf_vma_build_id_parse.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/bpf_vma_build_id_parse.c      | 40 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 189 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

[PATCHv3 bpf-next 1/2] 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.

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

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 8b32376ce746..7648188faa2c 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2805,4 +2805,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/bpf/verifier.c b/kernel/bpf/verifier.c
index 195d24316750..e20bad754a3a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8746,6 +8746,29 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
 	return 0;
 }
 
+BTF_ID_LIST_SINGLE(bpf_vma_build_id_parse_id, func, bpf_vma_build_id_parse)
+
+static int check_kfunc_caller(struct bpf_verifier_env *env, u32 func_id)
+{
+	struct bpf_func_state *cur;
+	struct bpf_insn *insn;
+
+	/* Allow bpf_vma_build_id_parse only from bpf_find_vma callback */
+	if (func_id == bpf_vma_build_id_parse_id[0]) {
+		cur = env->cur_state->frame[env->cur_state->curframe];
+		if (cur->callsite != BPF_MAIN_FUNC) {
+			insn = &env->prog->insnsi[cur->callsite];
+			if (insn->imm == BPF_FUNC_find_vma)
+				return 0;
+		}
+		verbose(env, "calling bpf_vma_build_id_parse outside bpf_find_vma "
+			"callback is not allowed\n");
+		return -1;
+	}
+
+	return 0;
+}
+
 static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 			    int *insn_idx_p)
 {
@@ -8797,6 +8820,9 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 		return -EACCES;
 	}
 
+	if (check_kfunc_caller(env, func_id))
+		return -EACCES;
+
 	/* Check the arguments */
 	err = check_kfunc_args(env, &meta);
 	if (err < 0)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index f2d8d070d024..7f08e6c3a080 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)
+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

[PATCHv3 bpf-next 2/2] selftests/bpf: Add bpf_vma_build_id_parse kfunc test

[Jiri Olsa]

Adding test for bpf_vma_build_id_parse kfunc.

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   | 88 +++++++++++++++++++
 .../bpf/progs/bpf_vma_build_id_parse.c        | 40 +++++++++
 2 files changed, 128 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..83030a3b2c42
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_vma_build_id_parse.c
@@ -0,0 +1,88 @@
+// 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"
+
+#define BUILDID_STR_SIZE (BPF_BUILD_ID_SIZE*2 + 1)
+
+static int read_buildid(char **build_id)
+{
+	char tmp[] = "/tmp/dataXXXXXX";
+	char buf[200];
+	int err, fd;
+	FILE *f;
+
+	fd = mkstemp(tmp);
+	if (fd == -1)
+		return -1;
+	close(fd);
+
+	snprintf(buf, sizeof(buf),
+		"readelf -n ./test_progs 2>/dev/null | grep 'Build ID' | awk '{print $3}' > %s",
+		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;
+}
+
+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_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;
+}
-- 
2.38.1

Re: [PATCHv3 bpf-next 1/2] bpf: Add bpf_vma_build_id_parse function and kfunc

[Alexei Starovoitov]

On Fri, Nov 18, 2022 at 7:40 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.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/linux/bpf.h      |  4 ++++
>  kernel/bpf/verifier.c    | 26 ++++++++++++++++++++++++++
>  kernel/trace/bpf_trace.c | 31 +++++++++++++++++++++++++++++++
>  3 files changed, 61 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 8b32376ce746..7648188faa2c 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2805,4 +2805,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/bpf/verifier.c b/kernel/bpf/verifier.c
> index 195d24316750..e20bad754a3a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -8746,6 +8746,29 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
>         return 0;
>  }
>
> +BTF_ID_LIST_SINGLE(bpf_vma_build_id_parse_id, func, bpf_vma_build_id_parse)
> +
> +static int check_kfunc_caller(struct bpf_verifier_env *env, u32 func_id)
> +{
> +       struct bpf_func_state *cur;
> +       struct bpf_insn *insn;
> +
> +       /* Allow bpf_vma_build_id_parse only from bpf_find_vma callback */
> +       if (func_id == bpf_vma_build_id_parse_id[0]) {
> +               cur = env->cur_state->frame[env->cur_state->curframe];
> +               if (cur->callsite != BPF_MAIN_FUNC) {
> +                       insn = &env->prog->insnsi[cur->callsite];
> +                       if (insn->imm == BPF_FUNC_find_vma)
> +                               return 0;
> +               }
> +               verbose(env, "calling bpf_vma_build_id_parse outside bpf_find_vma "
> +                       "callback is not allowed\n");
> +               return -1;
> +       }
> +
> +       return 0;
> +}

I understand that calling bpf_vma_build_id_parse from find_vma
is your only use case, but put yourself in the maintainer's shoes.
We just did an arbitrary restriction and helped a single user.
How are we going to explain this to other users?
Let's figure out a more generic way where this call is safe.
Have you looked at PTR_TRUSTED approach that David is doing
for task_struct ? Can something like this be used here?

Re: [PATCHv3 bpf-next 1/2] bpf: Add bpf_vma_build_id_parse function and kfunc

[Song Liu]

> On Nov 18, 2022, at 3:45 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> On Fri, Nov 18, 2022 at 7:40 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.
>> 
>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>> ---
>> include/linux/bpf.h      |  4 ++++
>> kernel/bpf/verifier.c    | 26 ++++++++++++++++++++++++++
>> kernel/trace/bpf_trace.c | 31 +++++++++++++++++++++++++++++++
>> 3 files changed, 61 insertions(+)
>> 
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 8b32376ce746..7648188faa2c 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -2805,4 +2805,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/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 195d24316750..e20bad754a3a 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -8746,6 +8746,29 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
>>        return 0;
>> }
>> 
>> +BTF_ID_LIST_SINGLE(bpf_vma_build_id_parse_id, func, bpf_vma_build_id_parse)
>> +
>> +static int check_kfunc_caller(struct bpf_verifier_env *env, u32 func_id)
>> +{
>> +       struct bpf_func_state *cur;
>> +       struct bpf_insn *insn;
>> +
>> +       /* Allow bpf_vma_build_id_parse only from bpf_find_vma callback */
>> +       if (func_id == bpf_vma_build_id_parse_id[0]) {
>> +               cur = env->cur_state->frame[env->cur_state->curframe];
>> +               if (cur->callsite != BPF_MAIN_FUNC) {
>> +                       insn = &env->prog->insnsi[cur->callsite];
>> +                       if (insn->imm == BPF_FUNC_find_vma)
>> +                               return 0;
>> +               }
>> +               verbose(env, "calling bpf_vma_build_id_parse outside bpf_find_vma "
>> +                       "callback is not allowed\n");
>> +               return -1;
>> +       }
>> +
>> +       return 0;
>> +}
> 
> I understand that calling bpf_vma_build_id_parse from find_vma
> is your only use case, but put yourself in the maintainer's shoes.
> We just did an arbitrary restriction and helped a single user.
> How are we going to explain this to other users?
> Let's figure out a more generic way where this call is safe.
> Have you looked at PTR_TRUSTED approach that David is doing
> for task_struct ? Can something like this be used here?

I guess that won't work, as the vma is not refcounted. :( This is
why we have to hold mmap_lock when calling task_vma programs. 

OTOH, I would image bpf_vma_build_id_parse being quite useful for 
task_vma programs. 

Thanks,
Song

Re: [PATCHv3 bpf-next 1/2] bpf: Add bpf_vma_build_id_parse function and kfunc

[Alexei Starovoitov]

On Fri, Nov 18, 2022 at 5:06 PM Song Liu <songliubraving@meta.com> wrote:
>
>
>
> > On Nov 18, 2022, at 3:45 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> >
> > On Fri, Nov 18, 2022 at 7:40 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.
> >>
> >> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> >> ---
> >> include/linux/bpf.h      |  4 ++++
> >> kernel/bpf/verifier.c    | 26 ++++++++++++++++++++++++++
> >> kernel/trace/bpf_trace.c | 31 +++++++++++++++++++++++++++++++
> >> 3 files changed, 61 insertions(+)
> >>
> >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> >> index 8b32376ce746..7648188faa2c 100644
> >> --- a/include/linux/bpf.h
> >> +++ b/include/linux/bpf.h
> >> @@ -2805,4 +2805,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/bpf/verifier.c b/kernel/bpf/verifier.c
> >> index 195d24316750..e20bad754a3a 100644
> >> --- a/kernel/bpf/verifier.c
> >> +++ b/kernel/bpf/verifier.c
> >> @@ -8746,6 +8746,29 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
> >>        return 0;
> >> }
> >>
> >> +BTF_ID_LIST_SINGLE(bpf_vma_build_id_parse_id, func, bpf_vma_build_id_parse)
> >> +
> >> +static int check_kfunc_caller(struct bpf_verifier_env *env, u32 func_id)
> >> +{
> >> +       struct bpf_func_state *cur;
> >> +       struct bpf_insn *insn;
> >> +
> >> +       /* Allow bpf_vma_build_id_parse only from bpf_find_vma callback */
> >> +       if (func_id == bpf_vma_build_id_parse_id[0]) {
> >> +               cur = env->cur_state->frame[env->cur_state->curframe];
> >> +               if (cur->callsite != BPF_MAIN_FUNC) {
> >> +                       insn = &env->prog->insnsi[cur->callsite];
> >> +                       if (insn->imm == BPF_FUNC_find_vma)
> >> +                               return 0;
> >> +               }
> >> +               verbose(env, "calling bpf_vma_build_id_parse outside bpf_find_vma "
> >> +                       "callback is not allowed\n");
> >> +               return -1;
> >> +       }
> >> +
> >> +       return 0;
> >> +}
> >
> > I understand that calling bpf_vma_build_id_parse from find_vma
> > is your only use case, but put yourself in the maintainer's shoes.
> > We just did an arbitrary restriction and helped a single user.
> > How are we going to explain this to other users?
> > Let's figure out a more generic way where this call is safe.
> > Have you looked at PTR_TRUSTED approach that David is doing
> > for task_struct ? Can something like this be used here?
>
> I guess that won't work, as the vma is not refcounted. :( This is
> why we have to hold mmap_lock when calling task_vma programs.
>
> OTOH, I would image bpf_vma_build_id_parse being quite useful for
> task_vma programs.

Of course we cannot increment non-existing refcnt in vma :)
I meant that PTR_TRUSTED part of the concept. The kfunc
bpf_vma_build_id_parse(struct vm_area_struct *vma, ...)
should have KF_TRUSTED_ARGS flag
and it will be the job of the verifier to pass a trusted vma pointer.
Meaning that the verifier needs to guarantee that
the pointer is safe to operate on.
That's what I was explaining to Kumar and David earlier
about KF_TRUSTED_ARGS semantics.

PTR_TRUSTED doesn't mean that the pointer is refcnted.
It means that it won't disappear and we can safely pass it
to kfunc or helpers.
For bpf_find_vma we can mark vma pointer PTR_TRUSTED on entry
into callback bpf prog and the prog will be able to pass it
to bpf_vma_build_id_parse() kfunc as long as the prog doesn't
add any offset to it.
The implementation of bpf_find_vma() guarantees that vma ptr
passed into callback_fn is valid.
So it's exactly PTR_TRUSTED.

Similarly task_vma programs will be receiving PTR_TRUSTED pointers too
and will be able to call bpf_vma_build_id_parse() kfunc as well.
Any place where we can guarantee the safety of the pointer
we should be marking it as PTR_TRUSTED.

David's series start with marking all tp_btf arguments as PTR_TRUSTED.
Doing this for iterators, bpf_find_vma callback
will be a continuation of PTR_TRUSTED logic.

Re: [PATCHv3 bpf-next 1/2] bpf: Add bpf_vma_build_id_parse function and kfunc

[Song Liu]

> On Nov 18, 2022, at 6:25 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> On Fri, Nov 18, 2022 at 5:06 PM Song Liu <songliubraving@meta.com> wrote:
>> 
>> 
>> 
>>> On Nov 18, 2022, at 3:45 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>>> 
>>> On Fri, Nov 18, 2022 at 7:40 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.
>>>> 
>>>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>>>> ---
>>>> include/linux/bpf.h      |  4 ++++
>>>> kernel/bpf/verifier.c    | 26 ++++++++++++++++++++++++++
>>>> kernel/trace/bpf_trace.c | 31 +++++++++++++++++++++++++++++++
>>>> 3 files changed, 61 insertions(+)
>>>> 
>>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>>> index 8b32376ce746..7648188faa2c 100644
>>>> --- a/include/linux/bpf.h
>>>> +++ b/include/linux/bpf.h
>>>> @@ -2805,4 +2805,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/bpf/verifier.c b/kernel/bpf/verifier.c
>>>> index 195d24316750..e20bad754a3a 100644
>>>> --- a/kernel/bpf/verifier.c
>>>> +++ b/kernel/bpf/verifier.c
>>>> @@ -8746,6 +8746,29 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
>>>>       return 0;
>>>> }
>>>> 
>>>> +BTF_ID_LIST_SINGLE(bpf_vma_build_id_parse_id, func, bpf_vma_build_id_parse)
>>>> +
>>>> +static int check_kfunc_caller(struct bpf_verifier_env *env, u32 func_id)
>>>> +{
>>>> +       struct bpf_func_state *cur;
>>>> +       struct bpf_insn *insn;
>>>> +
>>>> +       /* Allow bpf_vma_build_id_parse only from bpf_find_vma callback */
>>>> +       if (func_id == bpf_vma_build_id_parse_id[0]) {
>>>> +               cur = env->cur_state->frame[env->cur_state->curframe];
>>>> +               if (cur->callsite != BPF_MAIN_FUNC) {
>>>> +                       insn = &env->prog->insnsi[cur->callsite];
>>>> +                       if (insn->imm == BPF_FUNC_find_vma)
>>>> +                               return 0;
>>>> +               }
>>>> +               verbose(env, "calling bpf_vma_build_id_parse outside bpf_find_vma "
>>>> +                       "callback is not allowed\n");
>>>> +               return -1;
>>>> +       }
>>>> +
>>>> +       return 0;
>>>> +}
>>> 
>>> I understand that calling bpf_vma_build_id_parse from find_vma
>>> is your only use case, but put yourself in the maintainer's shoes.
>>> We just did an arbitrary restriction and helped a single user.
>>> How are we going to explain this to other users?
>>> Let's figure out a more generic way where this call is safe.
>>> Have you looked at PTR_TRUSTED approach that David is doing
>>> for task_struct ? Can something like this be used here?
>> 
>> I guess that won't work, as the vma is not refcounted. :( This is
>> why we have to hold mmap_lock when calling task_vma programs.
>> 
>> OTOH, I would image bpf_vma_build_id_parse being quite useful for
>> task_vma programs.
> 
> Of course we cannot increment non-existing refcnt in vma :)
> I meant that PTR_TRUSTED part of the concept. The kfunc
> bpf_vma_build_id_parse(struct vm_area_struct *vma, ...)
> should have KF_TRUSTED_ARGS flag
> and it will be the job of the verifier to pass a trusted vma pointer.
> Meaning that the verifier needs to guarantee that
> the pointer is safe to operate on.
> That's what I was explaining to Kumar and David earlier
> about KF_TRUSTED_ARGS semantics.
> 
> PTR_TRUSTED doesn't mean that the pointer is refcnted.
> It means that it won't disappear and we can safely pass it
> to kfunc or helpers.
> For bpf_find_vma we can mark vma pointer PTR_TRUSTED on entry
> into callback bpf prog and the prog will be able to pass it
> to bpf_vma_build_id_parse() kfunc as long as the prog doesn't
> add any offset to it.
> The implementation of bpf_find_vma() guarantees that vma ptr
> passed into callback_fn is valid.
> So it's exactly PTR_TRUSTED.
> 
> Similarly task_vma programs will be receiving PTR_TRUSTED pointers too
> and will be able to call bpf_vma_build_id_parse() kfunc as well.
> Any place where we can guarantee the safety of the pointer
> we should be marking it as PTR_TRUSTED.
> 
> David's series start with marking all tp_btf arguments as PTR_TRUSTED.
> Doing this for iterators, bpf_find_vma callback
> will be a continuation of PTR_TRUSTED logic.

I see. So PTR_TRUSTED task_struct is an refcounted task_struct; 
while PTR_TRUSTED vm_area_struct is a vma with its mm_struct locked. 
That makes perfect sense. 

Thanks for the explanation!

Song

Re: [PATCHv3 bpf-next 1/2] bpf: Add bpf_vma_build_id_parse function and kfunc

[Jiri Olsa]

On Fri, Nov 18, 2022 at 06:25:15PM -0800, Alexei Starovoitov wrote:
> On Fri, Nov 18, 2022 at 5:06 PM Song Liu <songliubraving@meta.com> wrote:
> >
> >
> >
> > > On Nov 18, 2022, at 3:45 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Fri, Nov 18, 2022 at 7:40 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.
> > >>
> > >> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > >> ---
> > >> include/linux/bpf.h      |  4 ++++
> > >> kernel/bpf/verifier.c    | 26 ++++++++++++++++++++++++++
> > >> kernel/trace/bpf_trace.c | 31 +++++++++++++++++++++++++++++++
> > >> 3 files changed, 61 insertions(+)
> > >>
> > >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > >> index 8b32376ce746..7648188faa2c 100644
> > >> --- a/include/linux/bpf.h
> > >> +++ b/include/linux/bpf.h
> > >> @@ -2805,4 +2805,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/bpf/verifier.c b/kernel/bpf/verifier.c
> > >> index 195d24316750..e20bad754a3a 100644
> > >> --- a/kernel/bpf/verifier.c
> > >> +++ b/kernel/bpf/verifier.c
> > >> @@ -8746,6 +8746,29 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
> > >>        return 0;
> > >> }
> > >>
> > >> +BTF_ID_LIST_SINGLE(bpf_vma_build_id_parse_id, func, bpf_vma_build_id_parse)
> > >> +
> > >> +static int check_kfunc_caller(struct bpf_verifier_env *env, u32 func_id)
> > >> +{
> > >> +       struct bpf_func_state *cur;
> > >> +       struct bpf_insn *insn;
> > >> +
> > >> +       /* Allow bpf_vma_build_id_parse only from bpf_find_vma callback */
> > >> +       if (func_id == bpf_vma_build_id_parse_id[0]) {
> > >> +               cur = env->cur_state->frame[env->cur_state->curframe];
> > >> +               if (cur->callsite != BPF_MAIN_FUNC) {
> > >> +                       insn = &env->prog->insnsi[cur->callsite];
> > >> +                       if (insn->imm == BPF_FUNC_find_vma)
> > >> +                               return 0;
> > >> +               }
> > >> +               verbose(env, "calling bpf_vma_build_id_parse outside bpf_find_vma "
> > >> +                       "callback is not allowed\n");
> > >> +               return -1;
> > >> +       }
> > >> +
> > >> +       return 0;
> > >> +}
> > >
> > > I understand that calling bpf_vma_build_id_parse from find_vma
> > > is your only use case, but put yourself in the maintainer's shoes.
> > > We just did an arbitrary restriction and helped a single user.
> > > How are we going to explain this to other users?
> > > Let's figure out a more generic way where this call is safe.
> > > Have you looked at PTR_TRUSTED approach that David is doing
> > > for task_struct ? Can something like this be used here?
> >
> > I guess that won't work, as the vma is not refcounted. :( This is
> > why we have to hold mmap_lock when calling task_vma programs.
> >
> > OTOH, I would image bpf_vma_build_id_parse being quite useful for
> > task_vma programs.
> 
> Of course we cannot increment non-existing refcnt in vma :)
> I meant that PTR_TRUSTED part of the concept. The kfunc
> bpf_vma_build_id_parse(struct vm_area_struct *vma, ...)
> should have KF_TRUSTED_ARGS flag
> and it will be the job of the verifier to pass a trusted vma pointer.
> Meaning that the verifier needs to guarantee that
> the pointer is safe to operate on.
> That's what I was explaining to Kumar and David earlier
> about KF_TRUSTED_ARGS semantics.
> 
> PTR_TRUSTED doesn't mean that the pointer is refcnted.
> It means that it won't disappear and we can safely pass it
> to kfunc or helpers.
> For bpf_find_vma we can mark vma pointer PTR_TRUSTED on entry
> into callback bpf prog and the prog will be able to pass it
> to bpf_vma_build_id_parse() kfunc as long as the prog doesn't
> add any offset to it.
> The implementation of bpf_find_vma() guarantees that vma ptr
> passed into callback_fn is valid.
> So it's exactly PTR_TRUSTED.
> 
> Similarly task_vma programs will be receiving PTR_TRUSTED pointers too
> and will be able to call bpf_vma_build_id_parse() kfunc as well.
> Any place where we can guarantee the safety of the pointer
> we should be marking it as PTR_TRUSTED.
> 
> David's series start with marking all tp_btf arguments as PTR_TRUSTED.
> Doing this for iterators, bpf_find_vma callback
> will be a continuation of PTR_TRUSTED logic.

ok, sounds much better.. generic solution for both bpf_find_vma
and task_vma iterator

thanks,
jirka