* [PATCH bpf-next 01/10] libbpf: deprecate btf__finalize_data() and move it into libbpf.c
2021-10-08 0:02 [PATCH bpf-next 00/10] libbpf: support custom .rodata.*/.data.* sections andrii.nakryiko
@ 2021-10-08 0:03 ` andrii.nakryiko
2021-10-08 6:06 ` Song Liu
2021-10-08 0:03 ` [PATCH bpf-next 02/10] libbpf: extract ELF processing state into separate struct andrii.nakryiko
` (10 subsequent siblings)
11 siblings, 1 reply; 42+ messages in thread
From: andrii.nakryiko @ 2021-10-08 0:03 UTC (permalink / raw)
To: bpf, ast, daniel; +Cc: andrii, kernel-team
From: Andrii Nakryiko <andrii@kernel.org>
There isn't a good use case where anyone but libbpf itself needs to call
btf__finalize_data(). It was implemented for internal use and it's not
clear why it was made into public API in the first place. To function, it
requires active ELF data, which is stored inside bpf_object for the
duration of opening phase only. But the only BTF that needs bpf_object's
ELF is that bpf_object's BTF itself, which libbpf fixes up automatically
during bpf_object__open() operation anyways. There is no need for any
additional fix up and no reasonable scenario where it's useful and
appropriate.
Thus, btf__finalize_data() is just an API atavism and is better removed.
So this patch marks it as deprecated immediately (v0.6+) and moves the
code from btf.c into libbpf.c where it's used in the context of
bpf_object opening phase. Such code co-location allows to make code
structure more straightforward and remove bpf_object__section_size() and
bpf_object__variable_offset() internal helpers from libbpf_internal.h,
making them static. Their naming is also adjusted to not create
a wrong illusion that they are some sort of method of bpf_object. They
are internal helpers and are called appropriately.
This is part of libbpf 1.0 effort ([0]).
[0] Closes: https://github.com/libbpf/libbpf/issues/276
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/lib/bpf/btf.c | 93 ----------------------------
tools/lib/bpf/btf.h | 1 +
tools/lib/bpf/libbpf.c | 106 ++++++++++++++++++++++++++++++--
tools/lib/bpf/libbpf_internal.h | 4 --
4 files changed, 102 insertions(+), 102 deletions(-)
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 60fbd1c6d466..b85ca8313247 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -1107,99 +1107,6 @@ struct btf *btf__parse_split(const char *path, struct btf *base_btf)
return libbpf_ptr(btf_parse(path, base_btf, NULL));
}
-static int compare_vsi_off(const void *_a, const void *_b)
-{
- const struct btf_var_secinfo *a = _a;
- const struct btf_var_secinfo *b = _b;
-
- return a->offset - b->offset;
-}
-
-static int btf_fixup_datasec(struct bpf_object *obj, struct btf *btf,
- struct btf_type *t)
-{
- __u32 size = 0, off = 0, i, vars = btf_vlen(t);
- const char *name = btf__name_by_offset(btf, t->name_off);
- const struct btf_type *t_var;
- struct btf_var_secinfo *vsi;
- const struct btf_var *var;
- int ret;
-
- if (!name) {
- pr_debug("No name found in string section for DATASEC kind.\n");
- return -ENOENT;
- }
-
- /* .extern datasec size and var offsets were set correctly during
- * extern collection step, so just skip straight to sorting variables
- */
- if (t->size)
- goto sort_vars;
-
- ret = bpf_object__section_size(obj, name, &size);
- if (ret || !size || (t->size && t->size != size)) {
- pr_debug("Invalid size for section %s: %u bytes\n", name, size);
- return -ENOENT;
- }
-
- t->size = size;
-
- for (i = 0, vsi = btf_var_secinfos(t); i < vars; i++, vsi++) {
- t_var = btf__type_by_id(btf, vsi->type);
- var = btf_var(t_var);
-
- if (!btf_is_var(t_var)) {
- pr_debug("Non-VAR type seen in section %s\n", name);
- return -EINVAL;
- }
-
- if (var->linkage == BTF_VAR_STATIC)
- continue;
-
- name = btf__name_by_offset(btf, t_var->name_off);
- if (!name) {
- pr_debug("No name found in string section for VAR kind\n");
- return -ENOENT;
- }
-
- ret = bpf_object__variable_offset(obj, name, &off);
- if (ret) {
- pr_debug("No offset found in symbol table for VAR %s\n",
- name);
- return -ENOENT;
- }
-
- vsi->offset = off;
- }
-
-sort_vars:
- qsort(btf_var_secinfos(t), vars, sizeof(*vsi), compare_vsi_off);
- return 0;
-}
-
-int btf__finalize_data(struct bpf_object *obj, struct btf *btf)
-{
- int err = 0;
- __u32 i;
-
- for (i = 1; i <= btf->nr_types; i++) {
- struct btf_type *t = btf_type_by_id(btf, i);
-
- /* Loader needs to fix up some of the things compiler
- * couldn't get its hands on while emitting BTF. This
- * is section size and global variable offset. We use
- * the info from the ELF itself for this purpose.
- */
- if (btf_is_datasec(t)) {
- err = btf_fixup_datasec(obj, btf, t);
- if (err)
- break;
- }
- }
-
- return libbpf_err(err);
-}
-
static void *btf_get_raw_data(const struct btf *btf, __u32 *size, bool swap_endian);
int btf__load_into_kernel(struct btf *btf)
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index 864eb51753a1..68fb340f2a6e 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -123,6 +123,7 @@ LIBBPF_API struct btf *btf__load_from_kernel_by_id_split(__u32 id, struct btf *b
LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead")
LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
+LIBBPF_DEPRECATED_SINCE(0, 6, "intended for internal libbpf use only")
LIBBPF_API int btf__finalize_data(struct bpf_object *obj, struct btf *btf);
LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_into_kernel instead")
LIBBPF_API int btf__load(struct btf *btf);
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index ed313fd491bd..994dd25e36cd 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1324,8 +1324,7 @@ static bool bpf_map_type__is_map_in_map(enum bpf_map_type type)
return false;
}
-int bpf_object__section_size(const struct bpf_object *obj, const char *name,
- __u32 *size)
+static int find_elf_sec_sz(const struct bpf_object *obj, const char *name, __u32 *size)
{
int ret = -ENOENT;
@@ -1357,8 +1356,7 @@ int bpf_object__section_size(const struct bpf_object *obj, const char *name,
return *size ? 0 : ret;
}
-int bpf_object__variable_offset(const struct bpf_object *obj, const char *name,
- __u32 *off)
+static int find_elf_var_offset(const struct bpf_object *obj, const char *name, __u32 *off)
{
Elf_Data *symbols = obj->efile.symbols;
const char *sname;
@@ -2650,6 +2648,104 @@ static int bpf_object__init_btf(struct bpf_object *obj,
return 0;
}
+static int compare_vsi_off(const void *_a, const void *_b)
+{
+ const struct btf_var_secinfo *a = _a;
+ const struct btf_var_secinfo *b = _b;
+
+ return a->offset - b->offset;
+}
+
+static int btf_fixup_datasec(struct bpf_object *obj, struct btf *btf,
+ struct btf_type *t)
+{
+ __u32 size = 0, off = 0, i, vars = btf_vlen(t);
+ const char *name = btf__name_by_offset(btf, t->name_off);
+ const struct btf_type *t_var;
+ struct btf_var_secinfo *vsi;
+ const struct btf_var *var;
+ int ret;
+
+ if (!name) {
+ pr_debug("No name found in string section for DATASEC kind.\n");
+ return -ENOENT;
+ }
+
+ /* .extern datasec size and var offsets were set correctly during
+ * extern collection step, so just skip straight to sorting variables
+ */
+ if (t->size)
+ goto sort_vars;
+
+ ret = find_elf_sec_sz(obj, name, &size);
+ if (ret || !size || (t->size && t->size != size)) {
+ pr_debug("Invalid size for section %s: %u bytes\n", name, size);
+ return -ENOENT;
+ }
+
+ t->size = size;
+
+ for (i = 0, vsi = btf_var_secinfos(t); i < vars; i++, vsi++) {
+ t_var = btf__type_by_id(btf, vsi->type);
+ var = btf_var(t_var);
+
+ if (!btf_is_var(t_var)) {
+ pr_debug("Non-VAR type seen in section %s\n", name);
+ return -EINVAL;
+ }
+
+ if (var->linkage == BTF_VAR_STATIC)
+ continue;
+
+ name = btf__name_by_offset(btf, t_var->name_off);
+ if (!name) {
+ pr_debug("No name found in string section for VAR kind\n");
+ return -ENOENT;
+ }
+
+ ret = find_elf_var_offset(obj, name, &off);
+ if (ret) {
+ pr_debug("No offset found in symbol table for VAR %s\n",
+ name);
+ return -ENOENT;
+ }
+
+ vsi->offset = off;
+ }
+
+sort_vars:
+ qsort(btf_var_secinfos(t), vars, sizeof(*vsi), compare_vsi_off);
+ return 0;
+}
+
+static int btf_finalize_data(struct bpf_object *obj, struct btf *btf)
+{
+ int err = 0;
+ __u32 i, n = btf__get_nr_types(btf);
+
+ for (i = 1; i <= n; i++) {
+ struct btf_type *t = btf_type_by_id(btf, i);
+
+ /* Loader needs to fix up some of the things compiler
+ * couldn't get its hands on while emitting BTF. This
+ * is section size and global variable offset. We use
+ * the info from the ELF itself for this purpose.
+ */
+ if (btf_is_datasec(t)) {
+ err = btf_fixup_datasec(obj, btf, t);
+ if (err)
+ break;
+ }
+ }
+
+ return libbpf_err(err);
+}
+
+int btf__finalize_data(struct bpf_object *obj, struct btf *btf)
+{
+ return btf_finalize_data(obj, btf);
+}
+
static int bpf_object__finalize_btf(struct bpf_object *obj)
{
int err;
@@ -2657,7 +2753,7 @@ static int bpf_object__finalize_btf(struct bpf_object *obj)
if (!obj->btf)
return 0;
- err = btf__finalize_data(obj, obj->btf);
+ err = btf_finalize_data(obj, obj->btf);
if (err) {
pr_warn("Error finalizing %s: %d.\n", BTF_ELF_SEC, err);
return err;
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index f7fd3944d46d..983da066092d 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -303,10 +303,6 @@ struct bpf_prog_load_params {
int libbpf__bpf_prog_load(const struct bpf_prog_load_params *load_attr);
-int bpf_object__section_size(const struct bpf_object *obj, const char *name,
- __u32 *size);
-int bpf_object__variable_offset(const struct bpf_object *obj, const char *name,
- __u32 *off);
struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf);
void btf_get_kernel_prefix_kind(enum bpf_attach_type attach_type,
const char **prefix, int *kind);
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH bpf-next 01/10] libbpf: deprecate btf__finalize_data() and move it into libbpf.c
2021-10-08 0:03 ` [PATCH bpf-next 01/10] libbpf: deprecate btf__finalize_data() and move it into libbpf.c andrii.nakryiko
@ 2021-10-08 6:06 ` Song Liu
0 siblings, 0 replies; 42+ messages in thread
From: Song Liu @ 2021-10-08 6:06 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Kernel Team
On Thu, Oct 7, 2021 at 5:03 PM <andrii.nakryiko@gmail.com> wrote:
>
> From: Andrii Nakryiko <andrii@kernel.org>
>
> There isn't a good use case where anyone but libbpf itself needs to call
> btf__finalize_data(). It was implemented for internal use and it's not
> clear why it was made into public API in the first place. To function, it
> requires active ELF data, which is stored inside bpf_object for the
> duration of opening phase only. But the only BTF that needs bpf_object's
> ELF is that bpf_object's BTF itself, which libbpf fixes up automatically
> during bpf_object__open() operation anyways. There is no need for any
> additional fix up and no reasonable scenario where it's useful and
> appropriate.
>
> Thus, btf__finalize_data() is just an API atavism and is better removed.
> So this patch marks it as deprecated immediately (v0.6+) and moves the
> code from btf.c into libbpf.c where it's used in the context of
> bpf_object opening phase. Such code co-location allows to make code
> structure more straightforward and remove bpf_object__section_size() and
> bpf_object__variable_offset() internal helpers from libbpf_internal.h,
> making them static. Their naming is also adjusted to not create
> a wrong illusion that they are some sort of method of bpf_object. They
> are internal helpers and are called appropriately.
>
> This is part of libbpf 1.0 effort ([0]).
>
> [0] Closes: https://github.com/libbpf/libbpf/issues/276
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Song Liu <songliubraving@fb.com>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH bpf-next 02/10] libbpf: extract ELF processing state into separate struct
2021-10-08 0:02 [PATCH bpf-next 00/10] libbpf: support custom .rodata.*/.data.* sections andrii.nakryiko
2021-10-08 0:03 ` [PATCH bpf-next 01/10] libbpf: deprecate btf__finalize_data() and move it into libbpf.c andrii.nakryiko
@ 2021-10-08 0:03 ` andrii.nakryiko
2021-10-08 6:06 ` Song Liu
2021-10-08 0:03 ` [PATCH bpf-next 03/10] libbpf: use Elf64-specific types explicitly for dealing with ELF andrii.nakryiko
` (9 subsequent siblings)
11 siblings, 1 reply; 42+ messages in thread
From: andrii.nakryiko @ 2021-10-08 0:03 UTC (permalink / raw)
To: bpf, ast, daniel; +Cc: andrii, kernel-team
From: Andrii Nakryiko <andrii@kernel.org>
Name currently anonymous internal struct that keeps ELF-related state
for bpf_object. Just a bit of clean up, no functional changes.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/lib/bpf/libbpf.c | 70 ++++++++++++++++++++----------------------
1 file changed, 34 insertions(+), 36 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 994dd25e36cd..b88e3259edba 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -462,6 +462,35 @@ struct module_btf {
int fd_array_idx;
};
+struct elf_state {
+ int fd;
+ const void *obj_buf;
+ size_t obj_buf_sz;
+ Elf *elf;
+ GElf_Ehdr ehdr;
+ Elf_Data *symbols;
+ Elf_Data *data;
+ Elf_Data *rodata;
+ Elf_Data *bss;
+ Elf_Data *st_ops_data;
+ size_t shstrndx; /* section index for section name strings */
+ size_t strtabidx;
+ struct {
+ GElf_Shdr shdr;
+ Elf_Data *data;
+ } *reloc_sects;
+ int nr_reloc_sects;
+ int maps_shndx;
+ int btf_maps_shndx;
+ __u32 btf_maps_sec_btf_id;
+ int text_shndx;
+ int symbols_shndx;
+ int data_shndx;
+ int rodata_shndx;
+ int bss_shndx;
+ int st_ops_shndx;
+};
+
struct bpf_object {
char name[BPF_OBJ_NAME_LEN];
char license[64];
@@ -484,40 +513,10 @@ struct bpf_object {
struct bpf_gen *gen_loader;
+ /* Information when doing ELF related work. Only valid if efile.elf is not NULL */
+ struct elf_state efile;
/*
- * Information when doing elf related work. Only valid if fd
- * is valid.
- */
- struct {
- int fd;
- const void *obj_buf;
- size_t obj_buf_sz;
- Elf *elf;
- GElf_Ehdr ehdr;
- Elf_Data *symbols;
- Elf_Data *data;
- Elf_Data *rodata;
- Elf_Data *bss;
- Elf_Data *st_ops_data;
- size_t shstrndx; /* section index for section name strings */
- size_t strtabidx;
- struct {
- GElf_Shdr shdr;
- Elf_Data *data;
- } *reloc_sects;
- int nr_reloc_sects;
- int maps_shndx;
- int btf_maps_shndx;
- __u32 btf_maps_sec_btf_id;
- int text_shndx;
- int symbols_shndx;
- int data_shndx;
- int rodata_shndx;
- int bss_shndx;
- int st_ops_shndx;
- } efile;
- /*
- * All loaded bpf_object is linked in a list, which is
+ * All loaded bpf_object are linked in a list, which is
* hidden to caller. bpf_objects__<func> handlers deal with
* all objects.
*/
@@ -551,7 +550,6 @@ struct bpf_object {
char path[];
};
-#define obj_elf_valid(o) ((o)->efile.elf)
static const char *elf_sym_str(const struct bpf_object *obj, size_t off);
static const char *elf_sec_str(const struct bpf_object *obj, size_t off);
@@ -1185,7 +1183,7 @@ static struct bpf_object *bpf_object__new(const char *path,
static void bpf_object__elf_finish(struct bpf_object *obj)
{
- if (!obj_elf_valid(obj))
+ if (!obj->efile.elf)
return;
if (obj->efile.elf) {
@@ -1210,7 +1208,7 @@ static int bpf_object__elf_init(struct bpf_object *obj)
int err = 0;
GElf_Ehdr *ep;
- if (obj_elf_valid(obj)) {
+ if (obj->efile.elf) {
pr_warn("elf: init internal error\n");
return -LIBBPF_ERRNO__LIBELF;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH bpf-next 03/10] libbpf: use Elf64-specific types explicitly for dealing with ELF
2021-10-08 0:02 [PATCH bpf-next 00/10] libbpf: support custom .rodata.*/.data.* sections andrii.nakryiko
2021-10-08 0:03 ` [PATCH bpf-next 01/10] libbpf: deprecate btf__finalize_data() and move it into libbpf.c andrii.nakryiko
2021-10-08 0:03 ` [PATCH bpf-next 02/10] libbpf: extract ELF processing state into separate struct andrii.nakryiko
@ 2021-10-08 0:03 ` andrii.nakryiko
2021-10-08 6:10 ` Song Liu
2021-10-08 0:03 ` [PATCH bpf-next 04/10] libbpf: remove assumptions about uniqueness of .rodata/.data/.bss maps andrii.nakryiko
` (8 subsequent siblings)
11 siblings, 1 reply; 42+ messages in thread
From: andrii.nakryiko @ 2021-10-08 0:03 UTC (permalink / raw)
To: bpf, ast, daniel; +Cc: andrii, kernel-team
From: Andrii Nakryiko <andrii@kernel.org>
Minimize the usage of class-agnostic gelf_xxx() APIs from libelf. These
APIs require copying ELF data structures into local GElf_xxx structs and
have a more cumbersome API. BPF ELF file is defined to be always 64-bit
ELF object, even when intended to be run on 32-bit host architectures,
so there is no need to do class-agnostic conversions everywhere. BPF
static linker implementation within libbpf has been using Elf64-specific
types since initial implementation.
Add two simple helpers, elf_sym_by_idx() and elf_rel_by_idx(), for more
succinct direct access to ELF symbol and relocation records within ELF
data itself and switch all the GElf_xxx usage into Elf64_xxx
equivalents. The only remaining place within libbpf.c that's still using
gelf API is gelf_getclass(), as there doesn't seem to be a direct way to
get underlying ELF bitness.
No functional changes intended.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/lib/bpf/libbpf.c | 353 ++++++++++++++++++--------------
tools/lib/bpf/libbpf_internal.h | 4 +-
tools/lib/bpf/linker.c | 1 -
3 files changed, 196 insertions(+), 162 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index b88e3259edba..16c6205b6178 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -467,7 +467,7 @@ struct elf_state {
const void *obj_buf;
size_t obj_buf_sz;
Elf *elf;
- GElf_Ehdr ehdr;
+ Elf64_Ehdr *ehdr;
Elf_Data *symbols;
Elf_Data *data;
Elf_Data *rodata;
@@ -476,7 +476,7 @@ struct elf_state {
size_t shstrndx; /* section index for section name strings */
size_t strtabidx;
struct {
- GElf_Shdr shdr;
+ Elf64_Shdr *shdr;
Elf_Data *data;
} *reloc_sects;
int nr_reloc_sects;
@@ -555,9 +555,11 @@ static const char *elf_sym_str(const struct bpf_object *obj, size_t off);
static const char *elf_sec_str(const struct bpf_object *obj, size_t off);
static Elf_Scn *elf_sec_by_idx(const struct bpf_object *obj, size_t idx);
static Elf_Scn *elf_sec_by_name(const struct bpf_object *obj, const char *name);
-static int elf_sec_hdr(const struct bpf_object *obj, Elf_Scn *scn, GElf_Shdr *hdr);
+static Elf64_Shdr *elf_sec_hdr(const struct bpf_object *obj, Elf_Scn *scn);
static const char *elf_sec_name(const struct bpf_object *obj, Elf_Scn *scn);
static Elf_Data *elf_sec_data(const struct bpf_object *obj, Elf_Scn *scn);
+static Elf64_Sym *elf_sym_by_idx(const struct bpf_object *obj, size_t idx);
+static Elf64_Rel *elf_rel_by_idx(Elf_Data *data, size_t idx);
void bpf_program__unload(struct bpf_program *prog)
{
@@ -699,25 +701,25 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data,
size_t sec_sz = sec_data->d_size, sec_off, prog_sz, nr_syms;
int nr_progs, err, i;
const char *name;
- GElf_Sym sym;
+ Elf64_Sym *sym;
progs = obj->programs;
nr_progs = obj->nr_programs;
- nr_syms = symbols->d_size / sizeof(GElf_Sym);
+ nr_syms = symbols->d_size / sizeof(Elf64_Sym);
sec_off = 0;
for (i = 0; i < nr_syms; i++) {
- if (!gelf_getsym(symbols, i, &sym))
- continue;
- if (sym.st_shndx != sec_idx)
+ sym = elf_sym_by_idx(obj, i);
+
+ if (sym->st_shndx != sec_idx)
continue;
- if (GELF_ST_TYPE(sym.st_info) != STT_FUNC)
+ if (ELF64_ST_TYPE(sym->st_info) != STT_FUNC)
continue;
- prog_sz = sym.st_size;
- sec_off = sym.st_value;
+ prog_sz = sym->st_size;
+ sec_off = sym->st_value;
- name = elf_sym_str(obj, sym.st_name);
+ name = elf_sym_str(obj, sym->st_name);
if (!name) {
pr_warn("sec '%s': failed to get symbol name for offset %zu\n",
sec_name, sec_off);
@@ -730,7 +732,7 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data,
return -LIBBPF_ERRNO__FORMAT;
}
- if (sec_idx != obj->efile.text_shndx && GELF_ST_BIND(sym.st_info) == STB_LOCAL) {
+ if (sec_idx != obj->efile.text_shndx && ELF64_ST_BIND(sym->st_info) == STB_LOCAL) {
pr_warn("sec '%s': program '%s' is static and not supported\n", sec_name, name);
return -ENOTSUP;
}
@@ -763,9 +765,9 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data,
* as static to enable more permissive BPF verification mode
* with more outside context available to BPF verifier
*/
- if (GELF_ST_BIND(sym.st_info) != STB_LOCAL
- && (GELF_ST_VISIBILITY(sym.st_other) == STV_HIDDEN
- || GELF_ST_VISIBILITY(sym.st_other) == STV_INTERNAL))
+ if (ELF64_ST_BIND(sym->st_info) != STB_LOCAL
+ && (ELF64_ST_VISIBILITY(sym->st_other) == STV_HIDDEN
+ || ELF64_ST_VISIBILITY(sym->st_other) == STV_INTERNAL))
prog->mark_btf_static = true;
nr_progs++;
@@ -1205,8 +1207,9 @@ static void bpf_object__elf_finish(struct bpf_object *obj)
static int bpf_object__elf_init(struct bpf_object *obj)
{
+ Elf64_Ehdr *ehdr;
int err = 0;
- GElf_Ehdr *ep;
+ Elf *elf;
if (obj->efile.elf) {
pr_warn("elf: init internal error\n");
@@ -1218,8 +1221,7 @@ static int bpf_object__elf_init(struct bpf_object *obj)
* obj_buf should have been validated by
* bpf_object__open_buffer().
*/
- obj->efile.elf = elf_memory((char *)obj->efile.obj_buf,
- obj->efile.obj_buf_sz);
+ elf = elf_memory((char *)obj->efile.obj_buf, obj->efile.obj_buf_sz);
} else {
obj->efile.fd = open(obj->path, O_RDONLY);
if (obj->efile.fd < 0) {
@@ -1231,23 +1233,37 @@ static int bpf_object__elf_init(struct bpf_object *obj)
return err;
}
- obj->efile.elf = elf_begin(obj->efile.fd, ELF_C_READ_MMAP, NULL);
+ elf = elf_begin(obj->efile.fd, ELF_C_READ_MMAP, NULL);
}
- if (!obj->efile.elf) {
+ if (!elf) {
pr_warn("elf: failed to open %s as ELF file: %s\n", obj->path, elf_errmsg(-1));
err = -LIBBPF_ERRNO__LIBELF;
goto errout;
}
- if (!gelf_getehdr(obj->efile.elf, &obj->efile.ehdr)) {
+ obj->efile.elf = elf;
+
+ if (elf_kind(elf) != ELF_K_ELF) {
+ err = -LIBBPF_ERRNO__FORMAT;
+ pr_warn("elf: '%s' is not a proper ELF object\n", obj->path);
+ goto errout;
+ }
+
+ if (gelf_getclass(elf) != ELFCLASS64) {
+ err = -LIBBPF_ERRNO__FORMAT;
+ pr_warn("elf: '%s' is not a 64-bit ELF object\n", obj->path);
+ goto errout;
+ }
+
+ obj->efile.ehdr = ehdr = elf64_getehdr(elf);
+ if (!obj->efile.ehdr) {
pr_warn("elf: failed to get ELF header from %s: %s\n", obj->path, elf_errmsg(-1));
err = -LIBBPF_ERRNO__FORMAT;
goto errout;
}
- ep = &obj->efile.ehdr;
- if (elf_getshdrstrndx(obj->efile.elf, &obj->efile.shstrndx)) {
+ if (elf_getshdrstrndx(elf, &obj->efile.shstrndx)) {
pr_warn("elf: failed to get section names section index for %s: %s\n",
obj->path, elf_errmsg(-1));
err = -LIBBPF_ERRNO__FORMAT;
@@ -1255,7 +1271,7 @@ static int bpf_object__elf_init(struct bpf_object *obj)
}
/* Elf is corrupted/truncated, avoid calling elf_strptr. */
- if (!elf_rawdata(elf_getscn(obj->efile.elf, obj->efile.shstrndx), NULL)) {
+ if (!elf_rawdata(elf_getscn(elf, obj->efile.shstrndx), NULL)) {
pr_warn("elf: failed to get section names strings from %s: %s\n",
obj->path, elf_errmsg(-1));
err = -LIBBPF_ERRNO__FORMAT;
@@ -1263,8 +1279,7 @@ static int bpf_object__elf_init(struct bpf_object *obj)
}
/* Old LLVM set e_machine to EM_NONE */
- if (ep->e_type != ET_REL ||
- (ep->e_machine && ep->e_machine != EM_BPF)) {
+ if (ehdr->e_type != ET_REL || (ehdr->e_machine && ehdr->e_machine != EM_BPF)) {
pr_warn("elf: %s is not a valid eBPF object file\n", obj->path);
err = -LIBBPF_ERRNO__FORMAT;
goto errout;
@@ -1279,10 +1294,10 @@ static int bpf_object__elf_init(struct bpf_object *obj)
static int bpf_object__check_endianness(struct bpf_object *obj)
{
#if __BYTE_ORDER == __LITTLE_ENDIAN
- if (obj->efile.ehdr.e_ident[EI_DATA] == ELFDATA2LSB)
+ if (obj->efile.ehdr->e_ident[EI_DATA] == ELFDATA2LSB)
return 0;
#elif __BYTE_ORDER == __BIG_ENDIAN
- if (obj->efile.ehdr.e_ident[EI_DATA] == ELFDATA2MSB)
+ if (obj->efile.ehdr->e_ident[EI_DATA] == ELFDATA2MSB)
return 0;
#else
# error "Unrecognized __BYTE_ORDER__"
@@ -1363,23 +1378,20 @@ static int find_elf_var_offset(const struct bpf_object *obj, const char *name, _
if (!name || !off)
return -EINVAL;
- for (si = 0; si < symbols->d_size / sizeof(GElf_Sym); si++) {
- GElf_Sym sym;
+ for (si = 0; si < symbols->d_size / sizeof(Elf64_Sym); si++) {
+ Elf64_Sym *sym = elf_sym_by_idx(obj, si);
- if (!gelf_getsym(symbols, si, &sym))
- continue;
- if (GELF_ST_BIND(sym.st_info) != STB_GLOBAL ||
- GELF_ST_TYPE(sym.st_info) != STT_OBJECT)
+ if (ELF64_ST_BIND(sym->st_info) != STB_GLOBAL ||
+ ELF64_ST_TYPE(sym->st_info) != STT_OBJECT)
continue;
- sname = elf_sym_str(obj, sym.st_name);
+ sname = elf_sym_str(obj, sym->st_name);
if (!sname) {
- pr_warn("failed to get sym name string for var %s\n",
- name);
+ pr_warn("failed to get sym name string for var %s\n", name);
return -EIO;
}
if (strcmp(name, sname) == 0) {
- *off = sym.st_value;
+ *off = sym->st_value;
return 0;
}
}
@@ -1866,15 +1878,13 @@ static int bpf_object__init_user_maps(struct bpf_object *obj, bool strict)
*
* TODO: Detect array of map and report error.
*/
- nr_syms = symbols->d_size / sizeof(GElf_Sym);
+ nr_syms = symbols->d_size / sizeof(Elf64_Sym);
for (i = 0; i < nr_syms; i++) {
- GElf_Sym sym;
+ Elf64_Sym *sym = elf_sym_by_idx(obj, i);
- if (!gelf_getsym(symbols, i, &sym))
+ if (sym->st_shndx != obj->efile.maps_shndx)
continue;
- if (sym.st_shndx != obj->efile.maps_shndx)
- continue;
- if (GELF_ST_TYPE(sym.st_info) == STT_SECTION)
+ if (ELF64_ST_TYPE(sym->st_info) == STT_SECTION)
continue;
nr_maps++;
}
@@ -1891,40 +1901,38 @@ static int bpf_object__init_user_maps(struct bpf_object *obj, bool strict)
/* Fill obj->maps using data in "maps" section. */
for (i = 0; i < nr_syms; i++) {
- GElf_Sym sym;
+ Elf64_Sym *sym = elf_sym_by_idx(obj, i);
const char *map_name;
struct bpf_map_def *def;
struct bpf_map *map;
- if (!gelf_getsym(symbols, i, &sym))
- continue;
- if (sym.st_shndx != obj->efile.maps_shndx)
+ if (sym->st_shndx != obj->efile.maps_shndx)
continue;
- if (GELF_ST_TYPE(sym.st_info) == STT_SECTION)
+ if (ELF64_ST_TYPE(sym->st_info) == STT_SECTION)
continue;
map = bpf_object__add_map(obj);
if (IS_ERR(map))
return PTR_ERR(map);
- map_name = elf_sym_str(obj, sym.st_name);
+ map_name = elf_sym_str(obj, sym->st_name);
if (!map_name) {
pr_warn("failed to get map #%d name sym string for obj %s\n",
i, obj->path);
return -LIBBPF_ERRNO__FORMAT;
}
- if (GELF_ST_BIND(sym.st_info) == STB_LOCAL) {
+ if (ELF64_ST_BIND(sym->st_info) == STB_LOCAL) {
pr_warn("map '%s' (legacy): static maps are not supported\n", map_name);
return -ENOTSUP;
}
map->libbpf_type = LIBBPF_MAP_UNSPEC;
- map->sec_idx = sym.st_shndx;
- map->sec_offset = sym.st_value;
+ map->sec_idx = sym->st_shndx;
+ map->sec_offset = sym->st_value;
pr_debug("map '%s' (legacy): at sec_idx %d, offset %zu.\n",
map_name, map->sec_idx, map->sec_offset);
- if (sym.st_value + map_def_sz > data->d_size) {
+ if (sym->st_value + map_def_sz > data->d_size) {
pr_warn("corrupted maps section in %s: last map \"%s\" too small\n",
obj->path, map_name);
return -EINVAL;
@@ -1936,7 +1944,7 @@ static int bpf_object__init_user_maps(struct bpf_object *obj, bool strict)
return -ENOMEM;
}
pr_debug("map %d is \"%s\"\n", i, map->name);
- def = (struct bpf_map_def *)(data->d_buf + sym.st_value);
+ def = (struct bpf_map_def *)(data->d_buf + sym->st_value);
/*
* If the definition of the map in the object file fits in
* bpf_map_def, copy it. Any extra fields in our version
@@ -2506,12 +2514,13 @@ static int bpf_object__init_maps(struct bpf_object *obj,
static bool section_have_execinstr(struct bpf_object *obj, int idx)
{
- GElf_Shdr sh;
+ Elf64_Shdr *sh;
- if (elf_sec_hdr(obj, elf_sec_by_idx(obj, idx), &sh))
+ sh = elf_sec_hdr(obj, elf_sec_by_idx(obj, idx));
+ if (!sh)
return false;
- return sh.sh_flags & SHF_EXECINSTR;
+ return sh->sh_flags & SHF_EXECINSTR;
}
static bool btf_needs_sanitization(struct bpf_object *obj)
@@ -2987,32 +2996,36 @@ static Elf_Scn *elf_sec_by_name(const struct bpf_object *obj, const char *name)
return NULL;
}
-static int elf_sec_hdr(const struct bpf_object *obj, Elf_Scn *scn, GElf_Shdr *hdr)
+static Elf64_Shdr *elf_sec_hdr(const struct bpf_object *obj, Elf_Scn *scn)
{
+ Elf64_Shdr *shdr;
+
if (!scn)
- return -EINVAL;
+ return NULL;
- if (gelf_getshdr(scn, hdr) != hdr) {
+ shdr = elf64_getshdr(scn);
+ if (!shdr) {
pr_warn("elf: failed to get section(%zu) header from %s: %s\n",
elf_ndxscn(scn), obj->path, elf_errmsg(-1));
- return -EINVAL;
+ return NULL;
}
- return 0;
+ return shdr;
}
static const char *elf_sec_name(const struct bpf_object *obj, Elf_Scn *scn)
{
const char *name;
- GElf_Shdr sh;
+ Elf64_Shdr *sh;
if (!scn)
return NULL;
- if (elf_sec_hdr(obj, scn, &sh))
+ sh = elf_sec_hdr(obj, scn);
+ if (!sh)
return NULL;
- name = elf_sec_str(obj, sh.sh_name);
+ name = elf_sec_str(obj, sh->sh_name);
if (!name) {
pr_warn("elf: failed to get section(%zu) name from %s: %s\n",
elf_ndxscn(scn), obj->path, elf_errmsg(-1));
@@ -3040,13 +3053,29 @@ static Elf_Data *elf_sec_data(const struct bpf_object *obj, Elf_Scn *scn)
return data;
}
+static Elf64_Sym *elf_sym_by_idx(const struct bpf_object *obj, size_t idx)
+{
+ if (idx >= obj->efile.symbols->d_size / sizeof(Elf64_Sym))
+ return NULL;
+
+ return (Elf64_Sym *)obj->efile.symbols->d_buf + idx;
+}
+
+static Elf64_Rel *elf_rel_by_idx(Elf_Data *data, size_t idx)
+{
+ if (idx >= data->d_size / sizeof(Elf64_Rel))
+ return NULL;
+
+ return (Elf64_Rel *)data->d_buf + idx;
+}
+
static bool is_sec_name_dwarf(const char *name)
{
/* approximation, but the actual list is too long */
return str_has_pfx(name, ".debug_");
}
-static bool ignore_elf_section(GElf_Shdr *hdr, const char *name)
+static bool ignore_elf_section(Elf64_Shdr *hdr, const char *name)
{
/* no special handling of .strtab */
if (hdr->sh_type == SHT_STRTAB)
@@ -3101,17 +3130,18 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
const char *name;
Elf_Data *data;
Elf_Scn *scn;
- GElf_Shdr sh;
+ Elf64_Shdr *sh;
/* a bunch of ELF parsing functionality depends on processing symbols,
* so do the first pass and find the symbol table
*/
scn = NULL;
while ((scn = elf_nextscn(elf, scn)) != NULL) {
- if (elf_sec_hdr(obj, scn, &sh))
+ sh = elf_sec_hdr(obj, scn);
+ if (!sh)
return -LIBBPF_ERRNO__FORMAT;
- if (sh.sh_type == SHT_SYMTAB) {
+ if (sh->sh_type == SHT_SYMTAB) {
if (obj->efile.symbols) {
pr_warn("elf: multiple symbol tables in %s\n", obj->path);
return -LIBBPF_ERRNO__FORMAT;
@@ -3123,7 +3153,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
obj->efile.symbols = data;
obj->efile.symbols_shndx = elf_ndxscn(scn);
- obj->efile.strtabidx = sh.sh_link;
+ obj->efile.strtabidx = sh->sh_link;
}
}
@@ -3137,14 +3167,15 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
while ((scn = elf_nextscn(elf, scn)) != NULL) {
idx++;
- if (elf_sec_hdr(obj, scn, &sh))
+ sh = elf_sec_hdr(obj, scn);
+ if (!sh)
return -LIBBPF_ERRNO__FORMAT;
- name = elf_sec_str(obj, sh.sh_name);
+ name = elf_sec_str(obj, sh->sh_name);
if (!name)
return -LIBBPF_ERRNO__FORMAT;
- if (ignore_elf_section(&sh, name))
+ if (ignore_elf_section(sh, name))
continue;
data = elf_sec_data(obj, scn);
@@ -3153,8 +3184,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
pr_debug("elf: section(%d) %s, size %ld, link %d, flags %lx, type=%d\n",
idx, name, (unsigned long)data->d_size,
- (int)sh.sh_link, (unsigned long)sh.sh_flags,
- (int)sh.sh_type);
+ (int)sh->sh_link, (unsigned long)sh->sh_flags,
+ (int)sh->sh_type);
if (strcmp(name, "license") == 0) {
err = bpf_object__init_license(obj, data->d_buf, data->d_size);
@@ -3172,10 +3203,10 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
btf_data = data;
} else if (strcmp(name, BTF_EXT_ELF_SEC) == 0) {
btf_ext_data = data;
- } else if (sh.sh_type == SHT_SYMTAB) {
+ } else if (sh->sh_type == SHT_SYMTAB) {
/* already processed during the first pass above */
- } else if (sh.sh_type == SHT_PROGBITS && data->d_size > 0) {
- if (sh.sh_flags & SHF_EXECINSTR) {
+ } else if (sh->sh_type == SHT_PROGBITS && data->d_size > 0) {
+ if (sh->sh_flags & SHF_EXECINSTR) {
if (strcmp(name, ".text") == 0)
obj->efile.text_shndx = idx;
err = bpf_object__add_programs(obj, data, name, idx);
@@ -3194,10 +3225,10 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
pr_info("elf: skipping unrecognized data section(%d) %s\n",
idx, name);
}
- } else if (sh.sh_type == SHT_REL) {
+ } else if (sh->sh_type == SHT_REL) {
int nr_sects = obj->efile.nr_reloc_sects;
void *sects = obj->efile.reloc_sects;
- int sec = sh.sh_info; /* points to other section */
+ int sec = sh->sh_info; /* points to other section */
/* Only do relo for section with exec instructions */
if (!section_have_execinstr(obj, sec) &&
@@ -3219,12 +3250,12 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
obj->efile.reloc_sects[nr_sects].shdr = sh;
obj->efile.reloc_sects[nr_sects].data = data;
- } else if (sh.sh_type == SHT_NOBITS && strcmp(name, BSS_SEC) == 0) {
+ } else if (sh->sh_type == SHT_NOBITS && strcmp(name, BSS_SEC) == 0) {
obj->efile.bss = data;
obj->efile.bss_shndx = idx;
} else {
pr_info("elf: skipping section(%d) %s (size %zu)\n", idx, name,
- (size_t)sh.sh_size);
+ (size_t)sh->sh_size);
}
}
@@ -3240,19 +3271,19 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
return bpf_object__init_btf(obj, btf_data, btf_ext_data);
}
-static bool sym_is_extern(const GElf_Sym *sym)
+static bool sym_is_extern(const Elf64_Sym *sym)
{
- int bind = GELF_ST_BIND(sym->st_info);
+ int bind = ELF64_ST_BIND(sym->st_info);
/* externs are symbols w/ type=NOTYPE, bind=GLOBAL|WEAK, section=UND */
return sym->st_shndx == SHN_UNDEF &&
(bind == STB_GLOBAL || bind == STB_WEAK) &&
- GELF_ST_TYPE(sym->st_info) == STT_NOTYPE;
+ ELF64_ST_TYPE(sym->st_info) == STT_NOTYPE;
}
-static bool sym_is_subprog(const GElf_Sym *sym, int text_shndx)
+static bool sym_is_subprog(const Elf64_Sym *sym, int text_shndx)
{
- int bind = GELF_ST_BIND(sym->st_info);
- int type = GELF_ST_TYPE(sym->st_info);
+ int bind = ELF64_ST_BIND(sym->st_info);
+ int type = ELF64_ST_TYPE(sym->st_info);
/* in .text section */
if (sym->st_shndx != text_shndx)
@@ -3450,30 +3481,31 @@ static int bpf_object__collect_externs(struct bpf_object *obj)
int i, n, off, dummy_var_btf_id;
const char *ext_name, *sec_name;
Elf_Scn *scn;
- GElf_Shdr sh;
+ Elf64_Shdr *sh;
if (!obj->efile.symbols)
return 0;
scn = elf_sec_by_idx(obj, obj->efile.symbols_shndx);
- if (elf_sec_hdr(obj, scn, &sh))
+ sh = elf_sec_hdr(obj, scn);
+ if (!sh)
return -LIBBPF_ERRNO__FORMAT;
dummy_var_btf_id = add_dummy_ksym_var(obj->btf);
if (dummy_var_btf_id < 0)
return dummy_var_btf_id;
- n = sh.sh_size / sh.sh_entsize;
+ n = sh->sh_size / sh->sh_entsize;
pr_debug("looking for externs among %d symbols...\n", n);
for (i = 0; i < n; i++) {
- GElf_Sym sym;
+ Elf64_Sym *sym = elf_sym_by_idx(obj, i);
- if (!gelf_getsym(obj->efile.symbols, i, &sym))
+ if (!sym)
return -LIBBPF_ERRNO__FORMAT;
- if (!sym_is_extern(&sym))
+ if (!sym_is_extern(sym))
continue;
- ext_name = elf_sym_str(obj, sym.st_name);
+ ext_name = elf_sym_str(obj, sym->st_name);
if (!ext_name || !ext_name[0])
continue;
@@ -3495,7 +3527,7 @@ static int bpf_object__collect_externs(struct bpf_object *obj)
t = btf__type_by_id(obj->btf, ext->btf_id);
ext->name = btf__name_by_offset(obj->btf, t->name_off);
ext->sym_idx = i;
- ext->is_weak = GELF_ST_BIND(sym.st_info) == STB_WEAK;
+ ext->is_weak = ELF64_ST_BIND(sym->st_info) == STB_WEAK;
ext->sec_btf_id = find_extern_sec_btf_id(obj->btf, ext->btf_id);
if (ext->sec_btf_id <= 0) {
@@ -3730,7 +3762,7 @@ bpf_object__section_to_libbpf_map_type(const struct bpf_object *obj, int shndx)
static int bpf_program__record_reloc(struct bpf_program *prog,
struct reloc_desc *reloc_desc,
__u32 insn_idx, const char *sym_name,
- const GElf_Sym *sym, const GElf_Rel *rel)
+ const Elf64_Sym *sym, const Elf64_Rel *rel)
{
struct bpf_insn *insn = &prog->insns[insn_idx];
size_t map_idx, nr_maps = prog->obj->nr_maps;
@@ -3747,7 +3779,7 @@ static int bpf_program__record_reloc(struct bpf_program *prog,
}
if (sym_is_extern(sym)) {
- int sym_idx = GELF_R_SYM(rel->r_info);
+ int sym_idx = ELF64_R_SYM(rel->r_info);
int i, n = obj->nr_extern;
struct extern_desc *ext;
@@ -3912,9 +3944,8 @@ static struct bpf_program *find_prog_by_sec_insn(const struct bpf_object *obj,
}
static int
-bpf_object__collect_prog_relos(struct bpf_object *obj, GElf_Shdr *shdr, Elf_Data *data)
+bpf_object__collect_prog_relos(struct bpf_object *obj, Elf64_Shdr *shdr, Elf_Data *data)
{
- Elf_Data *symbols = obj->efile.symbols;
const char *relo_sec_name, *sec_name;
size_t sec_idx = shdr->sh_info;
struct bpf_program *prog;
@@ -3924,8 +3955,8 @@ bpf_object__collect_prog_relos(struct bpf_object *obj, GElf_Shdr *shdr, Elf_Data
__u32 insn_idx;
Elf_Scn *scn;
Elf_Data *scn_data;
- GElf_Sym sym;
- GElf_Rel rel;
+ Elf64_Sym *sym;
+ Elf64_Rel *rel;
scn = elf_sec_by_idx(obj, sec_idx);
scn_data = elf_sec_data(obj, scn);
@@ -3940,33 +3971,36 @@ bpf_object__collect_prog_relos(struct bpf_object *obj, GElf_Shdr *shdr, Elf_Data
nrels = shdr->sh_size / shdr->sh_entsize;
for (i = 0; i < nrels; i++) {
- if (!gelf_getrel(data, i, &rel)) {
+ rel = elf_rel_by_idx(data, i);
+ if (!rel) {
pr_warn("sec '%s': failed to get relo #%d\n", relo_sec_name, i);
return -LIBBPF_ERRNO__FORMAT;
}
- if (!gelf_getsym(symbols, GELF_R_SYM(rel.r_info), &sym)) {
+
+ sym = elf_sym_by_idx(obj, ELF64_R_SYM(rel->r_info));
+ if (!sym) {
pr_warn("sec '%s': symbol 0x%zx not found for relo #%d\n",
- relo_sec_name, (size_t)GELF_R_SYM(rel.r_info), i);
+ relo_sec_name, (size_t)ELF64_R_SYM(rel->r_info), i);
return -LIBBPF_ERRNO__FORMAT;
}
- if (rel.r_offset % BPF_INSN_SZ || rel.r_offset >= scn_data->d_size) {
+ if (rel->r_offset % BPF_INSN_SZ || rel->r_offset >= scn_data->d_size) {
pr_warn("sec '%s': invalid offset 0x%zx for relo #%d\n",
- relo_sec_name, (size_t)GELF_R_SYM(rel.r_info), i);
+ relo_sec_name, (size_t)ELF64_R_SYM(rel->r_info), i);
return -LIBBPF_ERRNO__FORMAT;
}
- insn_idx = rel.r_offset / BPF_INSN_SZ;
+ insn_idx = rel->r_offset / BPF_INSN_SZ;
/* relocations against static functions are recorded as
* relocations against the section that contains a function;
* in such case, symbol will be STT_SECTION and sym.st_name
* will point to empty string (0), so fetch section name
* instead
*/
- if (GELF_ST_TYPE(sym.st_info) == STT_SECTION && sym.st_name == 0)
- sym_name = elf_sec_name(obj, elf_sec_by_idx(obj, sym.st_shndx));
+ if (ELF64_ST_TYPE(sym->st_info) == STT_SECTION && sym->st_name == 0)
+ sym_name = elf_sec_name(obj, elf_sec_by_idx(obj, sym->st_shndx));
else
- sym_name = elf_sym_str(obj, sym.st_name);
+ sym_name = elf_sym_str(obj, sym->st_name);
sym_name = sym_name ?: "<?";
pr_debug("sec '%s': relo #%d: insn #%u against '%s'\n",
@@ -3988,7 +4022,7 @@ bpf_object__collect_prog_relos(struct bpf_object *obj, GElf_Shdr *shdr, Elf_Data
/* adjust insn_idx to local BPF program frame of reference */
insn_idx -= prog->sec_insn_off;
err = bpf_program__record_reloc(prog, &relos[prog->nr_reloc],
- insn_idx, sym_name, &sym, &rel);
+ insn_idx, sym_name, sym, rel);
if (err)
return err;
@@ -6036,10 +6070,10 @@ bpf_object__relocate(struct bpf_object *obj, const char *targ_btf_path)
}
static int bpf_object__collect_st_ops_relos(struct bpf_object *obj,
- GElf_Shdr *shdr, Elf_Data *data);
+ Elf64_Shdr *shdr, Elf_Data *data);
static int bpf_object__collect_map_relos(struct bpf_object *obj,
- GElf_Shdr *shdr, Elf_Data *data)
+ Elf64_Shdr *shdr, Elf_Data *data)
{
const int bpf_ptr_sz = 8, host_ptr_sz = sizeof(void *);
int i, j, nrels, new_sz;
@@ -6048,10 +6082,9 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
struct bpf_map *map = NULL, *targ_map;
const struct btf_member *member;
const char *name, *mname;
- Elf_Data *symbols;
unsigned int moff;
- GElf_Sym sym;
- GElf_Rel rel;
+ Elf64_Sym *sym;
+ Elf64_Rel *rel;
void *tmp;
if (!obj->efile.btf_maps_sec_btf_id || !obj->btf)
@@ -6060,28 +6093,30 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
if (!sec)
return -EINVAL;
- symbols = obj->efile.symbols;
nrels = shdr->sh_size / shdr->sh_entsize;
for (i = 0; i < nrels; i++) {
- if (!gelf_getrel(data, i, &rel)) {
+ rel = elf_rel_by_idx(data, i);
+ if (!rel) {
pr_warn(".maps relo #%d: failed to get ELF relo\n", i);
return -LIBBPF_ERRNO__FORMAT;
}
- if (!gelf_getsym(symbols, GELF_R_SYM(rel.r_info), &sym)) {
+
+ sym = elf_sym_by_idx(obj, ELF64_R_SYM(rel->r_info));
+ if (!sym) {
pr_warn(".maps relo #%d: symbol %zx not found\n",
- i, (size_t)GELF_R_SYM(rel.r_info));
+ i, (size_t)ELF64_R_SYM(rel->r_info));
return -LIBBPF_ERRNO__FORMAT;
}
- name = elf_sym_str(obj, sym.st_name) ?: "<?>";
- if (sym.st_shndx != obj->efile.btf_maps_shndx) {
+ name = elf_sym_str(obj, sym->st_name) ?: "<?>";
+ if (sym->st_shndx != obj->efile.btf_maps_shndx) {
pr_warn(".maps relo #%d: '%s' isn't a BTF-defined map\n",
i, name);
return -LIBBPF_ERRNO__RELOC;
}
- pr_debug(".maps relo #%d: for %zd value %zd rel.r_offset %zu name %d ('%s')\n",
- i, (ssize_t)(rel.r_info >> 32), (size_t)sym.st_value,
- (size_t)rel.r_offset, sym.st_name, name);
+ pr_debug(".maps relo #%d: for %zd value %zd rel->r_offset %zu name %d ('%s')\n",
+ i, (ssize_t)(rel->r_info >> 32), (size_t)sym->st_value,
+ (size_t)rel->r_offset, sym->st_name, name);
for (j = 0; j < obj->nr_maps; j++) {
map = &obj->maps[j];
@@ -6089,13 +6124,13 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
continue;
vi = btf_var_secinfos(sec) + map->btf_var_idx;
- if (vi->offset <= rel.r_offset &&
- rel.r_offset + bpf_ptr_sz <= vi->offset + vi->size)
+ if (vi->offset <= rel->r_offset &&
+ rel->r_offset + bpf_ptr_sz <= vi->offset + vi->size)
break;
}
if (j == obj->nr_maps) {
- pr_warn(".maps relo #%d: cannot find map '%s' at rel.r_offset %zu\n",
- i, name, (size_t)rel.r_offset);
+ pr_warn(".maps relo #%d: cannot find map '%s' at rel->r_offset %zu\n",
+ i, name, (size_t)rel->r_offset);
return -EINVAL;
}
@@ -6122,10 +6157,10 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
return -EINVAL;
moff = btf_member_bit_offset(def, btf_vlen(def) - 1) / 8;
- if (rel.r_offset - vi->offset < moff)
+ if (rel->r_offset - vi->offset < moff)
return -EINVAL;
- moff = rel.r_offset - vi->offset - moff;
+ moff = rel->r_offset - vi->offset - moff;
/* here we use BPF pointer size, which is always 64 bit, as we
* are parsing ELF that was built for BPF target
*/
@@ -6171,7 +6206,7 @@ static int bpf_object__collect_relos(struct bpf_object *obj)
int i, err;
for (i = 0; i < obj->efile.nr_reloc_sects; i++) {
- GElf_Shdr *shdr = &obj->efile.reloc_sects[i].shdr;
+ Elf64_Shdr *shdr = obj->efile.reloc_sects[i].shdr;
Elf_Data *data = obj->efile.reloc_sects[i].data;
int idx = shdr->sh_info;
@@ -8360,7 +8395,7 @@ static struct bpf_map *find_struct_ops_map_by_offset(struct bpf_object *obj,
/* Collect the reloc from ELF and populate the st_ops->progs[] */
static int bpf_object__collect_st_ops_relos(struct bpf_object *obj,
- GElf_Shdr *shdr, Elf_Data *data)
+ Elf64_Shdr *shdr, Elf_Data *data)
{
const struct btf_member *member;
struct bpf_struct_ops *st_ops;
@@ -8368,58 +8403,58 @@ static int bpf_object__collect_st_ops_relos(struct bpf_object *obj,
unsigned int shdr_idx;
const struct btf *btf;
struct bpf_map *map;
- Elf_Data *symbols;
unsigned int moff, insn_idx;
const char *name;
__u32 member_idx;
- GElf_Sym sym;
- GElf_Rel rel;
+ Elf64_Sym *sym;
+ Elf64_Rel *rel;
int i, nrels;
- symbols = obj->efile.symbols;
btf = obj->btf;
nrels = shdr->sh_size / shdr->sh_entsize;
for (i = 0; i < nrels; i++) {
- if (!gelf_getrel(data, i, &rel)) {
+ rel = elf_rel_by_idx(data, i);
+ if (!rel) {
pr_warn("struct_ops reloc: failed to get %d reloc\n", i);
return -LIBBPF_ERRNO__FORMAT;
}
- if (!gelf_getsym(symbols, GELF_R_SYM(rel.r_info), &sym)) {
+ sym = elf_sym_by_idx(obj, ELF64_R_SYM(rel->r_info));
+ if (!sym) {
pr_warn("struct_ops reloc: symbol %zx not found\n",
- (size_t)GELF_R_SYM(rel.r_info));
+ (size_t)ELF64_R_SYM(rel->r_info));
return -LIBBPF_ERRNO__FORMAT;
}
- name = elf_sym_str(obj, sym.st_name) ?: "<?>";
- map = find_struct_ops_map_by_offset(obj, rel.r_offset);
+ name = elf_sym_str(obj, sym->st_name) ?: "<?>";
+ map = find_struct_ops_map_by_offset(obj, rel->r_offset);
if (!map) {
- pr_warn("struct_ops reloc: cannot find map at rel.r_offset %zu\n",
- (size_t)rel.r_offset);
+ pr_warn("struct_ops reloc: cannot find map at rel->r_offset %zu\n",
+ (size_t)rel->r_offset);
return -EINVAL;
}
- moff = rel.r_offset - map->sec_offset;
- shdr_idx = sym.st_shndx;
+ moff = rel->r_offset - map->sec_offset;
+ shdr_idx = sym->st_shndx;
st_ops = map->st_ops;
- pr_debug("struct_ops reloc %s: for %lld value %lld shdr_idx %u rel.r_offset %zu map->sec_offset %zu name %d (\'%s\')\n",
+ pr_debug("struct_ops reloc %s: for %lld value %lld shdr_idx %u rel->r_offset %zu map->sec_offset %zu name %d (\'%s\')\n",
map->name,
- (long long)(rel.r_info >> 32),
- (long long)sym.st_value,
- shdr_idx, (size_t)rel.r_offset,
- map->sec_offset, sym.st_name, name);
+ (long long)(rel->r_info >> 32),
+ (long long)sym->st_value,
+ shdr_idx, (size_t)rel->r_offset,
+ map->sec_offset, sym->st_name, name);
if (shdr_idx >= SHN_LORESERVE) {
- pr_warn("struct_ops reloc %s: rel.r_offset %zu shdr_idx %u unsupported non-static function\n",
- map->name, (size_t)rel.r_offset, shdr_idx);
+ pr_warn("struct_ops reloc %s: rel->r_offset %zu shdr_idx %u unsupported non-static function\n",
+ map->name, (size_t)rel->r_offset, shdr_idx);
return -LIBBPF_ERRNO__RELOC;
}
- if (sym.st_value % BPF_INSN_SZ) {
+ if (sym->st_value % BPF_INSN_SZ) {
pr_warn("struct_ops reloc %s: invalid target program offset %llu\n",
- map->name, (unsigned long long)sym.st_value);
+ map->name, (unsigned long long)sym->st_value);
return -LIBBPF_ERRNO__FORMAT;
}
- insn_idx = sym.st_value / BPF_INSN_SZ;
+ insn_idx = sym->st_value / BPF_INSN_SZ;
member = find_member_by_offset(st_ops->type, moff * 8);
if (!member) {
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index 983da066092d..cf39eef2c0f5 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -52,8 +52,8 @@
#endif
/* Older libelf all end up in this expression, for both 32 and 64 bit */
-#ifndef GELF_ST_VISIBILITY
-#define GELF_ST_VISIBILITY(o) ((o) & 0x03)
+#ifndef ELF64_ST_VISIBILITY
+#define ELF64_ST_VISIBILITY(o) ((o) & 0x03)
#endif
#define BTF_INFO_ENC(kind, kind_flag, vlen) \
diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
index 2df880cefdae..13e6fdc7d8cb 100644
--- a/tools/lib/bpf/linker.c
+++ b/tools/lib/bpf/linker.c
@@ -15,7 +15,6 @@
#include <linux/btf.h>
#include <elf.h>
#include <libelf.h>
-#include <gelf.h>
#include <fcntl.h>
#include "libbpf.h"
#include "btf.h"
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH bpf-next 03/10] libbpf: use Elf64-specific types explicitly for dealing with ELF
2021-10-08 0:03 ` [PATCH bpf-next 03/10] libbpf: use Elf64-specific types explicitly for dealing with ELF andrii.nakryiko
@ 2021-10-08 6:10 ` Song Liu
0 siblings, 0 replies; 42+ messages in thread
From: Song Liu @ 2021-10-08 6:10 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Kernel Team
On Thu, Oct 7, 2021 at 5:37 PM <andrii.nakryiko@gmail.com> wrote:
>
> From: Andrii Nakryiko <andrii@kernel.org>
>
> Minimize the usage of class-agnostic gelf_xxx() APIs from libelf. These
> APIs require copying ELF data structures into local GElf_xxx structs and
> have a more cumbersome API. BPF ELF file is defined to be always 64-bit
> ELF object, even when intended to be run on 32-bit host architectures,
> so there is no need to do class-agnostic conversions everywhere. BPF
> static linker implementation within libbpf has been using Elf64-specific
> types since initial implementation.
>
> Add two simple helpers, elf_sym_by_idx() and elf_rel_by_idx(), for more
> succinct direct access to ELF symbol and relocation records within ELF
> data itself and switch all the GElf_xxx usage into Elf64_xxx
> equivalents. The only remaining place within libbpf.c that's still using
> gelf API is gelf_getclass(), as there doesn't seem to be a direct way to
> get underlying ELF bitness.
>
> No functional changes intended.
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Song Liu <songliubraving@fb.com>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH bpf-next 04/10] libbpf: remove assumptions about uniqueness of .rodata/.data/.bss maps
2021-10-08 0:02 [PATCH bpf-next 00/10] libbpf: support custom .rodata.*/.data.* sections andrii.nakryiko
` (2 preceding siblings ...)
2021-10-08 0:03 ` [PATCH bpf-next 03/10] libbpf: use Elf64-specific types explicitly for dealing with ELF andrii.nakryiko
@ 2021-10-08 0:03 ` andrii.nakryiko
2021-10-08 6:05 ` Song Liu
2021-10-08 0:03 ` [PATCH bpf-next 05/10] bpftool: support multiple .rodata/.data internal maps in skeleton andrii.nakryiko
` (7 subsequent siblings)
11 siblings, 1 reply; 42+ messages in thread
From: andrii.nakryiko @ 2021-10-08 0:03 UTC (permalink / raw)
To: bpf, ast, daniel; +Cc: andrii, kernel-team
From: Andrii Nakryiko <andrii@kernel.org>
Remove internal libbpf assumption that there can be only one .rodata,
.data, and .bss map per BPF object. To achieve that, extend and
generalize the scheme that was used for keeping track of relocation ELF
sections. Now each ELF section has a temporary extra index that keeps
track of logical type of ELF section (relocations, data, read-only data,
BSS). Switch relocation to this scheme, as well as .rodata/.data/.bss
handling.
We don't yet allow multiple .rodata, .data, and .bss sections, but no
libbpf internal code makes an assumption that there can be only one of
each and thus they can be explicitly referenced by a single index. Next
patches will actually allow multiple .rodata and .data sections.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/lib/bpf/libbpf.c | 260 ++++++++++++++++++++++-------------------
1 file changed, 140 insertions(+), 120 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 16c6205b6178..bbfb847fd1ea 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -462,6 +462,20 @@ struct module_btf {
int fd_array_idx;
};
+enum sec_type {
+ SEC_UNUSED = 0,
+ SEC_RELO,
+ SEC_BSS,
+ SEC_DATA,
+ SEC_RODATA,
+};
+
+struct elf_sec_desc {
+ enum sec_type sec_type;
+ Elf64_Shdr *shdr;
+ Elf_Data *data;
+};
+
struct elf_state {
int fd;
const void *obj_buf;
@@ -469,25 +483,16 @@ struct elf_state {
Elf *elf;
Elf64_Ehdr *ehdr;
Elf_Data *symbols;
- Elf_Data *data;
- Elf_Data *rodata;
- Elf_Data *bss;
Elf_Data *st_ops_data;
size_t shstrndx; /* section index for section name strings */
size_t strtabidx;
- struct {
- Elf64_Shdr *shdr;
- Elf_Data *data;
- } *reloc_sects;
- int nr_reloc_sects;
+ struct elf_sec_desc *secs;
+ int sec_cnt;
int maps_shndx;
int btf_maps_shndx;
__u32 btf_maps_sec_btf_id;
int text_shndx;
int symbols_shndx;
- int data_shndx;
- int rodata_shndx;
- int bss_shndx;
int st_ops_shndx;
};
@@ -506,10 +511,10 @@ struct bpf_object {
struct extern_desc *externs;
int nr_extern;
int kconfig_map_idx;
- int rodata_map_idx;
bool loaded;
bool has_subcalls;
+ bool has_rodata;
struct bpf_gen *gen_loader;
@@ -1168,12 +1173,8 @@ static struct bpf_object *bpf_object__new(const char *path,
obj->efile.obj_buf_sz = obj_buf_sz;
obj->efile.maps_shndx = -1;
obj->efile.btf_maps_shndx = -1;
- obj->efile.data_shndx = -1;
- obj->efile.rodata_shndx = -1;
- obj->efile.bss_shndx = -1;
obj->efile.st_ops_shndx = -1;
obj->kconfig_map_idx = -1;
- obj->rodata_map_idx = -1;
obj->kern_version = get_kernel_version();
obj->loaded = false;
@@ -1193,13 +1194,10 @@ static void bpf_object__elf_finish(struct bpf_object *obj)
obj->efile.elf = NULL;
}
obj->efile.symbols = NULL;
- obj->efile.data = NULL;
- obj->efile.rodata = NULL;
- obj->efile.bss = NULL;
obj->efile.st_ops_data = NULL;
- zfree(&obj->efile.reloc_sects);
- obj->efile.nr_reloc_sects = 0;
+ zfree(&obj->efile.secs);
+ obj->efile.sec_cnt = 0;
zclose(obj->efile.fd);
obj->efile.obj_buf = NULL;
obj->efile.obj_buf_sz = 0;
@@ -1340,30 +1338,18 @@ static bool bpf_map_type__is_map_in_map(enum bpf_map_type type)
static int find_elf_sec_sz(const struct bpf_object *obj, const char *name, __u32 *size)
{
int ret = -ENOENT;
+ Elf_Data *data;
+ Elf_Scn *scn;
*size = 0;
- if (!name) {
+ if (!name)
return -EINVAL;
- } else if (!strcmp(name, DATA_SEC)) {
- if (obj->efile.data)
- *size = obj->efile.data->d_size;
- } else if (!strcmp(name, BSS_SEC)) {
- if (obj->efile.bss)
- *size = obj->efile.bss->d_size;
- } else if (!strcmp(name, RODATA_SEC)) {
- if (obj->efile.rodata)
- *size = obj->efile.rodata->d_size;
- } else if (!strcmp(name, STRUCT_OPS_SEC)) {
- if (obj->efile.st_ops_data)
- *size = obj->efile.st_ops_data->d_size;
- } else {
- Elf_Scn *scn = elf_sec_by_name(obj, name);
- Elf_Data *data = elf_sec_data(obj, scn);
- if (data) {
- ret = 0; /* found it */
- *size = data->d_size;
- }
+ scn = elf_sec_by_name(obj, name);
+ data = elf_sec_data(obj, scn);
+ if (data) {
+ ret = 0; /* found it */
+ *size = data->d_size;
}
return *size ? 0 : ret;
@@ -1516,34 +1502,39 @@ bpf_object__init_internal_map(struct bpf_object *obj, enum libbpf_map_type type,
static int bpf_object__init_global_data_maps(struct bpf_object *obj)
{
- int err;
+ struct elf_sec_desc *sec_desc;
+ int err = 0, sec_idx;
/*
* Populate obj->maps with libbpf internal maps.
*/
- if (obj->efile.data_shndx >= 0) {
- err = bpf_object__init_internal_map(obj, LIBBPF_MAP_DATA,
- obj->efile.data_shndx,
- obj->efile.data->d_buf,
- obj->efile.data->d_size);
- if (err)
- return err;
- }
- if (obj->efile.rodata_shndx >= 0) {
- err = bpf_object__init_internal_map(obj, LIBBPF_MAP_RODATA,
- obj->efile.rodata_shndx,
- obj->efile.rodata->d_buf,
- obj->efile.rodata->d_size);
- if (err)
- return err;
-
- obj->rodata_map_idx = obj->nr_maps - 1;
- }
- if (obj->efile.bss_shndx >= 0) {
- err = bpf_object__init_internal_map(obj, LIBBPF_MAP_BSS,
- obj->efile.bss_shndx,
- NULL,
- obj->efile.bss->d_size);
+ for (sec_idx = 1; sec_idx < obj->efile.sec_cnt; sec_idx++) {
+ sec_desc = &obj->efile.secs[sec_idx];
+
+ switch (sec_desc->sec_type) {
+ case SEC_DATA:
+ err = bpf_object__init_internal_map(obj, LIBBPF_MAP_DATA,
+ sec_idx,
+ sec_desc->data->d_buf,
+ sec_desc->data->d_size);
+ break;
+ case SEC_RODATA:
+ obj->has_rodata = true;
+ err = bpf_object__init_internal_map(obj, LIBBPF_MAP_RODATA,
+ sec_idx,
+ sec_desc->data->d_buf,
+ sec_desc->data->d_size);
+ break;
+ case SEC_BSS:
+ err = bpf_object__init_internal_map(obj, LIBBPF_MAP_BSS,
+ sec_idx,
+ NULL,
+ sec_desc->data->d_size);
+ break;
+ default:
+ /* skip */
+ break;
+ }
if (err)
return err;
}
@@ -3123,6 +3114,7 @@ static int cmp_progs(const void *_a, const void *_b)
static int bpf_object__elf_collect(struct bpf_object *obj)
{
+ struct elf_sec_desc *sec_desc;
Elf *elf = obj->efile.elf;
Elf_Data *btf_ext_data = NULL;
Elf_Data *btf_data = NULL;
@@ -3132,6 +3124,15 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
Elf_Scn *scn;
Elf64_Shdr *sh;
+ /* ELF section indices are 1-based, so allocate +1 element to keep
+ * indexing simple. Also include 0th invalid section into sec_cnt for
+ * simpler and more traditional iteration logic.
+ */
+ obj->efile.sec_cnt = 1 + obj->efile.ehdr->e_shnum;
+ obj->efile.secs = calloc(obj->efile.sec_cnt, sizeof(*obj->efile.secs));
+ if (!obj->efile.secs)
+ return -ENOMEM;
+
/* a bunch of ELF parsing functionality depends on processing symbols,
* so do the first pass and find the symbol table
*/
@@ -3151,8 +3152,10 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
if (!data)
return -LIBBPF_ERRNO__FORMAT;
+ idx = elf_ndxscn(scn);
+
obj->efile.symbols = data;
- obj->efile.symbols_shndx = elf_ndxscn(scn);
+ obj->efile.symbols_shndx = idx;
obj->efile.strtabidx = sh->sh_link;
}
}
@@ -3165,7 +3168,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
scn = NULL;
while ((scn = elf_nextscn(elf, scn)) != NULL) {
- idx++;
+ idx = elf_ndxscn(scn);
+ sec_desc = &obj->efile.secs[idx];
sh = elf_sec_hdr(obj, scn);
if (!sh)
@@ -3213,11 +3217,13 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
if (err)
return err;
} else if (strcmp(name, DATA_SEC) == 0) {
- obj->efile.data = data;
- obj->efile.data_shndx = idx;
+ sec_desc->sec_type = SEC_DATA;
+ sec_desc->shdr = sh;
+ sec_desc->data = data;
} else if (strcmp(name, RODATA_SEC) == 0) {
- obj->efile.rodata = data;
- obj->efile.rodata_shndx = idx;
+ sec_desc->sec_type = SEC_RODATA;
+ sec_desc->shdr = sh;
+ sec_desc->data = data;
} else if (strcmp(name, STRUCT_OPS_SEC) == 0) {
obj->efile.st_ops_data = data;
obj->efile.st_ops_shndx = idx;
@@ -3226,33 +3232,25 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
idx, name);
}
} else if (sh->sh_type == SHT_REL) {
- int nr_sects = obj->efile.nr_reloc_sects;
- void *sects = obj->efile.reloc_sects;
- int sec = sh->sh_info; /* points to other section */
+ int targ_sec_idx = sh->sh_info; /* points to other section */
/* Only do relo for section with exec instructions */
- if (!section_have_execinstr(obj, sec) &&
+ if (!section_have_execinstr(obj, targ_sec_idx) &&
strcmp(name, ".rel" STRUCT_OPS_SEC) &&
strcmp(name, ".rel" MAPS_ELF_SEC)) {
pr_info("elf: skipping relo section(%d) %s for section(%d) %s\n",
- idx, name, sec,
- elf_sec_name(obj, elf_sec_by_idx(obj, sec)) ?: "<?>");
+ idx, name, targ_sec_idx,
+ elf_sec_name(obj, elf_sec_by_idx(obj, targ_sec_idx)) ?: "<?>");
continue;
}
- sects = libbpf_reallocarray(sects, nr_sects + 1,
- sizeof(*obj->efile.reloc_sects));
- if (!sects)
- return -ENOMEM;
-
- obj->efile.reloc_sects = sects;
- obj->efile.nr_reloc_sects++;
-
- obj->efile.reloc_sects[nr_sects].shdr = sh;
- obj->efile.reloc_sects[nr_sects].data = data;
+ sec_desc->sec_type = SEC_RELO;
+ sec_desc->shdr = sh;
+ sec_desc->data = data;
} else if (sh->sh_type == SHT_NOBITS && strcmp(name, BSS_SEC) == 0) {
- obj->efile.bss = data;
- obj->efile.bss_shndx = idx;
+ sec_desc->sec_type = SEC_BSS;
+ sec_desc->shdr = sh;
+ sec_desc->data = data;
} else {
pr_info("elf: skipping section(%d) %s (size %zu)\n", idx, name,
(size_t)sh->sh_size);
@@ -3732,9 +3730,14 @@ bpf_object__find_program_by_name(const struct bpf_object *obj,
static bool bpf_object__shndx_is_data(const struct bpf_object *obj,
int shndx)
{
- return shndx == obj->efile.data_shndx ||
- shndx == obj->efile.bss_shndx ||
- shndx == obj->efile.rodata_shndx;
+ switch (obj->efile.secs[shndx].sec_type) {
+ case SEC_BSS:
+ case SEC_DATA:
+ case SEC_RODATA:
+ return true;
+ default:
+ return false;
+ }
}
static bool bpf_object__shndx_is_maps(const struct bpf_object *obj,
@@ -3747,16 +3750,19 @@ static bool bpf_object__shndx_is_maps(const struct bpf_object *obj,
static enum libbpf_map_type
bpf_object__section_to_libbpf_map_type(const struct bpf_object *obj, int shndx)
{
- if (shndx == obj->efile.data_shndx)
- return LIBBPF_MAP_DATA;
- else if (shndx == obj->efile.bss_shndx)
+ if (shndx == obj->efile.symbols_shndx)
+ return LIBBPF_MAP_KCONFIG;
+
+ switch (obj->efile.secs[shndx].sec_type) {
+ case SEC_BSS:
return LIBBPF_MAP_BSS;
- else if (shndx == obj->efile.rodata_shndx)
+ case SEC_DATA:
+ return LIBBPF_MAP_DATA;
+ case SEC_RODATA:
return LIBBPF_MAP_RODATA;
- else if (shndx == obj->efile.symbols_shndx)
- return LIBBPF_MAP_KCONFIG;
- else
+ default:
return LIBBPF_MAP_UNSPEC;
+ }
}
static int bpf_program__record_reloc(struct bpf_program *prog,
@@ -3892,7 +3898,7 @@ static int bpf_program__record_reloc(struct bpf_program *prog,
}
for (map_idx = 0; map_idx < nr_maps; map_idx++) {
map = &obj->maps[map_idx];
- if (map->libbpf_type != type)
+ if (map->libbpf_type != type || map->sec_idx != sym->st_shndx)
continue;
pr_debug("prog '%s': found data map %zd (%s, sec %d, off %zu) for insn %u\n",
prog->name, map_idx, map->name, map->sec_idx,
@@ -6205,10 +6211,18 @@ static int bpf_object__collect_relos(struct bpf_object *obj)
{
int i, err;
- for (i = 0; i < obj->efile.nr_reloc_sects; i++) {
- Elf64_Shdr *shdr = obj->efile.reloc_sects[i].shdr;
- Elf_Data *data = obj->efile.reloc_sects[i].data;
- int idx = shdr->sh_info;
+ for (i = 0; i < obj->efile.sec_cnt; i++) {
+ struct elf_sec_desc *sec_desc = &obj->efile.secs[i];
+ Elf64_Shdr *shdr;
+ Elf_Data *data;
+ int idx;
+
+ if (sec_desc->sec_type != SEC_RELO)
+ continue;
+
+ shdr = sec_desc->shdr;
+ data = sec_desc->data;
+ idx = shdr->sh_info;
if (shdr->sh_type != SHT_REL) {
pr_warn("internal error at %d\n", __LINE__);
@@ -6331,6 +6345,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
char *license, __u32 kern_version, int *pfd)
{
struct bpf_prog_load_params load_attr = {};
+ struct bpf_object *obj = prog->obj;
char *cp, errmsg[STRERR_BUFSIZE];
size_t log_buf_size = 0;
char *log_buf = NULL;
@@ -6351,7 +6366,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
load_attr.prog_type = prog->type;
load_attr.expected_attach_type = prog->expected_attach_type;
- if (kernel_supports(prog->obj, FEAT_PROG_NAME))
+ if (kernel_supports(obj, FEAT_PROG_NAME))
load_attr.name = prog->name;
load_attr.insns = insns;
load_attr.insn_cnt = insns_cnt;
@@ -6364,8 +6379,8 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
load_attr.prog_ifindex = prog->prog_ifindex;
/* specify func_info/line_info only if kernel supports them */
- btf_fd = bpf_object__btf_fd(prog->obj);
- if (btf_fd >= 0 && kernel_supports(prog->obj, FEAT_BTF_FUNC)) {
+ btf_fd = bpf_object__btf_fd(obj);
+ if (btf_fd >= 0 && kernel_supports(obj, FEAT_BTF_FUNC)) {
load_attr.prog_btf_fd = btf_fd;
load_attr.func_info = prog->func_info;
load_attr.func_info_rec_size = prog->func_info_rec_size;
@@ -6376,7 +6391,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
}
load_attr.log_level = prog->log_level;
load_attr.prog_flags = prog->prog_flags;
- load_attr.fd_array = prog->obj->fd_array;
+ load_attr.fd_array = obj->fd_array;
/* adjust load_attr if sec_def provides custom preload callback */
if (prog->sec_def && prog->sec_def->preload_fn) {
@@ -6388,9 +6403,9 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
}
}
- if (prog->obj->gen_loader) {
- bpf_gen__prog_load(prog->obj->gen_loader, &load_attr,
- prog - prog->obj->programs);
+ if (obj->gen_loader) {
+ bpf_gen__prog_load(obj->gen_loader, &load_attr,
+ prog - obj->programs);
*pfd = -1;
return 0;
}
@@ -6411,16 +6426,21 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
if (log_buf && load_attr.log_level)
pr_debug("verifier log:\n%s", log_buf);
- if (prog->obj->rodata_map_idx >= 0 &&
- kernel_supports(prog->obj, FEAT_PROG_BIND_MAP)) {
- struct bpf_map *rodata_map =
- &prog->obj->maps[prog->obj->rodata_map_idx];
+ if (obj->has_rodata && kernel_supports(obj, FEAT_PROG_BIND_MAP)) {
+ struct bpf_map *map;
+ int i;
+
+ for (i = 0; i < obj->nr_maps; i++) {
+ map = &prog->obj->maps[i];
+ if (map->libbpf_type != LIBBPF_MAP_RODATA)
+ continue;
- if (bpf_prog_bind_map(ret, bpf_map__fd(rodata_map), NULL)) {
- cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
- pr_warn("prog '%s': failed to bind .rodata map: %s\n",
- prog->name, cp);
- /* Don't fail hard if can't bind rodata. */
+ if (bpf_prog_bind_map(ret, bpf_map__fd(map), NULL)) {
+ cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
+ pr_warn("prog '%s': failed to bind .rodata map: %s\n",
+ prog->name, cp);
+ /* Don't fail hard if can't bind rodata. */
+ }
}
}
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH bpf-next 04/10] libbpf: remove assumptions about uniqueness of .rodata/.data/.bss maps
2021-10-08 0:03 ` [PATCH bpf-next 04/10] libbpf: remove assumptions about uniqueness of .rodata/.data/.bss maps andrii.nakryiko
@ 2021-10-08 6:05 ` Song Liu
0 siblings, 0 replies; 42+ messages in thread
From: Song Liu @ 2021-10-08 6:05 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Kernel Team
On Thu, Oct 7, 2021 at 5:04 PM <andrii.nakryiko@gmail.com> wrote:
>
> From: Andrii Nakryiko <andrii@kernel.org>
>
> Remove internal libbpf assumption that there can be only one .rodata,
> .data, and .bss map per BPF object. To achieve that, extend and
> generalize the scheme that was used for keeping track of relocation ELF
> sections. Now each ELF section has a temporary extra index that keeps
> track of logical type of ELF section (relocations, data, read-only data,
> BSS). Switch relocation to this scheme, as well as .rodata/.data/.bss
> handling.
>
> We don't yet allow multiple .rodata, .data, and .bss sections, but no
> libbpf internal code makes an assumption that there can be only one of
> each and thus they can be explicitly referenced by a single index. Next
> patches will actually allow multiple .rodata and .data sections.
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Song Liu <songliubraving@fb.com>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH bpf-next 05/10] bpftool: support multiple .rodata/.data internal maps in skeleton
2021-10-08 0:02 [PATCH bpf-next 00/10] libbpf: support custom .rodata.*/.data.* sections andrii.nakryiko
` (3 preceding siblings ...)
2021-10-08 0:03 ` [PATCH bpf-next 04/10] libbpf: remove assumptions about uniqueness of .rodata/.data/.bss maps andrii.nakryiko
@ 2021-10-08 0:03 ` andrii.nakryiko
2021-10-08 6:05 ` Song Liu
2021-10-08 0:03 ` [PATCH bpf-next 06/10] bpftool: improve skeleton generation for data maps without DATASEC type andrii.nakryiko
` (6 subsequent siblings)
11 siblings, 1 reply; 42+ messages in thread
From: andrii.nakryiko @ 2021-10-08 0:03 UTC (permalink / raw)
To: bpf, ast, daniel; +Cc: andrii, kernel-team
From: Andrii Nakryiko <andrii@kernel.org>
Remove the assumption about only single instance of each of .rodata and
.data internal maps. Nothing changes for '.rodata' and '.data' maps, but new
'.rodata.something' map will get 'rodata_something' section in BPF
skeleton for them (as well as having struct bpf_map * field in maps
section with the same field name).
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/bpf/bpftool/gen.c | 107 ++++++++++++++++++++++------------------
1 file changed, 60 insertions(+), 47 deletions(-)
diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index cc835859465b..5fbd90bb0c09 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -34,6 +34,11 @@ static void sanitize_identifier(char *name)
name[i] = '_';
}
+static bool str_has_prefix(const char *str, const char *prefix)
+{
+ return strncmp(str, prefix, strlen(prefix)) == 0;
+}
+
static bool str_has_suffix(const char *str, const char *suffix)
{
size_t i, n1 = strlen(str), n2 = strlen(suffix);
@@ -68,23 +73,47 @@ static void get_header_guard(char *guard, const char *obj_name)
guard[i] = toupper(guard[i]);
}
-static const char *get_map_ident(const struct bpf_map *map)
+static bool get_map_ident(const struct bpf_map *map, char *buf, size_t buf_sz)
{
+ static const char *sfxs[] = { ".data", ".rodata", ".bss", ".kconfig" };
const char *name = bpf_map__name(map);
+ int i, n;
+
+ if (!bpf_map__is_internal(map)) {
+ snprintf(buf, buf_sz, "%s", name);
+ return true;
+ }
+
+ for (i = 0, n = ARRAY_SIZE(sfxs); i < n; i++) {
+ const char *sfx = sfxs[i], *p;
+
+ p = strstr(name, sfx);
+ if (p) {
+ snprintf(buf, buf_sz, "%s", p + 1);
+ sanitize_identifier(buf);
+ return true;
+ }
+ }
- if (!bpf_map__is_internal(map))
- return name;
-
- if (str_has_suffix(name, ".data"))
- return "data";
- else if (str_has_suffix(name, ".rodata"))
- return "rodata";
- else if (str_has_suffix(name, ".bss"))
- return "bss";
- else if (str_has_suffix(name, ".kconfig"))
- return "kconfig";
- else
- return NULL;
+ return false;
+}
+
+static bool get_datasec_ident(const char *sec_name, char *buf, size_t buf_sz)
+{
+ static const char *pfxs[] = { ".data", ".rodata", ".bss", ".kconfig" };
+ int i, n;
+
+ for (i = 0, n = ARRAY_SIZE(pfxs); i < n; i++) {
+ const char *pfx = pfxs[i];
+
+ if (str_has_prefix(sec_name, pfx)) {
+ snprintf(buf, buf_sz, "%s", sec_name + 1);
+ sanitize_identifier(buf);
+ return true;
+ }
+ }
+
+ return false;
}
static void codegen_btf_dump_printf(void *ctx, const char *fmt, va_list args)
@@ -101,24 +130,14 @@ static int codegen_datasec_def(struct bpf_object *obj,
const char *sec_name = btf__name_by_offset(btf, sec->name_off);
const struct btf_var_secinfo *sec_var = btf_var_secinfos(sec);
int i, err, off = 0, pad_cnt = 0, vlen = btf_vlen(sec);
- const char *sec_ident;
- char var_ident[256];
+ char var_ident[256], sec_ident[256];
bool strip_mods = false;
- if (strcmp(sec_name, ".data") == 0) {
- sec_ident = "data";
- strip_mods = true;
- } else if (strcmp(sec_name, ".bss") == 0) {
- sec_ident = "bss";
- strip_mods = true;
- } else if (strcmp(sec_name, ".rodata") == 0) {
- sec_ident = "rodata";
- strip_mods = true;
- } else if (strcmp(sec_name, ".kconfig") == 0) {
- sec_ident = "kconfig";
- } else {
+ if (!get_datasec_ident(sec_name, sec_ident, sizeof(sec_ident)))
return 0;
- }
+
+ if (strcmp(sec_name, ".kconfig") != 0)
+ strip_mods = true;
printf(" struct %s__%s {\n", obj_name, sec_ident);
for (i = 0; i < vlen; i++, sec_var++) {
@@ -386,6 +405,7 @@ static void codegen_destroy(struct bpf_object *obj, const char *obj_name)
{
struct bpf_program *prog;
struct bpf_map *map;
+ char ident[256];
codegen("\
\n\
@@ -406,10 +426,7 @@ static void codegen_destroy(struct bpf_object *obj, const char *obj_name)
}
bpf_object__for_each_map(map, obj) {
- const char *ident;
-
- ident = get_map_ident(map);
- if (!ident)
+ if (!get_map_ident(map, ident, sizeof(ident)))
continue;
if (bpf_map__is_internal(map) &&
(bpf_map__def(map)->map_flags & BPF_F_MMAPABLE))
@@ -433,6 +450,7 @@ static int gen_trace(struct bpf_object *obj, const char *obj_name, const char *h
struct bpf_object_load_attr load_attr = {};
DECLARE_LIBBPF_OPTS(gen_loader_opts, opts);
struct bpf_map *map;
+ char ident[256];
int err = 0;
err = bpf_object__gen_loader(obj, &opts);
@@ -478,12 +496,10 @@ static int gen_trace(struct bpf_object *obj, const char *obj_name, const char *h
",
obj_name, opts.data_sz);
bpf_object__for_each_map(map, obj) {
- const char *ident;
const void *mmap_data = NULL;
size_t mmap_size = 0;
- ident = get_map_ident(map);
- if (!ident)
+ if (!get_map_ident(map, ident, sizeof(ident)))
continue;
if (!bpf_map__is_internal(map) ||
@@ -545,15 +561,15 @@ static int gen_trace(struct bpf_object *obj, const char *obj_name, const char *h
return err; \n\
", obj_name);
bpf_object__for_each_map(map, obj) {
- const char *ident, *mmap_flags;
+ const char *mmap_flags;
- ident = get_map_ident(map);
- if (!ident)
+ if (!get_map_ident(map, ident, sizeof(ident)))
continue;
if (!bpf_map__is_internal(map) ||
!(bpf_map__def(map)->map_flags & BPF_F_MMAPABLE))
continue;
+
if (bpf_map__def(map)->map_flags & BPF_F_RDONLY_PROG)
mmap_flags = "PROT_READ";
else
@@ -603,7 +619,8 @@ static int do_skeleton(int argc, char **argv)
DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
char obj_name[MAX_OBJ_NAME_LEN] = "", *obj_data;
struct bpf_object *obj = NULL;
- const char *file, *ident;
+ const char *file;
+ char ident[256];
struct bpf_program *prog;
int fd, err = -1;
struct bpf_map *map;
@@ -674,8 +691,7 @@ static int do_skeleton(int argc, char **argv)
}
bpf_object__for_each_map(map, obj) {
- ident = get_map_ident(map);
- if (!ident) {
+ if (!get_map_ident(map, ident, sizeof(ident))) {
p_err("ignoring unrecognized internal map '%s'...",
bpf_map__name(map));
continue;
@@ -728,8 +744,7 @@ static int do_skeleton(int argc, char **argv)
if (map_cnt) {
printf("\tstruct {\n");
bpf_object__for_each_map(map, obj) {
- ident = get_map_ident(map);
- if (!ident)
+ if (!get_map_ident(map, ident, sizeof(ident)))
continue;
if (use_loader)
printf("\t\tstruct bpf_map_desc %s;\n", ident);
@@ -898,9 +913,7 @@ static int do_skeleton(int argc, char **argv)
);
i = 0;
bpf_object__for_each_map(map, obj) {
- ident = get_map_ident(map);
-
- if (!ident)
+ if (!get_map_ident(map, ident, sizeof(ident)))
continue;
codegen("\
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH bpf-next 05/10] bpftool: support multiple .rodata/.data internal maps in skeleton
2021-10-08 0:03 ` [PATCH bpf-next 05/10] bpftool: support multiple .rodata/.data internal maps in skeleton andrii.nakryiko
@ 2021-10-08 6:05 ` Song Liu
0 siblings, 0 replies; 42+ messages in thread
From: Song Liu @ 2021-10-08 6:05 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Kernel Team
On Thu, Oct 7, 2021 at 5:04 PM <andrii.nakryiko@gmail.com> wrote:
>
> From: Andrii Nakryiko <andrii@kernel.org>
>
> Remove the assumption about only single instance of each of .rodata and
> .data internal maps. Nothing changes for '.rodata' and '.data' maps, but new
> '.rodata.something' map will get 'rodata_something' section in BPF
> skeleton for them (as well as having struct bpf_map * field in maps
> section with the same field name).
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Song Liu <songliubraving@fb.com>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH bpf-next 06/10] bpftool: improve skeleton generation for data maps without DATASEC type
2021-10-08 0:02 [PATCH bpf-next 00/10] libbpf: support custom .rodata.*/.data.* sections andrii.nakryiko
` (4 preceding siblings ...)
2021-10-08 0:03 ` [PATCH bpf-next 05/10] bpftool: support multiple .rodata/.data internal maps in skeleton andrii.nakryiko
@ 2021-10-08 0:03 ` andrii.nakryiko
2021-10-08 17:15 ` Song Liu
2021-10-08 0:03 ` [PATCH bpf-next 07/10] libbpf: support multiple .rodata.* and .data.* BPF maps andrii.nakryiko
` (5 subsequent siblings)
11 siblings, 1 reply; 42+ messages in thread
From: andrii.nakryiko @ 2021-10-08 0:03 UTC (permalink / raw)
To: bpf, ast, daniel; +Cc: andrii, kernel-team
From: Andrii Nakryiko <andrii@kernel.org>
It can happen that some data sections (e.g., .rodata.cst16, containing
compiler populated string constants) won't have a corresponding BTF
DATASEC type. Now that libbpf supports .rodata.* and .data.* sections,
situation like that will cause invalid BPF skeleton to be generated that
won't compile successfully, as some parts of skeleton would assume
memory-mapped struct definitions for each special data section.
Fix this by generating empty struct definitions for such data sections.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/bpf/bpftool/gen.c | 51 ++++++++++++++++++++++++++++++++++++-----
1 file changed, 45 insertions(+), 6 deletions(-)
diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index 5fbd90bb0c09..889a5b1542c9 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -214,22 +214,61 @@ static int codegen_datasecs(struct bpf_object *obj, const char *obj_name)
struct btf *btf = bpf_object__btf(obj);
int n = btf__get_nr_types(btf);
struct btf_dump *d;
+ struct bpf_map *map;
+ const struct btf_type *sec;
+ char sec_ident[256], map_ident[256];
int i, err = 0;
d = btf_dump__new(btf, NULL, NULL, codegen_btf_dump_printf);
if (IS_ERR(d))
return PTR_ERR(d);
- for (i = 1; i <= n; i++) {
- const struct btf_type *t = btf__type_by_id(btf, i);
+ bpf_object__for_each_map(map, obj) {
+ /* only generate definitions for memory-mapped internal maps */
+ if (!bpf_map__is_internal(map))
+ continue;
+ if (!(bpf_map__def(map)->map_flags & BPF_F_MMAPABLE))
+ continue;
- if (!btf_is_datasec(t))
+ if (!get_map_ident(map, map_ident, sizeof(map_ident)))
continue;
- err = codegen_datasec_def(obj, btf, d, t, obj_name);
- if (err)
- goto out;
+ sec = NULL;
+ for (i = 1; i <= n; i++) {
+ const struct btf_type *t = btf__type_by_id(btf, i);
+ const char *name;
+
+ if (!btf_is_datasec(t))
+ continue;
+
+ name = btf__str_by_offset(btf, t->name_off);
+ if (!get_datasec_ident(name, sec_ident, sizeof(sec_ident)))
+ continue;
+
+ if (strcmp(sec_ident, map_ident) == 0) {
+ sec = t;
+ break;
+ }
+ }
+
+ /* In some cases (e.g., sections like .rodata.cst16 containing
+ * compiler allocated string constants only) there will be
+ * special internal maps with no corresponding DATASEC BTF
+ * type. In such case, generate empty structs for each such
+ * map. It will still be memory-mapped and its contents
+ * accessible from user-space through BPF skeleton.
+ */
+ if (!sec) {
+ printf(" struct %s__%s {\n", obj_name, map_ident);
+ printf(" } *%s;\n", map_ident);
+ } else {
+ err = codegen_datasec_def(obj, btf, d, sec, obj_name);
+ if (err)
+ goto out;
+ }
}
+
+
out:
btf_dump__free(d);
return err;
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH bpf-next 06/10] bpftool: improve skeleton generation for data maps without DATASEC type
2021-10-08 0:03 ` [PATCH bpf-next 06/10] bpftool: improve skeleton generation for data maps without DATASEC type andrii.nakryiko
@ 2021-10-08 17:15 ` Song Liu
0 siblings, 0 replies; 42+ messages in thread
From: Song Liu @ 2021-10-08 17:15 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Kernel Team
On Thu, Oct 7, 2021 at 5:04 PM <andrii.nakryiko@gmail.com> wrote:
>
> From: Andrii Nakryiko <andrii@kernel.org>
>
> It can happen that some data sections (e.g., .rodata.cst16, containing
> compiler populated string constants) won't have a corresponding BTF
> DATASEC type. Now that libbpf supports .rodata.* and .data.* sections,
> situation like that will cause invalid BPF skeleton to be generated that
> won't compile successfully, as some parts of skeleton would assume
> memory-mapped struct definitions for each special data section.
>
> Fix this by generating empty struct definitions for such data sections.
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Song Liu <songliubraving@fb.com>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH bpf-next 07/10] libbpf: support multiple .rodata.* and .data.* BPF maps
2021-10-08 0:02 [PATCH bpf-next 00/10] libbpf: support custom .rodata.*/.data.* sections andrii.nakryiko
` (5 preceding siblings ...)
2021-10-08 0:03 ` [PATCH bpf-next 06/10] bpftool: improve skeleton generation for data maps without DATASEC type andrii.nakryiko
@ 2021-10-08 0:03 ` andrii.nakryiko
2021-10-08 22:05 ` Song Liu
2021-10-08 0:03 ` [PATCH bpf-next 08/10] selftests/bpf: demonstrate use of custom .rodata/.data sections andrii.nakryiko
` (4 subsequent siblings)
11 siblings, 1 reply; 42+ messages in thread
From: andrii.nakryiko @ 2021-10-08 0:03 UTC (permalink / raw)
To: bpf, ast, daniel; +Cc: andrii, kernel-team
From: Andrii Nakryiko <andrii@kernel.org>
Add support for having multiple .rodata and .data data sections ([0]).
.rodata/.data are supported like the usual, but now also
.rodata.<whatever> and .data.<whatever> are also supported. Each such
section will get its own backing BPF_MAP_TYPE_ARRAY, just like
.rodata and .data.
Multiple .bss maps are not supported, as the whole '.bss' name is
confusing and might be deprecated soon, as well as user would need to
specify custom ELF section with SEC() attribute anyway, so might as well
stick to just .data.* and .rodata.* convention.
User-visible map name for such new maps is going to be just their ELF
section names. When creating the map in the kernel, libbpf will still
try to prepend portion of object name. This feature is up for debate and
I'm open to dropping that for new maps entirely.
[0] https://github.com/libbpf/libbpf/issues/274
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/lib/bpf/libbpf.c | 124 +++++++++++++++++++++++++++++++----------
1 file changed, 95 insertions(+), 29 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index bbfb847fd1ea..054549846025 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -370,15 +370,14 @@ enum libbpf_map_type {
LIBBPF_MAP_KCONFIG,
};
-static const char * const libbpf_type_to_btf_name[] = {
- [LIBBPF_MAP_DATA] = DATA_SEC,
- [LIBBPF_MAP_BSS] = BSS_SEC,
- [LIBBPF_MAP_RODATA] = RODATA_SEC,
- [LIBBPF_MAP_KCONFIG] = KCONFIG_SEC,
-};
-
struct bpf_map {
char *name;
+ /* real_name is defined for special internal maps (.rodata*,
+ * .data*, .bss, .kconfig) and preserves their original ELF section
+ * name. This is important to be be able to find corresponding BTF
+ * DATASEC information.
+ */
+ char *real_name;
int fd;
int sec_idx;
size_t sec_offset;
@@ -1429,17 +1428,49 @@ static size_t bpf_map_mmap_sz(const struct bpf_map *map)
return map_sz;
}
-static char *internal_map_name(struct bpf_object *obj,
- enum libbpf_map_type type)
+static char *internal_map_name(struct bpf_object *obj, const char *real_name)
{
char map_name[BPF_OBJ_NAME_LEN], *p;
- const char *sfx = libbpf_type_to_btf_name[type];
- int sfx_len = max((size_t)7, strlen(sfx));
- int pfx_len = min((size_t)BPF_OBJ_NAME_LEN - sfx_len - 1,
- strlen(obj->name));
+ int pfx_len, sfx_len = max((size_t)7, strlen(real_name));
+
+ /* This is one of the more confusing parts of libbpf for various
+ * reasons, some of which are historical. The original idea for naming
+ * internal names was to include as much of BPF object name prefix as
+ * possible, so that it can be distinguished from similar internal
+ * maps of a different BPF object.
+ * As an example, let's say we have bpf_object named 'my_object_name'
+ * and internal map corresponding to '.rodata' ELF section. The final
+ * map name advertised to user and to the kernel will be
+ * 'my_objec.rodata', taking first 8 characters of object name and
+ * entire 7 characters of '.rodata'.
+ * Somewhat confusingly, if internal map ELF section name is shorter
+ * than 7 characters, e.g., '.bss', we'll still "reserve" 7 characters
+ * for the suffix, even though we only have 4 actual characters, and
+ * resulting map will be called 'my_objec.bss', not even using all 15
+ * characters allowed by the kernel. Oh well, at least the truncated
+ * object name is somewhat consistent in this case. But if the map
+ * name is '.kconfig', we'll still have entirety of '.kconfig' added
+ * (8 chars) and thus will be left with only first 7 characters of the
+ * object name ('my_obje'). Happy guessing, user, that the final map
+ * name will be "my_obje.kconfig".
+ * Now, with libbpf starting to support arbitrarily named .rodata.*
+ * and .data.* data sections, it's possible that ELF section name is
+ * longer than allowed 15 chars, so we now need to be careful to take
+ * only up to 15 first characters of ELF name, taking no BPF object
+ * name characters at all. So '.rodata.abracadabra' will result in
+ * '.rodata.abracad' kernel and user-visible name.
+ * We need to keep this convoluted logic intact (at least for now) to
+ * ensure that BPF skeletons are interoperable between multiple
+ * versions of libbpf (e.g., if BPF skeleton was generated using
+ * bpftool with new libbpf, while user app was built with older
+ * libbpf), because BPF skeletons internally record full map names.
+ */
+ if (sfx_len >= BPF_OBJ_NAME_LEN)
+ sfx_len = BPF_OBJ_NAME_LEN - 1;
+ pfx_len = min((size_t)BPF_OBJ_NAME_LEN - sfx_len - 1, strlen(obj->name));
snprintf(map_name, sizeof(map_name), "%.*s%.*s", pfx_len, obj->name,
- sfx_len, libbpf_type_to_btf_name[type]);
+ sfx_len, real_name);
/* sanitise map name to characters allowed by kernel */
for (p = map_name; *p && p < map_name + sizeof(map_name); p++)
@@ -1451,7 +1482,7 @@ static char *internal_map_name(struct bpf_object *obj,
static int
bpf_object__init_internal_map(struct bpf_object *obj, enum libbpf_map_type type,
- int sec_idx, void *data, size_t data_sz)
+ const char *real_name, int sec_idx, void *data, size_t data_sz)
{
struct bpf_map_def *def;
struct bpf_map *map;
@@ -1464,9 +1495,11 @@ bpf_object__init_internal_map(struct bpf_object *obj, enum libbpf_map_type type,
map->libbpf_type = type;
map->sec_idx = sec_idx;
map->sec_offset = 0;
- map->name = internal_map_name(obj, type);
- if (!map->name) {
- pr_warn("failed to alloc map name\n");
+ map->real_name = strdup(real_name);
+ map->name = internal_map_name(obj, real_name);
+ if (!map->real_name || !map->name) {
+ zfree(&map->real_name);
+ zfree(&map->name);
return -ENOMEM;
}
@@ -1489,6 +1522,7 @@ bpf_object__init_internal_map(struct bpf_object *obj, enum libbpf_map_type type,
map->mmaped = NULL;
pr_warn("failed to alloc map '%s' content buffer: %d\n",
map->name, err);
+ zfree(&map->real_name);
zfree(&map->name);
return err;
}
@@ -1503,6 +1537,7 @@ bpf_object__init_internal_map(struct bpf_object *obj, enum libbpf_map_type type,
static int bpf_object__init_global_data_maps(struct bpf_object *obj)
{
struct elf_sec_desc *sec_desc;
+ const char *sec_name;
int err = 0, sec_idx;
/*
@@ -1513,21 +1548,24 @@ static int bpf_object__init_global_data_maps(struct bpf_object *obj)
switch (sec_desc->sec_type) {
case SEC_DATA:
+ sec_name = elf_sec_name(obj, elf_sec_by_idx(obj, sec_idx));
err = bpf_object__init_internal_map(obj, LIBBPF_MAP_DATA,
- sec_idx,
+ sec_name, sec_idx,
sec_desc->data->d_buf,
sec_desc->data->d_size);
break;
case SEC_RODATA:
obj->has_rodata = true;
+ sec_name = elf_sec_name(obj, elf_sec_by_idx(obj, sec_idx));
err = bpf_object__init_internal_map(obj, LIBBPF_MAP_RODATA,
- sec_idx,
+ sec_name, sec_idx,
sec_desc->data->d_buf,
sec_desc->data->d_size);
break;
case SEC_BSS:
+ sec_name = elf_sec_name(obj, elf_sec_by_idx(obj, sec_idx));
err = bpf_object__init_internal_map(obj, LIBBPF_MAP_BSS,
- sec_idx,
+ sec_name, sec_idx,
NULL,
sec_desc->data->d_size);
break;
@@ -1831,7 +1869,7 @@ static int bpf_object__init_kconfig_map(struct bpf_object *obj)
map_sz = last_ext->kcfg.data_off + last_ext->kcfg.sz;
err = bpf_object__init_internal_map(obj, LIBBPF_MAP_KCONFIG,
- obj->efile.symbols_shndx,
+ ".kconfig", obj->efile.symbols_shndx,
NULL, map_sz);
if (err)
return err;
@@ -1931,7 +1969,7 @@ static int bpf_object__init_user_maps(struct bpf_object *obj, bool strict)
map->name = strdup(map_name);
if (!map->name) {
- pr_warn("failed to alloc map name\n");
+ pr_warn("map '%s': failed to alloc map name\n", map_name);
return -ENOMEM;
}
pr_debug("map %d is \"%s\"\n", i, map->name);
@@ -3216,11 +3254,13 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
err = bpf_object__add_programs(obj, data, name, idx);
if (err)
return err;
- } else if (strcmp(name, DATA_SEC) == 0) {
+ } else if (strcmp(name, DATA_SEC) == 0 ||
+ str_has_pfx(name, DATA_SEC ".")) {
sec_desc->sec_type = SEC_DATA;
sec_desc->shdr = sh;
sec_desc->data = data;
- } else if (strcmp(name, RODATA_SEC) == 0) {
+ } else if (strcmp(name, RODATA_SEC) == 0 ||
+ str_has_pfx(name, RODATA_SEC ".")) {
sec_desc->sec_type = SEC_RODATA;
sec_desc->shdr = sh;
sec_desc->data = data;
@@ -4060,8 +4100,7 @@ static int bpf_map_find_btf_info(struct bpf_object *obj, struct bpf_map *map)
* LLVM annotates global data differently in BTF, that is,
* only as '.data', '.bss' or '.rodata'.
*/
- ret = btf__find_by_name(obj->btf,
- libbpf_type_to_btf_name[map->libbpf_type]);
+ ret = btf__find_by_name(obj->btf, map->real_name);
}
if (ret < 0)
return ret;
@@ -7831,6 +7870,7 @@ static void bpf_map__destroy(struct bpf_map *map)
}
zfree(&map->name);
+ zfree(&map->real_name);
zfree(&map->pin_path);
if (map->fd >= 0)
@@ -8755,9 +8795,30 @@ const struct bpf_map_def *bpf_map__def(const struct bpf_map *map)
return map ? &map->def : libbpf_err_ptr(-EINVAL);
}
+static bool map_uses_real_name(const struct bpf_map *map)
+{
+ /* Since libbpf started to support custom .data.* and .rodata.* maps,
+ * their user-visible name differs from kernel-visible name. Users see
+ * such map's corresponding ELF section name as a map name.
+ * This check distinguishes .data/.rodata from .data.* and .rodata.*
+ * maps to know which name has to be returned to the user.
+ */
+ if (map->libbpf_type == LIBBPF_MAP_DATA && strcmp(map->real_name, DATA_SEC) != 0)
+ return true;
+ if (map->libbpf_type == LIBBPF_MAP_RODATA && strcmp(map->real_name, RODATA_SEC) != 0)
+ return true;
+ return false;
+}
+
const char *bpf_map__name(const struct bpf_map *map)
{
- return map ? map->name : NULL;
+ if (!map)
+ return NULL;
+
+ if (map_uses_real_name(map))
+ return map->real_name;
+
+ return map->name;
}
enum bpf_map_type bpf_map__type(const struct bpf_map *map)
@@ -8976,7 +9037,12 @@ bpf_object__find_map_by_name(const struct bpf_object *obj, const char *name)
struct bpf_map *pos;
bpf_object__for_each_map(pos, obj) {
- if (pos->name && !strcmp(pos->name, name))
+ if (map_uses_real_name(pos)) {
+ if (strcmp(pos->real_name, name) == 0)
+ return pos;
+ continue;
+ }
+ if (strcmp(pos->name, name) == 0)
return pos;
}
return errno = ENOENT, NULL;
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH bpf-next 07/10] libbpf: support multiple .rodata.* and .data.* BPF maps
2021-10-08 0:03 ` [PATCH bpf-next 07/10] libbpf: support multiple .rodata.* and .data.* BPF maps andrii.nakryiko
@ 2021-10-08 22:05 ` Song Liu
0 siblings, 0 replies; 42+ messages in thread
From: Song Liu @ 2021-10-08 22:05 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Kernel Team
On Thu, Oct 7, 2021 at 5:05 PM <andrii.nakryiko@gmail.com> wrote:
>
> From: Andrii Nakryiko <andrii@kernel.org>
>
> Add support for having multiple .rodata and .data data sections ([0]).
> .rodata/.data are supported like the usual, but now also
> .rodata.<whatever> and .data.<whatever> are also supported. Each such
> section will get its own backing BPF_MAP_TYPE_ARRAY, just like
> .rodata and .data.
>
> Multiple .bss maps are not supported, as the whole '.bss' name is
> confusing and might be deprecated soon, as well as user would need to
> specify custom ELF section with SEC() attribute anyway, so might as well
> stick to just .data.* and .rodata.* convention.
>
> User-visible map name for such new maps is going to be just their ELF
> section names. When creating the map in the kernel, libbpf will still
> try to prepend portion of object name. This feature is up for debate and
> I'm open to dropping that for new maps entirely.
>
> [0] https://github.com/libbpf/libbpf/issues/274
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Song Liu <songliubraving@fb.com>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH bpf-next 08/10] selftests/bpf: demonstrate use of custom .rodata/.data sections
2021-10-08 0:02 [PATCH bpf-next 00/10] libbpf: support custom .rodata.*/.data.* sections andrii.nakryiko
` (6 preceding siblings ...)
2021-10-08 0:03 ` [PATCH bpf-next 07/10] libbpf: support multiple .rodata.* and .data.* BPF maps andrii.nakryiko
@ 2021-10-08 0:03 ` andrii.nakryiko
2021-10-08 22:07 ` Song Liu
2021-10-11 13:57 ` Daniel Borkmann
2021-10-08 0:03 ` [PATCH bpf-next 09/10] libbpf: simplify look up by name of internal maps andrii.nakryiko
` (3 subsequent siblings)
11 siblings, 2 replies; 42+ messages in thread
From: andrii.nakryiko @ 2021-10-08 0:03 UTC (permalink / raw)
To: bpf, ast, daniel; +Cc: andrii, kernel-team
From: Andrii Nakryiko <andrii@kernel.org>
Enhance existing selftests to demonstrate the use of custom
.data/.rodata sections.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
.../selftests/bpf/prog_tests/skeleton.c | 25 +++++++++++++++++++
.../selftests/bpf/progs/test_skeleton.c | 10 ++++++++
2 files changed, 35 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/skeleton.c b/tools/testing/selftests/bpf/prog_tests/skeleton.c
index fe1e204a65c6..f2713eeac076 100644
--- a/tools/testing/selftests/bpf/prog_tests/skeleton.c
+++ b/tools/testing/selftests/bpf/prog_tests/skeleton.c
@@ -16,10 +16,13 @@ void test_skeleton(void)
struct test_skeleton* skel;
struct test_skeleton__bss *bss;
struct test_skeleton__data *data;
+ struct test_skeleton__data_dyn *data_dyn;
struct test_skeleton__rodata *rodata;
+ struct test_skeleton__rodata_dyn *rodata_dyn;
struct test_skeleton__kconfig *kcfg;
const void *elf_bytes;
size_t elf_bytes_sz = 0;
+ int i;
skel = test_skeleton__open();
if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
@@ -30,7 +33,12 @@ void test_skeleton(void)
bss = skel->bss;
data = skel->data;
+ data_dyn = skel->data_dyn;
rodata = skel->rodata;
+ rodata_dyn = skel->rodata_dyn;
+
+ ASSERT_STREQ(bpf_map__name(skel->maps.rodata_dyn), ".rodata.dyn", "rodata_dyn_name");
+ ASSERT_STREQ(bpf_map__name(skel->maps.data_dyn), ".data.dyn", "data_dyn_name");
/* validate values are pre-initialized correctly */
CHECK(data->in1 != -1, "in1", "got %d != exp %d\n", data->in1, -1);
@@ -46,6 +54,12 @@ void test_skeleton(void)
CHECK(rodata->in.in6 != 0, "in6", "got %d != exp %d\n", rodata->in.in6, 0);
CHECK(bss->out6 != 0, "out6", "got %d != exp %d\n", bss->out6, 0);
+ ASSERT_EQ(rodata_dyn->in_dynarr_sz, 0, "in_dynarr_sz");
+ for (i = 0; i < 4; i++)
+ ASSERT_EQ(rodata_dyn->in_dynarr[i], -(i + 1), "in_dynarr");
+ for (i = 0; i < 4; i++)
+ ASSERT_EQ(data_dyn->out_dynarr[i], i + 1, "out_dynarr");
+
/* validate we can pre-setup global variables, even in .bss */
data->in1 = 10;
data->in2 = 11;
@@ -53,6 +67,10 @@ void test_skeleton(void)
bss->in4 = 13;
rodata->in.in6 = 14;
+ rodata_dyn->in_dynarr_sz = 4;
+ for (i = 0; i < 4; i++)
+ rodata_dyn->in_dynarr[i] = i + 10;
+
err = test_skeleton__load(skel);
if (CHECK(err, "skel_load", "failed to load skeleton: %d\n", err))
goto cleanup;
@@ -64,6 +82,10 @@ void test_skeleton(void)
CHECK(bss->in4 != 13, "in4", "got %lld != exp %lld\n", bss->in4, 13LL);
CHECK(rodata->in.in6 != 14, "in6", "got %d != exp %d\n", rodata->in.in6, 14);
+ ASSERT_EQ(rodata_dyn->in_dynarr_sz, 4, "in_dynarr_sz");
+ for (i = 0; i < 4; i++)
+ ASSERT_EQ(rodata_dyn->in_dynarr[i], i + 10, "in_dynarr");
+
/* now set new values and attach to get them into outX variables */
data->in1 = 1;
data->in2 = 2;
@@ -93,6 +115,9 @@ void test_skeleton(void)
CHECK(bss->kern_ver != kcfg->LINUX_KERNEL_VERSION, "ext2",
"got %d != exp %d\n", bss->kern_ver, kcfg->LINUX_KERNEL_VERSION);
+ for (i = 0; i < 4; i++)
+ ASSERT_EQ(data_dyn->out_dynarr[i], i + 10, "out_dynarr");
+
elf_bytes = test_skeleton__elf_bytes(&elf_bytes_sz);
ASSERT_OK_PTR(elf_bytes, "elf_bytes");
ASSERT_GE(elf_bytes_sz, 0, "elf_bytes_sz");
diff --git a/tools/testing/selftests/bpf/progs/test_skeleton.c b/tools/testing/selftests/bpf/progs/test_skeleton.c
index 441fa1c552c8..47a7e76866c4 100644
--- a/tools/testing/selftests/bpf/progs/test_skeleton.c
+++ b/tools/testing/selftests/bpf/progs/test_skeleton.c
@@ -40,9 +40,16 @@ int kern_ver = 0;
struct s out5 = {};
+const volatile int in_dynarr_sz SEC(".rodata.dyn");
+const volatile int in_dynarr[4] SEC(".rodata.dyn") = { -1, -2, -3, -4 };
+
+int out_dynarr[4] SEC(".data.dyn") = { 1, 2, 3, 4 };
+
SEC("raw_tp/sys_enter")
int handler(const void *ctx)
{
+ int i;
+
out1 = in1;
out2 = in2;
out3 = in3;
@@ -53,6 +60,9 @@ int handler(const void *ctx)
bpf_syscall = CONFIG_BPF_SYSCALL;
kern_ver = LINUX_KERNEL_VERSION;
+ for (i = 0; i < in_dynarr_sz; i++)
+ out_dynarr[i] = in_dynarr[i];
+
return 0;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH bpf-next 08/10] selftests/bpf: demonstrate use of custom .rodata/.data sections
2021-10-08 0:03 ` [PATCH bpf-next 08/10] selftests/bpf: demonstrate use of custom .rodata/.data sections andrii.nakryiko
@ 2021-10-08 22:07 ` Song Liu
2021-10-11 13:57 ` Daniel Borkmann
1 sibling, 0 replies; 42+ messages in thread
From: Song Liu @ 2021-10-08 22:07 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Kernel Team
On Thu, Oct 7, 2021 at 5:04 PM <andrii.nakryiko@gmail.com> wrote:
>
> From: Andrii Nakryiko <andrii@kernel.org>
>
> Enhance existing selftests to demonstrate the use of custom
> .data/.rodata sections.
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Song Liu <songliubraving@fb.com>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH bpf-next 08/10] selftests/bpf: demonstrate use of custom .rodata/.data sections
2021-10-08 0:03 ` [PATCH bpf-next 08/10] selftests/bpf: demonstrate use of custom .rodata/.data sections andrii.nakryiko
2021-10-08 22:07 ` Song Liu
@ 2021-10-11 13:57 ` Daniel Borkmann
2021-10-12 3:47 ` Andrii Nakryiko
1 sibling, 1 reply; 42+ messages in thread
From: Daniel Borkmann @ 2021-10-11 13:57 UTC (permalink / raw)
To: andrii.nakryiko, bpf, ast; +Cc: andrii, kernel-team
On 10/8/21 2:03 AM, andrii.nakryiko@gmail.com wrote:
> From: Andrii Nakryiko <andrii@kernel.org>
>
> Enhance existing selftests to demonstrate the use of custom
> .data/.rodata sections.
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Just a thought, but wouldn't the actual demo / use case be better to show that we can
now have a __read_mostly attribute which implies SEC(".data.read_mostly") section?
Would be nice to add a ...
#define __read_mostly SEC(".data.read_mostly")
... into tools/lib/bpf/bpf_helpers.h along with the series for use out of BPF programs
as I think this should be a rather common use case. Thoughts?
> ---
> .../selftests/bpf/prog_tests/skeleton.c | 25 +++++++++++++++++++
> .../selftests/bpf/progs/test_skeleton.c | 10 ++++++++
> 2 files changed, 35 insertions(+)
[...]
> diff --git a/tools/testing/selftests/bpf/progs/test_skeleton.c b/tools/testing/selftests/bpf/progs/test_skeleton.c
> index 441fa1c552c8..47a7e76866c4 100644
> --- a/tools/testing/selftests/bpf/progs/test_skeleton.c
> +++ b/tools/testing/selftests/bpf/progs/test_skeleton.c
> @@ -40,9 +40,16 @@ int kern_ver = 0;
>
> struct s out5 = {};
>
> +const volatile int in_dynarr_sz SEC(".rodata.dyn");
> +const volatile int in_dynarr[4] SEC(".rodata.dyn") = { -1, -2, -3, -4 };
> +
> +int out_dynarr[4] SEC(".data.dyn") = { 1, 2, 3, 4 };
> +
> SEC("raw_tp/sys_enter")
> int handler(const void *ctx)
> {
> + int i;
> +
> out1 = in1;
> out2 = in2;
> out3 = in3;
> @@ -53,6 +60,9 @@ int handler(const void *ctx)
> bpf_syscall = CONFIG_BPF_SYSCALL;
> kern_ver = LINUX_KERNEL_VERSION;
>
> + for (i = 0; i < in_dynarr_sz; i++)
> + out_dynarr[i] = in_dynarr[i];
> +
> return 0;
> }
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH bpf-next 08/10] selftests/bpf: demonstrate use of custom .rodata/.data sections
2021-10-11 13:57 ` Daniel Borkmann
@ 2021-10-12 3:47 ` Andrii Nakryiko
2021-10-12 14:54 ` Daniel Borkmann
0 siblings, 1 reply; 42+ messages in thread
From: Andrii Nakryiko @ 2021-10-12 3:47 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Kernel Team
On Mon, Oct 11, 2021 at 3:57 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 10/8/21 2:03 AM, andrii.nakryiko@gmail.com wrote:
> > From: Andrii Nakryiko <andrii@kernel.org>
> >
> > Enhance existing selftests to demonstrate the use of custom
> > .data/.rodata sections.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>
> Just a thought, but wouldn't the actual demo / use case be better to show that we can
> now have a __read_mostly attribute which implies SEC(".data.read_mostly") section?
>
> Would be nice to add a ...
>
> #define __read_mostly SEC(".data.read_mostly")
>
> ... into tools/lib/bpf/bpf_helpers.h along with the series for use out of BPF programs
> as I think this should be a rather common use case. Thoughts?
But what's so special about the ".data.read_mostly" ELF section for
BPF programs? It's just another data section with no extra semantics.
So unclear why we need to have a dedicated #define for that?..
>
> > ---
> > .../selftests/bpf/prog_tests/skeleton.c | 25 +++++++++++++++++++
> > .../selftests/bpf/progs/test_skeleton.c | 10 ++++++++
> > 2 files changed, 35 insertions(+)
> [...]
> > diff --git a/tools/testing/selftests/bpf/progs/test_skeleton.c b/tools/testing/selftests/bpf/progs/test_skeleton.c
> > index 441fa1c552c8..47a7e76866c4 100644
> > --- a/tools/testing/selftests/bpf/progs/test_skeleton.c
> > +++ b/tools/testing/selftests/bpf/progs/test_skeleton.c
> > @@ -40,9 +40,16 @@ int kern_ver = 0;
> >
> > struct s out5 = {};
> >
> > +const volatile int in_dynarr_sz SEC(".rodata.dyn");
> > +const volatile int in_dynarr[4] SEC(".rodata.dyn") = { -1, -2, -3, -4 };
> > +
> > +int out_dynarr[4] SEC(".data.dyn") = { 1, 2, 3, 4 };
> > +
> > SEC("raw_tp/sys_enter")
> > int handler(const void *ctx)
> > {
> > + int i;
> > +
> > out1 = in1;
> > out2 = in2;
> > out3 = in3;
> > @@ -53,6 +60,9 @@ int handler(const void *ctx)
> > bpf_syscall = CONFIG_BPF_SYSCALL;
> > kern_ver = LINUX_KERNEL_VERSION;
> >
> > + for (i = 0; i < in_dynarr_sz; i++)
> > + out_dynarr[i] = in_dynarr[i];
> > +
> > return 0;
> > }
> >
> >
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH bpf-next 08/10] selftests/bpf: demonstrate use of custom .rodata/.data sections
2021-10-12 3:47 ` Andrii Nakryiko
@ 2021-10-12 14:54 ` Daniel Borkmann
2021-10-20 19:02 ` Andrii Nakryiko
0 siblings, 1 reply; 42+ messages in thread
From: Daniel Borkmann @ 2021-10-12 14:54 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Kernel Team
On 10/12/21 5:47 AM, Andrii Nakryiko wrote:
> On Mon, Oct 11, 2021 at 3:57 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 10/8/21 2:03 AM, andrii.nakryiko@gmail.com wrote:
>>> From: Andrii Nakryiko <andrii@kernel.org>
>>>
>>> Enhance existing selftests to demonstrate the use of custom
>>> .data/.rodata sections.
>>>
>>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>>
>> Just a thought, but wouldn't the actual demo / use case be better to show that we can
>> now have a __read_mostly attribute which implies SEC(".data.read_mostly") section?
>>
>> Would be nice to add a ...
>>
>> #define __read_mostly SEC(".data.read_mostly")
>>
>> ... into tools/lib/bpf/bpf_helpers.h along with the series for use out of BPF programs
>> as I think this should be a rather common use case. Thoughts?
>
> But what's so special about the ".data.read_mostly" ELF section for
> BPF programs? It's just another data section with no extra semantics.
> So unclear why we need to have a dedicated #define for that?..
I mean semantics are implicit that only vars would be located there which are
by far more read than written to. Placing into separate .data.read_mostly would
help to reduce cache misses due to false sharing e.g. if they are otherwise placed
near vars which are written more often (silly example would be some counter in
the prog).
>>> ---
>>> .../selftests/bpf/prog_tests/skeleton.c | 25 +++++++++++++++++++
>>> .../selftests/bpf/progs/test_skeleton.c | 10 ++++++++
>>> 2 files changed, 35 insertions(+)
>> [...]
>>> diff --git a/tools/testing/selftests/bpf/progs/test_skeleton.c b/tools/testing/selftests/bpf/progs/test_skeleton.c
>>> index 441fa1c552c8..47a7e76866c4 100644
>>> --- a/tools/testing/selftests/bpf/progs/test_skeleton.c
>>> +++ b/tools/testing/selftests/bpf/progs/test_skeleton.c
>>> @@ -40,9 +40,16 @@ int kern_ver = 0;
>>>
>>> struct s out5 = {};
>>>
>>> +const volatile int in_dynarr_sz SEC(".rodata.dyn");
>>> +const volatile int in_dynarr[4] SEC(".rodata.dyn") = { -1, -2, -3, -4 };
>>> +
>>> +int out_dynarr[4] SEC(".data.dyn") = { 1, 2, 3, 4 };
>>> +
>>> SEC("raw_tp/sys_enter")
>>> int handler(const void *ctx)
>>> {
>>> + int i;
>>> +
>>> out1 = in1;
>>> out2 = in2;
>>> out3 = in3;
>>> @@ -53,6 +60,9 @@ int handler(const void *ctx)
>>> bpf_syscall = CONFIG_BPF_SYSCALL;
>>> kern_ver = LINUX_KERNEL_VERSION;
>>>
>>> + for (i = 0; i < in_dynarr_sz; i++)
>>> + out_dynarr[i] = in_dynarr[i];
>>> +
>>> return 0;
>>> }
>>>
>>>
>>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH bpf-next 08/10] selftests/bpf: demonstrate use of custom .rodata/.data sections
2021-10-12 14:54 ` Daniel Borkmann
@ 2021-10-20 19:02 ` Andrii Nakryiko
0 siblings, 0 replies; 42+ messages in thread
From: Andrii Nakryiko @ 2021-10-20 19:02 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Kernel Team
On Tue, Oct 12, 2021 at 7:54 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 10/12/21 5:47 AM, Andrii Nakryiko wrote:
> > On Mon, Oct 11, 2021 at 3:57 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >> On 10/8/21 2:03 AM, andrii.nakryiko@gmail.com wrote:
> >>> From: Andrii Nakryiko <andrii@kernel.org>
> >>>
> >>> Enhance existing selftests to demonstrate the use of custom
> >>> .data/.rodata sections.
> >>>
> >>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> >>
> >> Just a thought, but wouldn't the actual demo / use case be better to show that we can
> >> now have a __read_mostly attribute which implies SEC(".data.read_mostly") section?
> >>
> >> Would be nice to add a ...
> >>
> >> #define __read_mostly SEC(".data.read_mostly")
> >>
> >> ... into tools/lib/bpf/bpf_helpers.h along with the series for use out of BPF programs
> >> as I think this should be a rather common use case. Thoughts?
> >
> > But what's so special about the ".data.read_mostly" ELF section for
> > BPF programs? It's just another data section with no extra semantics.
> > So unclear why we need to have a dedicated #define for that?..
>
> I mean semantics are implicit that only vars would be located there which are
> by far more read than written to. Placing into separate .data.read_mostly would
> help to reduce cache misses due to false sharing e.g. if they are otherwise placed
> near vars which are written more often (silly example would be some counter in
> the prog).
We've discussed this offline. We concluded that it's a bit premature
to add #define __read_mostly into bpf_helpers.h, but I'll use
__read_mostly as part of a selftests to demonstrate this concept.
>
> >>> ---
> >>> .../selftests/bpf/prog_tests/skeleton.c | 25 +++++++++++++++++++
> >>> .../selftests/bpf/progs/test_skeleton.c | 10 ++++++++
> >>> 2 files changed, 35 insertions(+)
> >> [...]
> >>> diff --git a/tools/testing/selftests/bpf/progs/test_skeleton.c b/tools/testing/selftests/bpf/progs/test_skeleton.c
> >>> index 441fa1c552c8..47a7e76866c4 100644
> >>> --- a/tools/testing/selftests/bpf/progs/test_skeleton.c
> >>> +++ b/tools/testing/selftests/bpf/progs/test_skeleton.c
> >>> @@ -40,9 +40,16 @@ int kern_ver = 0;
> >>>
> >>> struct s out5 = {};
> >>>
> >>> +const volatile int in_dynarr_sz SEC(".rodata.dyn");
> >>> +const volatile int in_dynarr[4] SEC(".rodata.dyn") = { -1, -2, -3, -4 };
> >>> +
> >>> +int out_dynarr[4] SEC(".data.dyn") = { 1, 2, 3, 4 };
> >>> +
> >>> SEC("raw_tp/sys_enter")
> >>> int handler(const void *ctx)
> >>> {
> >>> + int i;
> >>> +
> >>> out1 = in1;
> >>> out2 = in2;
> >>> out3 = in3;
> >>> @@ -53,6 +60,9 @@ int handler(const void *ctx)
> >>> bpf_syscall = CONFIG_BPF_SYSCALL;
> >>> kern_ver = LINUX_KERNEL_VERSION;
> >>>
> >>> + for (i = 0; i < in_dynarr_sz; i++)
> >>> + out_dynarr[i] = in_dynarr[i];
> >>> +
> >>> return 0;
> >>> }
> >>>
> >>>
> >>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH bpf-next 09/10] libbpf: simplify look up by name of internal maps
2021-10-08 0:02 [PATCH bpf-next 00/10] libbpf: support custom .rodata.*/.data.* sections andrii.nakryiko
` (7 preceding siblings ...)
2021-10-08 0:03 ` [PATCH bpf-next 08/10] selftests/bpf: demonstrate use of custom .rodata/.data sections andrii.nakryiko
@ 2021-10-08 0:03 ` andrii.nakryiko
2021-10-08 17:30 ` Toke Høiland-Jørgensen
2021-10-08 22:16 ` Song Liu
2021-10-08 0:03 ` [PATCH bpf-next 10/10] selftests/bpf: switch to ".bss"/".rodata"/".data" lookups for " andrii.nakryiko
` (2 subsequent siblings)
11 siblings, 2 replies; 42+ messages in thread
From: andrii.nakryiko @ 2021-10-08 0:03 UTC (permalink / raw)
To: bpf, ast, daniel
Cc: andrii, kernel-team, Toke Høiland-Jørgensen,
Stanislav Fomichev
From: Andrii Nakryiko <andrii@kernel.org>
Map name that's assigned to internal maps (.rodata, .data, .bss, etc)
consist of a small prefix of bpf_object's name and ELF section name as
a suffix. This makes it hard for users to "guess" the name to use for
looking up by name with bpf_object__find_map_by_name() API.
One proposal was to drop object name prefix from the map name and just
use ".rodata", ".data", etc, names. One downside called out was that
when multiple BPF applications are active on the host, it will be hard
to distinguish between multiple instances of .rodata and know which BPF
object (app) they belong to. Having few first characters, while quite
limiting, still can give a bit of a clue, in general.
Another downside of such approach is that it is not backwards compatible
and, among direct use of bpf_object__find_map_by_name() API, will break
any BPF skeleton generated using bpftool that was compiled with older
libbpf version.
Instead of causing all this pain, libbpf will still generate map name
using a combination of object name and ELF section name, but it will
allow looking such maps up by their natural names, which correspond to
their respective ELF section names. This means non-truncated ELF section
names longer than 15 characters are going to be expected and supported.
With such set up, we get the best of both worlds: leave small bits of
a clue about BPF application that instantiated such maps, as well as
making it easy for user apps to lookup such maps at runtime. In this
sense it closes corresponding libbpf 1.0 issue ([0]).
BPF skeletons will continue using full names for lookups.
[0] Closes: https://github.com/libbpf/libbpf/issues/275
Cc: Toke Høiland-Jørgensen <toke@redhat.com>
Cc: Stanislav Fomichev <sdf@google.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/lib/bpf/libbpf.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 054549846025..01ebdb8a36d1 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -9037,6 +9037,16 @@ bpf_object__find_map_by_name(const struct bpf_object *obj, const char *name)
struct bpf_map *pos;
bpf_object__for_each_map(pos, obj) {
+ /* if it's a special internal map name (which always starts
+ * with dot) then check if that special name matches the
+ * real map name (ELF section name)
+ */
+ if (name[0] == '.') {
+ if (pos->real_name && strcmp(pos->real_name, name) == 0)
+ return pos;
+ continue;
+ }
+ /* otherwise map name has to be an exact match */
if (map_uses_real_name(pos)) {
if (strcmp(pos->real_name, name) == 0)
return pos;
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH bpf-next 09/10] libbpf: simplify look up by name of internal maps
2021-10-08 0:03 ` [PATCH bpf-next 09/10] libbpf: simplify look up by name of internal maps andrii.nakryiko
@ 2021-10-08 17:30 ` Toke Høiland-Jørgensen
2021-10-08 18:21 ` Andrii Nakryiko
2021-10-08 22:16 ` Song Liu
1 sibling, 1 reply; 42+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-10-08 17:30 UTC (permalink / raw)
To: andrii.nakryiko, bpf, ast, daniel; +Cc: andrii, kernel-team, Stanislav Fomichev
andrii.nakryiko@gmail.com writes:
> From: Andrii Nakryiko <andrii@kernel.org>
>
> Map name that's assigned to internal maps (.rodata, .data, .bss, etc)
> consist of a small prefix of bpf_object's name and ELF section name as
> a suffix. This makes it hard for users to "guess" the name to use for
> looking up by name with bpf_object__find_map_by_name() API.
>
> One proposal was to drop object name prefix from the map name and just
> use ".rodata", ".data", etc, names. One downside called out was that
> when multiple BPF applications are active on the host, it will be hard
> to distinguish between multiple instances of .rodata and know which BPF
> object (app) they belong to. Having few first characters, while quite
> limiting, still can give a bit of a clue, in general.
>
> Another downside of such approach is that it is not backwards compatible
> and, among direct use of bpf_object__find_map_by_name() API, will break
> any BPF skeleton generated using bpftool that was compiled with older
> libbpf version.
>
> Instead of causing all this pain, libbpf will still generate map name
> using a combination of object name and ELF section name, but it will
> allow looking such maps up by their natural names, which correspond to
> their respective ELF section names. This means non-truncated ELF section
> names longer than 15 characters are going to be expected and supported.
>
> With such set up, we get the best of both worlds: leave small bits of
> a clue about BPF application that instantiated such maps, as well as
> making it easy for user apps to lookup such maps at runtime. In this
> sense it closes corresponding libbpf 1.0 issue ([0]).
I like this approach. Only possible problem I can see is that it might
be confusing that a map can be looked up with one name, but that it
disappears once it's loaded into the kernel (and the BPF object is
closed).
Hmm, couldn't we just extend the kernel to accept longer names? Kinda
like with the netdev name aliases: support a secondary label that can be
longer, and have bpftool display both?
-Toke
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH bpf-next 09/10] libbpf: simplify look up by name of internal maps
2021-10-08 17:30 ` Toke Høiland-Jørgensen
@ 2021-10-08 18:21 ` Andrii Nakryiko
2021-10-08 21:44 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 42+ messages in thread
From: Andrii Nakryiko @ 2021-10-08 18:21 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Kernel Team, Stanislav Fomichev
On Fri, Oct 8, 2021 at 10:31 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> andrii.nakryiko@gmail.com writes:
>
> > From: Andrii Nakryiko <andrii@kernel.org>
> >
> > Map name that's assigned to internal maps (.rodata, .data, .bss, etc)
> > consist of a small prefix of bpf_object's name and ELF section name as
> > a suffix. This makes it hard for users to "guess" the name to use for
> > looking up by name with bpf_object__find_map_by_name() API.
> >
> > One proposal was to drop object name prefix from the map name and just
> > use ".rodata", ".data", etc, names. One downside called out was that
> > when multiple BPF applications are active on the host, it will be hard
> > to distinguish between multiple instances of .rodata and know which BPF
> > object (app) they belong to. Having few first characters, while quite
> > limiting, still can give a bit of a clue, in general.
> >
> > Another downside of such approach is that it is not backwards compatible
> > and, among direct use of bpf_object__find_map_by_name() API, will break
> > any BPF skeleton generated using bpftool that was compiled with older
> > libbpf version.
> >
> > Instead of causing all this pain, libbpf will still generate map name
> > using a combination of object name and ELF section name, but it will
> > allow looking such maps up by their natural names, which correspond to
> > their respective ELF section names. This means non-truncated ELF section
> > names longer than 15 characters are going to be expected and supported.
> >
> > With such set up, we get the best of both worlds: leave small bits of
> > a clue about BPF application that instantiated such maps, as well as
> > making it easy for user apps to lookup such maps at runtime. In this
> > sense it closes corresponding libbpf 1.0 issue ([0]).
>
> I like this approach. Only possible problem I can see is that it might
> be confusing that a map can be looked up with one name, but that it
> disappears once it's loaded into the kernel (and the BPF object is
> closed).
>
> Hmm, couldn't we just extend the kernel to accept longer names? Kinda
> like with the netdev name aliases: support a secondary label that can be
> longer, and have bpftool display both?
Yes, this discrepancy can be confusing. I'd like all those internal
maps to be named after their corresponding ELF sections, tbh. We have
a mechanism now to make this transition (libbpf_set_strict_mode()),
but people have complained before that just seeing ".data" won't give
them enough information.
But if we are going to extend the kernel with longer map names, then
I'd rather stick to clean ".data.custom" naming from the very
beginning, and then switch all existing .data/.rodata/.bss/.kconfig
map naming to the same convention as well (guarded by opt-in flag in
libbpf_set_strict_mode() until libbpf 1.0). In the kernel, though,
instead of having two names (i.e., one is alias), I'd just allow to
provide one long name and then all existing UAPIs that have char[16]
everywhere would just be a potentially truncated prefix of such a
longer name. All the tooling can be updated to use long name when
available, of course. WDYT?
>
> -Toke
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH bpf-next 09/10] libbpf: simplify look up by name of internal maps
2021-10-08 18:21 ` Andrii Nakryiko
@ 2021-10-08 21:44 ` Toke Høiland-Jørgensen
2021-10-11 21:24 ` Alexei Starovoitov
0 siblings, 1 reply; 42+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-10-08 21:44 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Kernel Team, Stanislav Fomichev
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> On Fri, Oct 8, 2021 at 10:31 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> andrii.nakryiko@gmail.com writes:
>>
>> > From: Andrii Nakryiko <andrii@kernel.org>
>> >
>> > Map name that's assigned to internal maps (.rodata, .data, .bss, etc)
>> > consist of a small prefix of bpf_object's name and ELF section name as
>> > a suffix. This makes it hard for users to "guess" the name to use for
>> > looking up by name with bpf_object__find_map_by_name() API.
>> >
>> > One proposal was to drop object name prefix from the map name and just
>> > use ".rodata", ".data", etc, names. One downside called out was that
>> > when multiple BPF applications are active on the host, it will be hard
>> > to distinguish between multiple instances of .rodata and know which BPF
>> > object (app) they belong to. Having few first characters, while quite
>> > limiting, still can give a bit of a clue, in general.
>> >
>> > Another downside of such approach is that it is not backwards compatible
>> > and, among direct use of bpf_object__find_map_by_name() API, will break
>> > any BPF skeleton generated using bpftool that was compiled with older
>> > libbpf version.
>> >
>> > Instead of causing all this pain, libbpf will still generate map name
>> > using a combination of object name and ELF section name, but it will
>> > allow looking such maps up by their natural names, which correspond to
>> > their respective ELF section names. This means non-truncated ELF section
>> > names longer than 15 characters are going to be expected and supported.
>> >
>> > With such set up, we get the best of both worlds: leave small bits of
>> > a clue about BPF application that instantiated such maps, as well as
>> > making it easy for user apps to lookup such maps at runtime. In this
>> > sense it closes corresponding libbpf 1.0 issue ([0]).
>>
>> I like this approach. Only possible problem I can see is that it might
>> be confusing that a map can be looked up with one name, but that it
>> disappears once it's loaded into the kernel (and the BPF object is
>> closed).
>>
>> Hmm, couldn't we just extend the kernel to accept longer names? Kinda
>> like with the netdev name aliases: support a secondary label that can be
>> longer, and have bpftool display both?
>
> Yes, this discrepancy can be confusing. I'd like all those internal
> maps to be named after their corresponding ELF sections, tbh. We have
> a mechanism now to make this transition (libbpf_set_strict_mode()),
> but people have complained before that just seeing ".data" won't give
> them enough information.
Yeah, I do also sympathise with that complaint :)
> But if we are going to extend the kernel with longer map names, then
> I'd rather stick to clean ".data.custom" naming from the very
> beginning, and then switch all existing .data/.rodata/.bss/.kconfig
> map naming to the same convention as well (guarded by opt-in flag in
> libbpf_set_strict_mode() until libbpf 1.0). In the kernel, though,
> instead of having two names (i.e., one is alias), I'd just allow to
> provide one long name and then all existing UAPIs that have char[16]
> everywhere would just be a potentially truncated prefix of such a
> longer name. All the tooling can be updated to use long name when
> available, of course. WDYT?
Hmm, so introduce a new 'map_name_long' field, and on query the kernel
will fill in the old map_name with a truncated version, and put the full
name in the new field? Yeah, I guess that would work too!
-Toke
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH bpf-next 09/10] libbpf: simplify look up by name of internal maps
2021-10-08 21:44 ` Toke Høiland-Jørgensen
@ 2021-10-11 21:24 ` Alexei Starovoitov
2021-10-12 3:45 ` Andrii Nakryiko
0 siblings, 1 reply; 42+ messages in thread
From: Alexei Starovoitov @ 2021-10-11 21:24 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Kernel Team, Stanislav Fomichev
On 10/8/21 2:44 PM, Toke Høiland-Jørgensen wrote:
>
> Hmm, so introduce a new 'map_name_long' field, and on query the kernel
> will fill in the old map_name with a truncated version, and put the full
> name in the new field? Yeah, I guess that would work too!
Let's start storing full map names in BTF instead.
Like we do already for progs.
Some tools already fetch full prog names this way.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH bpf-next 09/10] libbpf: simplify look up by name of internal maps
2021-10-11 21:24 ` Alexei Starovoitov
@ 2021-10-12 3:45 ` Andrii Nakryiko
2021-10-12 15:29 ` Stanislav Fomichev
0 siblings, 1 reply; 42+ messages in thread
From: Andrii Nakryiko @ 2021-10-12 3:45 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Toke Høiland-Jørgensen, bpf, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Kernel Team,
Stanislav Fomichev
On Mon, Oct 11, 2021 at 11:24 PM Alexei Starovoitov <ast@fb.com> wrote:
>
> On 10/8/21 2:44 PM, Toke Høiland-Jørgensen wrote:
> >
> > Hmm, so introduce a new 'map_name_long' field, and on query the kernel
> > will fill in the old map_name with a truncated version, and put the full
> > name in the new field? Yeah, I guess that would work too!
>
> Let's start storing full map names in BTF instead.
> Like we do already for progs.
> Some tools already fetch full prog names this way.
We do have those names in BTF. Each map has either corresponding VAR
or DATASEC. The problem is that we don't know which.
Are you proposing to add some extra "btf_def_type_id" field to specify
BTF type ID of what "defines" the map (VAR or DATASEC)? That would
work. Would still require UAPI and kernel changes, of course.
The reason Toke and others were asking to preserve that object name
prefix for .rodata/.data maps was different though, and won't be
addressed by the above. Even if you know the BTF VAR/DATASEC, you
don't know the "object name" associated with the map. And the kernel
doesn't know because it's purely libbpf's abstraction. And sometimes
that abstraction doesn't make sense (e.g., if we create a map that's
pinned and reused from multiple BPF apps/objects).
We do have BPF metadata that Stanislav added a while ago, so maybe we
should just punt that problem to that? I'd love to have clean
".rodata" and ".data" names, of course.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH bpf-next 09/10] libbpf: simplify look up by name of internal maps
2021-10-12 3:45 ` Andrii Nakryiko
@ 2021-10-12 15:29 ` Stanislav Fomichev
2021-10-20 17:59 ` Andrii Nakryiko
0 siblings, 1 reply; 42+ messages in thread
From: Stanislav Fomichev @ 2021-10-12 15:29 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Toke Høiland-Jørgensen, bpf,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Kernel Team
On Mon, Oct 11, 2021 at 8:45 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Oct 11, 2021 at 11:24 PM Alexei Starovoitov <ast@fb.com> wrote:
> >
> > On 10/8/21 2:44 PM, Toke Høiland-Jørgensen wrote:
> > >
> > > Hmm, so introduce a new 'map_name_long' field, and on query the kernel
> > > will fill in the old map_name with a truncated version, and put the full
> > > name in the new field? Yeah, I guess that would work too!
> >
> > Let's start storing full map names in BTF instead.
> > Like we do already for progs.
> > Some tools already fetch full prog names this way.
>
> We do have those names in BTF. Each map has either corresponding VAR
> or DATASEC. The problem is that we don't know which.
>
> Are you proposing to add some extra "btf_def_type_id" field to specify
> BTF type ID of what "defines" the map (VAR or DATASEC)? That would
> work. Would still require UAPI and kernel changes, of course.
>
> The reason Toke and others were asking to preserve that object name
> prefix for .rodata/.data maps was different though, and won't be
> addressed by the above. Even if you know the BTF VAR/DATASEC, you
> don't know the "object name" associated with the map. And the kernel
> doesn't know because it's purely libbpf's abstraction. And sometimes
> that abstraction doesn't make sense (e.g., if we create a map that's
> pinned and reused from multiple BPF apps/objects).
[..]
> We do have BPF metadata that Stanislav added a while ago, so maybe we
> should just punt that problem to that? I'd love to have clean
> ".rodata" and ".data" names, of course.
Are you suggesting we add some option to associate the metadata with
the maps (might be an option)? IIRC, the metadata can only be
associated with the progs right now.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH bpf-next 09/10] libbpf: simplify look up by name of internal maps
2021-10-12 15:29 ` Stanislav Fomichev
@ 2021-10-20 17:59 ` Andrii Nakryiko
2021-10-20 18:09 ` Stanislav Fomichev
0 siblings, 1 reply; 42+ messages in thread
From: Andrii Nakryiko @ 2021-10-20 17:59 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Alexei Starovoitov, Toke Høiland-Jørgensen, bpf,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Kernel Team
On Tue, Oct 12, 2021 at 8:29 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> On Mon, Oct 11, 2021 at 8:45 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Oct 11, 2021 at 11:24 PM Alexei Starovoitov <ast@fb.com> wrote:
> > >
> > > On 10/8/21 2:44 PM, Toke Høiland-Jørgensen wrote:
> > > >
> > > > Hmm, so introduce a new 'map_name_long' field, and on query the kernel
> > > > will fill in the old map_name with a truncated version, and put the full
> > > > name in the new field? Yeah, I guess that would work too!
> > >
> > > Let's start storing full map names in BTF instead.
> > > Like we do already for progs.
> > > Some tools already fetch full prog names this way.
> >
> > We do have those names in BTF. Each map has either corresponding VAR
> > or DATASEC. The problem is that we don't know which.
> >
> > Are you proposing to add some extra "btf_def_type_id" field to specify
> > BTF type ID of what "defines" the map (VAR or DATASEC)? That would
> > work. Would still require UAPI and kernel changes, of course.
> >
> > The reason Toke and others were asking to preserve that object name
> > prefix for .rodata/.data maps was different though, and won't be
> > addressed by the above. Even if you know the BTF VAR/DATASEC, you
> > don't know the "object name" associated with the map. And the kernel
> > doesn't know because it's purely libbpf's abstraction. And sometimes
> > that abstraction doesn't make sense (e.g., if we create a map that's
> > pinned and reused from multiple BPF apps/objects).
>
> [..]
>
> > We do have BPF metadata that Stanislav added a while ago, so maybe we
> > should just punt that problem to that? I'd love to have clean
> > ".rodata" and ".data" names, of course.
>
> Are you suggesting we add some option to associate the metadata with
> the maps (might be an option)? IIRC, the metadata can only be
> associated with the progs right now.
Well, maps have associated BTF fd, when they are created, no? So you
can find all the same metadata for the map, no?
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH bpf-next 09/10] libbpf: simplify look up by name of internal maps
2021-10-20 17:59 ` Andrii Nakryiko
@ 2021-10-20 18:09 ` Stanislav Fomichev
2021-10-20 18:20 ` Andrii Nakryiko
0 siblings, 1 reply; 42+ messages in thread
From: Stanislav Fomichev @ 2021-10-20 18:09 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Toke Høiland-Jørgensen, bpf,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Kernel Team
On Wed, Oct 20, 2021 at 10:59 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Oct 12, 2021 at 8:29 AM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > On Mon, Oct 11, 2021 at 8:45 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Mon, Oct 11, 2021 at 11:24 PM Alexei Starovoitov <ast@fb.com> wrote:
> > > >
> > > > On 10/8/21 2:44 PM, Toke Høiland-Jørgensen wrote:
> > > > >
> > > > > Hmm, so introduce a new 'map_name_long' field, and on query the kernel
> > > > > will fill in the old map_name with a truncated version, and put the full
> > > > > name in the new field? Yeah, I guess that would work too!
> > > >
> > > > Let's start storing full map names in BTF instead.
> > > > Like we do already for progs.
> > > > Some tools already fetch full prog names this way.
> > >
> > > We do have those names in BTF. Each map has either corresponding VAR
> > > or DATASEC. The problem is that we don't know which.
> > >
> > > Are you proposing to add some extra "btf_def_type_id" field to specify
> > > BTF type ID of what "defines" the map (VAR or DATASEC)? That would
> > > work. Would still require UAPI and kernel changes, of course.
> > >
> > > The reason Toke and others were asking to preserve that object name
> > > prefix for .rodata/.data maps was different though, and won't be
> > > addressed by the above. Even if you know the BTF VAR/DATASEC, you
> > > don't know the "object name" associated with the map. And the kernel
> > > doesn't know because it's purely libbpf's abstraction. And sometimes
> > > that abstraction doesn't make sense (e.g., if we create a map that's
> > > pinned and reused from multiple BPF apps/objects).
> >
> > [..]
> >
> > > We do have BPF metadata that Stanislav added a while ago, so maybe we
> > > should just punt that problem to that? I'd love to have clean
> > > ".rodata" and ".data" names, of course.
> >
> > Are you suggesting we add some option to associate the metadata with
> > the maps (might be an option)? IIRC, the metadata can only be
> > associated with the progs right now.
>
> Well, maps have associated BTF fd, when they are created, no? So you
> can find all the same metadata for the map, no?
I guess that's true, we can store this metadata in the map itself
using something like existing bpf_metadata_ prefix.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH bpf-next 09/10] libbpf: simplify look up by name of internal maps
2021-10-20 18:09 ` Stanislav Fomichev
@ 2021-10-20 18:20 ` Andrii Nakryiko
2021-10-20 22:03 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 42+ messages in thread
From: Andrii Nakryiko @ 2021-10-20 18:20 UTC (permalink / raw)
To: Stanislav Fomichev, Quentin Monnet
Cc: Alexei Starovoitov, Toke Høiland-Jørgensen, bpf,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Kernel Team
On Wed, Oct 20, 2021 at 11:09 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> On Wed, Oct 20, 2021 at 10:59 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Oct 12, 2021 at 8:29 AM Stanislav Fomichev <sdf@google.com> wrote:
> > >
> > > On Mon, Oct 11, 2021 at 8:45 PM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > On Mon, Oct 11, 2021 at 11:24 PM Alexei Starovoitov <ast@fb.com> wrote:
> > > > >
> > > > > On 10/8/21 2:44 PM, Toke Høiland-Jørgensen wrote:
> > > > > >
> > > > > > Hmm, so introduce a new 'map_name_long' field, and on query the kernel
> > > > > > will fill in the old map_name with a truncated version, and put the full
> > > > > > name in the new field? Yeah, I guess that would work too!
> > > > >
> > > > > Let's start storing full map names in BTF instead.
> > > > > Like we do already for progs.
> > > > > Some tools already fetch full prog names this way.
> > > >
> > > > We do have those names in BTF. Each map has either corresponding VAR
> > > > or DATASEC. The problem is that we don't know which.
> > > >
> > > > Are you proposing to add some extra "btf_def_type_id" field to specify
> > > > BTF type ID of what "defines" the map (VAR or DATASEC)? That would
> > > > work. Would still require UAPI and kernel changes, of course.
> > > >
> > > > The reason Toke and others were asking to preserve that object name
> > > > prefix for .rodata/.data maps was different though, and won't be
> > > > addressed by the above. Even if you know the BTF VAR/DATASEC, you
> > > > don't know the "object name" associated with the map. And the kernel
> > > > doesn't know because it's purely libbpf's abstraction. And sometimes
> > > > that abstraction doesn't make sense (e.g., if we create a map that's
> > > > pinned and reused from multiple BPF apps/objects).
> > >
> > > [..]
> > >
> > > > We do have BPF metadata that Stanislav added a while ago, so maybe we
> > > > should just punt that problem to that? I'd love to have clean
> > > > ".rodata" and ".data" names, of course.
> > >
> > > Are you suggesting we add some option to associate the metadata with
> > > the maps (might be an option)? IIRC, the metadata can only be
> > > associated with the progs right now.
> >
> > Well, maps have associated BTF fd, when they are created, no? So you
> > can find all the same metadata for the map, no?
>
> I guess that's true, we can store this metadata in the map itself
> using something like existing bpf_metadata_ prefix.
We had a discussion during the inaugural BSC meeting about having a
small set of "standardized" metadata strings. "owner" and
"description" (or maybe "app" for "application name") were two that
were clearly useful and generally useful. So if we update bpftool and
other tooling to recognize bpf_metadata_owner and bpf_metadata_app and
print them in some nice and meaningful way in bpftool output (in
addition to general btf_metadata dump), it would be great.
Which is a long way to say that once we have bpf_metadat_app, you
already have associated bpf_object name (or whatever user will decide
to name their BPF application). Which solves this map naming problem
as well (with tooling support, of course).
cc Quentin also. Thoughts?
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH bpf-next 09/10] libbpf: simplify look up by name of internal maps
2021-10-20 18:20 ` Andrii Nakryiko
@ 2021-10-20 22:03 ` Toke Høiland-Jørgensen
2021-10-20 22:24 ` Stanislav Fomichev
2021-10-20 22:25 ` Andrii Nakryiko
0 siblings, 2 replies; 42+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-10-20 22:03 UTC (permalink / raw)
To: Andrii Nakryiko, Stanislav Fomichev, Quentin Monnet
Cc: Alexei Starovoitov, bpf, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Kernel Team
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> On Wed, Oct 20, 2021 at 11:09 AM Stanislav Fomichev <sdf@google.com> wrote:
>>
>> On Wed, Oct 20, 2021 at 10:59 AM Andrii Nakryiko
>> <andrii.nakryiko@gmail.com> wrote:
>> >
>> > On Tue, Oct 12, 2021 at 8:29 AM Stanislav Fomichev <sdf@google.com> wrote:
>> > >
>> > > On Mon, Oct 11, 2021 at 8:45 PM Andrii Nakryiko
>> > > <andrii.nakryiko@gmail.com> wrote:
>> > > >
>> > > > On Mon, Oct 11, 2021 at 11:24 PM Alexei Starovoitov <ast@fb.com> wrote:
>> > > > >
>> > > > > On 10/8/21 2:44 PM, Toke Høiland-Jørgensen wrote:
>> > > > > >
>> > > > > > Hmm, so introduce a new 'map_name_long' field, and on query the kernel
>> > > > > > will fill in the old map_name with a truncated version, and put the full
>> > > > > > name in the new field? Yeah, I guess that would work too!
>> > > > >
>> > > > > Let's start storing full map names in BTF instead.
>> > > > > Like we do already for progs.
>> > > > > Some tools already fetch full prog names this way.
>> > > >
>> > > > We do have those names in BTF. Each map has either corresponding VAR
>> > > > or DATASEC. The problem is that we don't know which.
>> > > >
>> > > > Are you proposing to add some extra "btf_def_type_id" field to specify
>> > > > BTF type ID of what "defines" the map (VAR or DATASEC)? That would
>> > > > work. Would still require UAPI and kernel changes, of course.
>> > > >
>> > > > The reason Toke and others were asking to preserve that object name
>> > > > prefix for .rodata/.data maps was different though, and won't be
>> > > > addressed by the above. Even if you know the BTF VAR/DATASEC, you
>> > > > don't know the "object name" associated with the map. And the kernel
>> > > > doesn't know because it's purely libbpf's abstraction. And sometimes
>> > > > that abstraction doesn't make sense (e.g., if we create a map that's
>> > > > pinned and reused from multiple BPF apps/objects).
>> > >
>> > > [..]
>> > >
>> > > > We do have BPF metadata that Stanislav added a while ago, so maybe we
>> > > > should just punt that problem to that? I'd love to have clean
>> > > > ".rodata" and ".data" names, of course.
>> > >
>> > > Are you suggesting we add some option to associate the metadata with
>> > > the maps (might be an option)? IIRC, the metadata can only be
>> > > associated with the progs right now.
>> >
>> > Well, maps have associated BTF fd, when they are created, no? So you
>> > can find all the same metadata for the map, no?
>>
>> I guess that's true, we can store this metadata in the map itself
>> using something like existing bpf_metadata_ prefix.
>
> We had a discussion during the inaugural BSC meeting about having a
> small set of "standardized" metadata strings. "owner" and
> "description" (or maybe "app" for "application name") were two that
> were clearly useful and generally useful. So if we update bpftool and
> other tooling to recognize bpf_metadata_owner and bpf_metadata_app and
> print them in some nice and meaningful way in bpftool output (in
> addition to general btf_metadata dump), it would be great.
I like the idea of specifying some well-known metadata names, especially
if libbpf can auto-populate them if the user doesn't.
Also, couldn't bpftool just print out all bpf_metadata_* fields? At
least in a verbose mode...
-Toke
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH bpf-next 09/10] libbpf: simplify look up by name of internal maps
2021-10-20 22:03 ` Toke Høiland-Jørgensen
@ 2021-10-20 22:24 ` Stanislav Fomichev
2021-10-20 22:25 ` Andrii Nakryiko
1 sibling, 0 replies; 42+ messages in thread
From: Stanislav Fomichev @ 2021-10-20 22:24 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Andrii Nakryiko, Quentin Monnet, Alexei Starovoitov, bpf,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Kernel Team
On Wed, Oct 20, 2021 at 3:03 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Wed, Oct 20, 2021 at 11:09 AM Stanislav Fomichev <sdf@google.com> wrote:
> >>
> >> On Wed, Oct 20, 2021 at 10:59 AM Andrii Nakryiko
> >> <andrii.nakryiko@gmail.com> wrote:
> >> >
> >> > On Tue, Oct 12, 2021 at 8:29 AM Stanislav Fomichev <sdf@google.com> wrote:
> >> > >
> >> > > On Mon, Oct 11, 2021 at 8:45 PM Andrii Nakryiko
> >> > > <andrii.nakryiko@gmail.com> wrote:
> >> > > >
> >> > > > On Mon, Oct 11, 2021 at 11:24 PM Alexei Starovoitov <ast@fb.com> wrote:
> >> > > > >
> >> > > > > On 10/8/21 2:44 PM, Toke Høiland-Jørgensen wrote:
> >> > > > > >
> >> > > > > > Hmm, so introduce a new 'map_name_long' field, and on query the kernel
> >> > > > > > will fill in the old map_name with a truncated version, and put the full
> >> > > > > > name in the new field? Yeah, I guess that would work too!
> >> > > > >
> >> > > > > Let's start storing full map names in BTF instead.
> >> > > > > Like we do already for progs.
> >> > > > > Some tools already fetch full prog names this way.
> >> > > >
> >> > > > We do have those names in BTF. Each map has either corresponding VAR
> >> > > > or DATASEC. The problem is that we don't know which.
> >> > > >
> >> > > > Are you proposing to add some extra "btf_def_type_id" field to specify
> >> > > > BTF type ID of what "defines" the map (VAR or DATASEC)? That would
> >> > > > work. Would still require UAPI and kernel changes, of course.
> >> > > >
> >> > > > The reason Toke and others were asking to preserve that object name
> >> > > > prefix for .rodata/.data maps was different though, and won't be
> >> > > > addressed by the above. Even if you know the BTF VAR/DATASEC, you
> >> > > > don't know the "object name" associated with the map. And the kernel
> >> > > > doesn't know because it's purely libbpf's abstraction. And sometimes
> >> > > > that abstraction doesn't make sense (e.g., if we create a map that's
> >> > > > pinned and reused from multiple BPF apps/objects).
> >> > >
> >> > > [..]
> >> > >
> >> > > > We do have BPF metadata that Stanislav added a while ago, so maybe we
> >> > > > should just punt that problem to that? I'd love to have clean
> >> > > > ".rodata" and ".data" names, of course.
> >> > >
> >> > > Are you suggesting we add some option to associate the metadata with
> >> > > the maps (might be an option)? IIRC, the metadata can only be
> >> > > associated with the progs right now.
> >> >
> >> > Well, maps have associated BTF fd, when they are created, no? So you
> >> > can find all the same metadata for the map, no?
> >>
> >> I guess that's true, we can store this metadata in the map itself
> >> using something like existing bpf_metadata_ prefix.
> >
> > We had a discussion during the inaugural BSC meeting about having a
> > small set of "standardized" metadata strings. "owner" and
> > "description" (or maybe "app" for "application name") were two that
> > were clearly useful and generally useful. So if we update bpftool and
> > other tooling to recognize bpf_metadata_owner and bpf_metadata_app and
> > print them in some nice and meaningful way in bpftool output (in
> > addition to general btf_metadata dump), it would be great.
>
> I like the idea of specifying some well-known metadata names, especially
> if libbpf can auto-populate them if the user doesn't.
Yeah, I'd +1 that. I was exploring the idea of adding process's
cmdline into map/prog info a while ago. That's where this whole
metadata came out, but I've yet to add something to libbpf that's
"standardized".
> Also, couldn't bpftool just print out all bpf_metadata_* fields? At
> least in a verbose mode...
It already prints everything, but it prints them in a plain list.
Maybe we can integrate some of the data more nicely.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH bpf-next 09/10] libbpf: simplify look up by name of internal maps
2021-10-20 22:03 ` Toke Høiland-Jørgensen
2021-10-20 22:24 ` Stanislav Fomichev
@ 2021-10-20 22:25 ` Andrii Nakryiko
2021-10-21 11:39 ` Toke Høiland-Jørgensen
1 sibling, 1 reply; 42+ messages in thread
From: Andrii Nakryiko @ 2021-10-20 22:25 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Stanislav Fomichev, Quentin Monnet, Alexei Starovoitov, bpf,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Kernel Team
On Wed, Oct 20, 2021 at 3:03 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Wed, Oct 20, 2021 at 11:09 AM Stanislav Fomichev <sdf@google.com> wrote:
> >>
> >> On Wed, Oct 20, 2021 at 10:59 AM Andrii Nakryiko
> >> <andrii.nakryiko@gmail.com> wrote:
> >> >
> >> > On Tue, Oct 12, 2021 at 8:29 AM Stanislav Fomichev <sdf@google.com> wrote:
> >> > >
> >> > > On Mon, Oct 11, 2021 at 8:45 PM Andrii Nakryiko
> >> > > <andrii.nakryiko@gmail.com> wrote:
> >> > > >
> >> > > > On Mon, Oct 11, 2021 at 11:24 PM Alexei Starovoitov <ast@fb.com> wrote:
> >> > > > >
> >> > > > > On 10/8/21 2:44 PM, Toke Høiland-Jørgensen wrote:
> >> > > > > >
> >> > > > > > Hmm, so introduce a new 'map_name_long' field, and on query the kernel
> >> > > > > > will fill in the old map_name with a truncated version, and put the full
> >> > > > > > name in the new field? Yeah, I guess that would work too!
> >> > > > >
> >> > > > > Let's start storing full map names in BTF instead.
> >> > > > > Like we do already for progs.
> >> > > > > Some tools already fetch full prog names this way.
> >> > > >
> >> > > > We do have those names in BTF. Each map has either corresponding VAR
> >> > > > or DATASEC. The problem is that we don't know which.
> >> > > >
> >> > > > Are you proposing to add some extra "btf_def_type_id" field to specify
> >> > > > BTF type ID of what "defines" the map (VAR or DATASEC)? That would
> >> > > > work. Would still require UAPI and kernel changes, of course.
> >> > > >
> >> > > > The reason Toke and others were asking to preserve that object name
> >> > > > prefix for .rodata/.data maps was different though, and won't be
> >> > > > addressed by the above. Even if you know the BTF VAR/DATASEC, you
> >> > > > don't know the "object name" associated with the map. And the kernel
> >> > > > doesn't know because it's purely libbpf's abstraction. And sometimes
> >> > > > that abstraction doesn't make sense (e.g., if we create a map that's
> >> > > > pinned and reused from multiple BPF apps/objects).
> >> > >
> >> > > [..]
> >> > >
> >> > > > We do have BPF metadata that Stanislav added a while ago, so maybe we
> >> > > > should just punt that problem to that? I'd love to have clean
> >> > > > ".rodata" and ".data" names, of course.
> >> > >
> >> > > Are you suggesting we add some option to associate the metadata with
> >> > > the maps (might be an option)? IIRC, the metadata can only be
> >> > > associated with the progs right now.
> >> >
> >> > Well, maps have associated BTF fd, when they are created, no? So you
> >> > can find all the same metadata for the map, no?
> >>
> >> I guess that's true, we can store this metadata in the map itself
> >> using something like existing bpf_metadata_ prefix.
> >
> > We had a discussion during the inaugural BSC meeting about having a
> > small set of "standardized" metadata strings. "owner" and
> > "description" (or maybe "app" for "application name") were two that
> > were clearly useful and generally useful. So if we update bpftool and
> > other tooling to recognize bpf_metadata_owner and bpf_metadata_app and
> > print them in some nice and meaningful way in bpftool output (in
> > addition to general btf_metadata dump), it would be great.
>
> I like the idea of specifying some well-known metadata names, especially
> if libbpf can auto-populate them if the user doesn't.
>
> Also, couldn't bpftool just print out all bpf_metadata_* fields? At
> least in a verbose mode...
Yes, bpftool dumps all bpf_metadata_* fields already. The point of
converging on few common ones (say, bpf_metadata_owner and
bpf_metadata_app) would allow all the tools to use consistent subset
to display meaningful short info about a prog or map. Dumping all
metadata fields for something like "bpf top" doesn't make sense.
re: libbpf auto-populating some of them. It can populate "app"
metadata from bpf_object's name, but we need to think through all the
logistics carefully (e.g., not setting if user already specified that
explicitly, etc). There is no way libbpf can know "owner" meta,
though.
>
> -Toke
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH bpf-next 09/10] libbpf: simplify look up by name of internal maps
2021-10-20 22:25 ` Andrii Nakryiko
@ 2021-10-21 11:39 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 42+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-10-21 11:39 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Stanislav Fomichev, Quentin Monnet, Alexei Starovoitov, bpf,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Kernel Team
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> On Wed, Oct 20, 2021 at 3:03 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>> > On Wed, Oct 20, 2021 at 11:09 AM Stanislav Fomichev <sdf@google.com> wrote:
>> >>
>> >> On Wed, Oct 20, 2021 at 10:59 AM Andrii Nakryiko
>> >> <andrii.nakryiko@gmail.com> wrote:
>> >> >
>> >> > On Tue, Oct 12, 2021 at 8:29 AM Stanislav Fomichev <sdf@google.com> wrote:
>> >> > >
>> >> > > On Mon, Oct 11, 2021 at 8:45 PM Andrii Nakryiko
>> >> > > <andrii.nakryiko@gmail.com> wrote:
>> >> > > >
>> >> > > > On Mon, Oct 11, 2021 at 11:24 PM Alexei Starovoitov <ast@fb.com> wrote:
>> >> > > > >
>> >> > > > > On 10/8/21 2:44 PM, Toke Høiland-Jørgensen wrote:
>> >> > > > > >
>> >> > > > > > Hmm, so introduce a new 'map_name_long' field, and on query the kernel
>> >> > > > > > will fill in the old map_name with a truncated version, and put the full
>> >> > > > > > name in the new field? Yeah, I guess that would work too!
>> >> > > > >
>> >> > > > > Let's start storing full map names in BTF instead.
>> >> > > > > Like we do already for progs.
>> >> > > > > Some tools already fetch full prog names this way.
>> >> > > >
>> >> > > > We do have those names in BTF. Each map has either corresponding VAR
>> >> > > > or DATASEC. The problem is that we don't know which.
>> >> > > >
>> >> > > > Are you proposing to add some extra "btf_def_type_id" field to specify
>> >> > > > BTF type ID of what "defines" the map (VAR or DATASEC)? That would
>> >> > > > work. Would still require UAPI and kernel changes, of course.
>> >> > > >
>> >> > > > The reason Toke and others were asking to preserve that object name
>> >> > > > prefix for .rodata/.data maps was different though, and won't be
>> >> > > > addressed by the above. Even if you know the BTF VAR/DATASEC, you
>> >> > > > don't know the "object name" associated with the map. And the kernel
>> >> > > > doesn't know because it's purely libbpf's abstraction. And sometimes
>> >> > > > that abstraction doesn't make sense (e.g., if we create a map that's
>> >> > > > pinned and reused from multiple BPF apps/objects).
>> >> > >
>> >> > > [..]
>> >> > >
>> >> > > > We do have BPF metadata that Stanislav added a while ago, so maybe we
>> >> > > > should just punt that problem to that? I'd love to have clean
>> >> > > > ".rodata" and ".data" names, of course.
>> >> > >
>> >> > > Are you suggesting we add some option to associate the metadata with
>> >> > > the maps (might be an option)? IIRC, the metadata can only be
>> >> > > associated with the progs right now.
>> >> >
>> >> > Well, maps have associated BTF fd, when they are created, no? So you
>> >> > can find all the same metadata for the map, no?
>> >>
>> >> I guess that's true, we can store this metadata in the map itself
>> >> using something like existing bpf_metadata_ prefix.
>> >
>> > We had a discussion during the inaugural BSC meeting about having a
>> > small set of "standardized" metadata strings. "owner" and
>> > "description" (or maybe "app" for "application name") were two that
>> > were clearly useful and generally useful. So if we update bpftool and
>> > other tooling to recognize bpf_metadata_owner and bpf_metadata_app and
>> > print them in some nice and meaningful way in bpftool output (in
>> > addition to general btf_metadata dump), it would be great.
>>
>> I like the idea of specifying some well-known metadata names, especially
>> if libbpf can auto-populate them if the user doesn't.
>>
>> Also, couldn't bpftool just print out all bpf_metadata_* fields? At
>> least in a verbose mode...
>
> Yes, bpftool dumps all bpf_metadata_* fields already. The point of
> converging on few common ones (say, bpf_metadata_owner and
> bpf_metadata_app) would allow all the tools to use consistent subset
> to display meaningful short info about a prog or map. Dumping all
> metadata fields for something like "bpf top" doesn't make sense.
>
> re: libbpf auto-populating some of them. It can populate "app"
> metadata from bpf_object's name, but we need to think through all the
> logistics carefully (e.g., not setting if user already specified that
> explicitly, etc). There is no way libbpf can know "owner" meta,
> though.
Right, I was mostly thinking about the "app" name; just so there's a
default if users don't set anything themselves, like there is today with
the prefix...
-Toke
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH bpf-next 09/10] libbpf: simplify look up by name of internal maps
2021-10-08 0:03 ` [PATCH bpf-next 09/10] libbpf: simplify look up by name of internal maps andrii.nakryiko
2021-10-08 17:30 ` Toke Høiland-Jørgensen
@ 2021-10-08 22:16 ` Song Liu
1 sibling, 0 replies; 42+ messages in thread
From: Song Liu @ 2021-10-08 22:16 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Kernel Team, Toke Høiland-Jørgensen,
Stanislav Fomichev
On Thu, Oct 7, 2021 at 5:05 PM <andrii.nakryiko@gmail.com> wrote:
>
[...]
>
> With such set up, we get the best of both worlds: leave small bits of
> a clue about BPF application that instantiated such maps, as well as
> making it easy for user apps to lookup such maps at runtime. In this
> sense it closes corresponding libbpf 1.0 issue ([0]).
>
> BPF skeletons will continue using full names for lookups.
>
> [0] Closes: https://github.com/libbpf/libbpf/issues/275
>
> Cc: Toke Høiland-Jørgensen <toke@redhat.com>
> Cc: Stanislav Fomichev <sdf@google.com>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Song Liu <songliubraving@fb.com>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH bpf-next 10/10] selftests/bpf: switch to ".bss"/".rodata"/".data" lookups for internal maps
2021-10-08 0:02 [PATCH bpf-next 00/10] libbpf: support custom .rodata.*/.data.* sections andrii.nakryiko
` (8 preceding siblings ...)
2021-10-08 0:03 ` [PATCH bpf-next 09/10] libbpf: simplify look up by name of internal maps andrii.nakryiko
@ 2021-10-08 0:03 ` andrii.nakryiko
2021-10-08 22:16 ` Song Liu
2021-10-11 21:30 ` [PATCH bpf-next 00/10] libbpf: support custom .rodata.*/.data.* sections Alexei Starovoitov
2021-10-12 4:15 ` Kumar Kartikeya Dwivedi
11 siblings, 1 reply; 42+ messages in thread
From: andrii.nakryiko @ 2021-10-08 0:03 UTC (permalink / raw)
To: bpf, ast, daniel; +Cc: andrii, kernel-team
From: Andrii Nakryiko <andrii@kernel.org>
Utilize libbpf's feature of allowing to lookup internal maps by their
ELF section names. No need to guess or calculate the exact truncated
prefix taken from the object name.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
.../testing/selftests/bpf/prog_tests/core_autosize.c | 2 +-
tools/testing/selftests/bpf/prog_tests/core_reloc.c | 2 +-
tools/testing/selftests/bpf/prog_tests/global_data.c | 11 +++++++++--
.../selftests/bpf/prog_tests/global_data_init.c | 2 +-
tools/testing/selftests/bpf/prog_tests/kfree_skb.c | 2 +-
tools/testing/selftests/bpf/prog_tests/rdonly_maps.c | 2 +-
6 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/core_autosize.c b/tools/testing/selftests/bpf/prog_tests/core_autosize.c
index 3d4b2a358d47..2a0dac6394ef 100644
--- a/tools/testing/selftests/bpf/prog_tests/core_autosize.c
+++ b/tools/testing/selftests/bpf/prog_tests/core_autosize.c
@@ -163,7 +163,7 @@ void test_core_autosize(void)
usleep(1);
- bss_map = bpf_object__find_map_by_name(skel->obj, "test_cor.bss");
+ bss_map = bpf_object__find_map_by_name(skel->obj, ".bss");
if (!ASSERT_OK_PTR(bss_map, "bss_map_find"))
goto cleanup;
diff --git a/tools/testing/selftests/bpf/prog_tests/core_reloc.c b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
index 763302e63a29..cc50f8feeca3 100644
--- a/tools/testing/selftests/bpf/prog_tests/core_reloc.c
+++ b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
@@ -867,7 +867,7 @@ void test_core_reloc(void)
goto cleanup;
}
- data_map = bpf_object__find_map_by_name(obj, "test_cor.bss");
+ data_map = bpf_object__find_map_by_name(obj, ".bss");
if (CHECK(!data_map, "find_data_map", "data map not found\n"))
goto cleanup;
diff --git a/tools/testing/selftests/bpf/prog_tests/global_data.c b/tools/testing/selftests/bpf/prog_tests/global_data.c
index 9efa7e50eab2..afd8639f9a94 100644
--- a/tools/testing/selftests/bpf/prog_tests/global_data.c
+++ b/tools/testing/selftests/bpf/prog_tests/global_data.c
@@ -103,11 +103,18 @@ static void test_global_data_struct(struct bpf_object *obj, __u32 duration)
static void test_global_data_rdonly(struct bpf_object *obj, __u32 duration)
{
int err = -ENOMEM, map_fd, zero = 0;
- struct bpf_map *map;
+ struct bpf_map *map, *map2;
__u8 *buff;
map = bpf_object__find_map_by_name(obj, "test_glo.rodata");
- if (CHECK_FAIL(!map || !bpf_map__is_internal(map)))
+ if (!ASSERT_OK_PTR(map, "map"))
+ return;
+ if (!ASSERT_TRUE(bpf_map__is_internal(map), "is_internal"))
+ return;
+
+ /* ensure we can lookup internal maps by their ELF names */
+ map2 = bpf_object__find_map_by_name(obj, ".rodata");
+ if (!ASSERT_EQ(map, map2, "same_maps"))
return;
map_fd = bpf_map__fd(map);
diff --git a/tools/testing/selftests/bpf/prog_tests/global_data_init.c b/tools/testing/selftests/bpf/prog_tests/global_data_init.c
index ee46b11f1f9a..1db86eab101b 100644
--- a/tools/testing/selftests/bpf/prog_tests/global_data_init.c
+++ b/tools/testing/selftests/bpf/prog_tests/global_data_init.c
@@ -16,7 +16,7 @@ void test_global_data_init(void)
if (CHECK_FAIL(err))
return;
- map = bpf_object__find_map_by_name(obj, "test_glo.rodata");
+ map = bpf_object__find_map_by_name(obj, ".rodata");
if (CHECK_FAIL(!map || !bpf_map__is_internal(map)))
goto out;
diff --git a/tools/testing/selftests/bpf/prog_tests/kfree_skb.c b/tools/testing/selftests/bpf/prog_tests/kfree_skb.c
index ddfb6bf97152..a9e00960127c 100644
--- a/tools/testing/selftests/bpf/prog_tests/kfree_skb.c
+++ b/tools/testing/selftests/bpf/prog_tests/kfree_skb.c
@@ -92,7 +92,7 @@ void test_kfree_skb(void)
if (CHECK(!fexit, "find_prog", "prog eth_type_trans not found\n"))
goto close_prog;
- global_data = bpf_object__find_map_by_name(obj2, "kfree_sk.bss");
+ global_data = bpf_object__find_map_by_name(obj2, ".bss");
if (CHECK(!global_data, "find global data", "not found\n"))
goto close_prog;
diff --git a/tools/testing/selftests/bpf/prog_tests/rdonly_maps.c b/tools/testing/selftests/bpf/prog_tests/rdonly_maps.c
index 5f9eaa3ab584..fd5d2ddfb062 100644
--- a/tools/testing/selftests/bpf/prog_tests/rdonly_maps.c
+++ b/tools/testing/selftests/bpf/prog_tests/rdonly_maps.c
@@ -37,7 +37,7 @@ void test_rdonly_maps(void)
if (CHECK(err, "obj_load", "err %d errno %d\n", err, errno))
goto cleanup;
- bss_map = bpf_object__find_map_by_name(obj, "test_rdo.bss");
+ bss_map = bpf_object__find_map_by_name(obj, ".bss");
if (CHECK(!bss_map, "find_bss_map", "failed\n"))
goto cleanup;
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH bpf-next 00/10] libbpf: support custom .rodata.*/.data.* sections
2021-10-08 0:02 [PATCH bpf-next 00/10] libbpf: support custom .rodata.*/.data.* sections andrii.nakryiko
` (9 preceding siblings ...)
2021-10-08 0:03 ` [PATCH bpf-next 10/10] selftests/bpf: switch to ".bss"/".rodata"/".data" lookups for " andrii.nakryiko
@ 2021-10-11 21:30 ` Alexei Starovoitov
2021-10-12 3:36 ` Andrii Nakryiko
2021-10-12 4:15 ` Kumar Kartikeya Dwivedi
11 siblings, 1 reply; 42+ messages in thread
From: Alexei Starovoitov @ 2021-10-11 21:30 UTC (permalink / raw)
To: andrii.nakryiko, bpf, ast, daniel; +Cc: andrii, kernel-team
On 10/7/21 5:02 PM, andrii.nakryiko@gmail.com wrote:
>
> One interesting possibility that is now open by these changes is that it's
> possible to do:
>
> bpf_trace_printk("My fmt %s", sizeof("My fmt %s"), "blah");
>
> and it will work as expected.
Could you explain what would be the difference vs existing bpf_printk macro?
Why these patches enable above to work ?
> I haven't updated libbpf-provided helpers in
> bpf_helpers.h for snprintf, seq_printf, and printk, because using
> `static const char ___fmt[] = fmt;` trick is still efficient and doesn't fill
> out the buffer at runtime (no copying), but it also enforces that format
> string is compile-time string literal:
>
> const char *s = NULL;
>
> bpf_printk("hi"); /* works */
> bpf_printk(s); /* compilation error */
>
> By passing fmt directly to bpf_trace_printk() would actually compile
> bpf_printk(s) above with no warnings and will not work at runtime, which is
> worse user experience, IMO.
What is the example of "compile with no warning and not work at runtime" ?
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH bpf-next 00/10] libbpf: support custom .rodata.*/.data.* sections
2021-10-11 21:30 ` [PATCH bpf-next 00/10] libbpf: support custom .rodata.*/.data.* sections Alexei Starovoitov
@ 2021-10-12 3:36 ` Andrii Nakryiko
0 siblings, 0 replies; 42+ messages in thread
From: Andrii Nakryiko @ 2021-10-12 3:36 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Kernel Team
On Mon, Oct 11, 2021 at 11:30 PM Alexei Starovoitov <ast@fb.com> wrote:
>
> On 10/7/21 5:02 PM, andrii.nakryiko@gmail.com wrote:
> >
> > One interesting possibility that is now open by these changes is that it's
> > possible to do:
> >
> > bpf_trace_printk("My fmt %s", sizeof("My fmt %s"), "blah");
> >
> > and it will work as expected.
>
> Could you explain what would be the difference vs existing bpf_printk macro?
> Why these patches enable above to work ?
Good question. Have you ever noticed warnings like this during selftests build:
libbpf: elf: skipping unrecognized data section(6) .rodata.str1.1
I'm recalling from memory a bit here, so excuse me if I get some small
details wrong. Let's say we have a `bpf_printk("Hello!\n");` call. It
is turned into:
static const char fmt[] = "Hello!\n";
bpf_trace_printk(fmt, 7);
`fmt` variable above is always in .rodata section, it will even have a
dedicated BTF VAR with the name '<func>.fmt'. For reasons unknown to
me (Yonghong will know, probably), the compiler *also* and *sometimes*
puts the same "Hello!\n" string into the .rodata.str1.1 section. So we
end up with this warning about unknown and skipped .rodata.str1.1.
Good news is that we actually never reference "Hello!\n" from
.rodata.str1.1, which is why the bpf_printk() works today.
But if you were to rewrite the above snippet as more natural:
bpf_trace_printk("Hello!\n", 7);
... and compiler puts that "Hello!\n" into .rodata.str1.1 (and *not*
into .rodata), then we'd have a relocation against .rodata.str1.1,
which will fail because up until now libbpf never did anything with
.rodata.str1.1.
So it's a bit roundabout way to say that with this patch set, no
matter which .rodata* section compiler decided to put compile-time
constants into (doesn't have to be string, could be struct literal for
initializing a struct, for example) it should work, because it's just
a normal relocation, so all the libbpf relocation logic works, and for
kernel it will be just another global data ARRAY, so everything works
as expected. What was necessary to make this work was internal
refactoring to remove a simplifying assumption that there could be
only one .rodata map, and one .data map, etc.
Hope that clears it a bit.
>
> > I haven't updated libbpf-provided helpers in
> > bpf_helpers.h for snprintf, seq_printf, and printk, because using
> > `static const char ___fmt[] = fmt;` trick is still efficient and doesn't fill
> > out the buffer at runtime (no copying), but it also enforces that format
> > string is compile-time string literal:
> >
> > const char *s = NULL;
> >
> > bpf_printk("hi"); /* works */
> > bpf_printk(s); /* compilation error */
> >
> > By passing fmt directly to bpf_trace_printk() would actually compile
> > bpf_printk(s) above with no warnings and will not work at runtime, which is
> > worse user experience, IMO.
>
> What is the example of "compile with no warning and not work at runtime" ?
Right, so imagine we rewritten bpf_printk to expand into
bpf_trace_printk(fmt, sizeof(fmt), args...));
Now, if you do bpf_printk("BLAH"), everything works, fmt string
literal is passed properly and its size is calculated as 5. But if you
now pass const char *, you end up doing:
bpf_trace_printk(s, sizeof(s) /* == 8 */, args...);
See how passing a pointer is right for the first argument, but using
it to determine the string size is completely wrong. And the worst
thing is it won't trigger any warning. There's a similar danger of
using sizeof() with arrays. Pure C language gotcha, nothing BPF
specific.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH bpf-next 00/10] libbpf: support custom .rodata.*/.data.* sections
2021-10-08 0:02 [PATCH bpf-next 00/10] libbpf: support custom .rodata.*/.data.* sections andrii.nakryiko
` (10 preceding siblings ...)
2021-10-11 21:30 ` [PATCH bpf-next 00/10] libbpf: support custom .rodata.*/.data.* sections Alexei Starovoitov
@ 2021-10-12 4:15 ` Kumar Kartikeya Dwivedi
2021-10-21 0:14 ` Andrii Nakryiko
11 siblings, 1 reply; 42+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-12 4:15 UTC (permalink / raw)
To: andrii.nakryiko; +Cc: bpf, ast, daniel, andrii, kernel-team
On Fri, Oct 08, 2021 at 05:32:59AM IST, andrii.nakryiko@gmail.com wrote:
> From: Andrii Nakryiko <andrii@kernel.org>
>
> [...]
> One interesting possibility that is now open by these changes is that it's
> possible to do:
>
> bpf_trace_printk("My fmt %s", sizeof("My fmt %s"), "blah");
>
> and it will work as expected. I haven't updated libbpf-provided helpers in
> bpf_helpers.h for snprintf, seq_printf, and printk, because using
> `static const char ___fmt[] = fmt;` trick is still efficient and doesn't fill
> out the buffer at runtime (no copying), but it also enforces that format
> string is compile-time string literal:
>
> const char *s = NULL;
>
> bpf_printk("hi"); /* works */
> bpf_printk(s); /* compilation error */
>
> By passing fmt directly to bpf_trace_printk() would actually compile
> bpf_printk(s) above with no warnings and will not work at runtime, which is
> worse user experience, IMO.
>
You could try the following (_Static_assert would probably be preferable, but
IDK if libbpf can use it).
#define IS_ARRAY(x) ({ sizeof(int[-__builtin_types_compatible_p(typeof(x), typeof(&*(x)))]); 1; })
In action: https://godbolt.org/z/4d4W61YPf
--
Kartikeya
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH bpf-next 00/10] libbpf: support custom .rodata.*/.data.* sections
2021-10-12 4:15 ` Kumar Kartikeya Dwivedi
@ 2021-10-21 0:14 ` Andrii Nakryiko
0 siblings, 0 replies; 42+ messages in thread
From: Andrii Nakryiko @ 2021-10-21 0:14 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Kernel Team
On Mon, Oct 11, 2021 at 9:15 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Fri, Oct 08, 2021 at 05:32:59AM IST, andrii.nakryiko@gmail.com wrote:
> > From: Andrii Nakryiko <andrii@kernel.org>
> >
> > [...]
> > One interesting possibility that is now open by these changes is that it's
> > possible to do:
> >
> > bpf_trace_printk("My fmt %s", sizeof("My fmt %s"), "blah");
> >
> > and it will work as expected. I haven't updated libbpf-provided helpers in
> > bpf_helpers.h for snprintf, seq_printf, and printk, because using
> > `static const char ___fmt[] = fmt;` trick is still efficient and doesn't fill
> > out the buffer at runtime (no copying), but it also enforces that format
> > string is compile-time string literal:
> >
> > const char *s = NULL;
> >
> > bpf_printk("hi"); /* works */
> > bpf_printk(s); /* compilation error */
> >
> > By passing fmt directly to bpf_trace_printk() would actually compile
> > bpf_printk(s) above with no warnings and will not work at runtime, which is
> > worse user experience, IMO.
> >
>
> You could try the following (_Static_assert would probably be preferable, but
> IDK if libbpf can use it).
Yeah, we definitely can use _Static_assert from BPF side (see
progs/test_cls_redirect.c in selftests/bpf).
>
> #define IS_ARRAY(x) ({ sizeof(int[-__builtin_types_compatible_p(typeof(x), typeof(&*(x)))]); 1; })
>
Thanks! This seems to be working well to detect arrays and string
literals. I'll keep it in my toolbox :) Ultimately I decided to not
touch bpf_printk() for now, because of the BPF_NO_GLOBAL_DATA trick.
If I go with direct "fmt" usage, I'll need to implementations of
__bpf_printk(), which doesn't seem worth it right now. I might revisit
it later, though.
> In action: https://godbolt.org/z/4d4W61YPf
>
> --
> Kartikeya
^ permalink raw reply [flat|nested] 42+ messages in thread