[PATCH bpf-next v2 0/2] tools/resolve_btfids: fix cross-compilation to non-host endianness

[PATCH bpf-next v2 0/2] tools/resolve_btfids: fix cross-compilation to non-host endianness

[Viktor Malik]

The .BTF_ids section is pre-filled with zeroed BTF ID entries during the
build and afterwards patched by resolve_btfids with correct values.
Since resolve_btfids always writes in host-native endianness, it relies
on libelf to do the translation when the target ELF is cross-compiled to
a different endianness (this was introduced in commit 61e8aeda9398
("bpf: Fix libelf endian handling in resolv_btfids")).

Unfortunately, the translation will corrupt the flags fields of SET8
entries because these were written during vmlinux compilation and are in
the correct endianness already. This will lead to numerous selftests
failures such as:

    $ sudo ./test_verifier 502 502
    #502/p sleepable fentry accept FAIL
    Failed to load prog 'Invalid argument'!
    bpf_fentry_test1 is not sleepable
    verification time 34 usec
    stack depth 0
    processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
    Summary: 0 PASSED, 0 SKIPPED, 1 FAILED

Since it's not possible to instruct libelf to translate just certain
values, let's manually bswap the flags in resolve_btfids when needed, so
that libelf then translates everything correctly.

The first patch of the series refactors resolve_btfids by using types
from btf_ids.h instead of accessing the BTF ID data using magic offsets.

Viktor Malik (2):
  tools/resolve_btfids: Refactor set sorting with types from btf_ids.h
  tools/resolve_btfids: fix cross-compilation to non-host endianness

 tools/bpf/resolve_btfids/main.c | 55 ++++++++++++++++++++++++++++-----
 tools/include/linux/btf_ids.h   |  9 ++++++
 2 files changed, 56 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH bpf-next v2 1/2] tools/resolve_btfids: Refactor set sorting with types from btf_ids.h

[Viktor Malik]

Instead of using magic offsets to access BTF ID set data, leverage types
from btf_ids.h (btf_id_set and btf_id_set8) which define the actual
layout of the data. Thanks to this change, set sorting should also
continue working if the layout changes.

This requires to sync the definition of 'struct btf_id_set8' from
include/linux/btf_ids.h to tools/include/linux/btf_ids.h. We don't sync
the rest of the file at the moment, b/c that would require to also sync
multiple dependent headers and we don't need any other defs from
btf_ids.h.

Signed-off-by: Viktor Malik <vmalik@redhat.com>
---
 tools/bpf/resolve_btfids/main.c | 30 ++++++++++++++++++++++--------
 tools/include/linux/btf_ids.h   |  9 +++++++++
 2 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
index 27a23196d58e..7badf1557e5c 100644
--- a/tools/bpf/resolve_btfids/main.c
+++ b/tools/bpf/resolve_btfids/main.c
@@ -70,6 +70,7 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <errno.h>
+#include <linux/btf_ids.h>
 #include <linux/rbtree.h>
 #include <linux/zalloc.h>
 #include <linux/err.h>
@@ -78,7 +79,7 @@
 #include <subcmd/parse-options.h>
 
 #define BTF_IDS_SECTION	".BTF_ids"
-#define BTF_ID		"__BTF_ID__"
+#define BTF_ID_PREFIX	"__BTF_ID__"
 
 #define BTF_STRUCT	"struct"
 #define BTF_UNION	"union"
@@ -161,7 +162,7 @@ static int eprintf(int level, int var, const char *fmt, ...)
 
 static bool is_btf_id(const char *name)
 {
-	return name && !strncmp(name, BTF_ID, sizeof(BTF_ID) - 1);
+	return name && !strncmp(name, BTF_ID_PREFIX, sizeof(BTF_ID_PREFIX) - 1);
 }
 
 static struct btf_id *btf_id__find(struct rb_root *root, const char *name)
@@ -441,7 +442,7 @@ static int symbols_collect(struct object *obj)
 		 * __BTF_ID__TYPE__vfs_truncate__0
 		 * prefix =  ^
 		 */
-		prefix = name + sizeof(BTF_ID) - 1;
+		prefix = name + sizeof(BTF_ID_PREFIX) - 1;
 
 		/* struct */
 		if (!strncmp(prefix, BTF_STRUCT, sizeof(BTF_STRUCT) - 1)) {
@@ -656,8 +657,8 @@ static int sets_patch(struct object *obj)
 	while (next) {
 		unsigned long addr, idx;
 		struct btf_id *id;
-		int *base;
-		int cnt;
+		void *base;
+		int cnt, size;
 
 		id   = rb_entry(next, struct btf_id, rb_node);
 		addr = id->addr[0];
@@ -671,13 +672,26 @@ static int sets_patch(struct object *obj)
 		}
 
 		idx = idx / sizeof(int);
-		base = &ptr[idx] + (id->is_set8 ? 2 : 1);
-		cnt = ptr[idx];
+		if (id->is_set) {
+			struct btf_id_set *set;
+
+			set = (struct btf_id_set *)&ptr[idx];
+			base = set->ids;
+			cnt = set->cnt;
+			size = sizeof(set->ids[0]);
+		} else {
+			struct btf_id_set8 *set8;
+
+			set8 = (struct btf_id_set8 *)&ptr[idx];
+			base = set8->pairs;
+			cnt = set8->cnt;
+			size = sizeof(set8->pairs[0]);
+		}
 
 		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
 			 (idx + 1) * sizeof(int), cnt, id->name);
 
-		qsort(base, cnt, id->is_set8 ? sizeof(uint64_t) : sizeof(int), cmp_id);
+		qsort(base, cnt, size, cmp_id);
 
 		next = rb_next(next);
 	}
diff --git a/tools/include/linux/btf_ids.h b/tools/include/linux/btf_ids.h
index 2f882d5cb30f..72535f00572f 100644
--- a/tools/include/linux/btf_ids.h
+++ b/tools/include/linux/btf_ids.h
@@ -8,6 +8,15 @@ struct btf_id_set {
 	u32 ids[];
 };
 
+struct btf_id_set8 {
+	u32 cnt;
+	u32 flags;
+	struct {
+		u32 id;
+		u32 flags;
+	} pairs[];
+};
+
 #ifdef CONFIG_DEBUG_INFO_BTF
 
 #include <linux/compiler.h> /* for __PASTE */
-- 
2.43.0

[PATCH bpf-next v2 2/2] tools/resolve_btfids: fix cross-compilation to non-host endianness

[Viktor Malik]

The .BTF_ids section is pre-filled with zeroed BTF ID entries during the
build and afterwards patched by resolve_btfids with correct values.
Since resolve_btfids always writes in host-native endianness, it relies
on libelf to do the translation when the target ELF is cross-compiled to
a different endianness (this was introduced in commit 61e8aeda9398
("bpf: Fix libelf endian handling in resolv_btfids")).

Unfortunately, the translation will corrupt the flags fields of SET8
entries because these were written during vmlinux compilation and are in
the correct endianness already. This will lead to numerous selftests
failures such as:

    $ sudo ./test_verifier 502 502
    #502/p sleepable fentry accept FAIL
    Failed to load prog 'Invalid argument'!
    bpf_fentry_test1 is not sleepable
    verification time 34 usec
    stack depth 0
    processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
    Summary: 0 PASSED, 0 SKIPPED, 1 FAILED

Since it's not possible to instruct libelf to translate just certain
values, let's manually bswap the flags in resolve_btfids when needed, so
that libelf then translates everything correctly.

Fixes: ef2c6f370a63 ("tools/resolve_btfids: Add support for 8-byte BTF sets")
Signed-off-by: Viktor Malik <vmalik@redhat.com>
---
 tools/bpf/resolve_btfids/main.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
index 7badf1557e5c..d01603ef6283 100644
--- a/tools/bpf/resolve_btfids/main.c
+++ b/tools/bpf/resolve_btfids/main.c
@@ -652,13 +652,23 @@ static int sets_patch(struct object *obj)
 	Elf_Data *data = obj->efile.idlist;
 	int *ptr = data->d_buf;
 	struct rb_node *next;
