diff mbox series

[bpf-next,v3,12/15] libbpf: rewrite btf datasec names starting from '?'

Message ID 20240304225156.24765-13-eddyz87@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series libbpf: type suffixes and autocreate flag for struct_ops maps | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/apply fail Patch does not apply to bpf-next-0
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 fail Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 fail Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 fail Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-32 fail Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-18 / test
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 fail Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 fail Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 fail Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-15 fail Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-29 fail Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-13 fail Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 fail Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 fail Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc

Commit Message

Eduard Zingerman March 4, 2024, 10:51 p.m. UTC 
Optional struct_ops maps are defined using question mark at the start
of the section name, e.g.:

    SEC("?.struct_ops")
    struct test_ops optional_map = { ... };

This commit teaches libbpf to detect if kernel allows '?' prefix
in datasec names, and if it doesn't then to rewrite such names
by removing '?' prefix and adding ".optional" suffix.
For example:

    DATASEC ?.struct_ops -> DATASEC .struct_ops.optional

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/lib/bpf/features.c        | 22 ++++++++++++++++++++++
 tools/lib/bpf/libbpf.c          | 30 +++++++++++++++++++++++++++++-
 tools/lib/bpf/libbpf_internal.h |  2 ++
 3 files changed, 53 insertions(+), 1 deletion(-)

Comments

