[PATCH v2 bpf-next 0/2] bpf: add a ksym BPF iterator

[PATCH v2 bpf-next 0/2] bpf: add a ksym BPF iterator

[Alan Maguire]

a ksym BPF iterator would be useful as it would allow more flexible
interactions with kernel symbols than are currently supported; it could
for example create more efficient map representations for lookup,
speed up symbol resolution etc.

The idea was initially discussed here [1].

Changes since RFC [2]:

- change name of iterator (and associated structures/fields) to "ksym" 
  (Andrii, patches 1, 2)
- remove dependency on CONFIG_PROC_FS; it was used for other BPF
  iterators, and I assumed it was needed because of seq ops but I
  don't think it is required on digging futher (Andrii, patch 1)

[1] https://lore.kernel.org/all/YjRPZj6Z8vuLeEZo@krava/
[2] https://lore.kernel.org/all/1656089118-577-1-git-send-email-alan.maguire@oracle.com/


Alan Maguire (2):
  bpf: add a ksym BPF iterator
  selftests/bpf: add a ksym iter subtest

 kernel/kallsyms.c                                 | 89 +++++++++++++++++++++++
 tools/testing/selftests/bpf/prog_tests/bpf_iter.c | 16 ++++
 tools/testing/selftests/bpf/progs/bpf_iter_ksym.c | 71 ++++++++++++++++++
 3 files changed, 176 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_ksym.c

-- 
1.8.3.1

[PATCH v2 bpf-next 1/2] bpf: add a ksym BPF iterator

[Alan Maguire]

add a "ksym" iterator which provides access to a "struct kallsym_iter"
for each symbol.  Intent is to support more flexible symbol parsing
as discussed in [1].