+	GElf_Ehdr ehdr;
+	int need_bswap;
+
+	if (gelf_getehdr(obj->efile.elf, &ehdr) == NULL) {
+		pr_err("FAILED cannot get ELF header: %s\n",
+			elf_errmsg(-1));
+		return -1;
+	}
+	need_bswap = (__BYTE_ORDER == __LITTLE_ENDIAN) !=
+		     (ehdr.e_ident[EI_DATA] == ELFDATA2LSB);
 
 	next = rb_first(&obj->sets);
 	while (next) {
 		unsigned long addr, idx;
 		struct btf_id *id;
 		void *base;
-		int cnt, size;
+		int cnt, size, i;
 
 		id   = rb_entry(next, struct btf_id, rb_node);
 		addr = id->addr[0];
@@ -686,6 +696,21 @@ static int sets_patch(struct object *obj)
 			base = set8->pairs;
 			cnt = set8->cnt;
 			size = sizeof(set8->pairs[0]);
+
+			/*
+			 * When ELF endianness does not match endianness of the
+			 * host, libelf will do the translation when updating
+			 * the ELF. This, however, corrupts SET8 flags which are
+			 * already in the target endianness. So, let's bswap
+			 * them to the host endianness and libelf will then
+			 * correctly translate everything.
+			 */
+			if (need_bswap) {
+				for (i = 0; i < cnt; i++) {
+					set8->pairs[i].flags =
+						bswap_32(set8->pairs[i].flags);
+				}
+			}
 		}
 
 		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
-- 
2.43.0

Re: [PATCH bpf-next v2 1/2] tools/resolve_btfids: Refactor set sorting with types from btf_ids.h

[Daniel Borkmann]

On 1/31/24 5:24 PM, Viktor Malik wrote:
> Instead of using magic offsets to access BTF ID set data, leverage types
> from btf_ids.h (btf_id_set and btf_id_set8) which define the actual
> layout of the data. Thanks to this change, set sorting should also
> continue working if the layout changes.
> 
> This requires to sync the definition of 'struct btf_id_set8' from
> include/linux/btf_ids.h to tools/include/linux/btf_ids.h. We don't sync
> the rest of the file at the moment, b/c that would require to also sync
> multiple dependent headers and we don't need any other defs from
> btf_ids.h.
> 
> Signed-off-by: Viktor Malik <vmalik@redhat.com>
> ---
>   tools/bpf/resolve_btfids/main.c | 30 ++++++++++++++++++++++--------
>   tools/include/linux/btf_ids.h   |  9 +++++++++
>   2 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> index 27a23196d58e..7badf1557e5c 100644
> --- a/tools/bpf/resolve_btfids/main.c
> +++ b/tools/bpf/resolve_btfids/main.c
> @@ -70,6 +70,7 @@
>   #include <sys/stat.h>
>   #include <fcntl.h>
>   #include <errno.h>
> +#include <linux/btf_ids.h>
>   #include <linux/rbtree.h>
>   #include <linux/zalloc.h>
>   #include <linux/err.h>
> @@ -78,7 +79,7 @@
>   #include <subcmd/parse-options.h>
>   
>   #define BTF_IDS_SECTION	".BTF_ids"
> -#define BTF_ID		"__BTF_ID__"
> +#define BTF_ID_PREFIX	"__BTF_ID__"
>   
>   #define BTF_STRUCT	"struct"
>   #define BTF_UNION	"union"
> @@ -161,7 +162,7 @@ static int eprintf(int level, int var, const char *fmt, ...)
>   
>   static bool is_btf_id(const char *name)
>   {
> -	return name && !strncmp(name, BTF_ID, sizeof(BTF_ID) - 1);
> +	return name && !strncmp(name, BTF_ID_PREFIX, sizeof(BTF_ID_PREFIX) - 1);
>   }
>   
>   static struct btf_id *btf_id__find(struct rb_root *root, const char *name)
> @@ -441,7 +442,7 @@ static int symbols_collect(struct object *obj)
>   		 * __BTF_ID__TYPE__vfs_truncate__0
>   		 * prefix =  ^
>   		 */
> -		prefix = name + sizeof(BTF_ID) - 1;
> +		prefix = name + sizeof(BTF_ID_PREFIX) - 1;
>   
>   		/* struct */
>   		if (!strncmp(prefix, BTF_STRUCT, sizeof(BTF_STRUCT) - 1)) {
> @@ -656,8 +657,8 @@ static int sets_patch(struct object *obj)
>   	while (next) {
>   		unsigned long addr, idx;
>   		struct btf_id *id;
> -		int *base;
> -		int cnt;
> +		void *base;
> +		int cnt, size;
>   
>   		id   = rb_entry(next, struct btf_id, rb_node);
>   		addr = id->addr[0];
> @@ -671,13 +672,26 @@ static int sets_patch(struct object *obj)
>   		}
>   
>   		idx = idx / sizeof(int);
> -		base = &ptr[idx] + (id->is_set8 ? 2 : 1);
> -		cnt = ptr[idx];
> +		if (id->is_set) {
> +			struct btf_id_set *set;
> +
> +			set = (struct btf_id_set *)&ptr[idx];
> +			base = set->ids;
> +			cnt = set->cnt;
> +			size = sizeof(set->ids[0]);
> +		} else {
> +			struct btf_id_set8 *set8;
> +
> +			set8 = (struct btf_id_set8 *)&ptr[idx];
> +			base = set8->pairs;
> +			cnt = set8->cnt;
> +			size = sizeof(set8->pairs[0]);
> +		}
>   
>   		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
>   			 (idx + 1) * sizeof(int), cnt, id->name);
>   
> -		qsort(base, cnt, id->is_set8 ? sizeof(uint64_t) : sizeof(int), cmp_id);
> +		qsort(base, cnt, size, cmp_id);

Looks good to me, one small remark: perhaps it would also make sense to have an assert
on the id location, such that we have a build error in case the id would not be the
first in the struct / pairs array anymore given then cmp_id would look at wrong data
for the latter given the plain int cast from there.

>   		next = rb_next(next);
>   	}
> diff --git a/tools/include/linux/btf_ids.h b/tools/include/linux/btf_ids.h
> index 2f882d5cb30f..72535f00572f 100644
> --- a/tools/include/linux/btf_ids.h
> +++ b/tools/include/linux/btf_ids.h
> @@ -8,6 +8,15 @@ struct btf_id_set {
>   	u32 ids[];
>   };
>   
> +struct btf_id_set8 {
> +	u32 cnt;
> +	u32 flags;
> +	struct {
> +		u32 id;
> +		u32 flags;
> +	} pairs[];
> +};
> +
>   #ifdef CONFIG_DEBUG_INFO_BTF
>   
>   #include <linux/compiler.h> /* for __PASTE */
> 

Re: [PATCH bpf-next v2 2/2] tools/resolve_btfids: fix cross-compilation to non-host endianness

[Daniel Borkmann]