Andrii Nakryiko March 5, 2024, 8:03 p.m. UTC | #1 
On Mon, Mar 4, 2024 at 2:52 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> Optional struct_ops maps are defined using question mark at the start
> of the section name, e.g.:
>
>     SEC("?.struct_ops")
>     struct test_ops optional_map = { ... };
>
> This commit teaches libbpf to detect if kernel allows '?' prefix
> in datasec names, and if it doesn't then to rewrite such names
> by removing '?' prefix and adding ".optional" suffix.
> For example:
>
>     DATASEC ?.struct_ops -> DATASEC .struct_ops.optional
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  tools/lib/bpf/features.c        | 22 ++++++++++++++++++++++
>  tools/lib/bpf/libbpf.c          | 30 +++++++++++++++++++++++++++++-
>  tools/lib/bpf/libbpf_internal.h |  2 ++
>  3 files changed, 53 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/features.c b/tools/lib/bpf/features.c
> index 6b0738ad7063..4e783cc7fc4b 100644
> --- a/tools/lib/bpf/features.c
> +++ b/tools/lib/bpf/features.c
> @@ -147,6 +147,25 @@ static int probe_kern_btf_datasec(int token_fd)
>                                              strs, sizeof(strs), token_fd));
>  }
>
> +static int probe_kern_btf_qmark_datasec(int token_fd)
> +{
> +       static const char strs[] = "\0x\0?.data";
> +       /* static int a; */
> +       __u32 types[] = {
> +               /* int */
> +               BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4),  /* [1] */
> +               /* VAR x */                                     /* [2] */
> +               BTF_TYPE_ENC(1, BTF_INFO_ENC(BTF_KIND_VAR, 0, 0), 1),
> +               BTF_VAR_STATIC,
> +               /* DATASEC ?.data */                            /* [3] */
> +               BTF_TYPE_ENC(3, BTF_INFO_ENC(BTF_KIND_DATASEC, 0, 1), 4),
> +               BTF_VAR_SECINFO_ENC(2, 0, 4),
> +       };
> +
> +       return probe_fd(libbpf__load_raw_btf((char *)types, sizeof(types),
> +                                            strs, sizeof(strs), token_fd));
> +}
> +
>  static int probe_kern_btf_float(int token_fd)
>  {
>         static const char strs[] = "\0float";
> @@ -534,6 +553,9 @@ static struct kern_feature_desc {
>         [FEAT_ARG_CTX_TAG] = {
>                 "kernel-side __arg_ctx tag", probe_kern_arg_ctx_tag,
>         },
> +       [FEAT_BTF_QMARK_DATASEC] = {
> +               "BTF DATASEC names starting from '?'", probe_kern_btf_qmark_datasec,
> +       },
>  };
>
>  bool feat_supported(struct kern_feature_cache *cache, enum kern_feature_id feat_id)
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index c0212244bdf7..cf60291db8fd 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -2874,6 +2874,11 @@ static bool section_have_execinstr(struct bpf_object *obj, int idx)
>         return sh->sh_flags & SHF_EXECINSTR;
>  }
>
> +static bool starts_with_qmark(const char *s)
> +{
> +       return s && s[0] == '?';
> +}
> +
>  static bool btf_needs_sanitization(struct bpf_object *obj)
>  {
>         bool has_func_global = kernel_supports(obj, FEAT_BTF_GLOBAL_FUNC);
> @@ -2883,9 +2888,10 @@ static bool btf_needs_sanitization(struct bpf_object *obj)
>         bool has_decl_tag = kernel_supports(obj, FEAT_BTF_DECL_TAG);
>         bool has_type_tag = kernel_supports(obj, FEAT_BTF_TYPE_TAG);
>         bool has_enum64 = kernel_supports(obj, FEAT_BTF_ENUM64);
> +       bool has_qmark_datasec = kernel_supports(obj, FEAT_BTF_QMARK_DATASEC);
>
>         return !has_func || !has_datasec || !has_func_global || !has_float ||
> -              !has_decl_tag || !has_type_tag || !has_enum64;
> +              !has_decl_tag || !has_type_tag || !has_enum64 || !has_qmark_datasec;
>  }
>
>  static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
> @@ -2897,6 +2903,7 @@ static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
>         bool has_decl_tag = kernel_supports(obj, FEAT_BTF_DECL_TAG);
>         bool has_type_tag = kernel_supports(obj, FEAT_BTF_TYPE_TAG);
>         bool has_enum64 = kernel_supports(obj, FEAT_BTF_ENUM64);
> +       bool has_qmark_datasec = kernel_supports(obj, FEAT_BTF_QMARK_DATASEC);
>         int enum64_placeholder_id = 0;
>         struct btf_type *t;
>         int i, j, vlen;
> @@ -2922,6 +2929,8 @@ static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
>                         char *name;
>
>                         name = (char *)btf__name_by_offset(btf, t->name_off);
> +                       if (*name == '?')
> +                               *name++ = '_';
>                         while (*name) {
>                                 if (*name == '.')

let's just extend this to `if (*name == '.' || *name == '?')` ?

>                                         *name = '_';
> @@ -2938,6 +2947,25 @@ static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
>                                 vt = (void *)btf__type_by_id(btf, v->type);
>                                 m->name_off = vt->name_off;
>                         }
> +               } else if (!has_qmark_datasec && btf_is_datasec(t) &&
> +                          starts_with_qmark(btf__name_by_offset(btf, t->name_off))) {
> +                       /* remove '?' prefix and add '.optional' suffix for
> +                        * DATASEC names staring from '?':
> +                        *
> +                        *   DATASEC ?.foo -> DATASEC .foo.optional
> +                        */
> +                       const char *name;
> +                       char buf[256];
> +                       int str;
> +
> +                       name = btf__name_by_offset(btf, t->name_off);
> +                       snprintf(buf, sizeof(buf), "%s.optional", &name[1] /* skip '?' */);
> +                       str = btf__add_str(btf, buf);
> +                       if (str < 0)
> +                               return str;
> +
> +                       t = (struct btf_type *)btf__type_by_id(btf, i);
> +                       t->name_off = str;

let's keep it simpler, just do in-place name sanitization like we did
for !has_datasec case? It's fine if "?.struct_ops" becomes
"_.struct_ops", kernel doesn't care and doesn't assign any special
meaning to DATASEC names

>                 } else if (!has_func && btf_is_func_proto(t)) {
>                         /* replace FUNC_PROTO with ENUM */
>                         vlen = btf_vlen(t);
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index ad936ac5e639..864b36177424 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -374,6 +374,8 @@ enum kern_feature_id {
>         FEAT_UPROBE_MULTI_LINK,
>         /* Kernel supports arg:ctx tag (__arg_ctx) for global subprogs natively */
>         FEAT_ARG_CTX_TAG,
> +       /* Kernel supports '?' at the front of datasec names */
> +       FEAT_BTF_QMARK_DATASEC,
>         __FEAT_CNT,
>  };
>
> --
> 2.43.0
>
Eduard Zingerman March 6, 2024, 1:34 a.m. UTC | #2 
On Tue, 2024-03-05 at 12:03 -0800, Andrii Nakryiko wrote:
[...]

> > @@ -2922,6 +2929,8 @@ static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
> >                         char *name;
> > 
> >                         name = (char *)btf__name_by_offset(btf, t->name_off);
> > +                       if (*name == '?')
> > +                               *name++ = '_';
> >                         while (*name) {
> >                                 if (*name == '.')
> 
> let's just extend this to `if (*name == '.' || *name == '?')` ?

Ok.

> > @@ -2938,6 +2947,25 @@ static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
> >                                 vt = (void *)btf__type_by_id(btf, v->type);
> >                                 m->name_off = vt->name_off;
> >                         }
> > +               } else if (!has_qmark_datasec && btf_is_datasec(t) &&
> > +                          starts_with_qmark(btf__name_by_offset(btf, t->name_off))) {
> > +                       /* remove '?' prefix and add '.optional' suffix for
> > +                        * DATASEC names staring from '?':
> > +                        *
> > +                        *   DATASEC ?.foo -> DATASEC .foo.optional
> > +                        */
> > +                       const char *name;
> > +                       char buf[256];
> > +                       int str;
> > +
> > +                       name = btf__name_by_offset(btf, t->name_off);
> > +                       snprintf(buf, sizeof(buf), "%s.optional", &name[1] /* skip '?' */);
> > +                       str = btf__add_str(btf, buf);
> > +                       if (str < 0)
> > +                               return str;
> > +
> > +                       t = (struct btf_type *)btf__type_by_id(btf, i);
> > +                       t->name_off = str;
> 
> let's keep it simpler, just do in-place name sanitization like we did
> for !has_datasec case? It's fine if "?.struct_ops" becomes
> "_.struct_ops", kernel doesn't care and doesn't assign any special
> meaning to DATASEC names

Well, in theory this string is shared between several locations,
though this is probably highly unlikely.
Anyways, I made requested change.

[...]
diff mbox series

Patch

diff --git a/tools/lib/bpf/features.c b/tools/lib/bpf/features.c
index 6b0738ad7063..4e783cc7fc4b 100644
--- a/tools/lib/bpf/features.c
+++ b/tools/lib/bpf/features.c
@@ -147,6 +147,25 @@  static int probe_kern_btf_datasec(int token_fd)
 					     strs, sizeof(strs), token_fd));
 }
 
