linux-debuggers.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] Fix duplicated VAR and secinfo
@ 2025-02-12  0:49 Stephen Brennan
  2025-02-12  0:49 ` [PATCH 1/3] btf_encoder: move btf_encoder__add_decl_tag() Stephen Brennan
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Stephen Brennan @ 2025-02-12  0:49 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Alan Maguire, dwarves, linux-debuggers, Stephen Brennan

Hello all,

I was recently able to clear up some issues related to the interaction
between kallsyms generation and BTF generation, which was interfering with
using BTF with global variables in the kernel. I sent some patches[1] to
the kernel to enable global_var. After further testing, I see that enabling
global_var results in the following validation errors:

    BPF: #011 type_id=23691 offset=435904 size=96
    fbcon: Taking over console
    BPF:
    BPF: Invalid offset
    BPF:

Thanks to Alan Maguire's quick debugging, we were able to find the root
cause. It turned out that several btf_var_secinfo entries had the exact
same offset (and name) as their predecessors. The reason was that these
entries corresponded to some variables are declared as "__weak" and then
overridden. For example:

   // kernel/bpf/helpers.c
   const struct bpf_func_proto bpf_get_current_task_proto __weak;
   // kernel/trace/bpf_trace.c
   const struct bpf_func_proto bpf_get_current_task_proto = {...

Both declarations appear in the DWARF as variable declarations, but it
seems that there is no way to find out which one of the declarations is
"__weak". (I checked this via llvm-dwarfdump). Overall, in a simple kernel
configuration, I found 47 btf_var_secinfo which had duplicated offsets. In
each case, both secinfos referred to distinct VARs, which had identical
names and types. All were due to the "__weak" symbol. We need to eliminate
these duplicates in order for the BTF to be validated by the kernel.

This patch series does the deduplication of the VAR and SECINFO in pahole,
by collecting the lists of variables for each ELF section, and then
outputting them all once the list is sorted by offset and duplicates are
identified. The libbpf btf__dedup() function does not deduplicate DATASEC
or VAR. It would probably be possible to implement this there, and I'm open
to feedback or suggestions regarding this. I implemented it in pahole
because I'm most familiar with that code, and because it seems to me like
it's reasonable for libbpf to expect that the input variable information is
already deduplicated.

I've gone ahead and tested this by building & booting a kernel with these
changes, and the kernel patch series at [1]. The result exhibited no BPF
varidation errors, and the drgn BTF branch[2] is working perfectly with it!

Thanks,
Stephen

[1]: https://lore.kernel.org/bpf/20250207012045.2129841-1-stephen.s.brennan@oracle.com/
[2]: https://github.com/brenns10/drgn/commits/btf_2024

Stephen Brennan (3):
  btf_encoder: move btf_encoder__add_decl_tag()
  btf_encoder: postpone VARs until encoding DATASEC
  btf_encoder: don't encode duplicate variables

 btf_encoder.c | 234 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 168 insertions(+), 66 deletions(-)

-- 
2.43.5


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

* [PATCH 1/3] btf_encoder: move btf_encoder__add_decl_tag()
  2025-02-12  0:49 [PATCH 0/3] Fix duplicated VAR and secinfo Stephen Brennan
@ 2025-02-12  0:49 ` Stephen Brennan
  2025-02-12  0:49 ` [PATCH 2/3] btf_encoder: postpone VARs until encoding DATASEC Stephen Brennan
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Stephen Brennan @ 2025-02-12  0:49 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Alan Maguire, dwarves, linux-debuggers, Stephen Brennan

It will need to be above btf_encoder__add_datasec for upcoming changes.
No functional changes.

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---
 btf_encoder.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 511c1ea..6a1f5c2 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -827,6 +827,27 @@ static int32_t btf_encoder__add_var(struct btf_encoder *encoder, uint32_t type,
 	return id;
 }
 
+static int32_t btf_encoder__add_decl_tag(struct btf_encoder *encoder, const char *value, uint32_t type,
+					 int component_idx)
+{
+	struct btf *btf = encoder->btf;
+	const struct btf_type *t;
+	int32_t id;
+
+	id = btf__add_decl_tag(btf, value, type, component_idx);
+	if (id > 0) {
+		t = btf__type_by_id(btf, id);
+		btf_encoder__log_type(encoder, t, false, true, "type_id=%u component_idx=%d",
+				      t->type, component_idx);
+	} else {
+		btf__log_err(btf, BTF_KIND_DECL_TAG, value, true, id,
+			     "component_idx=%d Error emitting BTF type",
+			     component_idx);
+	}
+
+	return id;
+}
+
 static int32_t btf_encoder__add_var_secinfo(struct btf_encoder *encoder, size_t shndx,
 					    uint32_t type, uint32_t offset, uint32_t size)
 {
@@ -883,27 +904,6 @@ static int32_t btf_encoder__add_datasec(struct btf_encoder *encoder, size_t shnd
 	return id;
 }
 
-static int32_t btf_encoder__add_decl_tag(struct btf_encoder *encoder, const char *value, uint32_t type,
-					 int component_idx)
-{
-	struct btf *btf = encoder->btf;
-	const struct btf_type *t;
-	int32_t id;
-
-	id = btf__add_decl_tag(btf, value, type, component_idx);
-	if (id > 0) {
-		t = btf__type_by_id(btf, id);
-		btf_encoder__log_type(encoder, t, false, true, "type_id=%u component_idx=%d",
-				      t->type, component_idx);
-	} else {
-		btf__log_err(btf, BTF_KIND_DECL_TAG, value, true, id,
-			     "component_idx=%d Error emitting BTF type",
-			     component_idx);
-	}
-
-	return id;
-}
-
 static void btf_encoder__log_func_skip(struct btf_encoder *encoder, struct elf_function *func,
 				       const char *fmt, ...)
 {
-- 
2.43.5


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

* [PATCH 2/3] btf_encoder: postpone VARs until encoding DATASEC
  2025-02-12  0:49 [PATCH 0/3] Fix duplicated VAR and secinfo Stephen Brennan
  2025-02-12  0:49 ` [PATCH 1/3] btf_encoder: move btf_encoder__add_decl_tag() Stephen Brennan