On 1/31/24 5:24 PM, Viktor Malik wrote:
> The .BTF_ids section is pre-filled with zeroed BTF ID entries during the
> build and afterwards patched by resolve_btfids with correct values.
> Since resolve_btfids always writes in host-native endianness, it relies
> on libelf to do the translation when the target ELF is cross-compiled to
> a different endianness (this was introduced in commit 61e8aeda9398
> ("bpf: Fix libelf endian handling in resolv_btfids")).
> 
> Unfortunately, the translation will corrupt the flags fields of SET8
> entries because these were written during vmlinux compilation and are in
> the correct endianness already. This will lead to numerous selftests
> failures such as:
> 
>      $ sudo ./test_verifier 502 502
>      #502/p sleepable fentry accept FAIL
>      Failed to load prog 'Invalid argument'!
>      bpf_fentry_test1 is not sleepable
>      verification time 34 usec
>      stack depth 0
>      processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
>      Summary: 0 PASSED, 0 SKIPPED, 1 FAILED
> 
> Since it's not possible to instruct libelf to translate just certain
> values, let's manually bswap the flags in resolve_btfids when needed, so
> that libelf then translates everything correctly.
> 
> Fixes: ef2c6f370a63 ("tools/resolve_btfids: Add support for 8-byte BTF sets")
> Signed-off-by: Viktor Malik <vmalik@redhat.com>
> ---
>   tools/bpf/resolve_btfids/main.c | 27 ++++++++++++++++++++++++++-
>   1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> index 7badf1557e5c..d01603ef6283 100644
> --- a/tools/bpf/resolve_btfids/main.c
> +++ b/tools/bpf/resolve_btfids/main.c
> @@ -652,13 +652,23 @@ static int sets_patch(struct object *obj)
>   	Elf_Data *data = obj->efile.idlist;
>   	int *ptr = data->d_buf;
>   	struct rb_node *next;
> +	GElf_Ehdr ehdr;
> +	int need_bswap;
> +
> +	if (gelf_getehdr(obj->efile.elf, &ehdr) == NULL) {
> +		pr_err("FAILED cannot get ELF header: %s\n",
> +			elf_errmsg(-1));
> +		return -1;
> +	}
> +	need_bswap = (__BYTE_ORDER == __LITTLE_ENDIAN) !=
> +		     (ehdr.e_ident[EI_DATA] == ELFDATA2LSB);
>   
>   	next = rb_first(&obj->sets);
>   	while (next) {
>   		unsigned long addr, idx;
>   		struct btf_id *id;
>   		void *base;
> -		int cnt, size;
> +		int cnt, size, i;
>   
>   		id   = rb_entry(next, struct btf_id, rb_node);
>   		addr = id->addr[0];
> @@ -686,6 +696,21 @@ static int sets_patch(struct object *obj)
>   			base = set8->pairs;
>   			cnt = set8->cnt;
>   			size = sizeof(set8->pairs[0]);
> +
> +			/*
> +			 * When ELF endianness does not match endianness of the
> +			 * host, libelf will do the translation when updating
> +			 * the ELF. This, however, corrupts SET8 flags which are
> +			 * already in the target endianness. So, let's bswap
> +			 * them to the host endianness and libelf will then
> +			 * correctly translate everything.
> +			 */
> +			if (need_bswap) {
> +				for (i = 0; i < cnt; i++) {
> +					set8->pairs[i].flags =
> +						bswap_32(set8->pairs[i].flags);
> +				}
> +			}
>   		}
>   

Could we improve that somewhat, e.g. gathering endianness could be moved into
elf_collect() and the test could also be simplified (if I'm not missing sth) ?

Like the below (not even compile-tested ...) :

diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
index 7badf1557e5c..7b5f592fe79c 100644
--- a/tools/bpf/resolve_btfids/main.c
+++ b/tools/bpf/resolve_btfids/main.c
@@ -90,6 +90,14 @@

  #define ADDR_CNT	100

+#if __BYTE_ORDER == __LITTLE_ENDIAN
+# define ELFDATANATIVE	ELFDATA2LSB
+#elif __BYTE_ORDER == __BIG_ENDIAN
+# define ELFDATANATIVE	ELFDATA2MSB
+#else
+# error "Unknown machine endianness!"
+#endif
+
  struct btf_id {
  	struct rb_node	 rb_node;
  	char		*name;
@@ -117,6 +125,7 @@ struct object {
  		int		 idlist_shndx;
  		size_t		 strtabidx;
  		unsigned long	 idlist_addr;
+		int		 encoding;
  	} efile;

  	struct rb_root	sets;
@@ -320,6 +329,7 @@ static int elf_collect(struct object *obj)
  {
  	Elf_Scn *scn = NULL;
  	size_t shdrstrndx;
+	GElf_Ehdr ehdr;
  	int idx = 0;
  	Elf *elf;
  	int fd;
@@ -351,6 +361,13 @@ static int elf_collect(struct object *obj)
  		return -1;
  	}

+	if (gelf_getehdr(obj->efile.elf, &ehdr) == NULL) {
+		pr_err("FAILED cannot get ELF header: %s\n", elf_errmsg(-1));
+		return -1;
+	}
+
+	obj->efile.encoding = ehdr.e_ident[EI_DATA];
+
  	/*
  	 * Scan all the elf sections and look for save data
  	 * from .BTF_ids section and symbols.
@@ -649,6 +666,7 @@ static int cmp_id(const void *pa, const void *pb)

  static int sets_patch(struct object *obj)
  {
+	bool need_bswap = obj->efile.encoding != ELFDATANATIVE;
  	Elf_Data *data = obj->efile.idlist;
  	int *ptr = data->d_buf;
  	struct rb_node *next;
@@ -658,7 +676,7 @@ static int sets_patch(struct object *obj)
  		unsigned long addr, idx;
  		struct btf_id *id;
  		void *base;
-		int cnt, size;
+		int cnt, size, i;

  		id   = rb_entry(next, struct btf_id, rb_node);
  		addr = id->addr[0];
@@ -686,6 +704,21 @@ static int sets_patch(struct object *obj)
  			base = set8->pairs;
  			cnt = set8->cnt;
  			size = sizeof(set8->pairs[0]);
+
+			/*
+			 * When ELF endianness does not match endianness of the
+			 * host, libelf will do the translation when updating
+			 * the ELF. This, however, corrupts SET8 flags which are
+			 * already in the target endianness. So, let's bswap
+			 * them to the host endianness and libelf will then
+			 * correctly translate everything.
+			 */
+			if (need_bswap) {
+				for (i = 0; i < cnt; i++) {
+					set8->pairs[i].flags =
+						bswap_32(set8->pairs[i].flags);
+				}
+			}
  		}

  		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
-- 
2.21.0

Re: [PATCH bpf-next v2 1/2] tools/resolve_btfids: Refactor set sorting with types from btf_ids.h

[Viktor Malik]

On 2/1/24 16:51, Daniel Borkmann wrote:
> On 1/31/24 5:24 PM, Viktor Malik wrote:
>> Instead of using magic offsets to access BTF ID set data, leverage types
>> from btf_ids.h (btf_id_set and btf_id_set8) which define the actual
>> layout of the data. Thanks to this change, set sorting should also
>> continue working if the layout changes.
>>
>> This requires to sync the definition of 'struct btf_id_set8' from
>> include/linux/btf_ids.h to tools/include/linux/btf_ids.h. We don't sync
>> the rest of the file at the moment, b/c that would require to also sync
>> multiple dependent headers and we don't need any other defs from
>> btf_ids.h.
>>
>> Signed-off-by: Viktor Malik <vmalik@redhat.com>
>> ---
>>   tools/bpf/resolve_btfids/main.c | 30 ++++++++++++++++++++++--------
>>   tools/include/linux/btf_ids.h   |  9 +++++++++
>>   2 files changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
>> index 27a23196d58e..7badf1557e5c 100644
>> --- a/tools/bpf/resolve_btfids/main.c
>> +++ b/tools/bpf/resolve_btfids/main.c
>> @@ -70,6 +70,7 @@
>>   #include <sys/stat.h>
>>   #include <fcntl.h>
>>   #include <errno.h>
>> +#include <linux/btf_ids.h>
>>   #include <linux/rbtree.h>
>>   #include <linux/zalloc.h>
>>   #include <linux/err.h>
>> @@ -78,7 +79,7 @@
>>   #include <subcmd/parse-options.h>
>>   
>>   #define BTF_IDS_SECTION	".BTF_ids"
>> -#define BTF_ID		"__BTF_ID__"
>> +#define BTF_ID_PREFIX	"__BTF_ID__"
>>   
>>   #define BTF_STRUCT	"struct"
>>   #define BTF_UNION	"union"
>> @@ -161,7 +162,7 @@ static int eprintf(int level, int var, const char *fmt, ...)
>>   
>>   static bool is_btf_id(const char *name)
>>   {
>> -	return name && !strncmp(name, BTF_ID, sizeof(BTF_ID) - 1);
>> +	return name && !strncmp(name, BTF_ID_PREFIX, sizeof(BTF_ID_PREFIX) - 1);
>>   }
>>   
>>   static struct btf_id *btf_id__find(struct rb_root *root, const char *name)
>> @@ -441,7 +442,7 @@ static int symbols_collect(struct object *obj)
>>   		 * __BTF_ID__TYPE__vfs_truncate__0
>>   		 * prefix =  ^
>>   		 */
>> -		prefix = name + sizeof(BTF_ID) - 1;
>> +		prefix = name + sizeof(BTF_ID_PREFIX) - 1;
>>   
>>   		/* struct */
>>   		if (!strncmp(prefix, BTF_STRUCT, sizeof(BTF_STRUCT) - 1)) {
>> @@ -656,8 +657,8 @@ static int sets_patch(struct object *obj)
>>   	while (next) {
>>   		unsigned long addr, idx;
>>   		struct btf_id *id;
>> -		int *base;
>> -		int cnt;
>> +		void *base;
>> +		int cnt, size;
>>   
>>   		id   = rb_entry(next, struct btf_id, rb_node);
>>   		addr = id->addr[0];
>> @@ -671,13 +672,26 @@ static int sets_patch(struct object *obj)
>>   		}
>>   
>>   		idx = idx / sizeof(int);
>> -		base = &ptr[idx] + (id->is_set8 ? 2 : 1);
>> -		cnt = ptr[idx];
>> +		if (id->is_set) {
>> +			struct btf_id_set *set;
>> +
>> +			set = (struct btf_id_set *)&ptr[idx];
>> +			base = set->ids;
>> +			cnt = set->cnt;
>> +			size = sizeof(set->ids[0]);
>> +		} else {
>> +			struct btf_id_set8 *set8;
>> +
>> +			set8 = (struct btf_id_set8 *)&ptr[idx];
>> +			base = set8->pairs;
>> +			cnt = set8->cnt;
>> +			size = sizeof(set8->pairs[0]);
>> +		}
>>   
>>   		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
>>   			 (idx + 1) * sizeof(int), cnt, id->name);
>>   
>> -		qsort(base, cnt, id->is_set8 ? sizeof(uint64_t) : sizeof(int), cmp_id);
>> +		qsort(base, cnt, size, cmp_id);
> 
> Looks good to me, one small remark: perhaps it would also make sense to have an assert
> on the id location, such that we have a build error in case the id would not be the
> first in the struct / pairs array anymore given then cmp_id would look at wrong data
> for the latter given the plain int cast from there.

That's a good idea, I'll add a BUILD_BUG_ON check for set8.

Thanks!

> 
>>   		next = rb_next(next);
>>   	}
>> diff --git a/tools/include/linux/btf_ids.h b/tools/include/linux/btf_ids.h
>> index 2f882d5cb30f..72535f00572f 100644
>> --- a/tools/include/linux/btf_ids.h
>> +++ b/tools/include/linux/btf_ids.h
>> @@ -8,6 +8,15 @@ struct btf_id_set {
>>   	u32 ids[];
>>   };
>>   
>> +struct btf_id_set8 {
>> +	u32 cnt;
>> +	u32 flags;
>> +	struct {
>> +		u32 id;
>> +		u32 flags;
>> +	} pairs[];
>> +};
>> +
>>   #ifdef CONFIG_DEBUG_INFO_BTF
>>   
>>   #include <linux/compiler.h> /* for __PASTE */
>>
> 