+static int probe_kern_btf_qmark_datasec(int token_fd)
+{
+	static const char strs[] = "\0x\0?.data";
+	/* static int a; */
+	__u32 types[] = {
+		/* int */
+		BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4),  /* [1] */
+		/* VAR x */                                     /* [2] */
+		BTF_TYPE_ENC(1, BTF_INFO_ENC(BTF_KIND_VAR, 0, 0), 1),
+		BTF_VAR_STATIC,
+		/* DATASEC ?.data */                            /* [3] */
+		BTF_TYPE_ENC(3, BTF_INFO_ENC(BTF_KIND_DATASEC, 0, 1), 4),
+		BTF_VAR_SECINFO_ENC(2, 0, 4),
+	};
+
+	return probe_fd(libbpf__load_raw_btf((char *)types, sizeof(types),
+					     strs, sizeof(strs), token_fd));
+}
+
 static int probe_kern_btf_float(int token_fd)
 {
 	static const char strs[] = "\0float";
@@ -534,6 +553,9 @@  static struct kern_feature_desc {
 	[FEAT_ARG_CTX_TAG] = {
 		"kernel-side __arg_ctx tag", probe_kern_arg_ctx_tag,
 	},
+	[FEAT_BTF_QMARK_DATASEC] = {
+		"BTF DATASEC names starting from '?'", probe_kern_btf_qmark_datasec,
+	},
 };
 
 bool feat_supported(struct kern_feature_cache *cache, enum kern_feature_id feat_id)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index c0212244bdf7..cf60291db8fd 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -2874,6 +2874,11 @@  static bool section_have_execinstr(struct bpf_object *obj, int idx)
 	return sh->sh_flags & SHF_EXECINSTR;
 }
 