@ 2025-02-12  0:49 ` Stephen Brennan
  2025-02-19 23:51   ` Eduard Zingerman
  2025-02-12  0:49 ` [PATCH 3/3] btf_encoder: don't encode duplicate variables Stephen Brennan
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Stephen Brennan @ 2025-02-12  0:49 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Alan Maguire, dwarves, linux-debuggers, Stephen Brennan

In order to handle duplicate variables in a data section, we'll need to
compare variables against the others in their data section. To do this,
we'll need to postpone adding the variables until they have all been
identified and collected.  Store all the necessary data to encode the
VARs (and their DECL_TAGs) and then encode them just before the DATASEC
containing them. No meaningful change in the output is intended, though
the ordering of type IDs generated will likely differ.

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---
 btf_encoder.c | 163 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 118 insertions(+), 45 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 6a1f5c2..d989fbd 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -97,7 +97,7 @@ struct elf_secinfo {
 	uint64_t    sz;
 	uint32_t    type;
 	bool        include;
-	struct gobuffer secinfo;
+	struct gobuffer vars;
 };
 
 struct elf_functions {
@@ -109,6 +109,20 @@ struct elf_functions {
 	int suffix_cnt; /* number of .isra, .part etc */
 };
 
+struct saved_annot {
+	const char *value;
+	int component_idx;
+};
+
+struct saved_var {
+	const char *name;
+	uint32_t type;
+	struct saved_annot *annots;
+	uint32_t linkage;
+	uint32_t offset;
+	uint32_t size;
+};
+
 /*
  * cu: cu being processed.
  */
@@ -243,12 +257,15 @@ static struct elf_functions *elf_functions__find(const Elf *elf, const struct li
 #define elf_error(fmt, ...) \
 	fprintf(stderr, "%s: " fmt ": %s.\n", __func__, ##__VA_ARGS__, elf_errmsg(-1))
 
-static int btf_var_secinfo_cmp(const void *a, const void *b)
+static int saved_var_cmp(const void *a, const void *b)
 {
-	const struct btf_var_secinfo *av = a;
-	const struct btf_var_secinfo *bv = b;
+	const struct saved_var *av = a;
+	const struct saved_var *bv = b;
 
-	return av->offset - bv->offset;
+	if (av->offset != bv->offset)
+		return av->offset - bv->offset;
+	else
+		return strcmp(av->name, bv->name);
 }
 
 #define BITS_PER_BYTE 8
@@ -848,55 +865,102 @@ static int32_t btf_encoder__add_decl_tag(struct btf_encoder *encoder, const char
 	return id;
 }
 
-static int32_t btf_encoder__add_var_secinfo(struct btf_encoder *encoder, size_t shndx,
-					    uint32_t type, uint32_t offset, uint32_t size)
+static int32_t btf_encoder__encode_stored_vars(struct btf_encoder *encoder,
+					       struct saved_var *var, uint16_t nr)
 {
-	struct btf_var_secinfo si = {
+	int i, id, annot_idx;
+
+	for (i = 0; i < nr; i++) {
+		id = btf_encoder__add_var(encoder, var[i].type,
+					  var[i].name, var[i].linkage);
+		if (id < 0) {
+			fprintf(stderr, "error: failed to encode variable '%s' at offset 0x%x\n",
+				var[i].name, var[i].offset);
+			return -1;
+		}
+
+		annot_idx = 0;
+		while (var->annots && var->annots[annot_idx].value) {
+			struct saved_annot *annot = &var->annots[annot_idx++];
+			int tag_type_id = btf_encoder__add_decl_tag(encoder, annot->value,
+								    id, annot->component_idx);
+			if (tag_type_id < 0) {
+				fprintf(stderr,
+					"error: failed to encode tag '%s' to variable '%s' with component_idx %d\n",
+					annot->value, var[i].name, annot->component_idx);
+				return -1;
+			}
+		}
+		free(var->annots);
+		var->annots = NULL;
+
+		var[i].type = id;
+	}
+	return 0;
+}
+
+static int32_t btf_encoder__store_var(struct btf_encoder *encoder, size_t shndx,
+				      const char *name, uint32_t type, uint32_t linkage,
+				      uint32_t offset, uint32_t size, struct saved_annot *annots)
+{
+	struct saved_var var = {
+		.name = name,
 		.type = type,
+		.linkage = linkage,
+		.annots = annots,
 		.offset = offset,
 		.size = size,
 	};
-	return gobuffer__add(&encoder->secinfo[shndx].secinfo, &si, sizeof(si));
+	return gobuffer__add(&encoder->secinfo[shndx].vars, &var, sizeof(var));
 }
 
 static int32_t btf_encoder__add_datasec(struct btf_encoder *encoder, size_t shndx)
 {
-	struct gobuffer *var_secinfo_buf = &encoder->secinfo[shndx].secinfo;
+	struct gobuffer *saved_var_buf = &encoder->secinfo[shndx].vars;
 	const char *section_name = encoder->secinfo[shndx].name;
 	struct btf *btf = encoder->btf;
-	size_t sz = gobuffer__size(var_secinfo_buf);
-	uint16_t nr_var_secinfo = sz / sizeof(struct btf_var_secinfo);
-	struct btf_var_secinfo *last_vsi, *vsi;
+	size_t sz = gobuffer__size(saved_var_buf);
+	uint16_t nr_saved_var = sz / sizeof(struct saved_var);
+	struct saved_var *last_sv, *sv;
 	const struct btf_type *t;
 	uint32_t datasec_sz;
 	int32_t err, id, i;
 
-	qsort(var_secinfo_buf->entries, nr_var_secinfo,
-	      sizeof(struct btf_var_secinfo), btf_var_secinfo_cmp);
+	qsort(saved_var_buf->entries, nr_saved_var,
+	      sizeof(struct saved_var), saved_var_cmp);
 
-	last_vsi = (struct btf_var_secinfo *)var_secinfo_buf->entries + nr_var_secinfo - 1;
-	datasec_sz = last_vsi->offset + last_vsi->size;
+	err = btf_encoder__encode_stored_vars(encoder,
+					      (struct saved_var *)saved_var_buf->entries,
+					      nr_saved_var);
+	if (err < 0)
+		return -1;
+
+	last_sv = (struct saved_var *)saved_var_buf->entries + nr_saved_var - 1;
+	datasec_sz = last_sv->offset + last_sv->size;
 
 	id = btf__add_datasec(btf, section_name, datasec_sz);
 	if (id < 0) {
 		btf__log_err(btf, BTF_KIND_DATASEC, section_name, true, id,
 			     "size=%u vlen=%u Error emitting BTF type",
-			     datasec_sz, nr_var_secinfo);
+			     datasec_sz, nr_saved_var);
 	} else {
 		t = btf__type_by_id(btf, id);
-		btf_encoder__log_type(encoder, t, false, true, "size=%u vlen=%u", t->size, nr_var_secinfo);
+		btf_encoder__log_type(encoder, t, false, true, "size=%u vlen=%u",
+				      t->size, nr_saved_var);
 	}
 
-	for (i = 0; i < nr_var_secinfo; i++) {
-		vsi = (struct btf_var_secinfo *)var_secinfo_buf->entries + i;
-		err = btf__add_datasec_var_info(btf, vsi->type, vsi->offset, vsi->size);
+	for (i = 0; i < nr_saved_var; i++) {
+		sv = (struct saved_var *)saved_var_buf->entries + i;
+		if (sv->type == 0)
+			continue;
+		err = btf__add_datasec_var_info(btf, sv->type, sv->offset, sv->size);
 		if (!err) {
 			if (encoder->verbose)
 				printf("\ttype=%u offset=%u size=%u\n",
-				       vsi->type, vsi->offset, vsi->size);
+				       sv->type, sv->offset, sv->size);
 		} else {
 			fprintf(stderr, "\ttype=%u offset=%u size=%u Error emitting BTF datasec var info\n",
-				       vsi->type, vsi->offset, vsi->size);
+				       sv->type, sv->offset, sv->size);
 			return -1;
 		}
 	}
@@ -2091,9 +2155,13 @@ int btf_encoder__encode(struct btf_encoder *encoder, struct conf_load *conf)
 	if (err < 0)
 		return err;
 
-	for (shndx = 1; shndx < encoder->seccnt; shndx++)
-		if (gobuffer__size(&encoder->secinfo[shndx].secinfo))
-			btf_encoder__add_datasec(encoder, shndx);
+	for (shndx = 1; shndx < encoder->seccnt; shndx++) {
+		if (gobuffer__size(&encoder->secinfo[shndx].vars)) {
+			err = btf_encoder__add_datasec(encoder, shndx);
+			if (err < 0)
+				return -1;
+		}
+	}
 
 	/* Empty file, nothing to do, so... done! */
 	if (btf__type_cnt(encoder->btf) == 1)
@@ -2266,10 +2334,11 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
 		uint32_t type, linkage;
 		const char *name;
 		struct llvm_annotation *annot;
+		int annot_count;
+		struct saved_annot *annots;
 		const struct tag *tag;
 		size_t shndx, size;
 		uint64_t addr;
-		int id;
 
 		/* Skip incomplete (non-defining) declarations */
 		if (var->declaration && !var->spec)
@@ -2359,30 +2428,34 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
 			       name, cu->name, addr);
 		}
 
-		/* add a BTF_KIND_VAR in encoder->types */
-		id = btf_encoder__add_var(encoder, type, name, linkage);
-		if (id < 0) {
-			fprintf(stderr, "error: failed to encode variable '%s' at addr 0x%" PRIx64 "\n",
-			        name, addr);
-			goto out;
+		/* Save the annotations */
+		annot_count = 0;
+		annots = NULL;
+		list_for_each_entry(annot, &var->annots, node) {
+			annot_count += 1;
 		}
+		if (annot_count) {
+			int idx = 0;
 
-		list_for_each_entry(annot, &var->annots, node) {
-			int tag_type_id = btf_encoder__add_decl_tag(encoder, annot->value, id, annot->component_idx);
-			if (tag_type_id < 0) {
-				fprintf(stderr, "error: failed to encode tag '%s' to variable '%s' with component_idx %d\n",
-					annot->value, name, annot->component_idx);
+			annots = calloc(annot_count + 1, sizeof(*annots));
+			if (!annots) {
+				fprintf(stderr, "error: allocation failure\n");
+				err = -ENOMEM;
 				goto out;
 			}
+			list_for_each_entry(annot, &var->annots, node) {
+				annots[idx].value = annot->value;
+				annots[idx].component_idx = annot->component_idx;
+			}
 		}
 
 		/*
-		 * Add the variable to the secinfo for the section it appears in.
-		 * Later we will generate a BTF_VAR_DATASEC for all any section with
-		 * an encoded variable.
+		 * Store all the collected information for the variable. Later,
+		 * we will deduplicate and output all.
 		 */
-		id = btf_encoder__add_var_secinfo(encoder, shndx, id, (uint32_t)addr, (uint32_t)size);
-		if (id < 0) {
+		err = btf_encoder__store_var(encoder, shndx, name, type, linkage,
+					    (uint32_t)addr, (uint32_t)size, annots);
+		if (err < 0) {
 			fprintf(stderr, "error: failed to encode section info for variable '%s' at addr 0x%" PRIx64 "\n",
 				name, addr);
 			goto out;
@@ -2515,7 +2588,7 @@ void btf_encoder__delete(struct btf_encoder *encoder)
 		return;
 
 	for (shndx = 0; shndx < encoder->seccnt; shndx++)
-		__gobuffer__delete(&encoder->secinfo[shndx].secinfo);
+		__gobuffer__delete(&encoder->secinfo[shndx].vars);
 	free(encoder->secinfo);
 	zfree(&encoder->filename);
 	zfree(&encoder->source_filename);
-- 
2.43.5


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

* [PATCH 3/3] btf_encoder: don't encode duplicate variables
  2025-02-12  0:49 [PATCH 0/3] Fix duplicated VAR and secinfo Stephen Brennan
  2025-02-12  0:49 ` [PATCH 1/3] btf_encoder: move btf_encoder__add_decl_tag() Stephen Brennan
  2025-02-12  0:49 ` [PATCH 2/3] btf_encoder: postpone VARs until encoding DATASEC Stephen Brennan
@ 2025-02-12  0:49 ` Stephen Brennan
  2025-02-12 17:57   ` Alan Maguire
  2025-02-18 10:36 ` [PATCH 0/3] Fix duplicated VAR and secinfo Alan Maguire
  2025-02-20  2:26 ` Eduard Zingerman
  4 siblings, 1 reply; 14+ messages in thread
From: Stephen Brennan @ 2025-02-12  0:49 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Alan Maguire, dwarves, linux-debuggers, Stephen Brennan

btf__dedup() does not deduplicate variables, nor the contents of the
DATASEC. This is reasonable, because there could very well be multiple
variables of the same name & type from different CUs, but with different
tags or DATASEC offsets. So it is up to us to ensure we don't provide
duplicate variables to libbpf.

Unfortunately, there are some cases where variables are declared
"__weak" in the source code, resulting in the same variable, with the
same name & type, at the same offset, being encoded twice. The DWARF
information does not encode any difference between the "__weak" symbol
and the overriding symbol. And even if it did, we couldn't filter out
"__weak" symbols, because they aren't always overridden. So, we must
deduplicate them.

We can't guarantee that the types are identical prior to deduplication,
but we can at least verify the offsets & names are identical. If this
happens, simply skip the duplicate copy (or copies) of the variable.

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---
 btf_encoder.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/btf_encoder.c b/btf_encoder.c
index d989fbd..29b2054 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -869,8 +869,36 @@ static int32_t btf_encoder__encode_stored_vars(struct btf_encoder *encoder,
 					       struct saved_var *var, uint16_t nr)
 {
 	int i, id, annot_idx;
+	int prev = 0;
 
 	for (i = 0; i < nr; i++) {
+		if (i && var[i].offset == var[prev].offset) {
+			/* The offset of this var is the same as the previous.
+			 * This can happen when a variable is declared in one CU
+			 * as "__weak", and then overridden in another CU. There
+			 * is no indication in the DWARF which symbol is weak,
+			 * so we need to deduplicate. If the duplicate is the
+			 * same name, this is expected, and we can just drop the
+			 * second instance. Otherwise, we need to raise an
+			 * error.
+			 *
+			 * Ideally, we would like to verify that the types of
+			 * the duplicated variables are the same type as well.
+			 * However, there's no way to do this without
+			 * deduplicating the BTF, but at that point, our type
+			 * IDs won't be valid.
+			 */
+			if (strcmp(var[i].name, var[prev].name) == 0) {
+				var[i].type = 0; /* Don't encode corresponding secinfo */
+				continue;
+			} else {
+				fprintf(stderr,
+					"error: encountered variable '%s' (type %x) at  offset 0x%x which duplicates variable '%s' at the same offset\n",
+					var[i].name, var[i].type, var[i].offset, var[prev].name);
+				return -1;
+			}
+		}
+
 		id = btf_encoder__add_var(encoder, var[i].type,
 					  var[i].name, var[i].linkage);
 		if (id < 0) {
@@ -894,6 +922,7 @@ static int32_t btf_encoder__encode_stored_vars(struct btf_encoder *encoder,
 		free(var->annots);
 		var->annots = NULL;
 
+		prev = i;
 		var[i].type = id;
 	}
 	return 0;
-- 
2.43.5


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

* Re: [PATCH 3/3] btf_encoder: don't encode duplicate variables
  2025-02-12  0:49 ` [PATCH 3/3] btf_encoder: don't encode duplicate variables Stephen Brennan