Re: [PATCH bpf-next v2 2/2] tools/resolve_btfids: fix cross-compilation to non-host endianness

[Viktor Malik]

On 2/1/24 17:36, Daniel Borkmann wrote:
> On 1/31/24 5:24 PM, Viktor Malik wrote:
>> The .BTF_ids section is pre-filled with zeroed BTF ID entries during the
>> build and afterwards patched by resolve_btfids with correct values.
>> Since resolve_btfids always writes in host-native endianness, it relies
>> on libelf to do the translation when the target ELF is cross-compiled to
>> a different endianness (this was introduced in commit 61e8aeda9398
>> ("bpf: Fix libelf endian handling in resolv_btfids")).
>>
>> Unfortunately, the translation will corrupt the flags fields of SET8
>> entries because these were written during vmlinux compilation and are in
>> the correct endianness already. This will lead to numerous selftests
>> failures such as:
>>
>>      $ sudo ./test_verifier 502 502
>>      #502/p sleepable fentry accept FAIL
>>      Failed to load prog 'Invalid argument'!
>>      bpf_fentry_test1 is not sleepable
>>      verification time 34 usec
>>      stack depth 0
>>      processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
>>      Summary: 0 PASSED, 0 SKIPPED, 1 FAILED
>>
>> Since it's not possible to instruct libelf to translate just certain
>> values, let's manually bswap the flags in resolve_btfids when needed, so
>> that libelf then translates everything correctly.
>>
>> Fixes: ef2c6f370a63 ("tools/resolve_btfids: Add support for 8-byte BTF sets")
>> Signed-off-by: Viktor Malik <vmalik@redhat.com>
>> ---
>>   tools/bpf/resolve_btfids/main.c | 27 ++++++++++++++++++++++++++-
>>   1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
>> index 7badf1557e5c..d01603ef6283 100644
>> --- a/tools/bpf/resolve_btfids/main.c
>> +++ b/tools/bpf/resolve_btfids/main.c
>> @@ -652,13 +652,23 @@ static int sets_patch(struct object *obj)
>>   	Elf_Data *data = obj->efile.idlist;
>>   	int *ptr = data->d_buf;
>>   	struct rb_node *next;
>> +	GElf_Ehdr ehdr;
>> +	int need_bswap;
>> +
>> +	if (gelf_getehdr(obj->efile.elf, &ehdr) == NULL) {
>> +		pr_err("FAILED cannot get ELF header: %s\n",
>> +			elf_errmsg(-1));
>> +		return -1;
>> +	}
>> +	need_bswap = (__BYTE_ORDER == __LITTLE_ENDIAN) !=
>> +		     (ehdr.e_ident[EI_DATA] == ELFDATA2LSB);
>>   
>>   	next = rb_first(&obj->sets);
>>   	while (next) {
>>   		unsigned long addr, idx;
>>   		struct btf_id *id;
>>   		void *base;
>> -		int cnt, size;
>> +		int cnt, size, i;
>>   
>>   		id   = rb_entry(next, struct btf_id, rb_node);
>>   		addr = id->addr[0];
>> @@ -686,6 +696,21 @@ static int sets_patch(struct object *obj)
>>   			base = set8->pairs;
>>   			cnt = set8->cnt;
>>   			size = sizeof(set8->pairs[0]);
>> +
>> +			/*
>> +			 * When ELF endianness does not match endianness of the
>> +			 * host, libelf will do the translation when updating
>> +			 * the ELF. This, however, corrupts SET8 flags which are
>> +			 * already in the target endianness. So, let's bswap
>> +			 * them to the host endianness and libelf will then
>> +			 * correctly translate everything.
>> +			 */
>> +			if (need_bswap) {
>> +				for (i = 0; i < cnt; i++) {
>> +					set8->pairs[i].flags =
>> +						bswap_32(set8->pairs[i].flags);
>> +				}
>> +			}
>>   		}
>>   
> 
> Could we improve that somewhat, e.g. gathering endianness could be moved into
> elf_collect() and the test could also be simplified (if I'm not missing sth) ?
> 
> Like the below (not even compile-tested ...) :

Thanks for the suggestion, that looks nicer than my version. I'll use
the below, it should work pretty much out of the box.

Viktor

> 
> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> index 7badf1557e5c..7b5f592fe79c 100644
> --- a/tools/bpf/resolve_btfids/main.c
> +++ b/tools/bpf/resolve_btfids/main.c
> @@ -90,6 +90,14 @@
> 
>   #define ADDR_CNT	100
> 
> +#if __BYTE_ORDER == __LITTLE_ENDIAN
> +# define ELFDATANATIVE	ELFDATA2LSB
> +#elif __BYTE_ORDER == __BIG_ENDIAN
> +# define ELFDATANATIVE	ELFDATA2MSB
> +#else
> +# error "Unknown machine endianness!"
> +#endif
> +
>   struct btf_id {
>   	struct rb_node	 rb_node;
>   	char		*name;
> @@ -117,6 +125,7 @@ struct object {
>   		int		 idlist_shndx;
>   		size_t		 strtabidx;
>   		unsigned long	 idlist_addr;
> +		int		 encoding;
>   	} efile;
> 
>   	struct rb_root	sets;
> @@ -320,6 +329,7 @@ static int elf_collect(struct object *obj)
>   {
>   	Elf_Scn *scn = NULL;
>   	size_t shdrstrndx;
> +	GElf_Ehdr ehdr;
>   	int idx = 0;
>   	Elf *elf;
>   	int fd;
> @@ -351,6 +361,13 @@ static int elf_collect(struct object *obj)
>   		return -1;
>   	}
> 
> +	if (gelf_getehdr(obj->efile.elf, &ehdr) == NULL) {
> +		pr_err("FAILED cannot get ELF header: %s\n", elf_errmsg(-1));
> +		return -1;
> +	}
> +
> +	obj->efile.encoding = ehdr.e_ident[EI_DATA];
> +
>   	/*
>   	 * Scan all the elf sections and look for save data
>   	 * from .BTF_ids section and symbols.
> @@ -649,6 +666,7 @@ static int cmp_id(const void *pa, const void *pb)
> 
>   static int sets_patch(struct object *obj)
>   {
> +	bool need_bswap = obj->efile.encoding != ELFDATANATIVE;
>   	Elf_Data *data = obj->efile.idlist;
>   	int *ptr = data->d_buf;
>   	struct rb_node *next;
> @@ -658,7 +676,7 @@ static int sets_patch(struct object *obj)
>   		unsigned long addr, idx;
>   		struct btf_id *id;
>   		void *base;
> -		int cnt, size;
> +		int cnt, size, i;
> 
>   		id   = rb_entry(next, struct btf_id, rb_node);
>   		addr = id->addr[0];
> @@ -686,6 +704,21 @@ static int sets_patch(struct object *obj)
>   			base = set8->pairs;
>   			cnt = set8->cnt;
>   			size = sizeof(set8->pairs[0]);
> +
> +			/*
> +			 * When ELF endianness does not match endianness of the
> +			 * host, libelf will do the translation when updating
> +			 * the ELF. This, however, corrupts SET8 flags which are
> +			 * already in the target endianness. So, let's bswap
> +			 * them to the host endianness and libelf will then
> +			 * correctly translate everything.
> +			 */
> +			if (need_bswap) {
> +				for (i = 0; i < cnt; i++) {
> +					set8->pairs[i].flags =
> +						bswap_32(set8->pairs[i].flags);
> +				}
> +			}
>   		}
> 
>   		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",

Re: [PATCH bpf-next v2 1/2] tools/resolve_btfids: Refactor set sorting with types from btf_ids.h

[Jiri Olsa]

On Wed, Jan 31, 2024 at 05:24:08PM +0100, Viktor Malik wrote:
> Instead of using magic offsets to access BTF ID set data, leverage types
> from btf_ids.h (btf_id_set and btf_id_set8) which define the actual
> layout of the data. Thanks to this change, set sorting should also
> continue working if the layout changes.
> 
> This requires to sync the definition of 'struct btf_id_set8' from
> include/linux/btf_ids.h to tools/include/linux/btf_ids.h. We don't sync
> the rest of the file at the moment, b/c that would require to also sync
> multiple dependent headers and we don't need any other defs from
> btf_ids.h.
> 
> Signed-off-by: Viktor Malik <vmalik@redhat.com>
> ---
>  tools/bpf/resolve_btfids/main.c | 30 ++++++++++++++++++++++--------
>  tools/include/linux/btf_ids.h   |  9 +++++++++
>  2 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> index 27a23196d58e..7badf1557e5c 100644
> --- a/tools/bpf/resolve_btfids/main.c
> +++ b/tools/bpf/resolve_btfids/main.c
> @@ -70,6 +70,7 @@
>  #include <sys/stat.h>
>  #include <fcntl.h>
>  #include <errno.h>
> +#include <linux/btf_ids.h>
>  #include <linux/rbtree.h>
>  #include <linux/zalloc.h>
>  #include <linux/err.h>
> @@ -78,7 +79,7 @@
>  #include <subcmd/parse-options.h>
>  
>  #define BTF_IDS_SECTION	".BTF_ids"
> -#define BTF_ID		"__BTF_ID__"
> +#define BTF_ID_PREFIX	"__BTF_ID__"

nit does not look necessary to me

>  
>  #define BTF_STRUCT	"struct"
>  #define BTF_UNION	"union"
> @@ -161,7 +162,7 @@ static int eprintf(int level, int var, const char *fmt, ...)
>  
>  static bool is_btf_id(const char *name)
>  {
> -	return name && !strncmp(name, BTF_ID, sizeof(BTF_ID) - 1);
> +	return name && !strncmp(name, BTF_ID_PREFIX, sizeof(BTF_ID_PREFIX) - 1);
>  }
>  
>  static struct btf_id *btf_id__find(struct rb_root *root, const char *name)
> @@ -441,7 +442,7 @@ static int symbols_collect(struct object *obj)
>  		 * __BTF_ID__TYPE__vfs_truncate__0
>  		 * prefix =  ^
>  		 */
> -		prefix = name + sizeof(BTF_ID) - 1;
> +		prefix = name + sizeof(BTF_ID_PREFIX) - 1;
>  
>  		/* struct */
>  		if (!strncmp(prefix, BTF_STRUCT, sizeof(BTF_STRUCT) - 1)) {
> @@ -656,8 +657,8 @@ static int sets_patch(struct object *obj)
>  	while (next) {
>  		unsigned long addr, idx;
>  		struct btf_id *id;
> -		int *base;
> -		int cnt;
> +		void *base;
> +		int cnt, size;
>  
>  		id   = rb_entry(next, struct btf_id, rb_node);
>  		addr = id->addr[0];
> @@ -671,13 +672,26 @@ static int sets_patch(struct object *obj)
>  		}
>  
>  		idx = idx / sizeof(int);
> -		base = &ptr[idx] + (id->is_set8 ? 2 : 1);
> -		cnt = ptr[idx];
> +		if (id->is_set) {
> +			struct btf_id_set *set;
> +
> +			set = (struct btf_id_set *)&ptr[idx];
> +			base = set->ids;
> +			cnt = set->cnt;
> +			size = sizeof(set->ids[0]);
> +		} else {
> +			struct btf_id_set8 *set8;
> +
> +			set8 = (struct btf_id_set8 *)&ptr[idx];
> +			base = set8->pairs;
> +			cnt = set8->cnt;
> +			size = sizeof(set8->pairs[0]);
> +		}
>  
>  		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
>  			 (idx + 1) * sizeof(int), cnt, id->name);
>  
> -		qsort(base, cnt, id->is_set8 ? sizeof(uint64_t) : sizeof(int), cmp_id);
> +		qsort(base, cnt, size, cmp_id);

maybe we could call qsort on top of each set type, seems simpler:

 	while (next) {
+		struct btf_id_set8 *set8;
+		struct btf_id_set *set;
 		unsigned long addr, idx;
 		struct btf_id *id;
-		int *base;
-		int cnt;
 
 		id   = rb_entry(next, struct btf_id, rb_node);
 		addr = id->addr[0];
@@ -671,13 +672,16 @@ static int sets_patch(struct object *obj)
 		}
 
 		idx = idx / sizeof(int);
-		base = &ptr[idx] + (id->is_set8 ? 2 : 1);
-		cnt = ptr[idx];
+		if (id->is_set) {
+			set = (struct btf_id_set *)&ptr[idx];
+			qsort(set->ids, set->cnt, sizeof(set->ids[0]), cmp_id);
+		} else {
+			set8 = (struct btf_id_set8 *)&ptr[idx];
+			qsort(set8->pairs, set8->cnt, sizeof(set8->pairs[0]), cmp_id);
+		}
 
 		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
-			 (idx + 1) * sizeof(int), cnt, id->name);
-
-		qsort(base, cnt, id->is_set8 ? sizeof(uint64_t) : sizeof(int), cmp_id);
+			 (idx + 1) * sizeof(int), id->is_set ? set->cnt : set8->cnt, id->name);
 
 		next = rb_next(next);
 	}


