bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH dwarves 0/3] dwarves: detect BTF kinds supported by kernel
@ 2023-09-13 14:26 Alan Maguire
  2023-09-13 14:26 ` [PATCH dwarves 1/3] dwarves: auto-detect maximum kind supported by vmlinux Alan Maguire
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Alan Maguire @ 2023-09-13 14:26 UTC (permalink / raw)
  To: acme
  Cc: andrii.nakryiko, ast, daniel, jolsa, eddyz87, martin.lau, song,
	yhs, john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf,
	Alan Maguire

When a newer pahole is run on an older kernel, it often knows about BTF
kinds that the kernel does not support, and adds them to the BTF
representation.  This is a problem because the BTF generated is then
embedded in the kernel image.  When it is later read - possibly by
a different older toolchain or by the kernel directly - it is not usable.

The scripts/pahole-flags.sh script enumerates the various pahole options
available associated with various versions of pahole, but in the case
of an older kernel is the set of BTF kinds the _kernel_ can handle that
is of more importance.

Because recent features such as BTF_KIND_ENUM64 are added by default
(and only skipped if --skip_encoding_btf_* is set), BTF will be
created with these newer kinds that the older kernel cannot read.
This can be (and has been) fixed by stable-backporting --skip options,
but this is cumbersome and would have to be done every time a new BTF kind
is introduced.

So this series attempts to detect the BTF kinds supported by the
kernel/modules so that this can inform BTF encoding for older
kernels.  We look for BTF_KIND_MAX - either as an enumerated value
in vmlinux DWARF (patch 1) or as an enumerated value in base vmlinux
BTF (patch 3).  Knowing this prior to encoding BTF allows us to specify
skip_encoding options to avoid having BTF with kinds the kernel itself
will not understand.

The aim is to minimize pain for older stable kernels when new BTF
kinds are introduced.  Kind encoding [1] can solve the parsing problem
with BTF, but this approach is intended to ensure generated BTF is
usable when newer pahole runs on older kernels.

This approach requires BTF kinds to be defined via an enumerated type,
which happened for 5.16 and later.  Older kernels than this used #defines
so the approach will only work for 5.16 stable kernels and later currently.

With this change in hand, adding new BTF kinds becomes a bit simpler,
at least for the user of pahole.  All that needs to be done is to add
internal "skip_new_kind" booleans to struct conf_load and set them
in dwarves__set_btf_kind_max() if the detected maximum kind is less
than the kind in question - in other words, if the kernel does not know
about that kind.  In that case, we will not use it in encoding.

The approach was tested on Linux 5.16 as released, i.e. prior to the
backports adding --skip_encoding logic, and the BTF generated did not
contain kinds > BTF_KIND_MAX for the kernel (corresponding to
BTF_KIND_DECL_TAG in that case).

Changes since RFC [2]:
 - added --skip_autodetect_btf_kind_max to disable kind autodetection
   (Jiri, patch 2)

[1] https://lore.kernel.org/bpf/20230616171728.530116-1-alan.maguire@oracle.com/
[2] https://lore.kernel.org/bpf/20230720201443.224040-1-alan.maguire@oracle.com/

Alan Maguire (3):
  dwarves: auto-detect maximum kind supported by vmlinux
  pahole: add --skip_autodetect_btf_kind_max to disable kind autodetect
  btf_encoder: learn BTF_KIND_MAX value from base BTF when generating
    split BTF

 btf_encoder.c      | 37 +++++++++++++++++++++++++++++++++
 btf_encoder.h      |  2 ++
 dwarf_loader.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++++
 dwarves.h          |  3 +++
 man-pages/pahole.1 |  4 ++++
 pahole.c           | 10 +++++++++
 6 files changed, 108 insertions(+)

-- 
2.39.3


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH dwarves 1/3] dwarves: auto-detect maximum kind supported by vmlinux
  2023-09-13 14:26 [PATCH dwarves 0/3] dwarves: detect BTF kinds supported by kernel Alan Maguire
@ 2023-09-13 14:26 ` Alan Maguire
  2023-09-13 16:58   ` Eduard Zingerman
  2023-09-13 14:26 ` [PATCH dwarves 2/3] pahole: add --skip_autodetect_btf_kind_max to disable kind autodetect Alan Maguire
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Alan Maguire @ 2023-09-13 14:26 UTC (permalink / raw)
  To: acme
  Cc: andrii.nakryiko, ast, daniel, jolsa, eddyz87, martin.lau, song,
	yhs, john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf,
	Alan Maguire

When a newer pahole is run on an older kernel, it often knows about BTF
kinds that the kernel does not support.  This is a problem because the BTF
generated is then embedded in the kernel image and read, and if unknown
kinds are found, BTF handling fails and core BPF functionality is
unavailable.

The scripts/pahole-flags.sh script enumerates the various pahole options
available associated with various versions of pahole, but the problem is
what matters in the case of an older kernel is the set of kinds the kernel
understands.  Because recent features such as BTF_KIND_ENUM64 are added
by default (and only skipped if --skip_encoding_btf_* is set), BTF will
be created with these newer kinds that the older kernel cannot read.
This can be fixed by stable-backporting --skip options, but this is
cumbersome and would have to be done every time a new BTF kind is
introduced.

Here instead we pre-process the DWARF information associated with the
target for BTF generation; if we find an enum with a BTF_KIND_MAX
value in the DWARF associated with the object, we use that to
determine the maximum BTF kind supported.  Note that the enum
representation of BTF kinds starts for the 5.16 kernel; prior to this
The benefit of auto-detection is that no work is required for older
kernels when new kinds are added, and --skip_encoding options are
less needed.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 btf_encoder.c  | 12 ++++++++++++
 dwarf_loader.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
 dwarves.h      |  2 ++
 3 files changed, 66 insertions(+)

diff --git a/btf_encoder.c b/btf_encoder.c
index 65f6e71..98c7529 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -1889,3 +1889,15 @@ struct btf *btf_encoder__btf(struct btf_encoder *encoder)
 {
 	return encoder->btf;
 }
+
+void dwarves__set_btf_kind_max(struct conf_load *conf_load, int btf_kind_max)
+{
+	if (btf_kind_max < 0 || btf_kind_max >= BTF_KIND_MAX)
+		return;
+	if (btf_kind_max < BTF_KIND_DECL_TAG)
+		conf_load->skip_encoding_btf_decl_tag = true;
+	if (btf_kind_max < BTF_KIND_TYPE_TAG)
+		conf_load->skip_encoding_btf_type_tag = true;
+	if (btf_kind_max < BTF_KIND_ENUM64)
+		conf_load->skip_encoding_btf_enum64 = true;
+}
diff --git a/dwarf_loader.c b/dwarf_loader.c
index ccf3194..8984043 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -3358,8 +3358,60 @@ static int __dwarf_cus__process_cus(struct dwarf_cus *dcus)
 	return 0;
 }
 