+static bool starts_with_qmark(const char *s)
+{
+	return s && s[0] == '?';
+}
+
 static bool btf_needs_sanitization(struct bpf_object *obj)
 {
 	bool has_func_global = kernel_supports(obj, FEAT_BTF_GLOBAL_FUNC);
@@ -2883,9 +2888,10 @@  static bool btf_needs_sanitization(struct bpf_object *obj)
 	bool has_decl_tag = kernel_supports(obj, FEAT_BTF_DECL_TAG);
 	bool has_type_tag = kernel_supports(obj, FEAT_BTF_TYPE_TAG);
 	bool has_enum64 = kernel_supports(obj, FEAT_BTF_ENUM64);
+	bool has_qmark_datasec = kernel_supports(obj, FEAT_BTF_QMARK_DATASEC);
 
 	return !has_func || !has_datasec || !has_func_global || !has_float ||
-	       !has_decl_tag || !has_type_tag || !has_enum64;
+	       !has_decl_tag || !has_type_tag || !has_enum64 || !has_qmark_datasec;
 }
 
 static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
@@ -2897,6 +2903,7 @@  static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
 	bool has_decl_tag = kernel_supports(obj, FEAT_BTF_DECL_TAG);
 	bool has_type_tag = kernel_supports(obj, FEAT_BTF_TYPE_TAG);
 	bool has_enum64 = kernel_supports(obj, FEAT_BTF_ENUM64);
+	bool has_qmark_datasec = kernel_supports(obj, FEAT_BTF_QMARK_DATASEC);
 	int enum64_placeholder_id = 0;
 	struct btf_type *t;
 	int i, j, vlen;
@@ -2922,6 +2929,8 @@  static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
 			char *name;
 
 			name = (char *)btf__name_by_offset(btf, t->name_off);
+			if (*name == '?')
+				*name++ = '_';
 			while (*name) {
 				if (*name == '.')
 					*name = '_';
@@ -2938,6 +2947,25 @@  static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
 				vt = (void *)btf__type_by_id(btf, v->type);
 				m->name_off = vt->name_off;
 			}
+		} else if (!has_qmark_datasec && btf_is_datasec(t) &&
+			   starts_with_qmark(btf__name_by_offset(btf, t->name_off))) {
+			/* remove '?' prefix and add '.optional' suffix for
+			 * DATASEC names staring from '?':
+			 *
+			 *   DATASEC ?.foo -> DATASEC .foo.optional
+			 */
+			const char *name;
+			char buf[256];
+			int str;
+
+			name = btf__name_by_offset(btf, t->name_off);
+			snprintf(buf, sizeof(buf), "%s.optional", &name[1] /* skip '?' */);
+			str = btf__add_str(btf, buf);
+			if (str < 0)
+				return str;
+
+			t = (struct btf_type *)btf__type_by_id(btf, i);
+			t->name_off = str;
 		} else if (!has_func && btf_is_func_proto(t)) {
 			/* replace FUNC_PROTO with ENUM */
 			vlen = btf_vlen(t);
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index ad936ac5e639..864b36177424 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -374,6 +374,8 @@  enum kern_feature_id {
 	FEAT_UPROBE_MULTI_LINK,
 	/* Kernel supports arg:ctx tag (__arg_ctx) for global subprogs natively */
 	FEAT_ARG_CTX_TAG,
+	/* Kernel supports '?' at the front of datasec names */
+	FEAT_BTF_QMARK_DATASEC,
 	__FEAT_CNT,
 };