jirka

Re: [PATCH bpf-next v2 2/2] tools/resolve_btfids: fix cross-compilation to non-host endianness

[Daniel Xu]

Hi Viktor,

On Wed, Jan 31, 2024 at 05:24:09PM +0100, Viktor Malik wrote:
> The .BTF_ids section is pre-filled with zeroed BTF ID entries during the
> build and afterwards patched by resolve_btfids with correct values.
> Since resolve_btfids always writes in host-native endianness, it relies
> on libelf to do the translation when the target ELF is cross-compiled to
> a different endianness (this was introduced in commit 61e8aeda9398
> ("bpf: Fix libelf endian handling in resolv_btfids")).
> 
> Unfortunately, the translation will corrupt the flags fields of SET8
> entries because these were written during vmlinux compilation and are in
> the correct endianness already. This will lead to numerous selftests
> failures such as:
> 
>     $ sudo ./test_verifier 502 502
>     #502/p sleepable fentry accept FAIL
>     Failed to load prog 'Invalid argument'!
>     bpf_fentry_test1 is not sleepable
>     verification time 34 usec
>     stack depth 0
>     processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
>     Summary: 0 PASSED, 0 SKIPPED, 1 FAILED
> 
> Since it's not possible to instruct libelf to translate just certain
> values, let's manually bswap the flags in resolve_btfids when needed, so
> that libelf then translates everything correctly.
> 
> Fixes: ef2c6f370a63 ("tools/resolve_btfids: Add support for 8-byte BTF sets")
> Signed-off-by: Viktor Malik <vmalik@redhat.com>
> ---
>  tools/bpf/resolve_btfids/main.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> index 7badf1557e5c..d01603ef6283 100644
> --- a/tools/bpf/resolve_btfids/main.c
> +++ b/tools/bpf/resolve_btfids/main.c
> @@ -652,13 +652,23 @@ static int sets_patch(struct object *obj)
>  	Elf_Data *data = obj->efile.idlist;
>  	int *ptr = data->d_buf;
>  	struct rb_node *next;
> +	GElf_Ehdr ehdr;
> +	int need_bswap;
> +
> +	if (gelf_getehdr(obj->efile.elf, &ehdr) == NULL) {
> +		pr_err("FAILED cannot get ELF header: %s\n",
> +			elf_errmsg(-1));
> +		return -1;
> +	}
> +	need_bswap = (__BYTE_ORDER == __LITTLE_ENDIAN) !=
> +		     (ehdr.e_ident[EI_DATA] == ELFDATA2LSB);
>  
>  	next = rb_first(&obj->sets);
>  	while (next) {
>  		unsigned long addr, idx;
>  		struct btf_id *id;
>  		void *base;
> -		int cnt, size;
> +		int cnt, size, i;
>  
>  		id   = rb_entry(next, struct btf_id, rb_node);
>  		addr = id->addr[0];
> @@ -686,6 +696,21 @@ static int sets_patch(struct object *obj)
>  			base = set8->pairs;
>  			cnt = set8->cnt;
>  			size = sizeof(set8->pairs[0]);
> +
> +			/*
> +			 * When ELF endianness does not match endianness of the
> +			 * host, libelf will do the translation when updating
> +			 * the ELF. This, however, corrupts SET8 flags which are
> +			 * already in the target endianness. So, let's bswap
> +			 * them to the host endianness and libelf will then
> +			 * correctly translate everything.
> +			 */
> +			if (need_bswap) {
> +				for (i = 0; i < cnt; i++) {
> +					set8->pairs[i].flags =
> +						bswap_32(set8->pairs[i].flags);
> +				}

Do we need this for btf_id_set8:flags as well? Didn't get a chance to
look too deeply yet.

Thanks,
Daniel

Re: [PATCH bpf-next v2 2/2] tools/resolve_btfids: fix cross-compilation to non-host endianness

[Manu Bretelle]

On Fri, Feb 02, 2024 at 06:38:18PM -0700, Daniel Xu wrote:
> Hi Viktor,
> 
> On Wed, Jan 31, 2024 at 05:24:09PM +0100, Viktor Malik wrote:
> > The .BTF_ids section is pre-filled with zeroed BTF ID entries during the
> > build and afterwards patched by resolve_btfids with correct values.
> > Since resolve_btfids always writes in host-native endianness, it relies
> > on libelf to do the translation when the target ELF is cross-compiled to
> > a different endianness (this was introduced in commit 61e8aeda9398
> > ("bpf: Fix libelf endian handling in resolv_btfids")).
> > 
> > Unfortunately, the translation will corrupt the flags fields of SET8
> > entries because these were written during vmlinux compilation and are in
> > the correct endianness already. This will lead to numerous selftests
> > failures such as:
> > 
> >     $ sudo ./test_verifier 502 502
> >     #502/p sleepable fentry accept FAIL
> >     Failed to load prog 'Invalid argument'!
> >     bpf_fentry_test1 is not sleepable
> >     verification time 34 usec
> >     stack depth 0
> >     processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
> >     Summary: 0 PASSED, 0 SKIPPED, 1 FAILED
> > 
> > Since it's not possible to instruct libelf to translate just certain
> > values, let's manually bswap the flags in resolve_btfids when needed, so
> > that libelf then translates everything correctly.
> > 
> > Fixes: ef2c6f370a63 ("tools/resolve_btfids: Add support for 8-byte BTF sets")
> > Signed-off-by: Viktor Malik <vmalik@redhat.com>
> > ---
> >  tools/bpf/resolve_btfids/main.c | 27 ++++++++++++++++++++++++++-
> >  1 file changed, 26 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> > index 7badf1557e5c..d01603ef6283 100644
> > --- a/tools/bpf/resolve_btfids/main.c
> > +++ b/tools/bpf/resolve_btfids/main.c
> > @@ -652,13 +652,23 @@ static int sets_patch(struct object *obj)
> >  	Elf_Data *data = obj->efile.idlist;
> >  	int *ptr = data->d_buf;
> >  	struct rb_node *next;
> > +	GElf_Ehdr ehdr;
> > +	int need_bswap;
> > +
> > +	if (gelf_getehdr(obj->efile.elf, &ehdr) == NULL) {
> > +		pr_err("FAILED cannot get ELF header: %s\n",
> > +			elf_errmsg(-1));
> > +		return -1;
> > +	}
> > +	need_bswap = (__BYTE_ORDER == __LITTLE_ENDIAN) !=
> > +		     (ehdr.e_ident[EI_DATA] == ELFDATA2LSB);
> >  
> >  	next = rb_first(&obj->sets);
> >  	while (next) {
> >  		unsigned long addr, idx;
> >  		struct btf_id *id;
> >  		void *base;
> > -		int cnt, size;
> > +		int cnt, size, i;
> >  
> >  		id   = rb_entry(next, struct btf_id, rb_node);
> >  		addr = id->addr[0];
> > @@ -686,6 +696,21 @@ static int sets_patch(struct object *obj)
> >  			base = set8->pairs;
> >  			cnt = set8->cnt;
> >  			size = sizeof(set8->pairs[0]);
> > +
> > +			/*
> > +			 * When ELF endianness does not match endianness of the
> > +			 * host, libelf will do the translation when updating
> > +			 * the ELF. This, however, corrupts SET8 flags which are
> > +			 * already in the target endianness. So, let's bswap
> > +			 * them to the host endianness and libelf will then
> > +			 * correctly translate everything.
> > +			 */
> > +			if (need_bswap) {
> > +				for (i = 0; i < cnt; i++) {
> > +					set8->pairs[i].flags =
> > +						bswap_32(set8->pairs[i].flags);
> > +				}
> 
> Do we need this for btf_id_set8:flags as well? Didn't get a chance to
> look too deeply yet.
> 
> Thanks,
> Daniel

I ran some test and tried and validated Jiri's patch in
https://lore.kernel.org/bpf/Zb6Jt30bNcNhM6zR@surya/

Manu

Re: [PATCH bpf-next v2 2/2] tools/resolve_btfids: fix cross-compilation to non-host endianness

[Jiri Olsa]

On Fri, Feb 02, 2024 at 06:38:18PM -0700, Daniel Xu wrote:
> Hi Viktor,
> 
> On Wed, Jan 31, 2024 at 05:24:09PM +0100, Viktor Malik wrote:
> > The .BTF_ids section is pre-filled with zeroed BTF ID entries during the
> > build and afterwards patched by resolve_btfids with correct values.
> > Since resolve_btfids always writes in host-native endianness, it relies
> > on libelf to do the translation when the target ELF is cross-compiled to
> > a different endianness (this was introduced in commit 61e8aeda9398
> > ("bpf: Fix libelf endian handling in resolv_btfids")).
> > 
> > Unfortunately, the translation will corrupt the flags fields of SET8
> > entries because these were written during vmlinux compilation and are in
> > the correct endianness already. This will lead to numerous selftests
> > failures such as:
> > 
> >     $ sudo ./test_verifier 502 502
> >     #502/p sleepable fentry accept FAIL
> >     Failed to load prog 'Invalid argument'!
> >     bpf_fentry_test1 is not sleepable
> >     verification time 34 usec
> >     stack depth 0
> >     processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
> >     Summary: 0 PASSED, 0 SKIPPED, 1 FAILED
> > 
> > Since it's not possible to instruct libelf to translate just certain
> > values, let's manually bswap the flags in resolve_btfids when needed, so
> > that libelf then translates everything correctly.
> > 
> > Fixes: ef2c6f370a63 ("tools/resolve_btfids: Add support for 8-byte BTF sets")
> > Signed-off-by: Viktor Malik <vmalik@redhat.com>
> > ---
> >  tools/bpf/resolve_btfids/main.c | 27 ++++++++++++++++++++++++++-
> >  1 file changed, 26 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> > index 7badf1557e5c..d01603ef6283 100644
> > --- a/tools/bpf/resolve_btfids/main.c
> > +++ b/tools/bpf/resolve_btfids/main.c
> > @@ -652,13 +652,23 @@ static int sets_patch(struct object *obj)
> >  	Elf_Data *data = obj->efile.idlist;
> >  	int *ptr = data->d_buf;
> >  	struct rb_node *next;
> > +	GElf_Ehdr ehdr;
> > +	int need_bswap;
> > +
> > +	if (gelf_getehdr(obj->efile.elf, &ehdr) == NULL) {
> > +		pr_err("FAILED cannot get ELF header: %s\n",
> > +			elf_errmsg(-1));
> > +		return -1;
> > +	}
> > +	need_bswap = (__BYTE_ORDER == __LITTLE_ENDIAN) !=
> > +		     (ehdr.e_ident[EI_DATA] == ELFDATA2LSB);
> >  
> >  	next = rb_first(&obj->sets);
> >  	while (next) {
> >  		unsigned long addr, idx;
> >  		struct btf_id *id;
> >  		void *base;
> > -		int cnt, size;
> > +		int cnt, size, i;
> >  
> >  		id   = rb_entry(next, struct btf_id, rb_node);
> >  		addr = id->addr[0];
> > @@ -686,6 +696,21 @@ static int sets_patch(struct object *obj)
> >  			base = set8->pairs;
> >  			cnt = set8->cnt;
> >  			size = sizeof(set8->pairs[0]);
> > +
> > +			/*
> > +			 * When ELF endianness does not match endianness of the
> > +			 * host, libelf will do the translation when updating
> > +			 * the ELF. This, however, corrupts SET8 flags which are
> > +			 * already in the target endianness. So, let's bswap
> > +			 * them to the host endianness and libelf will then
> > +			 * correctly translate everything.
> > +			 */
> > +			if (need_bswap) {
> > +				for (i = 0; i < cnt; i++) {
> > +					set8->pairs[i].flags =
> > +						bswap_32(set8->pairs[i].flags);
> > +				}
> 
> Do we need this for btf_id_set8:flags as well? Didn't get a chance to
> look too deeply yet.

ah did not this, right, looks like we need that

jirka

Re: [PATCH bpf-next v2 2/2] tools/resolve_btfids: fix cross-compilation to non-host endianness

[Viktor Malik]

On 2/3/24 19:48, Manu Bretelle wrote:
> On Fri, Feb 02, 2024 at 06:38:18PM -0700, Daniel Xu wrote:
>> Hi Viktor,
>>
>> On Wed, Jan 31, 2024 at 05:24:09PM +0100, Viktor Malik wrote:
>>> The .BTF_ids section is pre-filled with zeroed BTF ID entries during the
>>> build and afterwards patched by resolve_btfids with correct values.
>>> Since resolve_btfids always writes in host-native endianness, it relies
>>> on libelf to do the translation when the target ELF is cross-compiled to
>>> a different endianness (this was introduced in commit 61e8aeda9398
>>> ("bpf: Fix libelf endian handling in resolv_btfids")).
>>>
>>> Unfortunately, the translation will corrupt the flags fields of SET8
>>> entries because these were written during vmlinux compilation and are in
>>> the correct endianness already. This will lead to numerous selftests
>>> failures such as:
>>>
>>>     $ sudo ./test_verifier 502 502
>>>     #502/p sleepable fentry accept FAIL
>>>     Failed to load prog 'Invalid argument'!
>>>     bpf_fentry_test1 is not sleepable
>>>     verification time 34 usec
>>>     stack depth 0
>>>     processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
>>>     Summary: 0 PASSED, 0 SKIPPED, 1 FAILED
>>>
>>> Since it's not possible to instruct libelf to translate just certain
>>> values, let's manually bswap the flags in resolve_btfids when needed, so
>>> that libelf then translates everything correctly.
>>>
>>> Fixes: ef2c6f370a63 ("tools/resolve_btfids: Add support for 8-byte BTF sets")
>>> Signed-off-by: Viktor Malik <vmalik@redhat.com>
>>> ---
>>>  tools/bpf/resolve_btfids/main.c | 27 ++++++++++++++++++++++++++-
>>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
>>> index 7badf1557e5c..d01603ef6283 100644
>>> --- a/tools/bpf/resolve_btfids/main.c
>>> +++ b/tools/bpf/resolve_btfids/main.c
>>> @@ -652,13 +652,23 @@ static int sets_patch(struct object *obj)
>>>  	Elf_Data *data = obj->efile.idlist;
>>>  	int *ptr = data->d_buf;
>>>  	struct rb_node *next;
>>> +	GElf_Ehdr ehdr;
>>> +	int need_bswap;
>>> +
>>> +	if (gelf_getehdr(obj->efile.elf, &ehdr) == NULL) {
>>> +		pr_err("FAILED cannot get ELF header: %s\n",
>>> +			elf_errmsg(-1));
>>> +		return -1;
>>> +	}
>>> +	need_bswap = (__BYTE_ORDER == __LITTLE_ENDIAN) !=
>>> +		     (ehdr.e_ident[EI_DATA] == ELFDATA2LSB);
>>>  
>>>  	next = rb_first(&obj->sets);
>>>  	while (next) {
>>>  		unsigned long addr, idx;
>>>  		struct btf_id *id;
>>>  		void *base;
>>> -		int cnt, size;
>>> +		int cnt, size, i;
>>>  
>>>  		id   = rb_entry(next, struct btf_id, rb_node);
>>>  		addr = id->addr[0];
>>> @@ -686,6 +696,21 @@ static int sets_patch(struct object *obj)
>>>  			base = set8->pairs;
>>>  			cnt = set8->cnt;
>>>  			size = sizeof(set8->pairs[0]);
>>> +
>>> +			/*
>>> +			 * When ELF endianness does not match endianness of the
>>> +			 * host, libelf will do the translation when updating
>>> +			 * the ELF. This, however, corrupts SET8 flags which are
>>> +			 * already in the target endianness. So, let's bswap
>>> +			 * them to the host endianness and libelf will then
>>> +			 * correctly translate everything.
>>> +			 */
>>> +			if (need_bswap) {
>>> +				for (i = 0; i < cnt; i++) {
>>> +					set8->pairs[i].flags =
>>> +						bswap_32(set8->pairs[i].flags);
>>> +				}
>>
>> Do we need this for btf_id_set8:flags as well? Didn't get a chance to
>> look too deeply yet.
>>
>> Thanks,
>> Daniel
> 
> I ran some test and tried and validated Jiri's patch in
> https://lore.kernel.org/bpf/Zb6Jt30bNcNhM6zR@surya/

Right, I'll include that patch. Thanks for testing!

Viktor

> 
> Manu
> 

Re: [PATCH bpf-next v2 1/2] tools/resolve_btfids: Refactor set sorting with types from btf_ids.h

[Viktor Malik]

On 2/2/24 14:06, Jiri Olsa wrote:
> On Wed, Jan 31, 2024 at 05:24:08PM +0100, Viktor Malik wrote:
>> Instead of using magic offsets to access BTF ID set data, leverage types
>> from btf_ids.h (btf_id_set and btf_id_set8) which define the actual
>> layout of the data. Thanks to this change, set sorting should also
>> continue working if the layout changes.
>>
>> This requires to sync the definition of 'struct btf_id_set8' from
>> include/linux/btf_ids.h to tools/include/linux/btf_ids.h. We don't sync
>> the rest of the file at the moment, b/c that would require to also sync
>> multiple dependent headers and we don't need any other defs from
>> btf_ids.h.
>>
>> Signed-off-by: Viktor Malik <vmalik@redhat.com>
>> ---
>>  tools/bpf/resolve_btfids/main.c | 30 ++++++++++++++++++++++--------
>>  tools/include/linux/btf_ids.h   |  9 +++++++++
>>  2 files changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
>> index 27a23196d58e..7badf1557e5c 100644
>> --- a/tools/bpf/resolve_btfids/main.c
>> +++ b/tools/bpf/resolve_btfids/main.c
>> @@ -70,6 +70,7 @@
>>  #include <sys/stat.h>
>>  #include <fcntl.h>
>>  #include <errno.h>
>> +#include <linux/btf_ids.h>
>>  #include <linux/rbtree.h>
>>  #include <linux/zalloc.h>
>>  #include <linux/err.h>
>> @@ -78,7 +79,7 @@
>>  #include <subcmd/parse-options.h>
>>  
>>  #define BTF_IDS_SECTION	".BTF_ids"
>> -#define BTF_ID		"__BTF_ID__"
>> +#define BTF_ID_PREFIX	"__BTF_ID__"
> 
> nit does not look necessary to me

There's a conflicting definition of BTF_ID in btf_ids.h, this change is
to prevent a warning after we include the header.

> 
>>  
>>  #define BTF_STRUCT	"struct"
>>  #define BTF_UNION	"union"
>> @@ -161,7 +162,7 @@ static int eprintf(int level, int var, const char *fmt, ...)
>>  
>>  static bool is_btf_id(const char *name)
>>  {
>> -	return name && !strncmp(name, BTF_ID, sizeof(BTF_ID) - 1);
>> +	return name && !strncmp(name, BTF_ID_PREFIX, sizeof(BTF_ID_PREFIX) - 1);
>>  }
>>  
>>  static struct btf_id *btf_id__find(struct rb_root *root, const char *name)
>> @@ -441,7 +442,7 @@ static int symbols_collect(struct object *obj)
>>  		 * __BTF_ID__TYPE__vfs_truncate__0
>>  		 * prefix =  ^
>>  		 */
>> -		prefix = name + sizeof(BTF_ID) - 1;
>> +		prefix = name + sizeof(BTF_ID_PREFIX) - 1;
>>  
>>  		/* struct */
>>  		if (!strncmp(prefix, BTF_STRUCT, sizeof(BTF_STRUCT) - 1)) {
>> @@ -656,8 +657,8 @@ static int sets_patch(struct object *obj)
>>  	while (next) {
>>  		unsigned long addr, idx;
>>  		struct btf_id *id;
>> -		int *base;
>> -		int cnt;
>> +		void *base;
>> +		int cnt, size;
>>  
>>  		id   = rb_entry(next, struct btf_id, rb_node);
>>  		addr = id->addr[0];
>> @@ -671,13 +672,26 @@ static int sets_patch(struct object *obj)
>>  		}
>>  
>>  		idx = idx / sizeof(int);
>> -		base = &ptr[idx] + (id->is_set8 ? 2 : 1);
>> -		cnt = ptr[idx];
>> +		if (id->is_set) {
>> +			struct btf_id_set *set;
>> +
>> +			set = (struct btf_id_set *)&ptr[idx];
>> +			base = set->ids;
>> +			cnt = set->cnt;
>> +			size = sizeof(set->ids[0]);
>> +		} else {
>> +			struct btf_id_set8 *set8;
>> +
>> +			set8 = (struct btf_id_set8 *)&ptr[idx];
>> +			base = set8->pairs;
>> +			cnt = set8->cnt;
>> +			size = sizeof(set8->pairs[0]);
>> +		}
>>  
>>  		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
>>  			 (idx + 1) * sizeof(int), cnt, id->name);
>>  
>> -		qsort(base, cnt, id->is_set8 ? sizeof(uint64_t) : sizeof(int), cmp_id);
>> +		qsort(base, cnt, size, cmp_id);
> 
> maybe we could call qsort on top of each set type, seems simpler:

That looks much cleaner, I'll use that, thanks!

V.

> 
>  	while (next) {
> +		struct btf_id_set8 *set8;
> +		struct btf_id_set *set;
>  		unsigned long addr, idx;
>  		struct btf_id *id;
> -		int *base;
> -		int cnt;
>  
>  		id   = rb_entry(next, struct btf_id, rb_node);
>  		addr = id->addr[0];
> @@ -671,13 +672,16 @@ static int sets_patch(struct object *obj)
>  		}
>  
>  		idx = idx / sizeof(int);
> -		base = &ptr[idx] + (id->is_set8 ? 2 : 1);
> -		cnt = ptr[idx];
> +		if (id->is_set) {
> +			set = (struct btf_id_set *)&ptr[idx];
> +			qsort(set->ids, set->cnt, sizeof(set->ids[0]), cmp_id);
> +		} else {
> +			set8 = (struct btf_id_set8 *)&ptr[idx];
> +			qsort(set8->pairs, set8->cnt, sizeof(set8->pairs[0]), cmp_id);
> +		}
>  
>  		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
> -			 (idx + 1) * sizeof(int), cnt, id->name);
> -
> -		qsort(base, cnt, id->is_set8 ? sizeof(uint64_t) : sizeof(int), cmp_id);
> +			 (idx + 1) * sizeof(int), id->is_set ? set->cnt : set8->cnt, id->name);
>  
>  		next = rb_next(next);
>  	}
> 
> 
> jirka
>