+/* Find enumeration value for BTF_KIND_MAX; replace conf_load->btf_kind_max with
+ * this value if found since it indicates that the target object does not know
+ * about kinds > its BTF_KIND_MAX value.  This is valuable for kernel/module
+ * BTF where a newer pahole/libbpf operate on an older kernel which cannot
+ * parse some of the newer kinds pahole can generate.
+ */
+static void dwarf__find_btf_kind_max(struct dwarf_cus *dcus)
+{
+	struct conf_load *conf = dcus->conf;
+	uint8_t pointer_size, offset_size;
+	Dwarf_Off off = 0, noff;
+	size_t cuhl;
+
+	while (dwarf_nextcu(dcus->dw, off, &noff, &cuhl, NULL, &pointer_size, &offset_size) == 0) {
+		Dwarf_Die die_mem;
+		Dwarf_Die *cu_die = dwarf_offdie(dcus->dw, off + cuhl, &die_mem);
+		Dwarf_Die child;
+
+		if (cu_die == NULL)
+			break;
+		if (dwarf_child(cu_die, &child) == 0) {
+			Dwarf_Die *die = &child;
+
+			do {
+				Dwarf_Die echild, *edie;
+
+				if (dwarf_tag(die) != DW_TAG_enumeration_type ||
+				    !dwarf_haschildren(die) ||
+				    dwarf_child(die, &echild) != 0)
+					continue;
+				edie = &echild;
+				do {
+					const char *ename;
+					int btf_kind_max;
+
+					if (dwarf_tag(edie) != DW_TAG_enumerator)
+						continue;
+					ename = attr_string(edie, DW_AT_name, conf);
+					if (!ename || strcmp(ename, "BTF_KIND_MAX") != 0)
+						continue;
+					btf_kind_max = attr_numeric(edie, DW_AT_const_value);
+					dwarves__set_btf_kind_max(conf, btf_kind_max);
+					return;
+				} while (dwarf_siblingof(edie, edie) == 0);
+			} while (dwarf_siblingof(die, die) == 0);
+		}
+		off = noff;
+	}
+}
+
 static int dwarf_cus__process_cus(struct dwarf_cus *dcus)
 {
+	dwarf__find_btf_kind_max(dcus);
+
 	if (dcus->conf->nr_jobs > 1)
 		return dwarf_cus__threaded_process_cus(dcus);
 
diff --git a/dwarves.h b/dwarves.h
index eb1a6df..f4d9347 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -1480,4 +1480,6 @@ extern const char tabs[];
 #define DW_TAG_skeleton_unit 0x4a
 #endif
 
+void dwarves__set_btf_kind_max(struct conf_load *conf_load, int btf_kind_max);
+
 #endif /* _DWARVES_H_ */
-- 
2.39.3


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH dwarves 2/3] pahole: add --skip_autodetect_btf_kind_max to disable kind autodetect
  2023-09-13 14:26 [PATCH dwarves 0/3] dwarves: detect BTF kinds supported by kernel Alan Maguire
  2023-09-13 14:26 ` [PATCH dwarves 1/3] dwarves: auto-detect maximum kind supported by vmlinux Alan Maguire
@ 2023-09-13 14:26 ` Alan Maguire
  2023-09-13 14:26 ` [PATCH dwarves 3/3] btf_encoder: learn BTF_KIND_MAX value from base BTF when generating split BTF Alan Maguire
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Alan Maguire @ 2023-09-13 14:26 UTC (permalink / raw)
  To: acme
  Cc: andrii.nakryiko, ast, daniel, jolsa, eddyz87, martin.lau, song,
	yhs, john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf,
	Alan Maguire, Jiri Olsa

Autodetection of BTF kinds supported may not be wanted or may be
broken at some point; it is prudent to provide a way to switch it
off.

Suggested-by: Jiri Olsa <olsajiri@gmail.com>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 btf_encoder.c      | 3 ++-
 dwarves.h          | 1 +
 man-pages/pahole.1 | 4 ++++
 pahole.c           | 8 ++++++++
 4 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 98c7529..ad0158f 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -1892,7 +1892,8 @@ struct btf *btf_encoder__btf(struct btf_encoder *encoder)
 
 void dwarves__set_btf_kind_max(struct conf_load *conf_load, int btf_kind_max)
 {
-	if (btf_kind_max < 0 || btf_kind_max >= BTF_KIND_MAX)
+	if (conf_load->skip_autodetect_btf_kind_max ||
+	    btf_kind_max < 0 || btf_kind_max >= BTF_KIND_MAX)
 		return;
 	if (btf_kind_max < BTF_KIND_DECL_TAG)
 		conf_load->skip_encoding_btf_decl_tag = true;
diff --git a/dwarves.h b/dwarves.h
index f4d9347..04a4c29 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -68,6 +68,7 @@ struct conf_load {
 	bool			skip_encoding_btf_enum64;
 	bool			btf_gen_optimized;
 	bool			skip_encoding_btf_inconsistent_proto;
+	bool			skip_autodetect_btf_kind_max;
 	uint8_t			hashtable_bits;
 	uint8_t			max_hashtable_bits;
 	uint16_t		kabi_prefix_len;
diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
index c1b48de..523d4fd 100644
--- a/man-pages/pahole.1
+++ b/man-pages/pahole.1
@@ -233,6 +233,10 @@ Do not encode type tags in BTF.
 .B \-\-skip_encoding_btf_inconsistent_proto
 Do not encode functions with multiple inconsistent prototypes or unexpected register use for their parameters, where the registers used do not match calling conventions.
 
+.TP
+.B \-\-skip_autodetect_btf_kind_max
+Do not scan DWARF to find out which BTF kinds are supported by the underlying object.
+
 .TP
 .B \-j, \-\-jobs=N
 Run N jobs in parallel. Defaults to number of online processors + 10% (like
diff --git a/pahole.c b/pahole.c
index e843999..aca2704 100644
--- a/pahole.c
+++ b/pahole.c
@@ -1232,6 +1232,7 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version;
 #define ARGP_skip_emitting_atomic_typedefs 338
 #define ARGP_btf_gen_optimized  339
 #define ARGP_skip_encoding_btf_inconsistent_proto 340
+#define ARGP_skip_autodetect_btf_kind_max 341
 
 static const struct argp_option pahole__options[] = {
 	{
@@ -1654,6 +1655,11 @@ static const struct argp_option pahole__options[] = {
 		.key = ARGP_skip_encoding_btf_inconsistent_proto,
 		.doc = "Skip functions that have multiple inconsistent function prototypes sharing the same name, or that use unexpected registers for parameter values."
 	},
+	{
+		.name = "skip_autodetect_btf_kind_max",
+		.key = ARGP_skip_autodetect_btf_kind_max,
+		.doc = "Skip auto-detection of maximum BTF kind supported."
+	},
 	{
 		.name = NULL,
 	}
@@ -1829,6 +1835,8 @@ static error_t pahole__options_parser(int key, char *arg,
 		conf_load.btf_gen_optimized = true;		break;
 	case ARGP_skip_encoding_btf_inconsistent_proto:
 		conf_load.skip_encoding_btf_inconsistent_proto = true; break;
+	case ARGP_skip_autodetect_btf_kind_max:
+		conf_load.skip_autodetect_btf_kind_max = true;	break;
 	default:
 		return ARGP_ERR_UNKNOWN;
 	}
-- 
2.39.3


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH dwarves 3/3] btf_encoder: learn BTF_KIND_MAX value from base BTF when generating split BTF
  2023-09-13 14:26 [PATCH dwarves 0/3] dwarves: detect BTF kinds supported by kernel Alan Maguire
  2023-09-13 14:26 ` [PATCH dwarves 1/3] dwarves: auto-detect maximum kind supported by vmlinux Alan Maguire
  2023-09-13 14:26 ` [PATCH dwarves 2/3] pahole: add --skip_autodetect_btf_kind_max to disable kind autodetect Alan Maguire
@ 2023-09-13 14:26 ` Alan Maguire
  2023-09-13 16:58   ` Eduard Zingerman
  2023-09-13 16:58 ` [PATCH dwarves 0/3] dwarves: detect BTF kinds supported by kernel Eduard Zingerman
  2023-09-14 17:58 ` Andrii Nakryiko
  4 siblings, 1 reply; 13+ messages in thread
From: Alan Maguire @ 2023-09-13 14:26 UTC (permalink / raw)
  To: acme
  Cc: andrii.nakryiko, ast, daniel, jolsa, eddyz87, martin.lau, song,
	yhs, john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf,
	Alan Maguire

When creating module BTF, the module likely will not have a DWARF
specificiation of BTF_KIND_MAX, so look for it in the base BTF.  For
vmlinux base BTF, the enumeration value is present, so the base BTF
can be checked to limit BTF kind representation.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 btf_encoder.c | 24 ++++++++++++++++++++++++
 btf_encoder.h |  2 ++
 pahole.c      |  2 ++
 3 files changed, 28 insertions(+)

diff --git a/btf_encoder.c b/btf_encoder.c
index ad0158f..6cb3df6 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -1902,3 +1902,27 @@ void dwarves__set_btf_kind_max(struct conf_load *conf_load, int btf_kind_max)
 	if (btf_kind_max < BTF_KIND_ENUM64)
 		conf_load->skip_encoding_btf_enum64 = true;
 }
+
+void btf__set_btf_kind_max(struct conf_load *conf_load, struct btf *btf)
+{
+	__u32 id, type_cnt = btf__type_cnt(btf);
+
+	for (id = 1; id < type_cnt; id++) {
+		const struct btf_type *t = btf__type_by_id(btf, id);
+		const struct btf_enum *e;
+		__u16 vlen, i;
+
+		if (!t || !btf_is_enum(t))
+			continue;
+		vlen = btf_vlen(t);
+		e = btf_enum(t);
+		for (i = 0; i < vlen; e++, i++) {
+			const char *name = btf__name_by_offset(btf, e->name_off);
+
+			if (!name || strcmp(name, "BTF_KIND_MAX"))
+				continue;
+			dwarves__set_btf_kind_max(conf_load, e->val);
+			return;
+		}
+	}
+}
diff --git a/btf_encoder.h b/btf_encoder.h
index 34516bb..e5e12ef 100644
--- a/btf_encoder.h
+++ b/btf_encoder.h
@@ -27,4 +27,6 @@ struct btf *btf_encoder__btf(struct btf_encoder *encoder);
 
 int btf_encoder__add_encoder(struct btf_encoder *encoder, struct btf_encoder *other);
 
+void btf__set_btf_kind_max(struct conf_load *conf_load, struct btf *btf);
+
 #endif /* _BTF_ENCODER_H_ */
diff --git a/pahole.c b/pahole.c
index aca2704..4d6d059 100644
--- a/pahole.c
+++ b/pahole.c
@@ -3470,6 +3470,7 @@ int main(int argc, char *argv[])
 				base_btf_file, libbpf_get_error(conf_load.base_btf));
 			goto out;
 		}
+		btf__set_btf_kind_max(&conf_load, conf_load.base_btf);
 		if (!btf_encode && !ctf_encode) {
 			// Force "btf" since a btf_base is being informed
 			conf_load.format_path = "btf";
@@ -3513,6 +3514,7 @@ try_sole_arg_as_class_names:
 					base_btf_file, libbpf_get_error(conf_load.base_btf));
 				goto out;
 			}
+			btf__set_btf_kind_max(&conf_load, conf_load.base_btf);
 		}
 	}
 
-- 
2.39.3


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH dwarves 0/3] dwarves: detect BTF kinds supported by kernel
  2023-09-13 14:26 [PATCH dwarves 0/3] dwarves: detect BTF kinds supported by kernel Alan Maguire
                   ` (2 preceding siblings ...)
  2023-09-13 14:26 ` [PATCH dwarves 3/3] btf_encoder: learn BTF_KIND_MAX value from base BTF when generating split BTF Alan Maguire
@ 2023-09-13 16:58 ` Eduard Zingerman
  2023-09-14 17:58 ` Andrii Nakryiko
  4 siblings, 0 replies; 13+ messages in thread
From: Eduard Zingerman @ 2023-09-13 16:58 UTC (permalink / raw)
  To: Alan Maguire, acme
  Cc: andrii.nakryiko, ast, daniel, jolsa, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf

On Wed, 2023-09-13 at 15:26 +0100, Alan Maguire wrote:
> > > > When a newer pahole is run on an older kernel, it often knows about BTF
> > > > kinds that the kernel does not support, and adds them to the BTF
> > > > representation.  This is a problem because the BTF generated is then
> > > > embedded in the kernel image.  When it is later read - possibly by
> > > > a different older toolchain or by the kernel directly - it is not usable.
> > > > 
> > > > The scripts/pahole-flags.sh script enumerates the various pahole options
> > > > available associated with various versions of pahole, but in the case
> > > > of an older kernel is the set of BTF kinds the _kernel_ can handle that
> > > > is of more importance.
> > > > 
> > > > Because recent features such as BTF_KIND_ENUM64 are added by default
> > > > (and only skipped if --skip_encoding_btf_* is set), BTF will be
> > > > created with these newer kinds that the older kernel cannot read.
> > > > This can be (and has been) fixed by stable-backporting --skip options,
> > > > but this is cumbersome and would have to be done every time a new BTF kind
> > > > is introduced.
> > > > 
> > > > So this series attempts to detect the BTF kinds supported by the
> > > > kernel/modules so that this can inform BTF encoding for older
> > > > kernels.  We look for BTF_KIND_MAX - either as an enumerated value
> > > > in vmlinux DWARF (patch 1) or as an enumerated value in base vmlinux
> > > > BTF (patch 3).  Knowing this prior to encoding BTF allows us to specify
> > > > skip_encoding options to avoid having BTF with kinds the kernel itself
> > > > will not understand.
> > > > 
> > > > The aim is to minimize pain for older stable kernels when new BTF
> > > > kinds are introduced.  Kind encoding [1] can solve the parsing problem
> > > > with BTF, but this approach is intended to ensure generated BTF is
> > > > usable when newer pahole runs on older kernels.
> > > > 
> > > > This approach requires BTF kinds to be defined via an enumerated type,
> > > > which happened for 5.16 and later.  Older kernels than this used #defines
> > > > so the approach will only work for 5.16 stable kernels and later currently.
> > > > 
> > > > With this change in hand, adding new BTF kinds becomes a bit simpler,
> > > > at least for the user of pahole.  All that needs to be done is to add
> > > > internal "skip_new_kind" booleans to struct conf_load and set them
> > > > in dwarves__set_btf_kind_max() if the detected maximum kind is less
> > > > than the kind in question - in other words, if the kernel does not know
> > > > about that kind.  In that case, we will not use it in encoding.
> > > > 
> > > > The approach was tested on Linux 5.16 as released, i.e. prior to the
> > > > backports adding --skip_encoding logic, and the BTF generated did not
> > > > contain kinds > BTF_KIND_MAX for the kernel (corresponding to
> > > > BTF_KIND_DECL_TAG in that case).

Hi Alan,

I tested this patch (by tweaking BTF_KIND_MAX value and comparing generated BTF)
and it seems to work as intended. Left a few nitpicks.

Thanks,
Eduard


> > > > 
> > > > Changes since RFC [2]:
> > > >  - added --skip_autodetect_btf_kind_max to disable kind autodetection
> > > >    (Jiri, patch 2)
> > > > 
> > > > [1] https://lore.kernel.org/bpf/20230616171728.530116-1-alan.maguire@oracle.com/
> > > > [2] https://lore.kernel.org/bpf/20230720201443.224040-1-alan.maguire@oracle.com/
> > > > 
> > > > Alan Maguire (3):
> > > >   dwarves: auto-detect maximum kind supported by vmlinux
> > > >   pahole: add --skip_autodetect_btf_kind_max to disable kind autodetect
> > > >   btf_encoder: learn BTF_KIND_MAX value from base BTF when generating
> > > >     split BTF
> > > > 
> > > >  btf_encoder.c      | 37 +++++++++++++++++++++++++++++++++
> > > >  btf_encoder.h      |  2 ++
> > > >  dwarf_loader.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++++
> > > >  dwarves.h          |  3 +++
> > > >  man-pages/pahole.1 |  4 ++++
> > > >  pahole.c           | 10 +++++++++
> > > >  6 files changed, 108 insertions(+)
> > > > 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH dwarves 1/3] dwarves: auto-detect maximum kind supported by vmlinux
  2023-09-13 14:26 ` [PATCH dwarves 1/3] dwarves: auto-detect maximum kind supported by vmlinux Alan Maguire