@ 2025-02-12 17:57   ` Alan Maguire
  2025-02-12 18:21     ` Stephen Brennan
  0 siblings, 1 reply; 14+ messages in thread
From: Alan Maguire @ 2025-02-12 17:57 UTC (permalink / raw)
  To: Stephen Brennan, Arnaldo Carvalho de Melo; +Cc: dwarves, linux-debuggers

On 12/02/2025 00:49, Stephen Brennan wrote:
> btf__dedup() does not deduplicate variables, nor the contents of the
> DATASEC. This is reasonable, because there could very well be multiple
> variables of the same name & type from different CUs, but with different
> tags or DATASEC offsets. So it is up to us to ensure we don't provide
> duplicate variables to libbpf.
> 
> Unfortunately, there are some cases where variables are declared
> "__weak" in the source code, resulting in the same variable, with the
> same name & type, at the same offset, being encoded twice. The DWARF
> information does not encode any difference between the "__weak" symbol
> and the overriding symbol. And even if it did, we couldn't filter out
> "__weak" symbols, because they aren't always overridden. So, we must
> deduplicate them.
> 
> We can't guarantee that the types are identical prior to deduplication,
> but we can at least verify the offsets & names are identical. If this
> happens, simply skip the duplicate copy (or copies) of the variable.
>