[1] https://lore.kernel.org/all/YjRPZj6Z8vuLeEZo@krava/

Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 kernel/kallsyms.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index fbdf8d3..8b662da 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -30,6 +30,7 @@
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/bsearch.h>
+#include <linux/btf_ids.h>
 
 /*
  * These will be re-linked against their real values
@@ -799,6 +800,91 @@ static int s_show(struct seq_file *m, void *p)
 	.show = s_show
 };
 
+#ifdef CONFIG_BPF_SYSCALL
+
+struct bpf_iter__ksym {
+	__bpf_md_ptr(struct bpf_iter_meta *, meta);
+	__bpf_md_ptr(struct kallsym_iter *, ksym);
+};
+
+static int ksym_prog_seq_show(struct seq_file *m, bool in_stop)
+{
+	struct bpf_iter__ksym ctx;
+	struct bpf_iter_meta meta;
+	struct bpf_prog *prog;
+
+	meta.seq = m;
+	prog = bpf_iter_get_info(&meta, in_stop);
+	if (!prog)
+		return 0;
+
+	ctx.meta = &meta;
+	ctx.ksym = m ? m->private : NULL;
+	return bpf_iter_run_prog(prog, &ctx);
+}
+
+static int bpf_iter_ksym_seq_show(struct seq_file *m, void *p)
+{
+	return ksym_prog_seq_show(m, false);
+}
+
+static void bpf_iter_ksym_seq_stop(struct seq_file *m, void *p)
+{
+	if (!p)
+		(void) ksym_prog_seq_show(m, true);
+	else
+		s_stop(m, p);
+}
+
+static const struct seq_operations bpf_iter_ksym_ops = {
+	.start = s_start,
+	.next = s_next,
+	.stop = bpf_iter_ksym_seq_stop,
+	.show = bpf_iter_ksym_seq_show,
+};
+
+static int bpf_iter_ksym_init(void *priv_data, struct bpf_iter_aux_info *aux)
+{
+	struct kallsym_iter *iter = priv_data;
+
+	reset_iter(iter, 0);
+
+	iter->show_value = true;
+
+	return 0;
+}
+
+DEFINE_BPF_ITER_FUNC(ksym, struct bpf_iter_meta *meta, struct kallsym_iter *ksym)
+
+static const struct bpf_iter_seq_info ksym_iter_seq_info = {
+	.seq_ops		= &bpf_iter_ksym_ops,
+	.init_seq_private	= bpf_iter_ksym_init,
+	.fini_seq_private	= NULL,
+	.seq_priv_size		= sizeof(struct kallsym_iter),
+};
+
+static struct bpf_iter_reg ksym_iter_reg_info = {
+	.target                 = "ksym",
+	.ctx_arg_info_size	= 1,
+	.ctx_arg_info		= {
+		{ offsetof(struct bpf_iter__ksym, ksym),
+		  PTR_TO_BTF_ID_OR_NULL },
+	},
+	.seq_info		= &ksym_iter_seq_info,
+};
+
+BTF_ID_LIST(btf_ksym_iter_id)
+BTF_ID(struct, kallsym_iter)
+
+static void __init bpf_ksym_iter_register(void)
+{
+	ksym_iter_reg_info.ctx_arg_info[0].btf_id = *btf_ksym_iter_id;
+	if (bpf_iter_reg_target(&ksym_iter_reg_info))
+		pr_warn("Warning: could not register bpf ksym iterator\n");
+}
+
+#endif /* CONFIG_BPF_SYSCALL */
+
 static inline int kallsyms_for_perf(void)
 {
 #ifdef CONFIG_PERF_EVENTS
@@ -885,6 +971,9 @@ const char *kdb_walk_kallsyms(loff_t *pos)
 static int __init kallsyms_init(void)
 {
 	proc_create("kallsyms", 0444, NULL, &kallsyms_proc_ops);
+#if defined(CONFIG_BPF_SYSCALL)
+	bpf_ksym_iter_register();
+#endif
 	return 0;
 }
 device_initcall(kallsyms_init);
-- 
1.8.3.1

[PATCH v2 bpf-next 2/2] selftests/bpf: add a ksym iter subtest

[Alan Maguire]

add subtest verifying BPF ksym iter behaviour.  The BPF ksym
iter program shows an example of dumping a format different to
/proc/kallsyms.  It adds KIND and MAX_SIZE fields which represent the
kind of symbol (core kernel, module, ftrace, bpf, or kprobe) and
the maximum size the symbol can be.  The latter is calculated from
the difference between current symbol value and the next symbol
value.

The key benefit for this iterator will likely be supporting in-kernel
data-gathering rather than dumping symbol details to userspace and
parsing the results.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 tools/testing/selftests/bpf/prog_tests/bpf_iter.c | 16 +++++
 tools/testing/selftests/bpf/progs/bpf_iter_ksym.c | 71 +++++++++++++++++++++++
 2 files changed, 87 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_ksym.c

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
index 7ff5fa9..a33874b 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
@@ -27,6 +27,7 @@
 #include "bpf_iter_test_kern5.skel.h"
 #include "bpf_iter_test_kern6.skel.h"
 #include "bpf_iter_bpf_link.skel.h"
+#include "bpf_iter_ksym.skel.h"
 
 static int duration;
 
@@ -1120,6 +1121,19 @@ static void test_link_iter(void)
 	bpf_iter_bpf_link__destroy(skel);
 }
 
+static void test_ksym_iter(void)
+{
+	struct bpf_iter_ksym *skel;
+
+	skel = bpf_iter_ksym__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "bpf_iter_ksym__open_and_load"))
+		return;
+
+	do_dummy_read(skel->progs.dump_ksym);
+
+	bpf_iter_ksym__destroy(skel);
+}
+
 #define CMP_BUFFER_SIZE 1024
 static char task_vma_output[CMP_BUFFER_SIZE];
 static char proc_maps_output[CMP_BUFFER_SIZE];
@@ -1267,4 +1281,6 @@ void test_bpf_iter(void)
 		test_buf_neg_offset();
 	if (test__start_subtest("link-iter"))
 		test_link_iter();