@ 2023-09-13 16:58   ` Eduard Zingerman
  2023-09-13 17:44     ` Alan Maguire
  0 siblings, 1 reply; 13+ messages in thread
From: Eduard Zingerman @ 2023-09-13 16:58 UTC (permalink / raw)
  To: Alan Maguire, acme
  Cc: andrii.nakryiko, ast, daniel, jolsa, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf

On Wed, 2023-09-13 at 15:26 +0100, Alan Maguire wrote:
> When a newer pahole is run on an older kernel, it often knows about BTF
> kinds that the kernel does not support.  This is a problem because the BTF
> generated is then embedded in the kernel image and read, and if unknown
> kinds are found, BTF handling fails and core BPF functionality is
> unavailable.
> 
> The scripts/pahole-flags.sh script enumerates the various pahole options
> available associated with various versions of pahole, but the problem is
> what matters in the case of an older kernel is the set of kinds the kernel
> understands.  Because recent features such as BTF_KIND_ENUM64 are added
> by default (and only skipped if --skip_encoding_btf_* is set), BTF will
> be created with these newer kinds that the older kernel cannot read.
> This can be fixed by stable-backporting --skip options, but this is
> cumbersome and would have to be done every time a new BTF kind is
> introduced.
> 
> Here instead we pre-process the DWARF information associated with the
> target for BTF generation; if we find an enum with a BTF_KIND_MAX
> value in the DWARF associated with the object, we use that to
> determine the maximum BTF kind supported.  Note that the enum
> representation of BTF kinds starts for the 5.16 kernel; prior to this
> The benefit of auto-detection is that no work is required for older
> kernels when new kinds are added, and --skip_encoding options are
> less needed.
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  btf_encoder.c  | 12 ++++++++++++
>  dwarf_loader.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  dwarves.h      |  2 ++
>  3 files changed, 66 insertions(+)
> 
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 65f6e71..98c7529 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -1889,3 +1889,15 @@ struct btf *btf_encoder__btf(struct btf_encoder *encoder)
>  {
>  	return encoder->btf;
>  }
> +
> +void dwarves__set_btf_kind_max(struct conf_load *conf_load, int btf_kind_max)
> +{
> +	if (btf_kind_max < 0 || btf_kind_max >= BTF_KIND_MAX)
> +		return;
> +	if (btf_kind_max < BTF_KIND_DECL_TAG)
> +		conf_load->skip_encoding_btf_decl_tag = true;
> +	if (btf_kind_max < BTF_KIND_TYPE_TAG)
> +		conf_load->skip_encoding_btf_type_tag = true;
> +	if (btf_kind_max < BTF_KIND_ENUM64)
> +		conf_load->skip_encoding_btf_enum64 = true;
> +}
> diff --git a/dwarf_loader.c b/dwarf_loader.c
> index ccf3194..8984043 100644
> --- a/dwarf_loader.c
> +++ b/dwarf_loader.c
> @@ -3358,8 +3358,60 @@ static int __dwarf_cus__process_cus(struct dwarf_cus *dcus)
>  	return 0;
>  }
>  
> +/* Find enumeration value for BTF_KIND_MAX; replace conf_load->btf_kind_max with
> + * this value if found since it indicates that the target object does not know
> + * about kinds > its BTF_KIND_MAX value.  This is valuable for kernel/module
> + * BTF where a newer pahole/libbpf operate on an older kernel which cannot
> + * parse some of the newer kinds pahole can generate.
> + */
> +static void dwarf__find_btf_kind_max(struct dwarf_cus *dcus)
> +{
> +	struct conf_load *conf = dcus->conf;
> +	uint8_t pointer_size, offset_size;
> +	Dwarf_Off off = 0, noff;
> +	size_t cuhl;
> +
> +	while (dwarf_nextcu(dcus->dw, off, &noff, &cuhl, NULL, &pointer_size, &offset_size) == 0) {
> +		Dwarf_Die die_mem;
> +		Dwarf_Die *cu_die = dwarf_offdie(dcus->dw, off + cuhl, &die_mem);
> +		Dwarf_Die child;
> +
> +		if (cu_die == NULL)
> +			break;
> +		if (dwarf_child(cu_die, &child) == 0) {
> +			Dwarf_Die *die = &child;
> +
> +			do {
> +				Dwarf_Die echild, *edie;
> +
> +				if (dwarf_tag(die) != DW_TAG_enumeration_type ||
> +				    !dwarf_haschildren(die) ||
> +				    dwarf_child(die, &echild) != 0)
> +					continue;
> +				edie = &echild;
> +				do {
> +					const char *ename;
> +					int btf_kind_max;
> +
> +					if (dwarf_tag(edie) != DW_TAG_enumerator)
> +						continue;
> +					ename = attr_string(edie, DW_AT_name, conf);
> +					if (!ename || strcmp(ename, "BTF_KIND_MAX") != 0)
> +						continue;
> +					btf_kind_max = attr_numeric(edie, DW_AT_const_value);

Nitpick: attr_numeric() returns 0 in case of an error, when 0 is passed to
         dwarves__set_btf_kind_max() it would turn off all optional kinds.
         Probably should bail out on 0 instead.

> +					dwarves__set_btf_kind_max(conf, btf_kind_max);
> +					return;
> +				} while (dwarf_siblingof(edie, edie) == 0);
> +			} while (dwarf_siblingof(die, die) == 0);
> +		}
> +		off = noff;
> +	}
> +}
> +
>  static int dwarf_cus__process_cus(struct dwarf_cus *dcus)
>  {
> +	dwarf__find_btf_kind_max(dcus);
> +
>  	if (dcus->conf->nr_jobs > 1)
>  		return dwarf_cus__threaded_process_cus(dcus);
>  
> diff --git a/dwarves.h b/dwarves.h
> index eb1a6df..f4d9347 100644
> --- a/dwarves.h
> +++ b/dwarves.h
> @@ -1480,4 +1480,6 @@ extern const char tabs[];
>  #define DW_TAG_skeleton_unit 0x4a
>  #endif
>  
> +void dwarves__set_btf_kind_max(struct conf_load *conf_load, int btf_kind_max);
> +
>  #endif /* _DWARVES_H_ */



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH dwarves 3/3] btf_encoder: learn BTF_KIND_MAX value from base BTF when generating split BTF
  2023-09-13 14:26 ` [PATCH dwarves 3/3] btf_encoder: learn BTF_KIND_MAX value from base BTF when generating split BTF Alan Maguire
@ 2023-09-13 16:58   ` Eduard Zingerman
  0 siblings, 0 replies; 13+ messages in thread
From: Eduard Zingerman @ 2023-09-13 16:58 UTC (permalink / raw)
  To: Alan Maguire, acme
  Cc: andrii.nakryiko, ast, daniel, jolsa, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf

