On Fri, May 17, 2024 at 3:24 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
>
> Share relocation implementation with the kernel. As part of this,
> we also need the type/string visitation functions so add them to a
> btf_common.c file that also gets shared with the kernel. Relocation
> code in kernel and userspace is identical save for the impementation
typo: implementation
> of the reparenting of split BTF to the relocated base BTF and
> retrieval of BTF header from "struct btf"; these small functions
> need separate user-space and kernel implementations.
>
> One other wrinkle on the kernel side is we have to map .BTF.ids in
> modules as they were generated with the type ids used at BTF encoding
> time. btf_relocate() optionally returns an array mapping from old BTF
> ids to relocated ids, so we use that to fix up these references where
> needed for kfuncs.
>
> Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
> ---
> include/linux/btf.h | 45 ++++++++++
> kernel/bpf/Makefile | 8 ++
> kernel/bpf/btf.c | 166 +++++++++++++++++++++++++----------
> tools/lib/bpf/Build | 2 +-
> tools/lib/bpf/btf.c | 130 ---------------------------
> tools/lib/bpf/btf_common.c | 143 ++++++++++++++++++++++++++++++
not a big fan of "btf_common" name, it tells nothing about what that
is about. Thinking a bit ahead, we are going to replace all those
callback-calling visitor helpers with iterators soon, so maybe we can
call this btf_iter.c (or at least btf_utils.c) for a more meaningful
name?
> tools/lib/bpf/btf_relocate.c | 23 +++++
> 7 files changed, 341 insertions(+), 176 deletions(-)
> create mode 100644 tools/lib/bpf/btf_common.c
>
[...]
> #ifdef CONFIG_BPF_SYSCALL
> const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id);
> +void btf_set_base_btf(struct btf *btf, struct btf *base_btf);
> +int btf_relocate(struct btf *btf, const struct btf *base_btf, __u32 **map_ids);
> +int btf_type_visit_type_ids(struct btf_type *t, type_id_visit_fn visit, void *ctx);
> +int btf_type_visit_str_offs(struct btf_type *t, str_off_visit_fn visit, void *ctx);
> const char *btf_name_by_offset(const struct btf *btf, u32 offset);
> +const char *btf_str_by_offset(const struct btf *btf, u32 offset);
> struct btf *btf_parse_vmlinux(void);
> struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
> u32 *btf_kfunc_id_set_contains(const struct btf *btf, u32 kfunc_btf_id,
> @@ -543,6 +564,30 @@ static inline const struct btf_type *btf_type_by_id(const struct btf *btf,
> {
> return NULL;
> }
> +
> +static inline void btf_set_base_btf(struct btf *btf, struct btf *base_btf)
> +{
> + return;
> +}
> +
> +static inline int btf_relocate(void *log, struct btf *btf, const struct btf *base_btf,
> + __u32 **map_ids)
> +{
> + return 0;
-EOPNOTSUPP?
> +}
> +
> +static inline int btf_type_visit_type_ids(struct btf_type *t, type_id_visit_fn visit,
> + void *ctx)
> +{
> + return 0;
ditto
> +}
> +
> +static inline int btf_type_visit_str_offs(struct btf_type *t, str_off_visit_fn visit,
> + void *ctx)
> +{
> + return 0;
ditto
> +}
> +
> static inline const char *btf_name_by_offset(const struct btf *btf,
> u32 offset)
> {
> diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> index 7eb9ad3a3ae6..612eef1228ca 100644
> --- a/kernel/bpf/Makefile
> +++ b/kernel/bpf/Makefile
> @@ -52,3 +52,11 @@ obj-$(CONFIG_BPF_PRELOAD) += preload/
> obj-$(CONFIG_BPF_SYSCALL) += relo_core.o
> $(obj)/relo_core.o: $(srctree)/tools/lib/bpf/relo_core.c FORCE
> $(call if_changed_rule,cc_o_c)
> +
> +obj-$(CONFIG_BPF_SYSCALL) += btf_common.o
> +$(obj)/btf_common.o: $(srctree)/tools/lib/bpf/btf_common.c FORCE
> + $(call if_changed_rule,cc_o_c)
> +
> +obj-$(CONFIG_BPF_SYSCALL) += btf_relocate.o
> +$(obj)/btf_relocate.o: $(srctree)/tools/lib/bpf/btf_relocate.c FORCE
> + $(call if_changed_rule,cc_o_c)
I believe make should allow us to do this with one rule, see all those
magical % rules
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 821063660d9f..ebc127da4d79 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -274,6 +274,7 @@ struct btf {
> u32 start_str_off; /* first string offset (0 for base BTF) */
> char name[MODULE_NAME_LEN];
> bool kernel_btf;
> + __u32 *base_map; /* map from distilled base BTF -> vmlinux BTF ids */
please point out that it's an ID map in the name, so base_id_map. map
by itself is confusing.
> };
>
> enum verifier_phase {
> @@ -530,6 +531,11 @@ static bool btf_type_is_decl_tag_target(const struct btf_type *t)
> btf_type_is_var(t) || btf_type_is_typedef(t);
> }
>
> +static bool btf_is_vmlinux(const struct btf *btf)
> +{
> + return btf->kernel_btf && !btf->base_btf;
there is actually a helper like this somewhere in the kernel. I can't
recall the name, but it checks the name ("vmlinux"), it would be nice
to avoid duplication of logic
> +}
> +
> u32 btf_nr_types(const struct btf *btf)
> {
> u32 total = 0;
> @@ -772,7 +778,7 @@ static bool __btf_name_char_ok(char c, bool first)
> return true;
> }
>
[...]
> +struct btf *btf_parse_vmlinux(void)
> +{
> + struct btf_verifier_env *env = NULL;
> + struct bpf_verifier_log *log;
> + struct btf *btf;
> + int err;
> +
> + env = kzalloc(sizeof(*env), GFP_KERNEL | __GFP_NOWARN);
> + if (!env)
> + return ERR_PTR(-ENOMEM);
> +
> + log = &env->log;
> + log->level = BPF_LOG_KERNEL;
> + btf = btf_parse_base(env, "vmlinux", __start_BTF, __stop_BTF - __start_BTF);
> + if (!IS_ERR(btf)) {
nit: let's keep success case logic linear, instead of nesting it. It's
better to have a few goto err_out for error condition, but have a
linear unnested steps for success
> + /* btf_parse_vmlinux() runs under bpf_verifier_lock */
> + bpf_ctx_convert.t = btf_type_by_id(btf, bpf_ctx_convert_btf_id[0]);
> + err = btf_alloc_id(btf);
> + if (err) {
> + btf_free(btf);
> + btf = ERR_PTR(err);
> + }
> + }
> + btf_verifier_env_free(env);
> + return btf;
> +}
> +
> #ifdef CONFIG_DEBUG_INFO_BTF_MODULES
>
> -static struct btf *btf_parse_module(const char *module_name, const void *data, unsigned int data_size)
> +/* If .BTF_ids section was created with distilled base BTF, both base and
> + * split BTF ids will need to be mapped to actual base/split ids for
> + * BTF now that it has been relocated.
> + */
> +static __u32 btf_id_map(const struct btf *btf, __u32 id)
... and this should be named "btf_map_id" (prefix + verb + subject),
IMO (or even better btf_remap_id or btf_relocate_id, actually)
> {
> + if (!btf->base_btf || !btf->base_map)
> + return id;
> + return btf->base_map[id];
> +}
> +
[...]