+	if (test__start_subtest("ksym"))
+		test_ksym_iter();
 }
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_ksym.c b/tools/testing/selftests/bpf/progs/bpf_iter_ksym.c
new file mode 100644
index 0000000..bc3a479
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_ksym.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022, Oracle and/or its affiliates. */
+#include "bpf_iter.h"
+#include <bpf/bpf_helpers.h>
+
+char _license[] SEC("license") = "GPL";
+
+unsigned long last_sym_value = 0;
+
+static inline char tolower(char c)
+{
+	if (c >= 'A' && c <= 'Z')
+		c += ('a' - 'A');
+	return c;
+}
+
+static inline char toupper(char c)
+{
+	if (c >= 'a' && c <= 'z')
+		c -= ('a' - 'A');
+	return c;
+}
+
+/* Dump symbols with max size; the latter is calculated by caching symbol N value
+ * and when iterating on symbol N+1, we can print max size of symbol N via
+ * address of N+1 - address of N.
+ */
+SEC("iter/ksym")
+int dump_ksym(struct bpf_iter__ksym *ctx)
+{
+	struct seq_file *seq = ctx->meta->seq;
+	struct kallsym_iter *iter = ctx->ksym;
+	__u32 seq_num = ctx->meta->seq_num;
+	char type;
+	int ret;
+
+	if (!iter)
+		return 0;
+
+	if (seq_num == 0) {
+		BPF_SEQ_PRINTF(seq, "ADDR TYPE NAME MODULE_NAME KIND MAX_SIZE\n");
+		return 0;
+	}
+	if (last_sym_value)
+		BPF_SEQ_PRINTF(seq, "0x%x\n", iter->value - last_sym_value);
+	else
+		BPF_SEQ_PRINTF(seq, "\n");
+
+	last_sym_value = iter->value;
+
+	type = iter->type;
+
+	if (iter->module_name[0]) {
+		type = iter->exported ? toupper(type) : tolower(type);
+		BPF_SEQ_PRINTF(seq, "0x%llx %c %s [ %s ] ",
+			       iter->value, type, iter->name, iter->module_name);
+	} else {
+		BPF_SEQ_PRINTF(seq, "0x%llx %c %s ", iter->value, type, iter->name);
+	}
+	if (!iter->pos_arch_end || iter->pos_arch_end > iter->pos)
+		BPF_SEQ_PRINTF(seq, "CORE ");
+	else if (!iter->pos_mod_end || iter->pos_mod_end > iter->pos)
+		BPF_SEQ_PRINTF(seq, "MOD ");
+	else if (!iter->pos_ftrace_mod_end || iter->pos_ftrace_mod_end > iter->pos)
+		BPF_SEQ_PRINTF(seq, "FTRACE_MOD ");
+	else if (!iter->pos_bpf_end || iter->pos_bpf_end > iter->pos)
+		BPF_SEQ_PRINTF(seq, "BPF ");
+	else
+		BPF_SEQ_PRINTF(seq, "KPROBE ");
+	return 0;
+}
-- 
1.8.3.1

Re: [PATCH v2 bpf-next 1/2] bpf: add a ksym BPF iterator

[Yonghong Song]