On Wed, 2023-09-13 at 15:26 +0100, Alan Maguire wrote:
> > When creating module BTF, the module likely will not have a DWARF
> > specificiation of BTF_KIND_MAX, so look for it in the base BTF.  For
> > vmlinux base BTF, the enumeration value is present, so the base BTF
> > can be checked to limit BTF kind representation.
> > 
> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > ---
> >  btf_encoder.c | 24 ++++++++++++++++++++++++
> >  btf_encoder.h |  2 ++
> >  pahole.c      |  2 ++
> >  3 files changed, 28 insertions(+)
> > 
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index ad0158f..6cb3df6 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> > @@ -1902,3 +1902,27 @@ void dwarves__set_btf_kind_max(struct conf_load *conf_load, int btf_kind_max)
> >  	if (btf_kind_max < BTF_KIND_ENUM64)
> >  		conf_load->skip_encoding_btf_enum64 = true;
> >  }
> > +
> > +void btf__set_btf_kind_max(struct conf_load *conf_load, struct btf *btf)
> > +{

Nitpick: same function for DWARF has name dwarf__find_btf_kind_max,
 which seems to be a bit inconsistent.

> > +	__u32 id, type_cnt = btf__type_cnt(btf);
> > +
> > +	for (id = 1; id < type_cnt; id++) {
> > +		const struct btf_type *t = btf__type_by_id(btf, id);
> > +		const struct btf_enum *e;
> > +		__u16 vlen, i;
> > +
> > +		if (!t || !btf_is_enum(t))
> > +			continue;
> > +		vlen = btf_vlen(t);
> > +		e = btf_enum(t);
> > +		for (i = 0; i < vlen; e++, i++) {
> > +			const char *name = btf__name_by_offset(btf, e->name_off);
> > +
> > +			if (!name || strcmp(name, "BTF_KIND_MAX"))
> > +				continue;
> > +			dwarves__set_btf_kind_max(conf_load, e->val);
> > +			return;
> > +		}
> > +	}
> > +}
> > diff --git a/btf_encoder.h b/btf_encoder.h
> > index 34516bb..e5e12ef 100644
> > --- a/btf_encoder.h
> > +++ b/btf_encoder.h
> > @@ -27,4 +27,6 @@ struct btf *btf_encoder__btf(struct btf_encoder *encoder);
> >  
> >  int btf_encoder__add_encoder(struct btf_encoder *encoder, struct btf_encoder *other);
> >  
> > +void btf__set_btf_kind_max(struct conf_load *conf_load, struct btf *btf);
> > +
> >  #endif /* _BTF_ENCODER_H_ */
> > diff --git a/pahole.c b/pahole.c
> > index aca2704..4d6d059 100644
> > --- a/pahole.c
> > +++ b/pahole.c
> > @@ -3470,6 +3470,7 @@ int main(int argc, char *argv[])
> >  				base_btf_file, libbpf_get_error(conf_load.base_btf));
> >  			goto out;
> >  		}
> > +		btf__set_btf_kind_max(&conf_load, conf_load.base_btf);
> >  		if (!btf_encode && !ctf_encode) {
> >  			// Force "btf" since a btf_base is being informed
> >  			conf_load.format_path = "btf";
> > @@ -3513,6 +3514,7 @@ try_sole_arg_as_class_names:
> >  					base_btf_file, libbpf_get_error(conf_load.base_btf));
> >  				goto out;
> >  			}
> > +			btf__set_btf_kind_max(&conf_load, conf_load.base_btf);
> >  		}
> >  	}
> >  


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH dwarves 1/3] dwarves: auto-detect maximum kind supported by vmlinux
  2023-09-13 16:58   ` Eduard Zingerman
@ 2023-09-13 17:44     ` Alan Maguire
  0 siblings, 0 replies; 13+ messages in thread
From: Alan Maguire @ 2023-09-13 17:44 UTC (permalink / raw)
  To: Eduard Zingerman, acme
  Cc: andrii.nakryiko, ast, daniel, jolsa, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf

On 13/09/2023 17:58, Eduard Zingerman wrote:
> On Wed, 2023-09-13 at 15:26 +0100, Alan Maguire wrote:
>> When a newer pahole is run on an older kernel, it often knows about BTF
>> kinds that the kernel does not support.  This is a problem because the BTF
>> generated is then embedded in the kernel image and read, and if unknown
>> kinds are found, BTF handling fails and core BPF functionality is
>> unavailable.
>>
>> The scripts/pahole-flags.sh script enumerates the various pahole options
>> available associated with various versions of pahole, but the problem is
>> what matters in the case of an older kernel is the set of kinds the kernel
>> understands.  Because recent features such as BTF_KIND_ENUM64 are added
>> by default (and only skipped if --skip_encoding_btf_* is set), BTF will
>> be created with these newer kinds that the older kernel cannot read.
>> This can be fixed by stable-backporting --skip options, but this is
>> cumbersome and would have to be done every time a new BTF kind is
>> introduced.
>>
>> Here instead we pre-process the DWARF information associated with the
>> target for BTF generation; if we find an enum with a BTF_KIND_MAX
>> value in the DWARF associated with the object, we use that to
>> determine the maximum BTF kind supported.  Note that the enum
>> representation of BTF kinds starts for the 5.16 kernel; prior to this
>> The benefit of auto-detection is that no work is required for older
>> kernels when new kinds are added, and --skip_encoding options are
>> less needed.
>>
>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>> ---
>>  btf_encoder.c  | 12 ++++++++++++
>>  dwarf_loader.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  dwarves.h      |  2 ++
>>  3 files changed, 66 insertions(+)
>>
>> diff --git a/btf_encoder.c b/btf_encoder.c
>> index 65f6e71..98c7529 100644
>> --- a/btf_encoder.c
>> +++ b/btf_encoder.c
>> @@ -1889,3 +1889,15 @@ struct btf *btf_encoder__btf(struct btf_encoder *encoder)
>>  {
>>  	return encoder->btf;
>>  }
>> +
>> +void dwarves__set_btf_kind_max(struct conf_load *conf_load, int btf_kind_max)
>> +{
>> +	if (btf_kind_max < 0 || btf_kind_max >= BTF_KIND_MAX)
>> +		return;
>> +	if (btf_kind_max < BTF_KIND_DECL_TAG)
>> +		conf_load->skip_encoding_btf_decl_tag = true;
>> +	if (btf_kind_max < BTF_KIND_TYPE_TAG)
>> +		conf_load->skip_encoding_btf_type_tag = true;
>> +	if (btf_kind_max < BTF_KIND_ENUM64)
>> +		conf_load->skip_encoding_btf_enum64 = true;
>> +}
>> diff --git a/dwarf_loader.c b/dwarf_loader.c
>> index ccf3194..8984043 100644
>> --- a/dwarf_loader.c
>> +++ b/dwarf_loader.c
>> @@ -3358,8 +3358,60 @@ static int __dwarf_cus__process_cus(struct dwarf_cus *dcus)
>>  	return 0;
>>  }
>>  
>> +/* Find enumeration value for BTF_KIND_MAX; replace conf_load->btf_kind_max with
>> + * this value if found since it indicates that the target object does not know
>> + * about kinds > its BTF_KIND_MAX value.  This is valuable for kernel/module
>> + * BTF where a newer pahole/libbpf operate on an older kernel which cannot
>> + * parse some of the newer kinds pahole can generate.
>> + */
>> +static void dwarf__find_btf_kind_max(struct dwarf_cus *dcus)
>> +{
>> +	struct conf_load *conf = dcus->conf;
>> +	uint8_t pointer_size, offset_size;
>> +	Dwarf_Off off = 0, noff;
>> +	size_t cuhl;
>> +
>> +	while (dwarf_nextcu(dcus->dw, off, &noff, &cuhl, NULL, &pointer_size, &offset_size) == 0) {
>> +		Dwarf_Die die_mem;
>> +		Dwarf_Die *cu_die = dwarf_offdie(dcus->dw, off + cuhl, &die_mem);
>> +		Dwarf_Die child;
>> +
>> +		if (cu_die == NULL)
>> +			break;
>> +		if (dwarf_child(cu_die, &child) == 0) {
>> +			Dwarf_Die *die = &child;
>> +
>> +			do {
>> +				Dwarf_Die echild, *edie;
>> +
>> +				if (dwarf_tag(die) != DW_TAG_enumeration_type ||
>> +				    !dwarf_haschildren(die) ||
>> +				    dwarf_child(die, &echild) != 0)
>> +					continue;
>> +				edie = &echild;
>> +				do {
>> +					const char *ename;
>> +					int btf_kind_max;
>> +
>> +					if (dwarf_tag(edie) != DW_TAG_enumerator)
>> +						continue;
>> +					ename = attr_string(edie, DW_AT_name, conf);
>> +					if (!ename || strcmp(ename, "BTF_KIND_MAX") != 0)
>> +						continue;
>> +					btf_kind_max = attr_numeric(edie, DW_AT_const_value);
> 
> Nitpick: attr_numeric() returns 0 in case of an error, when 0 is passed to
>          dwarves__set_btf_kind_max() it would turn off all optional kinds.
>          Probably should bail out on 0 instead.
>

not a nitpick at all, great catch! will fix this and make the naming
consistent in patch 3 (using btf__find_btf_kind_max() as you suggest).
And many thanks for testing this at your end!

Alan

>> +					dwarves__set_btf_kind_max(conf, btf_kind_max);
>> +					return;
>> +				} while (dwarf_siblingof(edie, edie) == 0);
>> +			} while (dwarf_siblingof(die, die) == 0);
>> +		}
>> +		off = noff;
>> +	}
>> +}
>> +
>>  static int dwarf_cus__process_cus(struct dwarf_cus *dcus)
>>  {
>> +	dwarf__find_btf_kind_max(dcus);
>> +
>>  	if (dcus->conf->nr_jobs > 1)
>>  		return dwarf_cus__threaded_process_cus(dcus);
>>  
>> diff --git a/dwarves.h b/dwarves.h
>> index eb1a6df..f4d9347 100644
>> --- a/dwarves.h
>> +++ b/dwarves.h
>> @@ -1480,4 +1480,6 @@ extern const char tabs[];
>>  #define DW_TAG_skeleton_unit 0x4a
>>  #endif
>>  
>> +void dwarves__set_btf_kind_max(struct conf_load *conf_load, int btf_kind_max);
>> +
>>  #endif /* _DWARVES_H_ */
> 
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH dwarves 0/3] dwarves: detect BTF kinds supported by kernel
  2023-09-13 14:26 [PATCH dwarves 0/3] dwarves: detect BTF kinds supported by kernel Alan Maguire
                   ` (3 preceding siblings ...)
  2023-09-13 16:58 ` [PATCH dwarves 0/3] dwarves: detect BTF kinds supported by kernel Eduard Zingerman