hi Stephen, quick question; could we do without patch 2, and if we get
an offset match, then just look up the names of the offset-matching
variables in the code directly to see if they refer to the same
variable? that way we would only incur costs for the rare cases where we
get an  offset match. You could use the names__match() function to help
with that I think too. Would that be simpler maybe?

> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
> ---
>  btf_encoder.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/btf_encoder.c b/btf_encoder.c
> index d989fbd..29b2054 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -869,8 +869,36 @@ static int32_t btf_encoder__encode_stored_vars(struct btf_encoder *encoder,
>  					       struct saved_var *var, uint16_t nr)
>  {
>  	int i, id, annot_idx;
> +	int prev = 0;
>  
>  	for (i = 0; i < nr; i++) {
> +		if (i && var[i].offset == var[prev].offset) {
> +			/* The offset of this var is the same as the previous.
> +			 * This can happen when a variable is declared in one CU
> +			 * as "__weak", and then overridden in another CU. There
> +			 * is no indication in the DWARF which symbol is weak,
> +			 * so we need to deduplicate. If the duplicate is the
> +			 * same name, this is expected, and we can just drop the
> +			 * second instance. Otherwise, we need to raise an
> +			 * error.
> +			 *
> +			 * Ideally, we would like to verify that the types of
> +			 * the duplicated variables are the same type as well.
> +			 * However, there's no way to do this without
> +			 * deduplicating the BTF, but at that point, our type
> +			 * IDs won't be valid.
> +			 */
> +			if (strcmp(var[i].name, var[prev].name) == 0) {
> +				var[i].type = 0; /* Don't encode corresponding secinfo */
> +				continue;
> +			} else {
> +				fprintf(stderr,
> +					"error: encountered variable '%s' (type %x) at  offset 0x%x which duplicates variable '%s' at the same offset\n",
> +					var[i].name, var[i].type, var[i].offset, var[prev].name);
> +				return -1;
> +			}
> +		}
> +
>  		id = btf_encoder__add_var(encoder, var[i].type,
>  					  var[i].name, var[i].linkage);
>  		if (id < 0) {
> @@ -894,6 +922,7 @@ static int32_t btf_encoder__encode_stored_vars(struct btf_encoder *encoder,
>  		free(var->annots);
>  		var->annots = NULL;
>  
> +		prev = i;
>  		var[i].type = id;
>  	}
>  	return 0;


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

* Re: [PATCH 3/3] btf_encoder: don't encode duplicate variables
  2025-02-12 17:57   ` Alan Maguire
@ 2025-02-12 18:21     ` Stephen Brennan
  0 siblings, 0 replies; 14+ messages in thread
From: Stephen Brennan @ 2025-02-12 18:21 UTC (permalink / raw)
  To: Alan Maguire, Arnaldo Carvalho de Melo; +Cc: dwarves, linux-debuggers

Alan Maguire <alan.maguire@oracle.com> writes:
> On 12/02/2025 00:49, Stephen Brennan wrote:
>> btf__dedup() does not deduplicate variables, nor the contents of the
>> DATASEC. This is reasonable, because there could very well be multiple
>> variables of the same name & type from different CUs, but with different
>> tags or DATASEC offsets. So it is up to us to ensure we don't provide
>> duplicate variables to libbpf.
>> 
>> Unfortunately, there are some cases where variables are declared
>> "__weak" in the source code, resulting in the same variable, with the
>> same name & type, at the same offset, being encoded twice. The DWARF
>> information does not encode any difference between the "__weak" symbol
>> and the overriding symbol. And even if it did, we couldn't filter out
>> "__weak" symbols, because they aren't always overridden. So, we must
>> deduplicate them.
>> 
>> We can't guarantee that the types are identical prior to deduplication,
>> but we can at least verify the offsets & names are identical. If this
>> happens, simply skip the duplicate copy (or copies) of the variable.
>>
>
> hi Stephen, quick question; could we do without patch 2, and if we get
> an offset match, then just look up the names of the offset-matching
> variables in the code directly to see if they refer to the same
> variable? that way we would only incur costs for the rare cases where we
> get an  offset match. You could use the names__match() function to help
> with that I think too. Would that be simpler maybe?

Without patch 2, we would have already emitted the VAR and DECL_TAG
types. We can still skip emitting the var_secinfo which avoids the
validation error, but it feels a bit messy to leave around the extra
VARs that aren't covered by any var_secinfo. You could probably say with
more confidence whether that's an actual problem.

But as far as the costs incurred by this & patch 2, I _think_
there's not really much extra: yes, we have to store a bit more data
into the buffer for each ELF section, so there is that. (I can probably
reduce some of that in v2, some of the stuff I did is a bit silly in
hindsight.) Mostly we just rearrange the order of when we add the VAR
and DATASEC, and we only check for name matches when we have an offset
match anyway. I do concede that prior to patch 2, VARs may be created in
parallel, and now, they are created in serial, so for parallel BTF that
may be a small regression. Ultimately, I haven't benchmarked and can try
that as well if you'd like.

Let me know how you feel about the extra VARs & the performance!

Thanks,
Stephen

>> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
>> ---
>>  btf_encoder.c | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>> 
>> diff --git a/btf_encoder.c b/btf_encoder.c
>> index d989fbd..29b2054 100644
>> --- a/btf_encoder.c
>> +++ b/btf_encoder.c
>> @@ -869,8 +869,36 @@ static int32_t btf_encoder__encode_stored_vars(struct btf_encoder *encoder,
>>  					       struct saved_var *var, uint16_t nr)
>>  {
>>  	int i, id, annot_idx;
>> +	int prev = 0;
>>  
>>  	for (i = 0; i < nr; i++) {
>> +		if (i && var[i].offset == var[prev].offset) {
>> +			/* The offset of this var is the same as the previous.
>> +			 * This can happen when a variable is declared in one CU
>> +			 * as "__weak", and then overridden in another CU. There
>> +			 * is no indication in the DWARF which symbol is weak,
>> +			 * so we need to deduplicate. If the duplicate is the
>> +			 * same name, this is expected, and we can just drop the
>> +			 * second instance. Otherwise, we need to raise an
>> +			 * error.
>> +			 *
>> +			 * Ideally, we would like to verify that the types of
>> +			 * the duplicated variables are the same type as well.
>> +			 * However, there's no way to do this without
>> +			 * deduplicating the BTF, but at that point, our type
>> +			 * IDs won't be valid.
>> +			 */
>> +			if (strcmp(var[i].name, var[prev].name) == 0) {
>> +				var[i].type = 0; /* Don't encode corresponding secinfo */
>> +				continue;
>> +			} else {
>> +				fprintf(stderr,
>> +					"error: encountered variable '%s' (type %x) at  offset 0x%x which duplicates variable '%s' at the same offset\n",
>> +					var[i].name, var[i].type, var[i].offset, var[prev].name);
>> +				return -1;
>> +			}
>> +		}
>> +
>>  		id = btf_encoder__add_var(encoder, var[i].type,
>>  					  var[i].name, var[i].linkage);
>>  		if (id < 0) {
>> @@ -894,6 +922,7 @@ static int32_t btf_encoder__encode_stored_vars(struct btf_encoder *encoder,
>>  		free(var->annots);
>>  		var->annots = NULL;
>>  
>> +		prev = i;
>>  		var[i].type = id;
>>  	}
>>  	return 0;

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

* Re: [PATCH 0/3] Fix duplicated VAR and secinfo
  2025-02-12  0:49 [PATCH 0/3] Fix duplicated VAR and secinfo Stephen Brennan
                   ` (2 preceding siblings ...)
  2025-02-12  0:49 ` [PATCH 3/3] btf_encoder: don't encode duplicate variables Stephen Brennan
@ 2025-02-18 10:36 ` Alan Maguire
  2025-02-18 16:54   ` Stephen Brennan
  2025-02-20  2:26 ` Eduard Zingerman
  4 siblings, 1 reply; 14+ messages in thread
From: Alan Maguire @ 2025-02-18 10:36 UTC (permalink / raw)
  To: Stephen Brennan, Arnaldo Carvalho de Melo, Andrii Nakryiko
  Cc: dwarves, linux-debuggers, Eduard Zingerman

On 12/02/2025 00:49, Stephen Brennan wrote:
> Hello all,
> 
> I was recently able to clear up some issues related to the interaction
> between kallsyms generation and BTF generation, which was interfering with
> using BTF with global variables in the kernel. I sent some patches[1] to
> the kernel to enable global_var. After further testing, I see that enabling
> global_var results in the following validation errors:
> 
>     BPF: #011 type_id=23691 offset=435904 size=96
>     fbcon: Taking over console
>     BPF:
>     BPF: Invalid offset
>     BPF:
> 
> Thanks to Alan Maguire's quick debugging, we were able to find the root
> cause. It turned out that several btf_var_secinfo entries had the exact
> same offset (and name) as their predecessors. The reason was that these
> entries corresponded to some variables are declared as "__weak" and then
> overridden. For example:
> 
>    // kernel/bpf/helpers.c
>    const struct bpf_func_proto bpf_get_current_task_proto __weak;
>    // kernel/trace/bpf_trace.c
>    const struct bpf_func_proto bpf_get_current_task_proto = {...
> 
> Both declarations appear in the DWARF as variable declarations, but it
> seems that there is no way to find out which one of the declarations is
> "__weak". (I checked this via llvm-dwarfdump). Overall, in a simple kernel
> configuration, I found 47 btf_var_secinfo which had duplicated offsets. In
> each case, both secinfos referred to distinct VARs, which had identical
> names and types. All were due to the "__weak" symbol. We need to eliminate
> these duplicates in order for the BTF to be validated by the kernel.
> 
> This patch series does the deduplication of the VAR and SECINFO in pahole,
> by collecting the lists of variables for each ELF section, and then
> outputting them all once the list is sorted by offset and duplicates are
> identified. The libbpf btf__dedup() function does not deduplicate DATASEC
> or VAR. It would probably be possible to implement this there, and I'm open
> to feedback or suggestions regarding this. I implemented it in pahole
> because I'm most familiar with that code, and because it seems to me like
> it's reasonable for libbpf to expect that the input variable information is
> already deduplicated.
> 

I've been thinking about whether core BTF deduplication should handle
this case - I'll try and lay out the process and maybe we can think
about whether it's better to solve in core dedup or within pahole.

The deduplication of VARs should be straightforward - they are
considered reference types and since in cases like this they share a
name and refer to the same type, the reference type deduplication could
be extended to cover them.  However, the DATASEC references to such
variables are a bit trickier.

As seen above, the kernel currently disallows DATASEC btf_var_secinfo
references that are overlapping, i.e. if I have

[123520] DATASEC '.data..percpu' size=229632 vlen=392
	type_id=7708 offset=0 size=48 (VAR 'fixed_percpu_data')
	type_id=5559 offset=4096 size=4096 (VAR 'cpu_debug_store')
	type_id=7060 offset=8192 size=16384 (VAR 'irq_stack_backing_store')


..the kernel enforces that irq_stack_backing_store starts at >= the
offset of the previous var (cpu_debug_store) + its size (4096+4096 in
this case). This is why the kernel rejects BTF with multiple instances
of the same variable, since they overlap.

So if we consider the case of deduplicating variables; before dedup we
would have something like this in the DATASEC

	type_id=8188 offset=256 size=48 (VAR 'foo')
	type_id=9190 offset=256 size=48 (VAR 'foo')


...and after dedup + remapping it would be:

	type_id=8188 offset=256 size=48 (VAR 'foo')
	type_id=8188 offset=256 size=48 (VAR 'foo')

This still violates the overlap check. So there are a few options here:

- change the kernel to relax overlap check when multiple references have
same type id/offset/size; i.e. they refer to the same variable
- have pahole weed out such occurrences
- something else?

Ideally we don't want to have to resize DATASECs with such duplicate
entries as that would add more complexity to dedup.

Anyway I'd be interested to hear what others think about whether solving
this in BTF dedup itself or pahole makes most sense. Thanks!

Alan

> I've gone ahead and tested this by building & booting a kernel with these
> changes, and the kernel patch series at [1]. The result exhibited no BPF
> varidation errors, and the drgn BTF branch[2] is working perfectly with it!
> 
> Thanks,
> Stephen
> 
> [1]: https://lore.kernel.org/bpf/20250207012045.2129841-1-stephen.s.brennan@oracle.com/
> [2]: https://github.com/brenns10/drgn/commits/btf_2024
> 
> Stephen Brennan (3):
>   btf_encoder: move btf_encoder__add_decl_tag()
>   btf_encoder: postpone VARs until encoding DATASEC
>   btf_encoder: don't encode duplicate variables
> 
>  btf_encoder.c | 234 ++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 168 insertions(+), 66 deletions(-)
> 


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

* Re: [PATCH 0/3] Fix duplicated VAR and secinfo
  2025-02-18 10:36 ` [PATCH 0/3] Fix duplicated VAR and secinfo Alan Maguire
@ 2025-02-18 16:54   ` Stephen Brennan
  2025-02-19  7:48     ` Eduard Zingerman
  0 siblings, 1 reply; 14+ messages in thread
From: Stephen Brennan @ 2025-02-18 16:54 UTC (permalink / raw)
  To: Alan Maguire, Arnaldo Carvalho de Melo, Andrii Nakryiko
  Cc: dwarves, linux-debuggers, Eduard Zingerman

Alan Maguire <alan.maguire@oracle.com> writes:
> On 12/02/2025 00:49, Stephen Brennan wrote:
>> [...]
>> to feedback or suggestions regarding this. I implemented it in pahole
>> because I'm most familiar with that code, and because it seems to me like
>> it's reasonable for libbpf to expect that the input variable information is
>> already deduplicated.
>> 
>
> I've been thinking about whether core BTF deduplication should handle
> this case - I'll try and lay out the process and maybe we can think
> about whether it's better to solve in core dedup or within pahole.
>
> The deduplication of VARs should be straightforward - they are
> considered reference types and since in cases like this they share a
> name and refer to the same type, the reference type deduplication could
> be extended to cover them.

I wonder if there's a hidden catch here... DECL_TAGs can refer to VARs,
but as far as I can tell, the semantics are that the DECL_TAG actually
*modifies* the VAR - it says "this variable declaration had this
annotation / tag / whatever". This is fundamentally because the
var_secinfo doesn't point at a chain of DECL_TAGs, it points directly at
the VAR. (Compare that to, e.g. const/volatile/restrict qualifiers)

So, in a hypothetical situation where there are two variables of the
same name & type, but one of them has a DECL_TAG referring to it, would
the deduplication keep these separate?  It seems to me that would be the
correct behavior.

> However, the DATASEC references to such
> variables are a bit trickier.
>
> As seen above, the kernel currently disallows DATASEC btf_var_secinfo
> references that are overlapping, i.e. if I have
>
> [123520] DATASEC '.data..percpu' size=229632 vlen=392
> 	type_id=7708 offset=0 size=48 (VAR 'fixed_percpu_data')
> 	type_id=5559 offset=4096 size=4096 (VAR 'cpu_debug_store')
> 	type_id=7060 offset=8192 size=16384 (VAR 'irq_stack_backing_store')
>
>
> ..the kernel enforces that irq_stack_backing_store starts at >= the
> offset of the previous var (cpu_debug_store) + its size (4096+4096 in
> this case). This is why the kernel rejects BTF with multiple instances
> of the same variable, since they overlap.
>
> So if we consider the case of deduplicating variables; before dedup we
> would have something like this in the DATASEC
>
> 	type_id=8188 offset=256 size=48 (VAR 'foo')
> 	type_id=9190 offset=256 size=48 (VAR 'foo')
>
>
> ...and after dedup + remapping it would be:
>
> 	type_id=8188 offset=256 size=48 (VAR 'foo')
> 	type_id=8188 offset=256 size=48 (VAR 'foo')
>
> This still violates the overlap check. So there are a few options here:
>
> - change the kernel to relax overlap check when multiple references have
> same type id/offset/size; i.e. they refer to the same variable
> - have pahole weed out such occurrences
> - something else?
>
> Ideally we don't want to have to resize DATASECs with such duplicate
> entries as that would add more complexity to dedup.

Do you think you could share a bit more about the added complexity &
cost? I admittedly don't have any knowledge about the deduplicator, but
naively it seems like the ideal solution would be to delete the
duplicated var_secinfo and resize the DATASEC.

> Anyway I'd be interested to hear what others think about whether solving
> this in BTF dedup itself or pahole makes most sense. Thanks!

Thanks for taking a look into this! Hoping to hear from other
experienced voices on this question as well.

Thanks,
Stephen

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

* Re: [PATCH 0/3] Fix duplicated VAR and secinfo
  2025-02-18 16:54   ` Stephen Brennan
@ 2025-02-19  7:48     ` Eduard Zingerman
  0 siblings, 0 replies; 14+ messages in thread
From: Eduard Zingerman @ 2025-02-19  7:48 UTC (permalink / raw)
  To: Stephen Brennan, Alan Maguire, Arnaldo Carvalho de Melo,
	Andrii Nakryiko
  Cc: dwarves, linux-debuggers

On Tue, 2025-02-18 at 08:54 -0800, Stephen Brennan wrote:
> Alan Maguire <alan.maguire@oracle.com> writes:

Hi Stephen, Alan,

[...]

> I wonder if there's a hidden catch here... DECL_TAGs can refer to VARs,
> but as far as I can tell, the semantics are that the DECL_TAG actually
> *modifies* the VAR - it says "this variable declaration had this
> annotation / tag / whatever". This is fundamentally because the
> var_secinfo doesn't point at a chain of DECL_TAGs, it points directly at
> the VAR. (Compare that to, e.g. const/volatile/restrict qualifiers)
> 
> So, in a hypothetical situation where there are two variables of the
> same name & type, but one of them has a DECL_TAG referring to it, would
> the deduplication keep these separate?  It seems to me that would be the
> correct behavior.

This is something too look out for.
E.g. this is certainly true for decl tags attached to structs.

[...]

> > Anyway I'd be interested to hear what others think about whether solving
> > this in BTF dedup itself or pahole makes most sense. Thanks!
> 
> Thanks for taking a look into this! Hoping to hear from other
> experienced voices on this question as well.

[...]

Deduplication for variables would be a bit of a weird one.
As Alan notes in the previous email, for each data section there is a:
- 'btf_type' entry of kind BTF_KIND_DATASEC where 'vlen' specifies
  the number of variables in the section;
- followed by a number of 'btf_var_secinfo' entries,
  with 'type' referring to 'btf_type' entries of kind 'BTF_KIND_VAR,
  and 'offset' specifying offset of the variable within section.

So, we are talking about deduplication of these 'btf_var_secinfo'
entries. Nothing in existing dedup algorithm handles deduplication of
"addendum" items like 'btf_var_secinfo', the subject of deduplication
always has an ID.

A new step, walking each BTF_KIND_DATASEC and collapsing identical
'btf_var_secinfo' entries can of-course be added.
(Which also raises a question, whether this has to be done in a loop
 with an attempt to collapse duplicate BTF_KIND_DATASEC entries?).

A case can be made both for and against this, but given the
"weirdness" described above and the fact that Stephen already has the
code, I think performing this deduplication at the pahole level is
Ok at the moment.

Thanks,
Eduard



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

* Re: [PATCH 2/3] btf_encoder: postpone VARs until encoding DATASEC
  2025-02-12  0:49 ` [PATCH 2/3] btf_encoder: postpone VARs until encoding DATASEC Stephen Brennan
@ 2025-02-19 23:51   ` Eduard Zingerman
  0 siblings, 0 replies; 14+ messages in thread
From: Eduard Zingerman @ 2025-02-19 23:51 UTC (permalink / raw)
  To: Stephen Brennan, Arnaldo Carvalho de Melo
  Cc: Alan Maguire, dwarves, linux-debuggers

On Tue, 2025-02-11 at 16:49 -0800, Stephen Brennan wrote:
> In order to handle duplicate variables in a data section, we'll need to
> compare variables against the others in their data section. To do this,
> we'll need to postpone adding the variables until they have all been
> identified and collected.  Store all the necessary data to encode the
> VARs (and their DECL_TAGs) and then encode them just before the DATASEC
> containing them. No meaningful change in the output is intended, though
> the ordering of type IDs generated will likely differ.
> 
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
> ---

I think this patch looks good,
but annotation index handling needs a small a fix,
see below.

Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

> @@ -109,6 +109,20 @@ struct elf_functions {
>  	int suffix_cnt; /* number of .isra, .part etc */
>  };
>  
> +struct saved_annot {
> +	const char *value;
> +	int component_idx;
> +};
> +
> +struct saved_var {
> +	const char *name;
> +	uint32_t type;

Nit: maybe name this 'id' or 'var_id'?

> +	struct saved_annot *annots;
> +	uint32_t linkage;
> +	uint32_t offset;
> +	uint32_t size;
> +};
> +

[...]

> @@ -2359,30 +2428,34 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
>  			       name, cu->name, addr);
>  		}
>  
> -		/* add a BTF_KIND_VAR in encoder->types */
> -		id = btf_encoder__add_var(encoder, type, name, linkage);
> -		if (id < 0) {
> -			fprintf(stderr, "error: failed to encode variable '%s' at addr 0x%" PRIx64 "\n",
> -			        name, addr);
> -			goto out;
> +		/* Save the annotations */
> +		annot_count = 0;
> +		annots = NULL;
> +		list_for_each_entry(annot, &var->annots, node) {
> +			annot_count += 1;
>  		}
> +		if (annot_count) {
> +			int idx = 0;
>  
> -		list_for_each_entry(annot, &var->annots, node) {
> -			int tag_type_id = btf_encoder__add_decl_tag(encoder, annot->value, id, annot->component_idx);
> -			if (tag_type_id < 0) {
> -				fprintf(stderr, "error: failed to encode tag '%s' to variable '%s' with component_idx %d\n",
> -					annot->value, name, annot->component_idx);
> +			annots = calloc(annot_count + 1, sizeof(*annots));
> +			if (!annots) {
> +				fprintf(stderr, "error: allocation failure\n");
> +				err = -ENOMEM;
>  				goto out;
>  			}
> +			list_for_each_entry(annot, &var->annots, node) {
> +				annots[idx].value = annot->value;
> +				annots[idx].component_idx = annot->component_idx;
> +			}

I think 'idx++' is missing here.

>  		}
>  
>  		/*
> -		 * Add the variable to the secinfo for the section it appears in.
> -		 * Later we will generate a BTF_VAR_DATASEC for all any section with
> -		 * an encoded variable.
> +		 * Store all the collected information for the variable. Later,
> +		 * we will deduplicate and output all.
>  		 */
> -		id = btf_encoder__add_var_secinfo(encoder, shndx, id, (uint32_t)addr, (uint32_t)size);
> -		if (id < 0) {
> +		err = btf_encoder__store_var(encoder, shndx, name, type, linkage,
> +					    (uint32_t)addr, (uint32_t)size, annots);
> +		if (err < 0) {
>  			fprintf(stderr, "error: failed to encode section info for variable '%s' at addr 0x%" PRIx64 "\n",
>  				name, addr);

Free 'annots' in case of an error?

>  			goto out;

[...]


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

* Re: [PATCH 0/3] Fix duplicated VAR and secinfo
  2025-02-12  0:49 [PATCH 0/3] Fix duplicated VAR and secinfo Stephen Brennan
                   ` (3 preceding siblings ...)
  2025-02-18 10:36 ` [PATCH 0/3] Fix duplicated VAR and secinfo Alan Maguire
@ 2025-02-20  2:26 ` Eduard Zingerman
  2025-02-21  1:05   ` Stephen Brennan
  4 siblings, 1 reply; 14+ messages in thread
From: Eduard Zingerman @ 2025-02-20  2:26 UTC (permalink / raw)
  To: Stephen Brennan, Arnaldo Carvalho de Melo
  Cc: Alan Maguire, dwarves, linux-debuggers

On Tue, 2025-02-11 at 16:49 -0800, Stephen Brennan wrote:

[...]

> I've gone ahead and tested this by building & booting a kernel with these
> changes, and the kernel patch series at [1]. The result exhibited no BPF
> varidation errors, and the drgn BTF branch[2] is working perfectly with it!

I tried out this patch-set:
- applied it to pahole;
- applied [1] to kernel;
- configured kernel as in [2] to run BPF selftests.

With this all done none of BPF selftests could be run.
E.g. doing './test_progs -vvv -n 1' gets me the following error:

  libbpf: Error in bpf_object__probe_loading(): -EINVAL. Couldn't load trivial BPF program. Make sure your kernel supports BPF (CONFIG_BPF_SYSCALL=y) and/or that RLIMIT_MEMLOCK is set to big enough value.

If libbpf is hacked to print verifier log for
bpf_object__probe_loading, the log looks as follows:

  in-kernel BTF is malformed
  processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0

BTF is broken in some nasty way, as nothing is printed to dmesg,
even if I add some printk's inside bpf_get_btf_vmlinux().
All I can see is that 'btf_vmlinux' is set to 0xffffffffffffffea very
early in the boot process.

[1] https://lore.kernel.org/bpf/20250207012045.2129841-1-stephen.s.brennan@oracle.com/
[2] https://gist.github.com/eddyz87/5282db54255b81eed731b955dc5ea690


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

* Re: [PATCH 0/3] Fix duplicated VAR and secinfo
  2025-02-20  2:26 ` Eduard Zingerman
@ 2025-02-21  1:05   ` Stephen Brennan
  2025-02-21  1:30     ` Eduard Zingerman
  0 siblings, 1 reply; 14+ messages in thread
From: Stephen Brennan @ 2025-02-21  1:05 UTC (permalink / raw)
  To: Eduard Zingerman, Arnaldo Carvalho de Melo
  Cc: Alan Maguire, dwarves, linux-debuggers

Eduard Zingerman <eddyz87@gmail.com> writes:

> On Tue, 2025-02-11 at 16:49 -0800, Stephen Brennan wrote:
>
> [...]
>
>> I've gone ahead and tested this by building & booting a kernel with these
>> changes, and the kernel patch series at [1]. The result exhibited no BPF
>> varidation errors, and the drgn BTF branch[2] is working perfectly with it!
>
> I tried out this patch-set:
> - applied it to pahole;
> - applied [1] to kernel;
> - configured kernel as in [2] to run BPF selftests.
>
> With this all done none of BPF selftests could be run.
> E.g. doing './test_progs -vvv -n 1' gets me the following error:
>
>   libbpf: Error in bpf_object__probe_loading(): -EINVAL. Couldn't load trivial BPF program. Make sure your kernel supports BPF (CONFIG_BPF_SYSCALL=y) and/or that RLIMIT_MEMLOCK is set to big enough value.
>
> If libbpf is hacked to print verifier log for
> bpf_object__probe_loading, the log looks as follows:
>
>   in-kernel BTF is malformed
>   processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
>
> BTF is broken in some nasty way, as nothing is printed to dmesg,
> even if I add some printk's inside bpf_get_btf_vmlinux().
> All I can see is that 'btf_vmlinux' is set to 0xffffffffffffffea very
> early in the boot process.
>
> [1] https://lore.kernel.org/bpf/20250207012045.2129841-1-stephen.s.brennan@oracle.com/
> [2] https://gist.github.com/eddyz87/5282db54255b81eed731b955dc5ea690

Thank you for the testing (and the review). I haven't yet been able to
reproduce this. But I see you're using clang. I'll spend the morning
building & running selftests with clang to see whether that is the
issue.

Thanks,
Stephen

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

* Re: [PATCH 0/3] Fix duplicated VAR and secinfo
  2025-02-21  1:05   ` Stephen Brennan
@ 2025-02-21  1:30     ` Eduard Zingerman
  2025-02-25  1:16       ` Stephen Brennan
  0 siblings, 1 reply; 14+ messages in thread
From: Eduard Zingerman @ 2025-02-21  1:30 UTC (permalink / raw)
  To: Stephen Brennan, Arnaldo Carvalho de Melo
  Cc: Alan Maguire, dwarves, linux-debuggers

On Thu, 2025-02-20 at 17:05 -0800, Stephen Brennan wrote:

[...]

> Thank you for the testing (and the review). I haven't yet been able to
> reproduce this. But I see you're using clang. I'll spend the morning
> building & running selftests with clang to see whether that is the
> issue.

Right, should have pointed that out in the email.
Clang version is 19.1.7.
Just tried with gcc, and `./test_progs -n 1` indeed works.
Then re-tried with clang, same result as yesterday, test fails.

Thanks,
Eduard


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

* Re: [PATCH 0/3] Fix duplicated VAR and secinfo
  2025-02-21  1:30     ` Eduard Zingerman
@ 2025-02-25  1:16       ` Stephen Brennan
  0 siblings, 0 replies; 14+ messages in thread
From: Stephen Brennan @ 2025-02-25  1:16 UTC (permalink / raw)
  To: Eduard Zingerman, Arnaldo Carvalho de Melo
  Cc: Alan Maguire, dwarves, linux-debuggers

Eduard Zingerman <eddyz87@gmail.com> writes:

> On Thu, 2025-02-20 at 17:05 -0800, Stephen Brennan wrote:
>
> [...]
>
>> Thank you for the testing (and the review). I haven't yet been able to
>> reproduce this. But I see you're using clang. I'll spend the morning
>> building & running selftests with clang to see whether that is the
>> issue.
>
> Right, should have pointed that out in the email.
> Clang version is 19.1.7.
> Just tried with gcc, and `./test_progs -n 1` indeed works.
> Then re-tried with clang, same result as yesterday, test fails.

So I've set-up a Fedora 41 environment with the same clang version, and
the plot thickens, at least for me. I haven't even gotten to the BPF
self-tests, because clang is giving other duplicate variables!
(To get these values, I uncommented the error return from my pahole
patch)

From vmlinux:

error: encountered variable "fs_ftype_by_dtype" (type 149818) at offset "3d7500" which duplicates variable "ext4_type_by_mode" at the same offset
error: encountered variable "fs_dtype_by_ftype" (type 149814) at offset "3d8c60" which duplicates variable "ext4_filetype_table" at the same offset
error: encountered variable "pt_regoff" (type 52d893) at offset "3d8e50" which duplicates variable "addrmode_regoffs" at the same offset

And from modules:

error: encountered variable "evergreen_dp_offsets" (type 4e9b6) at offset "19760" which duplicates variable "crtc_offsets" at the same offset
error: encountered variable "ni_dig_offsets" (type 4e9b6) at offset "19760" which duplicates variable "crtc_offsets" at the same offset
error: encountered variable "rv770_smc_int_vectors" (type 5cd3d) at offset "2f220" which duplicates variable "rv740_smc_int_vectors" at the same offset
error: encountered variable "cypress_smc_int_vectors" (type 5cd3d) at offset "2f260" which duplicates variable "cedar_smc_int_vectors" at the same offset
error: encountered variable "juniper_smc_int_vectors" (type 5cd3d) at offset "2f260" which duplicates variable "cedar_smc_int_vectors" at the same offset
error: encountered variable "redwood_smc_int_vectors" (type 5cd3d) at offset "2f260" which duplicates variable "cedar_smc_int_vectors" at the same offset
error: encountered variable "caicos_smc_int_vectors" (type 5cd3d) at offset "2f2a0" which duplicates variable "barts_smc_int_vectors" at the same offset
error: encountered variable "turks_smc_int_vectors" (type 5cd3d) at offset "2f2a0" which duplicates variable "barts_smc_int_vectors" at the same offset
error: encountered variable "caicos_cgcg_cgls_default" (type 5e61d) at offset "2f550" which duplicates variable "barts_cgcg_cgls_default" at the same offset
error: encountered variable "turks_cgcg_cgls_default" (type 5e61d) at offset "2f550" which duplicates variable "barts_cgcg_cgls_default" at the same offset
error: encountered variable "turks_sysls_default" (type 5e622) at offset "30660" which duplicates variable "barts_sysls_default" at the same offset
error: encountered variable "caicos_cgcg_cgls_enable" (type 5e624) at offset "30920" which duplicates variable "barts_cgcg_cgls_enable" at the same offset
error: encountered variable "turks_cgcg_cgls_enable" (type 5e624) at offset "30920" which duplicates variable "barts_cgcg_cgls_enable" at the same offset
error: encountered variable "caicos_cgcg_cgls_disable" (type 5e625) at offset "30b70" which duplicates variable "barts_cgcg_cgls_disable" at the same offset
error: encountered variable "turks_cgcg_cgls_disable" (type 5e625) at offset "30b70" which duplicates variable "barts_cgcg_cgls_disable" at the same offset
error: encountered variable "caicos_mgcg_disable" (type 5e626) at offset "30f50" which duplicates variable "barts_mgcg_disable" at the same offset
error: encountered variable "turks_mgcg_disable" (type 5e626) at offset "30f50" which duplicates variable "barts_mgcg_disable" at the same offset
error: encountered variable "turks_sysls_enable" (type 5e622) at offset "30fd0" which duplicates variable "barts_sysls_enable" at the same offset
error: encountered variable "turks_sysls_disable" (type 5e622) at offset "31140" which duplicates variable "barts_sysls_disable" at the same offset
error: encountered variable "sumo_utc" (type 5f2dc) at offset "313f0" which duplicates variable "sumo_dtc" at the same offset
error: encountered variable "dte_data_curacao_xt" (type 63dee) at offset "38f00" which duplicates variable "dte_data_curacao_pro" at the same offset

From what I can tell, these are all const arrays thrown into the r/o
data section. Their data are duplicated, so it looks like clang has
mapped them to the same addresses. Which is a pretty neat optimization!

As far as I can tell, this is perfectly valid, and I think BTF should
allow these variables to be aliased. So we may need to revisit the
approach more: (1) pahole should still deduplicate identically-named
variables at the same address due to the weak symbol issue. That would
just save space and avoid silliness. But (2), I think we'll need the
verifier to allow variables to overlap.

I'll do that and continue testing.

Stephen

>
> Thanks,
> Eduard

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

end of thread, other threads:[~2025-02-25  1:16 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-12  0:49 [PATCH 0/3] Fix duplicated VAR and secinfo Stephen Brennan
2025-02-12  0:49 ` [PATCH 1/3] btf_encoder: move btf_encoder__add_decl_tag() Stephen Brennan
2025-02-12  0:49 ` [PATCH 2/3] btf_encoder: postpone VARs until encoding DATASEC Stephen Brennan
2025-02-19 23:51   ` Eduard Zingerman
2025-02-12  0:49 ` [PATCH 3/3] btf_encoder: don't encode duplicate variables Stephen Brennan
2025-02-12 17:57   ` Alan Maguire
2025-02-12 18:21     ` Stephen Brennan
2025-02-18 10:36 ` [PATCH 0/3] Fix duplicated VAR and secinfo Alan Maguire
2025-02-18 16:54   ` Stephen Brennan
2025-02-19  7:48     ` Eduard Zingerman
2025-02-20  2:26 ` Eduard Zingerman
2025-02-21  1:05   ` Stephen Brennan
2025-02-21  1:30     ` Eduard Zingerman
2025-02-25  1:16       ` Stephen Brennan

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).