On 7/1/22 2:26 AM, Alan Maguire wrote:
> add a "ksym" iterator which provides access to a "struct kallsym_iter"
> for each symbol.  Intent is to support more flexible symbol parsing
> as discussed in [1].
> 
> [1] https://lore.kernel.org/all/YjRPZj6Z8vuLeEZo@krava/
> 
> Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>   kernel/kallsyms.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 89 insertions(+)
> 
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index fbdf8d3..8b662da 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -30,6 +30,7 @@
>   #include <linux/module.h>
>   #include <linux/kernel.h>
>   #include <linux/bsearch.h>
> +#include <linux/btf_ids.h>
>   
>   /*
>    * These will be re-linked against their real values
> @@ -799,6 +800,91 @@ static int s_show(struct seq_file *m, void *p)
>   	.show = s_show
>   };
>   
> +#ifdef CONFIG_BPF_SYSCALL
> +
> +struct bpf_iter__ksym {
> +	__bpf_md_ptr(struct bpf_iter_meta *, meta);
> +	__bpf_md_ptr(struct kallsym_iter *, ksym);
> +};
> +
> +static int ksym_prog_seq_show(struct seq_file *m, bool in_stop)
> +{
> +	struct bpf_iter__ksym ctx;
> +	struct bpf_iter_meta meta;
> +	struct bpf_prog *prog;
> +
> +	meta.seq = m;
> +	prog = bpf_iter_get_info(&meta, in_stop);
> +	if (!prog)
> +		return 0;
> +
> +	ctx.meta = &meta;
> +	ctx.ksym = m ? m->private : NULL;
> +	return bpf_iter_run_prog(prog, &ctx);
> +}
> +
> +static int bpf_iter_ksym_seq_show(struct seq_file *m, void *p)
> +{
> +	return ksym_prog_seq_show(m, false);
> +}
> +
> +static void bpf_iter_ksym_seq_stop(struct seq_file *m, void *p)
> +{
> +	if (!p)
> +		(void) ksym_prog_seq_show(m, true);
> +	else
> +		s_stop(m, p);
> +}
> +
> +static const struct seq_operations bpf_iter_ksym_ops = {
> +	.start = s_start,
> +	.next = s_next,
> +	.stop = bpf_iter_ksym_seq_stop,
> +	.show = bpf_iter_ksym_seq_show,
> +};
> +
> +static int bpf_iter_ksym_init(void *priv_data, struct bpf_iter_aux_info *aux)
> +{
> +	struct kallsym_iter *iter = priv_data;
> +
> +	reset_iter(iter, 0);
> +
> +	iter->show_value = true;

I think instead of always having show_value = true, we should have
    iter->show_value = kallsyms_show_value(...);

this is consistent with what `cat /proc/kallsyms` is doing, and
also consistent with bpf_dump_raw_ok() used when dumping various
kernel info in syscall.c.

We don't have a file here, so credential can be from the current
process with current_cred().

> +
> +	return 0;
> +}
> +
> +DEFINE_BPF_ITER_FUNC(ksym, struct bpf_iter_meta *meta, struct kallsym_iter *ksym)
> +
> +static const struct bpf_iter_seq_info ksym_iter_seq_info = {
> +	.seq_ops		= &bpf_iter_ksym_ops,
> +	.init_seq_private	= bpf_iter_ksym_init,
> +	.fini_seq_private	= NULL,
> +	.seq_priv_size		= sizeof(struct kallsym_iter),
> +};
> +
> +static struct bpf_iter_reg ksym_iter_reg_info = {
> +	.target                 = "ksym",
> +	.ctx_arg_info_size	= 1,
> +	.ctx_arg_info		= {
> +		{ offsetof(struct bpf_iter__ksym, ksym),
> +		  PTR_TO_BTF_ID_OR_NULL },
> +	},
> +	.seq_info		= &ksym_iter_seq_info,
> +};
> +
> +BTF_ID_LIST(btf_ksym_iter_id)
> +BTF_ID(struct, kallsym_iter)
> +
> +static void __init bpf_ksym_iter_register(void)
> +{
> +	ksym_iter_reg_info.ctx_arg_info[0].btf_id = *btf_ksym_iter_id;
> +	if (bpf_iter_reg_target(&ksym_iter_reg_info))
> +		pr_warn("Warning: could not register bpf ksym iterator\n");
> +}
> +
> +#endif /* CONFIG_BPF_SYSCALL */
> +
>   static inline int kallsyms_for_perf(void)
>   {
>   #ifdef CONFIG_PERF_EVENTS
> @@ -885,6 +971,9 @@ const char *kdb_walk_kallsyms(loff_t *pos)
>   static int __init kallsyms_init(void)
>   {
>   	proc_create("kallsyms", 0444, NULL, &kallsyms_proc_ops);
> +#if defined(CONFIG_BPF_SYSCALL)
> +	bpf_ksym_iter_register();

You can inline this function here and if bpf_iter_reg_target(...) 
failed, just return the error code.

> +#endif
>   	return 0;
>   }
>   device_initcall(kallsyms_init);

Re: [PATCH v2 bpf-next 2/2] selftests/bpf: add a ksym iter subtest

[Yonghong Song]

On 7/1/22 2:27 AM, Alan Maguire wrote:
> add subtest verifying BPF ksym iter behaviour.  The BPF ksym
> iter program shows an example of dumping a format different to
> /proc/kallsyms.  It adds KIND and MAX_SIZE fields which represent the
> kind of symbol (core kernel, module, ftrace, bpf, or kprobe) and
> the maximum size the symbol can be.  The latter is calculated from
> the difference between current symbol value and the next symbol
> value.
> 
> The key benefit for this iterator will likely be supporting in-kernel
> data-gathering rather than dumping symbol details to userspace and
> parsing the results.
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>

LGTM. Thanks!

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   tools/testing/selftests/bpf/prog_tests/bpf_iter.c | 16 +++++
>   tools/testing/selftests/bpf/progs/bpf_iter_ksym.c | 71 +++++++++++++++++++++++
>   2 files changed, 87 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_ksym.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> index 7ff5fa9..a33874b 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> @@ -27,6 +27,7 @@
>   #include "bpf_iter_test_kern5.skel.h"
>   #include "bpf_iter_test_kern6.skel.h"
>   #include "bpf_iter_bpf_link.skel.h"
> +#include "bpf_iter_ksym.skel.h"
>   
[...]

Re: [PATCH v2 bpf-next 1/2] bpf: add a ksym BPF iterator

[Andrii Nakryiko]

On Fri, Jul 1, 2022 at 10:58 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 7/1/22 2:26 AM, Alan Maguire wrote:
> > add a "ksym" iterator which provides access to a "struct kallsym_iter"
> > for each symbol.  Intent is to support more flexible symbol parsing
> > as discussed in [1].
> >
> > [1] https://lore.kernel.org/all/YjRPZj6Z8vuLeEZo@krava/
> >
> > Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > ---
> >   kernel/kallsyms.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 89 insertions(+)
> >
> > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> > index fbdf8d3..8b662da 100644
> > --- a/kernel/kallsyms.c
> > +++ b/kernel/kallsyms.c
> > @@ -30,6 +30,7 @@
> >   #include <linux/module.h>
> >   #include <linux/kernel.h>
> >   #include <linux/bsearch.h>
> > +#include <linux/btf_ids.h>
> >
> >   /*
> >    * These will be re-linked against their real values
> > @@ -799,6 +800,91 @@ static int s_show(struct seq_file *m, void *p)
> >       .show = s_show
> >   };
> >
> > +#ifdef CONFIG_BPF_SYSCALL
> > +
> > +struct bpf_iter__ksym {
> > +     __bpf_md_ptr(struct bpf_iter_meta *, meta);
> > +     __bpf_md_ptr(struct kallsym_iter *, ksym);
> > +};
> > +
> > +static int ksym_prog_seq_show(struct seq_file *m, bool in_stop)
> > +{
> > +     struct bpf_iter__ksym ctx;
> > +     struct bpf_iter_meta meta;
> > +     struct bpf_prog *prog;
> > +
> > +     meta.seq = m;
> > +     prog = bpf_iter_get_info(&meta, in_stop);
> > +     if (!prog)
> > +             return 0;
> > +
> > +     ctx.meta = &meta;
> > +     ctx.ksym = m ? m->private : NULL;
> > +     return bpf_iter_run_prog(prog, &ctx);
> > +}
> > +
> > +static int bpf_iter_ksym_seq_show(struct seq_file *m, void *p)
> > +{
> > +     return ksym_prog_seq_show(m, false);
> > +}
> > +
> > +static void bpf_iter_ksym_seq_stop(struct seq_file *m, void *p)
> > +{
> > +     if (!p)
> > +             (void) ksym_prog_seq_show(m, true);
> > +     else
> > +             s_stop(m, p);
> > +}
> > +
> > +static const struct seq_operations bpf_iter_ksym_ops = {
> > +     .start = s_start,
> > +     .next = s_next,
> > +     .stop = bpf_iter_ksym_seq_stop,
> > +     .show = bpf_iter_ksym_seq_show,
> > +};
> > +
> > +static int bpf_iter_ksym_init(void *priv_data, struct bpf_iter_aux_info *aux)
> > +{
> > +     struct kallsym_iter *iter = priv_data;
> > +
> > +     reset_iter(iter, 0);
> > +
> > +     iter->show_value = true;
>
> I think instead of always having show_value = true, we should have
>     iter->show_value = kallsyms_show_value(...);
>
> this is consistent with what `cat /proc/kallsyms` is doing, and
> also consistent with bpf_dump_raw_ok() used when dumping various
> kernel info in syscall.c.
>
> We don't have a file here, so credential can be from the current
> process with current_cred().

This seems wrong to use current_cred(). show_value is used to not
"leak" pointer values to unprivileged user-space, right? In our case
BPF iterator is privileged, so there is no need to hide (or mangle,
didn't check) values.

If it happens that a privileged process loads iter/ksym program and
then passes prog FD to unprivileged one to read iterator output,
iter/ksym should still get correct symbol values.

I think the initial approach with show_value = true is the right one
-- give all the information as it is to BPF iterator.


>
> > +
> > +     return 0;
> > +}
> > +
> > +DEFINE_BPF_ITER_FUNC(ksym, struct bpf_iter_meta *meta, struct kallsym_iter *ksym)
> > +
> > +static const struct bpf_iter_seq_info ksym_iter_seq_info = {
> > +     .seq_ops                = &bpf_iter_ksym_ops,
> > +     .init_seq_private       = bpf_iter_ksym_init,
> > +     .fini_seq_private       = NULL,
> > +     .seq_priv_size          = sizeof(struct kallsym_iter),
> > +};
> > +
> > +static struct bpf_iter_reg ksym_iter_reg_info = {
> > +     .target                 = "ksym",
> > +     .ctx_arg_info_size      = 1,
> > +     .ctx_arg_info           = {
> > +             { offsetof(struct bpf_iter__ksym, ksym),
> > +               PTR_TO_BTF_ID_OR_NULL },
> > +     },
> > +     .seq_info               = &ksym_iter_seq_info,
> > +};
> > +
> > +BTF_ID_LIST(btf_ksym_iter_id)
> > +BTF_ID(struct, kallsym_iter)
> > +
> > +static void __init bpf_ksym_iter_register(void)
> > +{
> > +     ksym_iter_reg_info.ctx_arg_info[0].btf_id = *btf_ksym_iter_id;
> > +     if (bpf_iter_reg_target(&ksym_iter_reg_info))
> > +             pr_warn("Warning: could not register bpf ksym iterator\n");
> > +}
> > +
> > +#endif /* CONFIG_BPF_SYSCALL */
> > +
> >   static inline int kallsyms_for_perf(void)
> >   {
> >   #ifdef CONFIG_PERF_EVENTS
> > @@ -885,6 +971,9 @@ const char *kdb_walk_kallsyms(loff_t *pos)
> >   static int __init kallsyms_init(void)
> >   {
> >       proc_create("kallsyms", 0444, NULL, &kallsyms_proc_ops);
> > +#if defined(CONFIG_BPF_SYSCALL)
> > +     bpf_ksym_iter_register();
>
> You can inline this function here and if bpf_iter_reg_target(...)
> failed, just return the error code.
>
> > +#endif
> >       return 0;
> >   }
> >   device_initcall(kallsyms_init);

Re: [PATCH v2 bpf-next 1/2] bpf: add a ksym BPF iterator

[Andrii Nakryiko]

On Tue, Jul 5, 2022 at 9:44 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Jul 1, 2022 at 10:58 PM Yonghong Song <yhs@fb.com> wrote:
> >
> >
> >
> > On 7/1/22 2:26 AM, Alan Maguire wrote:
> > > add a "ksym" iterator which provides access to a "struct kallsym_iter"
> > > for each symbol.  Intent is to support more flexible symbol parsing
> > > as discussed in [1].
> > >
> > > [1] https://lore.kernel.org/all/YjRPZj6Z8vuLeEZo@krava/
> > >
> > > Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > > ---
> > >   kernel/kallsyms.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 89 insertions(+)
> > >
> > > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> > > index fbdf8d3..8b662da 100644
> > > --- a/kernel/kallsyms.c
> > > +++ b/kernel/kallsyms.c
> > > @@ -30,6 +30,7 @@
> > >   #include <linux/module.h>
> > >   #include <linux/kernel.h>
> > >   #include <linux/bsearch.h>
> > > +#include <linux/btf_ids.h>
> > >
> > >   /*
> > >    * These will be re-linked against their real values
> > > @@ -799,6 +800,91 @@ static int s_show(struct seq_file *m, void *p)
> > >       .show = s_show
> > >   };
> > >
> > > +#ifdef CONFIG_BPF_SYSCALL
> > > +
> > > +struct bpf_iter__ksym {
> > > +     __bpf_md_ptr(struct bpf_iter_meta *, meta);
> > > +     __bpf_md_ptr(struct kallsym_iter *, ksym);
> > > +};
> > > +
> > > +static int ksym_prog_seq_show(struct seq_file *m, bool in_stop)
> > > +{
> > > +     struct bpf_iter__ksym ctx;
> > > +     struct bpf_iter_meta meta;
> > > +     struct bpf_prog *prog;
> > > +
> > > +     meta.seq = m;
> > > +     prog = bpf_iter_get_info(&meta, in_stop);
> > > +     if (!prog)
> > > +             return 0;
> > > +
> > > +     ctx.meta = &meta;
> > > +     ctx.ksym = m ? m->private : NULL;
> > > +     return bpf_iter_run_prog(prog, &ctx);
> > > +}
> > > +
> > > +static int bpf_iter_ksym_seq_show(struct seq_file *m, void *p)
> > > +{
> > > +     return ksym_prog_seq_show(m, false);
> > > +}
> > > +
> > > +static void bpf_iter_ksym_seq_stop(struct seq_file *m, void *p)
> > > +{
> > > +     if (!p)
> > > +             (void) ksym_prog_seq_show(m, true);
> > > +     else
> > > +             s_stop(m, p);
> > > +}
> > > +
> > > +static const struct seq_operations bpf_iter_ksym_ops = {
> > > +     .start = s_start,
> > > +     .next = s_next,
> > > +     .stop = bpf_iter_ksym_seq_stop,
> > > +     .show = bpf_iter_ksym_seq_show,
> > > +};
> > > +
> > > +static int bpf_iter_ksym_init(void *priv_data, struct bpf_iter_aux_info *aux)
> > > +{
> > > +     struct kallsym_iter *iter = priv_data;
> > > +
> > > +     reset_iter(iter, 0);
> > > +
> > > +     iter->show_value = true;
> >
> > I think instead of always having show_value = true, we should have
> >     iter->show_value = kallsyms_show_value(...);
> >
> > this is consistent with what `cat /proc/kallsyms` is doing, and
> > also consistent with bpf_dump_raw_ok() used when dumping various
> > kernel info in syscall.c.
> >
> > We don't have a file here, so credential can be from the current
> > process with current_cred().
>
> This seems wrong to use current_cred(). show_value is used to not
> "leak" pointer values to unprivileged user-space, right? In our case
> BPF iterator is privileged, so there is no need to hide (or mangle,
> didn't check) values.
>
> If it happens that a privileged process loads iter/ksym program and
> then passes prog FD to unprivileged one to read iterator output,
> iter/ksym should still get correct symbol values.
>
> I think the initial approach with show_value = true is the right one
> -- give all the information as it is to BPF iterator.

Ok, I should have looked at the selftest first. Seems like this just
passes indicator to iter/ksym program and program can choose to ignore
it, if necessary. In which case I retract my comment, sorry :)

>
>
> >
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +DEFINE_BPF_ITER_FUNC(ksym, struct bpf_iter_meta *meta, struct kallsym_iter *ksym)
> > > +
> > > +static const struct bpf_iter_seq_info ksym_iter_seq_info = {
> > > +     .seq_ops                = &bpf_iter_ksym_ops,
> > > +     .init_seq_private       = bpf_iter_ksym_init,
> > > +     .fini_seq_private       = NULL,
> > > +     .seq_priv_size          = sizeof(struct kallsym_iter),
> > > +};
> > > +
> > > +static struct bpf_iter_reg ksym_iter_reg_info = {
> > > +     .target                 = "ksym",
> > > +     .ctx_arg_info_size      = 1,
> > > +     .ctx_arg_info           = {
> > > +             { offsetof(struct bpf_iter__ksym, ksym),
> > > +               PTR_TO_BTF_ID_OR_NULL },
> > > +     },
> > > +     .seq_info               = &ksym_iter_seq_info,
> > > +};
> > > +
> > > +BTF_ID_LIST(btf_ksym_iter_id)
> > > +BTF_ID(struct, kallsym_iter)
> > > +
> > > +static void __init bpf_ksym_iter_register(void)
> > > +{
> > > +     ksym_iter_reg_info.ctx_arg_info[0].btf_id = *btf_ksym_iter_id;
> > > +     if (bpf_iter_reg_target(&ksym_iter_reg_info))
> > > +             pr_warn("Warning: could not register bpf ksym iterator\n");
> > > +}
> > > +
> > > +#endif /* CONFIG_BPF_SYSCALL */
> > > +
> > >   static inline int kallsyms_for_perf(void)
> > >   {
> > >   #ifdef CONFIG_PERF_EVENTS
> > > @@ -885,6 +971,9 @@ const char *kdb_walk_kallsyms(loff_t *pos)
> > >   static int __init kallsyms_init(void)
> > >   {
> > >       proc_create("kallsyms", 0444, NULL, &kallsyms_proc_ops);
> > > +#if defined(CONFIG_BPF_SYSCALL)
> > > +     bpf_ksym_iter_register();
> >
> > You can inline this function here and if bpf_iter_reg_target(...)
> > failed, just return the error code.
> >
> > > +#endif
> > >       return 0;
> > >   }
> > >   device_initcall(kallsyms_init);

Re: [PATCH v2 bpf-next 1/2] bpf: add a ksym BPF iterator

[Hao Luo]

On Fri, Jul 1, 2022 at 2:28 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> add a "ksym" iterator which provides access to a "struct kallsym_iter"
> for each symbol.  Intent is to support more flexible symbol parsing
> as discussed in [1].
>
> [1] https://lore.kernel.org/all/YjRPZj6Z8vuLeEZo@krava/
>
> Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  kernel/kallsyms.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
>
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index fbdf8d3..8b662da 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -30,6 +30,7 @@
[...]
> +
> +static struct bpf_iter_reg ksym_iter_reg_info = {
> +       .target                 = "ksym",
> +       .ctx_arg_info_size      = 1,
> +       .ctx_arg_info           = {
> +               { offsetof(struct bpf_iter__ksym, ksym),
> +                 PTR_TO_BTF_ID_OR_NULL },
> +       },
> +       .seq_info               = &ksym_iter_seq_info,
> +};

It would be great to allow cond_resched() while iterating. Disabling
resched is unnecessary for iterating ksyms IMO.

.feature = BPF_ITER_RESCHED,

Hao

> +
[...]
> --
> 1.8.3.1
>