@ 2023-09-14 17:58 ` Andrii Nakryiko
  2023-09-19 16:30   ` Alan Maguire
  4 siblings, 1 reply; 13+ messages in thread
From: Andrii Nakryiko @ 2023-09-14 17:58 UTC (permalink / raw)
  To: Alan Maguire
  Cc: acme, ast, daniel, jolsa, eddyz87, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf

On Wed, Sep 13, 2023 at 7:26 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> When a newer pahole is run on an older kernel, it often knows about BTF
> kinds that the kernel does not support, and adds them to the BTF
> representation.  This is a problem because the BTF generated is then
> embedded in the kernel image.  When it is later read - possibly by
> a different older toolchain or by the kernel directly - it is not usable.
>
> The scripts/pahole-flags.sh script enumerates the various pahole options
> available associated with various versions of pahole, but in the case
> of an older kernel is the set of BTF kinds the _kernel_ can handle that
> is of more importance.
>
> Because recent features such as BTF_KIND_ENUM64 are added by default
> (and only skipped if --skip_encoding_btf_* is set), BTF will be
> created with these newer kinds that the older kernel cannot read.
> This can be (and has been) fixed by stable-backporting --skip options,
> but this is cumbersome and would have to be done every time a new BTF kind
> is introduced.
>

Yes, this is indeed the problem, but I don't think any sort of auto
detection by pahole itself of what is the BTF_KIND_MAX is the best
solution. Sometimes new features are added to existing kinds (like
kflag usage, etc), and that will still break even with "auto
detection".

I think the solution is to design pahole behavior in such a way that
it allows full control for old kernels to specify which BTF features
are expected to be generated, while also allowing to default to all
the latest and greatest BTF features by default for any other
application.

So, how about something like the following. By default, pahole
generates all the BTF features it knows about. But we add a new flag
that says to stay conservative and only generate a specified subset of
BTF features. E.g.:

1) `pahole -J <eLF.o>`  will generate everything

2) `pahole -J <elf.o> --btf_feature=basic` will generate only the very
basic stuff (we can decide what constitutes basic, we can go all the
way to before we added variables, or can pick any random state after
that)

3) `pahole -J <elf.o> --btf_feature=basic --btf_feature=enum64
--btf_feature=fancy_funcs` will generate only requested bits.

We can have --btf_feature=all as a convenience as well, but kernel
scripts won't use it.

From the very beginning, pahole should not fail with a feature name
that it doesn't recognize, though (maybe emit a warning, don't know).
So that kernel-side scripts can be simpler: when kernel starts to
recognize some new BTF functionality, we just unconditionally add
another `--btf_feature=<something>`. And that works starting from the
first pahole version that supports this `--btf_feature` flag.


All this cleverness in trying to guess what kernel supports and what
not (without actually running the kernel and feature-testing) will
just come biting us later on. This never works reliably.


> So this series attempts to detect the BTF kinds supported by the
> kernel/modules so that this can inform BTF encoding for older
> kernels.  We look for BTF_KIND_MAX - either as an enumerated value
> in vmlinux DWARF (patch 1) or as an enumerated value in base vmlinux
> BTF (patch 3).  Knowing this prior to encoding BTF allows us to specify
> skip_encoding options to avoid having BTF with kinds the kernel itself
> will not understand.
>
> The aim is to minimize pain for older stable kernels when new BTF
> kinds are introduced.  Kind encoding [1] can solve the parsing problem
> with BTF, but this approach is intended to ensure generated BTF is
> usable when newer pahole runs on older kernels.
>
> This approach requires BTF kinds to be defined via an enumerated type,
> which happened for 5.16 and later.  Older kernels than this used #defines
> so the approach will only work for 5.16 stable kernels and later currently.
>
> With this change in hand, adding new BTF kinds becomes a bit simpler,
> at least for the user of pahole.  All that needs to be done is to add
> internal "skip_new_kind" booleans to struct conf_load and set them
> in dwarves__set_btf_kind_max() if the detected maximum kind is less
> than the kind in question - in other words, if the kernel does not know
> about that kind.  In that case, we will not use it in encoding.
>
> The approach was tested on Linux 5.16 as released, i.e. prior to the
> backports adding --skip_encoding logic, and the BTF generated did not
> contain kinds > BTF_KIND_MAX for the kernel (corresponding to
> BTF_KIND_DECL_TAG in that case).
>
> Changes since RFC [2]:
>  - added --skip_autodetect_btf_kind_max to disable kind autodetection
>    (Jiri, patch 2)
>
> [1] https://lore.kernel.org/bpf/20230616171728.530116-1-alan.maguire@oracle.com/
> [2] https://lore.kernel.org/bpf/20230720201443.224040-1-alan.maguire@oracle.com/
>
> Alan Maguire (3):
>   dwarves: auto-detect maximum kind supported by vmlinux
>   pahole: add --skip_autodetect_btf_kind_max to disable kind autodetect
>   btf_encoder: learn BTF_KIND_MAX value from base BTF when generating
>     split BTF
>
>  btf_encoder.c      | 37 +++++++++++++++++++++++++++++++++
>  btf_encoder.h      |  2 ++
>  dwarf_loader.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++++
>  dwarves.h          |  3 +++
>  man-pages/pahole.1 |  4 ++++
>  pahole.c           | 10 +++++++++
>  6 files changed, 108 insertions(+)
>
> --
> 2.39.3
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH dwarves 0/3] dwarves: detect BTF kinds supported by kernel
  2023-09-14 17:58 ` Andrii Nakryiko
@ 2023-09-19 16:30   ` Alan Maguire
  2023-09-19 18:58     ` Andrii Nakryiko
  0 siblings, 1 reply; 13+ messages in thread
From: Alan Maguire @ 2023-09-19 16:30 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: acme, ast, daniel, jolsa, eddyz87, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf

On 14/09/2023 18:58, Andrii Nakryiko wrote:
> On Wed, Sep 13, 2023 at 7:26 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>>
>> When a newer pahole is run on an older kernel, it often knows about BTF
>> kinds that the kernel does not support, and adds them to the BTF
>> representation.  This is a problem because the BTF generated is then
>> embedded in the kernel image.  When it is later read - possibly by
>> a different older toolchain or by the kernel directly - it is not usable.
>>
>> The scripts/pahole-flags.sh script enumerates the various pahole options
>> available associated with various versions of pahole, but in the case
>> of an older kernel is the set of BTF kinds the _kernel_ can handle that
>> is of more importance.
>>
>> Because recent features such as BTF_KIND_ENUM64 are added by default
>> (and only skipped if --skip_encoding_btf_* is set), BTF will be
>> created with these newer kinds that the older kernel cannot read.
>> This can be (and has been) fixed by stable-backporting --skip options,
>> but this is cumbersome and would have to be done every time a new BTF kind
>> is introduced.
>>
> 
> Yes, this is indeed the problem, but I don't think any sort of auto
> detection by pahole itself of what is the BTF_KIND_MAX is the best
> solution. Sometimes new features are added to existing kinds (like
> kflag usage, etc), and that will still break even with "auto
> detection".
> 
> I think the solution is to design pahole behavior in such a way that
> it allows full control for old kernels to specify which BTF features
> are expected to be generated, while also allowing to default to all
> the latest and greatest BTF features by default for any other
> application.
> 
> So, how about something like the following. By default, pahole
> generates all the BTF features it knows about. But we add a new flag
> that says to stay conservative and only generate a specified subset of
> BTF features. E.g.:
> 
> 1) `pahole -J <eLF.o>`  will generate everything
> 
> 2) `pahole -J <elf.o> --btf_feature=basic` will generate only the very
> basic stuff (we can decide what constitutes basic, we can go all the
> way to before we added variables, or can pick any random state after
> that)
> 
> 3) `pahole -J <elf.o> --btf_feature=basic --btf_feature=enum64
> --btf_feature=fancy_funcs` will generate only requested bits.
> 
> We can have --btf_feature=all as a convenience as well, but kernel
> scripts won't use it.
> 
> From the very beginning, pahole should not fail with a feature name
> that it doesn't recognize, though (maybe emit a warning, don't know).
> So that kernel-side scripts can be simpler: when kernel starts to
> recognize some new BTF functionality, we just unconditionally add
> another `--btf_feature=<something>`. And that works starting from the
> first pahole version that supports this `--btf_feature` flag.
>

The idea of a BTF feature flag set - not restricted to BTF kinds -
is a good one. I think it should be in the UAPI also though
as "enum btf_features". A set of bitmask values - probably closely
mirroring the FEAT_BTF* . Something like this perhaps:

enum btf_features {
	BTF_FEATURE_BASIC	=	0x1,	/* _FUNC, _FUNC_PROTO */
	BTF_FEATURE_DATASEC 	=	0x2,	/* _VAR, _DATASEC */

..etc. A bitmask value would also be amenable to inclusion in
an updated struct btf_header.

So at BTF encoding time - if we support the newer header - we could
add the feature set supported by the BTF encoding along with CRCs.
That would be useful for tools - for example if bpftool encounters
features it doesn't support in BTF it is trying to parse, it can
complain upfront. Ditto for libbpf.

With respect to the kind layout support, it probably isn't worth it.
It would be a tax on every BTF encoding, and it only helps with
parsing - as opposed to using - newer BTF features. As long as
we can guarantee that a kernel doesn't wind up with BTF features
it doesn't support in vmlinux/module BTF, I think that's enough.

Alan

> 
> All this cleverness in trying to guess what kernel supports and what
> not (without actually running the kernel and feature-testing) will
> just come biting us later on. This never works reliably.
> 
> 
>> So this series attempts to detect the BTF kinds supported by the
>> kernel/modules so that this can inform BTF encoding for older
>> kernels.  We look for BTF_KIND_MAX - either as an enumerated value
>> in vmlinux DWARF (patch 1) or as an enumerated value in base vmlinux
>> BTF (patch 3).  Knowing this prior to encoding BTF allows us to specify
>> skip_encoding options to avoid having BTF with kinds the kernel itself
>> will not understand.
>>
>> The aim is to minimize pain for older stable kernels when new BTF
>> kinds are introduced.  Kind encoding [1] can solve the parsing problem
>> with BTF, but this approach is intended to ensure generated BTF is
>> usable when newer pahole runs on older kernels.
>>
>> This approach requires BTF kinds to be defined via an enumerated type,
>> which happened for 5.16 and later.  Older kernels than this used #defines
>> so the approach will only work for 5.16 stable kernels and later currently.
>>
>> With this change in hand, adding new BTF kinds becomes a bit simpler,
>> at least for the user of pahole.  All that needs to be done is to add
>> internal "skip_new_kind" booleans to struct conf_load and set them
>> in dwarves__set_btf_kind_max() if the detected maximum kind is less
>> than the kind in question - in other words, if the kernel does not know
>> about that kind.  In that case, we will not use it in encoding.
>>
>> The approach was tested on Linux 5.16 as released, i.e. prior to the
>> backports adding --skip_encoding logic, and the BTF generated did not
>> contain kinds > BTF_KIND_MAX for the kernel (corresponding to
>> BTF_KIND_DECL_TAG in that case).
>>
>> Changes since RFC [2]:
>>  - added --skip_autodetect_btf_kind_max to disable kind autodetection
>>    (Jiri, patch 2)
>>
>> [1] https://lore.kernel.org/bpf/20230616171728.530116-1-alan.maguire@oracle.com/
>> [2] https://lore.kernel.org/bpf/20230720201443.224040-1-alan.maguire@oracle.com/
>>
>> Alan Maguire (3):
>>   dwarves: auto-detect maximum kind supported by vmlinux
>>   pahole: add --skip_autodetect_btf_kind_max to disable kind autodetect
>>   btf_encoder: learn BTF_KIND_MAX value from base BTF when generating
>>     split BTF
>>
>>  btf_encoder.c      | 37 +++++++++++++++++++++++++++++++++
>>  btf_encoder.h      |  2 ++
>>  dwarf_loader.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++++
>>  dwarves.h          |  3 +++
>>  man-pages/pahole.1 |  4 ++++
>>  pahole.c           | 10 +++++++++
>>  6 files changed, 108 insertions(+)
>>
>> --
>> 2.39.3
>>
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH dwarves 0/3] dwarves: detect BTF kinds supported by kernel
  2023-09-19 16:30   ` Alan Maguire
@ 2023-09-19 18:58     ` Andrii Nakryiko
  2023-10-11  9:27       ` Alan Maguire
  0 siblings, 1 reply; 13+ messages in thread
From: Andrii Nakryiko @ 2023-09-19 18:58 UTC (permalink / raw)
  To: Alan Maguire
  Cc: acme, ast, daniel, jolsa, eddyz87, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf

On Tue, Sep 19, 2023 at 9:30 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> On 14/09/2023 18:58, Andrii Nakryiko wrote:
> > On Wed, Sep 13, 2023 at 7:26 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> >>
> >> When a newer pahole is run on an older kernel, it often knows about BTF
> >> kinds that the kernel does not support, and adds them to the BTF
> >> representation.  This is a problem because the BTF generated is then
> >> embedded in the kernel image.  When it is later read - possibly by
> >> a different older toolchain or by the kernel directly - it is not usable.
> >>
> >> The scripts/pahole-flags.sh script enumerates the various pahole options
> >> available associated with various versions of pahole, but in the case
> >> of an older kernel is the set of BTF kinds the _kernel_ can handle that
> >> is of more importance.
> >>
> >> Because recent features such as BTF_KIND_ENUM64 are added by default
> >> (and only skipped if --skip_encoding_btf_* is set), BTF will be
> >> created with these newer kinds that the older kernel cannot read.
> >> This can be (and has been) fixed by stable-backporting --skip options,
> >> but this is cumbersome and would have to be done every time a new BTF kind
> >> is introduced.
> >>
> >
> > Yes, this is indeed the problem, but I don't think any sort of auto
> > detection by pahole itself of what is the BTF_KIND_MAX is the best
> > solution. Sometimes new features are added to existing kinds (like
> > kflag usage, etc), and that will still break even with "auto
> > detection".
> >
> > I think the solution is to design pahole behavior in such a way that
> > it allows full control for old kernels to specify which BTF features
> > are expected to be generated, while also allowing to default to all
> > the latest and greatest BTF features by default for any other
> > application.
> >
> > So, how about something like the following. By default, pahole
> > generates all the BTF features it knows about. But we add a new flag
> > that says to stay conservative and only generate a specified subset of
> > BTF features. E.g.:
> >
> > 1) `pahole -J <eLF.o>`  will generate everything
> >
> > 2) `pahole -J <elf.o> --btf_feature=basic` will generate only the very
> > basic stuff (we can decide what constitutes basic, we can go all the
> > way to before we added variables, or can pick any random state after
> > that)
> >
> > 3) `pahole -J <elf.o> --btf_feature=basic --btf_feature=enum64
> > --btf_feature=fancy_funcs` will generate only requested bits.
> >
> > We can have --btf_feature=all as a convenience as well, but kernel
> > scripts won't use it.
> >
> > From the very beginning, pahole should not fail with a feature name
> > that it doesn't recognize, though (maybe emit a warning, don't know).
> > So that kernel-side scripts can be simpler: when kernel starts to
> > recognize some new BTF functionality, we just unconditionally add
> > another `--btf_feature=<something>`. And that works starting from the
> > first pahole version that supports this `--btf_feature` flag.
> >
>
> The idea of a BTF feature flag set - not restricted to BTF kinds -

so what about not trying to auto-detect anything and let kernel
strictly opt into BTF functionality it expects from pahole and
recognizes?

> is a good one. I think it should be in the UAPI also though
> as "enum btf_features". A set of bitmask values - probably closely
> mirroring the FEAT_BTF* . Something like this perhaps:
>
> enum btf_features {
>         BTF_FEATURE_BASIC       =       0x1,    /* _FUNC, _FUNC_PROTO */
>         BTF_FEATURE_DATASEC     =       0x2,    /* _VAR, _DATASEC */
>
> ..etc. A bitmask value would also be amenable to inclusion in
> an updated struct btf_header.

I don't know if I agree to add this to UAPI. It seems like an
overkill, and it also raises a question of "what is a feature"? Any
tiny addition, extension, etc could be considered a feature and we'll
end up using all the bits very soon. With self-describing btf_type
sizes, tools should be able to skip BTF types they don't recognize.
And if there is some fancy kflag usage within an old BTF KIND, for
example, then it will be up to the application to either complain,
skip, or ignore. E.g., bpftool should try to emit all possible
information during bpftool btf dump, even if it doesn't recognize a
particular flag or enum.

>
> So at BTF encoding time - if we support the newer header - we could
> add the feature set supported by the BTF encoding along with CRCs.
> That would be useful for tools - for example if bpftool encounters
> features it doesn't support in BTF it is trying to parse, it can
> complain upfront. Ditto for libbpf.
>
> With respect to the kind layout support, it probably isn't worth it.
> It would be a tax on every BTF encoding, and it only helps with
> parsing - as opposed to using - newer BTF features. As long as
> we can guarantee that a kernel doesn't wind up with BTF features
> it doesn't support in vmlinux/module BTF, I think that's enough.
>
> Alan
>
> >
> > All this cleverness in trying to guess what kernel supports and what
> > not (without actually running the kernel and feature-testing) will
> > just come biting us later on. This never works reliably.
> >
> >
> >> So this series attempts to detect the BTF kinds supported by the
> >> kernel/modules so that this can inform BTF encoding for older
> >> kernels.  We look for BTF_KIND_MAX - either as an enumerated value
> >> in vmlinux DWARF (patch 1) or as an enumerated value in base vmlinux
> >> BTF (patch 3).  Knowing this prior to encoding BTF allows us to specify
> >> skip_encoding options to avoid having BTF with kinds the kernel itself
> >> will not understand.
> >>
> >> The aim is to minimize pain for older stable kernels when new BTF
> >> kinds are introduced.  Kind encoding [1] can solve the parsing problem
> >> with BTF, but this approach is intended to ensure generated BTF is
> >> usable when newer pahole runs on older kernels.
> >>
> >> This approach requires BTF kinds to be defined via an enumerated type,
> >> which happened for 5.16 and later.  Older kernels than this used #defines
> >> so the approach will only work for 5.16 stable kernels and later currently.
> >>
> >> With this change in hand, adding new BTF kinds becomes a bit simpler,
> >> at least for the user of pahole.  All that needs to be done is to add
> >> internal "skip_new_kind" booleans to struct conf_load and set them
> >> in dwarves__set_btf_kind_max() if the detected maximum kind is less
> >> than the kind in question - in other words, if the kernel does not know
> >> about that kind.  In that case, we will not use it in encoding.
> >>
> >> The approach was tested on Linux 5.16 as released, i.e. prior to the
> >> backports adding --skip_encoding logic, and the BTF generated did not
> >> contain kinds > BTF_KIND_MAX for the kernel (corresponding to
> >> BTF_KIND_DECL_TAG in that case).
> >>
> >> Changes since RFC [2]:
> >>  - added --skip_autodetect_btf_kind_max to disable kind autodetection
> >>    (Jiri, patch 2)
> >>
> >> [1] https://lore.kernel.org/bpf/20230616171728.530116-1-alan.maguire@oracle.com/
> >> [2] https://lore.kernel.org/bpf/20230720201443.224040-1-alan.maguire@oracle.com/
> >>
> >> Alan Maguire (3):
> >>   dwarves: auto-detect maximum kind supported by vmlinux
> >>   pahole: add --skip_autodetect_btf_kind_max to disable kind autodetect
> >>   btf_encoder: learn BTF_KIND_MAX value from base BTF when generating
> >>     split BTF
> >>
> >>  btf_encoder.c      | 37 +++++++++++++++++++++++++++++++++
> >>  btf_encoder.h      |  2 ++
> >>  dwarf_loader.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  dwarves.h          |  3 +++
> >>  man-pages/pahole.1 |  4 ++++
> >>  pahole.c           | 10 +++++++++
> >>  6 files changed, 108 insertions(+)
> >>
> >> --
> >> 2.39.3
> >>
> >

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH dwarves 0/3] dwarves: detect BTF kinds supported by kernel
  2023-09-19 18:58     ` Andrii Nakryiko
@ 2023-10-11  9:27       ` Alan Maguire
  2023-10-13  0:22         ` Andrii Nakryiko
  0 siblings, 1 reply; 13+ messages in thread
From: Alan Maguire @ 2023-10-11  9:27 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: acme, ast, daniel, jolsa, eddyz87, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf

On 19/09/2023 19:58, Andrii Nakryiko wrote:
> On Tue, Sep 19, 2023 at 9:30 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>>
>> On 14/09/2023 18:58, Andrii Nakryiko wrote:
>>> On Wed, Sep 13, 2023 at 7:26 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>>>>
>>>> When a newer pahole is run on an older kernel, it often knows about BTF
>>>> kinds that the kernel does not support, and adds them to the BTF
>>>> representation.  This is a problem because the BTF generated is then
>>>> embedded in the kernel image.  When it is later read - possibly by
>>>> a different older toolchain or by the kernel directly - it is not usable.
>>>>
>>>> The scripts/pahole-flags.sh script enumerates the various pahole options
>>>> available associated with various versions of pahole, but in the case
>>>> of an older kernel is the set of BTF kinds the _kernel_ can handle that
>>>> is of more importance.
>>>>
>>>> Because recent features such as BTF_KIND_ENUM64 are added by default
>>>> (and only skipped if --skip_encoding_btf_* is set), BTF will be
>>>> created with these newer kinds that the older kernel cannot read.
>>>> This can be (and has been) fixed by stable-backporting --skip options,
>>>> but this is cumbersome and would have to be done every time a new BTF kind
>>>> is introduced.
>>>>
>>>
>>> Yes, this is indeed the problem, but I don't think any sort of auto
>>> detection by pahole itself of what is the BTF_KIND_MAX is the best
>>> solution. Sometimes new features are added to existing kinds (like
>>> kflag usage, etc), and that will still break even with "auto
>>> detection".
>>>
>>> I think the solution is to design pahole behavior in such a way that
>>> it allows full control for old kernels to specify which BTF features
>>> are expected to be generated, while also allowing to default to all
>>> the latest and greatest BTF features by default for any other
>>> application.
>>>
>>> So, how about something like the following. By default, pahole
>>> generates all the BTF features it knows about. But we add a new flag
>>> that says to stay conservative and only generate a specified subset of
>>> BTF features. E.g.:
>>>
>>> 1) `pahole -J <eLF.o>`  will generate everything
>>>
>>> 2) `pahole -J <elf.o> --btf_feature=basic` will generate only the very
>>> basic stuff (we can decide what constitutes basic, we can go all the
>>> way to before we added variables, or can pick any random state after
>>> that)
>>>
>>> 3) `pahole -J <elf.o> --btf_feature=basic --btf_feature=enum64
>>> --btf_feature=fancy_funcs` will generate only requested bits.
>>>
>>> We can have --btf_feature=all as a convenience as well, but kernel
>>> scripts won't use it.
>>>
>>> From the very beginning, pahole should not fail with a feature name
>>> that it doesn't recognize, though (maybe emit a warning, don't know).
>>> So that kernel-side scripts can be simpler: when kernel starts to
>>> recognize some new BTF functionality, we just unconditionally add
>>> another `--btf_feature=<something>`. And that works starting from the
>>> first pahole version that supports this `--btf_feature` flag.
>>>
>>
>> The idea of a BTF feature flag set - not restricted to BTF kinds -
> 
> so what about not trying to auto-detect anything and let kernel
> strictly opt into BTF functionality it expects from pahole and
> recognizes?
> 
>> is a good one. I think it should be in the UAPI also though
>> as "enum btf_features". A set of bitmask values - probably closely
>> mirroring the FEAT_BTF* . Something like this perhaps:
>>
>> enum btf_features {
>>         BTF_FEATURE_BASIC       =       0x1,    /* _FUNC, _FUNC_PROTO */
>>         BTF_FEATURE_DATASEC     =       0x2,    /* _VAR, _DATASEC */
>>
>> ..etc. A bitmask value would also be amenable to inclusion in
>> an updated struct btf_header.
> 
> I don't know if I agree to add this to UAPI. It seems like an
> overkill, and it also raises a question of "what is a feature"? Any
> tiny addition, extension, etc could be considered a feature and we'll
> end up using all the bits very soon. With self-describing btf_type
> sizes, tools should be able to skip BTF types they don't recognize.
> And if there is some fancy kflag usage within an old BTF KIND, for
> example, then it will be up to the application to either complain,
> skip, or ignore. E.g., bpftool should try to emit all possible
> information during bpftool btf dump, even if it doesn't recognize a
> particular flag or enum.
> 

Based on the above, I've put together an RFC implementing a

--btf_features=feature1[,feature2]

...parameter for pahole [1]. I _think_ it's roughly what you've
described above, and I think it has the characteristics we need
to simplify scripts/pahole-flags.sh (features are opt-in, no
complaints on unrecognized features) such that we'll only
need one more version-check clause, something like this:

if [ "${pahole_ver}" -ge "126" ]; then
        extra_pahole_opt="-j --lang_exclude=rust
--btf_features=encode_force,var,float,decl_tag,type_tag,enum64,optimized,consistent"
fi

New features would simply be added to the list above without a
version check requirement and ignored for pahole versions that
don't support them.

I'll follow up with the kind layout/crc stuff once we converge on
how we want to handle new BTF features. Thanks!

Alan

[1]
https://lore.kernel.org/bpf/20231011091732.93254-1-alan.maguire@oracle.com/

>>
>> So at BTF encoding time - if we support the newer header - we could
>> add the feature set supported by the BTF encoding along with CRCs.
>> That would be useful for tools - for example if bpftool encounters
>> features it doesn't support in BTF it is trying to parse, it can
>> complain upfront. Ditto for libbpf.
>>
>> With respect to the kind layout support, it probably isn't worth it.
>> It would be a tax on every BTF encoding, and it only helps with
>> parsing - as opposed to using - newer BTF features. As long as
>> we can guarantee that a kernel doesn't wind up with BTF features
>> it doesn't support in vmlinux/module BTF, I think that's enough.
>>
>> Alan
>>
>>>
>>> All this cleverness in trying to guess what kernel supports and what
>>> not (without actually running the kernel and feature-testing) will
>>> just come biting us later on. This never works reliably.
>>>
>>>
>>>> So this series attempts to detect the BTF kinds supported by the
>>>> kernel/modules so that this can inform BTF encoding for older
>>>> kernels.  We look for BTF_KIND_MAX - either as an enumerated value
>>>> in vmlinux DWARF (patch 1) or as an enumerated value in base vmlinux
>>>> BTF (patch 3).  Knowing this prior to encoding BTF allows us to specify
>>>> skip_encoding options to avoid having BTF with kinds the kernel itself
>>>> will not understand.
>>>>
>>>> The aim is to minimize pain for older stable kernels when new BTF
>>>> kinds are introduced.  Kind encoding [1] can solve the parsing problem
>>>> with BTF, but this approach is intended to ensure generated BTF is
>>>> usable when newer pahole runs on older kernels.
>>>>
>>>> This approach requires BTF kinds to be defined via an enumerated type,
>>>> which happened for 5.16 and later.  Older kernels than this used #defines
>>>> so the approach will only work for 5.16 stable kernels and later currently.
>>>>
>>>> With this change in hand, adding new BTF kinds becomes a bit simpler,
>>>> at least for the user of pahole.  All that needs to be done is to add
>>>> internal "skip_new_kind" booleans to struct conf_load and set them
>>>> in dwarves__set_btf_kind_max() if the detected maximum kind is less
>>>> than the kind in question - in other words, if the kernel does not know
>>>> about that kind.  In that case, we will not use it in encoding.
>>>>
>>>> The approach was tested on Linux 5.16 as released, i.e. prior to the
>>>> backports adding --skip_encoding logic, and the BTF generated did not
>>>> contain kinds > BTF_KIND_MAX for the kernel (corresponding to
>>>> BTF_KIND_DECL_TAG in that case).
>>>>
>>>> Changes since RFC [2]:
>>>>  - added --skip_autodetect_btf_kind_max to disable kind autodetection
>>>>    (Jiri, patch 2)
>>>>
>>>> [1] https://lore.kernel.org/bpf/20230616171728.530116-1-alan.maguire@oracle.com/
>>>> [2] https://lore.kernel.org/bpf/20230720201443.224040-1-alan.maguire@oracle.com/
>>>>
>>>> Alan Maguire (3):
>>>>   dwarves: auto-detect maximum kind supported by vmlinux
>>>>   pahole: add --skip_autodetect_btf_kind_max to disable kind autodetect
>>>>   btf_encoder: learn BTF_KIND_MAX value from base BTF when generating
>>>>     split BTF
>>>>
>>>>  btf_encoder.c      | 37 +++++++++++++++++++++++++++++++++
>>>>  btf_encoder.h      |  2 ++
>>>>  dwarf_loader.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>  dwarves.h          |  3 +++
>>>>  man-pages/pahole.1 |  4 ++++
>>>>  pahole.c           | 10 +++++++++
>>>>  6 files changed, 108 insertions(+)
>>>>
>>>> --
>>>> 2.39.3
>>>>
>>>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH dwarves 0/3] dwarves: detect BTF kinds supported by kernel
  2023-10-11  9:27       ` Alan Maguire
@ 2023-10-13  0:22         ` Andrii Nakryiko
  0 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2023-10-13  0:22 UTC (permalink / raw)
  To: Alan Maguire
  Cc: acme, ast, daniel, jolsa, eddyz87, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf

On Wed, Oct 11, 2023 at 2:27 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> On 19/09/2023 19:58, Andrii Nakryiko wrote:
> > On Tue, Sep 19, 2023 at 9:30 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> >>
> >> On 14/09/2023 18:58, Andrii Nakryiko wrote:
> >>> On Wed, Sep 13, 2023 at 7:26 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> >>>>
> >>>> When a newer pahole is run on an older kernel, it often knows about BTF
> >>>> kinds that the kernel does not support, and adds them to the BTF
> >>>> representation.  This is a problem because the BTF generated is then
> >>>> embedded in the kernel image.  When it is later read - possibly by
> >>>> a different older toolchain or by the kernel directly - it is not usable.
> >>>>
> >>>> The scripts/pahole-flags.sh script enumerates the various pahole options
> >>>> available associated with various versions of pahole, but in the case
> >>>> of an older kernel is the set of BTF kinds the _kernel_ can handle that
> >>>> is of more importance.
> >>>>
> >>>> Because recent features such as BTF_KIND_ENUM64 are added by default
> >>>> (and only skipped if --skip_encoding_btf_* is set), BTF will be
> >>>> created with these newer kinds that the older kernel cannot read.
> >>>> This can be (and has been) fixed by stable-backporting --skip options,
> >>>> but this is cumbersome and would have to be done every time a new BTF kind
> >>>> is introduced.
> >>>>
> >>>
> >>> Yes, this is indeed the problem, but I don't think any sort of auto
> >>> detection by pahole itself of what is the BTF_KIND_MAX is the best
> >>> solution. Sometimes new features are added to existing kinds (like
> >>> kflag usage, etc), and that will still break even with "auto
> >>> detection".
> >>>
> >>> I think the solution is to design pahole behavior in such a way that
> >>> it allows full control for old kernels to specify which BTF features
> >>> are expected to be generated, while also allowing to default to all
> >>> the latest and greatest BTF features by default for any other
> >>> application.
> >>>
> >>> So, how about something like the following. By default, pahole
> >>> generates all the BTF features it knows about. But we add a new flag
> >>> that says to stay conservative and only generate a specified subset of
> >>> BTF features. E.g.:
> >>>
> >>> 1) `pahole -J <eLF.o>`  will generate everything
> >>>
> >>> 2) `pahole -J <elf.o> --btf_feature=basic` will generate only the very
> >>> basic stuff (we can decide what constitutes basic, we can go all the
> >>> way to before we added variables, or can pick any random state after
> >>> that)
> >>>
> >>> 3) `pahole -J <elf.o> --btf_feature=basic --btf_feature=enum64
> >>> --btf_feature=fancy_funcs` will generate only requested bits.
> >>>
> >>> We can have --btf_feature=all as a convenience as well, but kernel
> >>> scripts won't use it.
> >>>
> >>> From the very beginning, pahole should not fail with a feature name
> >>> that it doesn't recognize, though (maybe emit a warning, don't know).
> >>> So that kernel-side scripts can be simpler: when kernel starts to
> >>> recognize some new BTF functionality, we just unconditionally add
> >>> another `--btf_feature=<something>`. And that works starting from the
> >>> first pahole version that supports this `--btf_feature` flag.
> >>>
> >>
> >> The idea of a BTF feature flag set - not restricted to BTF kinds -
> >
> > so what about not trying to auto-detect anything and let kernel
> > strictly opt into BTF functionality it expects from pahole and
> > recognizes?
> >
> >> is a good one. I think it should be in the UAPI also though
> >> as "enum btf_features". A set of bitmask values - probably closely
> >> mirroring the FEAT_BTF* . Something like this perhaps:
> >>
> >> enum btf_features {
> >>         BTF_FEATURE_BASIC       =       0x1,    /* _FUNC, _FUNC_PROTO */
> >>         BTF_FEATURE_DATASEC     =       0x2,    /* _VAR, _DATASEC */
> >>
> >> ..etc. A bitmask value would also be amenable to inclusion in
> >> an updated struct btf_header.
> >
> > I don't know if I agree to add this to UAPI. It seems like an
> > overkill, and it also raises a question of "what is a feature"? Any
> > tiny addition, extension, etc could be considered a feature and we'll
> > end up using all the bits very soon. With self-describing btf_type
> > sizes, tools should be able to skip BTF types they don't recognize.
> > And if there is some fancy kflag usage within an old BTF KIND, for
> > example, then it will be up to the application to either complain,
> > skip, or ignore. E.g., bpftool should try to emit all possible
> > information during bpftool btf dump, even if it doesn't recognize a
> > particular flag or enum.
> >
>
> Based on the above, I've put together an RFC implementing a
>
> --btf_features=feature1[,feature2]
>
> ...parameter for pahole [1]. I _think_ it's roughly what you've
> described above, and I think it has the characteristics we need
> to simplify scripts/pahole-flags.sh (features are opt-in, no
> complaints on unrecognized features) such that we'll only
> need one more version-check clause, something like this:
>
> if [ "${pahole_ver}" -ge "126" ]; then
>         extra_pahole_opt="-j --lang_exclude=rust
> --btf_features=encode_force,var,float,decl_tag,type_tag,enum64,optimized,consistent"
> fi
>
> New features would simply be added to the list above without a
> version check requirement and ignored for pahole versions that
> don't support them.

Yes, that's the hope. I left a few comments, I think this looks great
overall, thanks!

>
> I'll follow up with the kind layout/crc stuff once we converge on
> how we want to handle new BTF features. Thanks!

Makes sense, and I think we have converged :)

>
> Alan
>
> [1]
> https://lore.kernel.org/bpf/20231011091732.93254-1-alan.maguire@oracle.com/
>
> >>
> >> So at BTF encoding time - if we support the newer header - we could
> >> add the feature set supported by the BTF encoding along with CRCs.
> >> That would be useful for tools - for example if bpftool encounters
> >> features it doesn't support in BTF it is trying to parse, it can
> >> complain upfront. Ditto for libbpf.
> >>
> >> With respect to the kind layout support, it probably isn't worth it.
> >> It would be a tax on every BTF encoding, and it only helps with
> >> parsing - as opposed to using - newer BTF features. As long as
> >> we can guarantee that a kernel doesn't wind up with BTF features
> >> it doesn't support in vmlinux/module BTF, I think that's enough.
> >>
> >> Alan
> >>
> >>>
> >>> All this cleverness in trying to guess what kernel supports and what
> >>> not (without actually running the kernel and feature-testing) will
> >>> just come biting us later on. This never works reliably.
> >>>
> >>>
> >>>> So this series attempts to detect the BTF kinds supported by the
> >>>> kernel/modules so that this can inform BTF encoding for older
> >>>> kernels.  We look for BTF_KIND_MAX - either as an enumerated value
> >>>> in vmlinux DWARF (patch 1) or as an enumerated value in base vmlinux
> >>>> BTF (patch 3).  Knowing this prior to encoding BTF allows us to specify
> >>>> skip_encoding options to avoid having BTF with kinds the kernel itself
> >>>> will not understand.
> >>>>
> >>>> The aim is to minimize pain for older stable kernels when new BTF
> >>>> kinds are introduced.  Kind encoding [1] can solve the parsing problem
> >>>> with BTF, but this approach is intended to ensure generated BTF is
> >>>> usable when newer pahole runs on older kernels.
> >>>>
> >>>> This approach requires BTF kinds to be defined via an enumerated type,
> >>>> which happened for 5.16 and later.  Older kernels than this used #defines
> >>>> so the approach will only work for 5.16 stable kernels and later currently.
> >>>>
> >>>> With this change in hand, adding new BTF kinds becomes a bit simpler,
> >>>> at least for the user of pahole.  All that needs to be done is to add
> >>>> internal "skip_new_kind" booleans to struct conf_load and set them
> >>>> in dwarves__set_btf_kind_max() if the detected maximum kind is less
> >>>> than the kind in question - in other words, if the kernel does not know
> >>>> about that kind.  In that case, we will not use it in encoding.
> >>>>
> >>>> The approach was tested on Linux 5.16 as released, i.e. prior to the
> >>>> backports adding --skip_encoding logic, and the BTF generated did not
> >>>> contain kinds > BTF_KIND_MAX for the kernel (corresponding to
> >>>> BTF_KIND_DECL_TAG in that case).
> >>>>
> >>>> Changes since RFC [2]:
> >>>>  - added --skip_autodetect_btf_kind_max to disable kind autodetection
> >>>>    (Jiri, patch 2)
> >>>>
> >>>> [1] https://lore.kernel.org/bpf/20230616171728.530116-1-alan.maguire@oracle.com/
> >>>> [2] https://lore.kernel.org/bpf/20230720201443.224040-1-alan.maguire@oracle.com/
> >>>>
> >>>> Alan Maguire (3):
> >>>>   dwarves: auto-detect maximum kind supported by vmlinux
> >>>>   pahole: add --skip_autodetect_btf_kind_max to disable kind autodetect
> >>>>   btf_encoder: learn BTF_KIND_MAX value from base BTF when generating
> >>>>     split BTF
> >>>>
> >>>>  btf_encoder.c      | 37 +++++++++++++++++++++++++++++++++
> >>>>  btf_encoder.h      |  2 ++
> >>>>  dwarf_loader.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++++
> >>>>  dwarves.h          |  3 +++
> >>>>  man-pages/pahole.1 |  4 ++++
> >>>>  pahole.c           | 10 +++++++++
> >>>>  6 files changed, 108 insertions(+)
> >>>>
> >>>> --
> >>>> 2.39.3
> >>>>
> >>>

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2023-10-13  0:23 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-13 14:26 [PATCH dwarves 0/3] dwarves: detect BTF kinds supported by kernel Alan Maguire
2023-09-13 14:26 ` [PATCH dwarves 1/3] dwarves: auto-detect maximum kind supported by vmlinux Alan Maguire
2023-09-13 16:58   ` Eduard Zingerman
2023-09-13 17:44     ` Alan Maguire
2023-09-13 14:26 ` [PATCH dwarves 2/3] pahole: add --skip_autodetect_btf_kind_max to disable kind autodetect Alan Maguire
2023-09-13 14:26 ` [PATCH dwarves 3/3] btf_encoder: learn BTF_KIND_MAX value from base BTF when generating split BTF Alan Maguire
2023-09-13 16:58   ` Eduard Zingerman
2023-09-13 16:58 ` [PATCH dwarves 0/3] dwarves: detect BTF kinds supported by kernel Eduard Zingerman
2023-09-14 17:58 ` Andrii Nakryiko
2023-09-19 16:30   ` Alan Maguire
2023-09-19 18:58     ` Andrii Nakryiko
2023-10-11  9:27       ` Alan Maguire
2023-10-13  0:22         ` Andrii Nakryiko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).