[PATCH bpf-next v1 0/3] Improve .BTF_ids patching and alignment

[PATCH bpf-next v1 0/3] Improve .BTF_ids patching and alignment

[Tony Ambardar]

Hello all,

This patch series offers improvements to the way .BTF_ids section data is
created and later patched by resolve_btfids.

Patch #1 simplifies the byte-order translation in resolve_btfids while
making it more resilient to future .BTF_ids encoding updates.

Patch #2 makes sure all BTF ID data is 4-byte aligned, and not only the
.BTF_ids used for vmlinux.

Patch #3 syncs the above changes in btf_ids.h to tools/include, obviating
a previous alignment fix in selftests/bpf.

Feedback and suggestions are welcome!

Best regards,
Tony


Tony Ambardar (3):
  tools/resolve_btfids: Simplify handling cross-endian compilation
  bpf: btf: Ensure natural alignment of .BTF_ids section
  tools/bpf, selftests/bpf : Sync btf_ids.h to tools

 include/linux/btf_ids.h                       |  1 +
 tools/bpf/resolve_btfids/main.c               | 60 +++++---------
 tools/include/linux/btf_ids.h                 | 80 +++++++++++++++++--
 .../selftests/bpf/prog_tests/resolve_btfids.c |  6 --
 4 files changed, 97 insertions(+), 50 deletions(-)

-- 
2.34.1

[PATCH bpf-next v1 1/3] tools/resolve_btfids: Simplify handling cross-endian compilation

[Tony Ambardar]

Initially, the .BTF_ids section was created zero-filled and then patched
with BTF IDs by resolve_btfids on the build host. Patching was done in
native endianness and thus failed to work for cross-endian compile targets.
This was fixed in [1] by using libelf-based translation to output patched
data in target byte order.

The addition of 8-byte BTF sets in [2] lead to .BTF_ids creation with both
target-endian values and zero-filled data to be later patched. This again
broke cross-endian compilation as the already-correct target-endian values
were translated on output by libelf [1]. The problem was worked around [3]
by manually converting BTF SET8 values to native endianness, so that final
libelf output translation yields data in target byte order.

Simplify and make the code more robust against future changes like [2] by
employing libelf-based endian translation on both input and output, which
is typical of libelf usage.

[1]: 61e8aeda9398 ("bpf: Fix libelf endian handling in resolv_btfids")
[2]: ef2c6f370a63 ("tools/resolve_btfids: Add support for 8-byte BTF sets")
[3]: 903fad439466 ("tools/resolve_btfids: Fix cross-compilation to non-host endianness")

CC: Viktor Malik <vmalik@redhat.com>
Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 tools/bpf/resolve_btfids/main.c | 60 ++++++++++++---------------------
 1 file changed, 22 insertions(+), 38 deletions(-)

diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
index d54aaa0619df..9f1ab23ed014 100644
--- a/tools/bpf/resolve_btfids/main.c
+++ b/tools/bpf/resolve_btfids/main.c
@@ -90,14 +90,6 @@
 
 #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;
@@ -125,7 +117,6 @@ struct object {
 		int		 idlist_shndx;
 		size_t		 strtabidx;
 		unsigned long	 idlist_addr;
-		int		 encoding;
 	} efile;
 
 	struct rb_root	sets;
@@ -325,11 +316,30 @@ static int compressed_section_fix(Elf *elf, Elf_Scn *scn, GElf_Shdr *sh)
 	return 0;
 }
 
+static int btfids_endian_fix(struct object *obj)
+{
+	Elf_Data *btfids = obj->efile.idlist;
+	Elf *elf = obj->efile.elf;
+	int file_byteorder;
+
+	/* This should always succeed due to prior ELF checks */
+	file_byteorder = elf_getident(elf, NULL)[EI_DATA];
+
+	/* Set type to ensure endian translation occurs, and manually invoke
+	 * translation on input since .BTF_ids section as created disables it.
+	 */
+	btfids->d_type = ELF_T_WORD;
+	if (gelf_xlatetom(elf, btfids, btfids, file_byteorder) == NULL) {
+		pr_err("FAILED xlatetom .BTF_ids data: %s\n", elf_errmsg(-1));
+		return -1;
+	}
+	return 0;
+}
+
 static int elf_collect(struct object *obj)
 {
 	Elf_Scn *scn = NULL;
 	size_t shdrstrndx;
-	GElf_Ehdr ehdr;
 	int idx = 0;
 	Elf *elf;
 	int fd;
@@ -361,13 +371,6 @@ 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.
@@ -409,6 +412,8 @@ static int elf_collect(struct object *obj)
 			obj->efile.idlist       = data;
 			obj->efile.idlist_shndx = idx;
 			obj->efile.idlist_addr  = sh.sh_addr;
+			if (btfids_endian_fix(obj))
+				return -1;
 		} else if (!strcmp(name, BTF_BASE_ELF_SEC)) {
 			/* If a .BTF.base section is found, do not resolve
 			 * BTF ids relative to vmlinux; resolve relative
@@ -706,24 +711,6 @@ static int sets_patch(struct object *obj)
 			 */
 			BUILD_BUG_ON((u32 *)set8->pairs != &set8->pairs[0].id);
 			qsort(set8->pairs, set8->cnt, sizeof(set8->pairs[0]), cmp_id);
-
-			/*
-			 * 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 (obj->efile.encoding != ELFDATANATIVE) {
-				int i;
-
-				set8->flags = bswap_32(set8->flags);
-				for (i = 0; i < set8->cnt; i++) {
-					set8->pairs[i].flags =
-						bswap_32(set8->pairs[i].flags);
-				}
-			}
 		}
 
 		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
@@ -748,9 +735,6 @@ static int symbols_patch(struct object *obj)
 	if (sets_patch(obj))
 		return -1;
 
-	/* Set type to ensure endian translation occurs. */
-	obj->efile.idlist->d_type = ELF_T_WORD;
-
 	elf_flagdata(obj->efile.idlist, ELF_C_SET, ELF_F_DIRTY);
 
 	err = elf_update(obj->efile.elf, ELF_C_WRITE);
-- 
2.34.1

Re: [PATCH bpf-next v1 1/3] tools/resolve_btfids: Simplify handling cross-endian compilation

[Eduard Zingerman]

On Fri, 2024-09-20 at 00:49 -0700, Tony Ambardar wrote:
> Initially, the .BTF_ids section was created zero-filled and then patched
> with BTF IDs by resolve_btfids on the build host. Patching was done in
> native endianness and thus failed to work for cross-endian compile targets.
> This was fixed in [1] by using libelf-based translation to output patched
> data in target byte order.
> 
> The addition of 8-byte BTF sets in [2] lead to .BTF_ids creation with both
> target-endian values and zero-filled data to be later patched. This again
> broke cross-endian compilation as the already-correct target-endian values
> were translated on output by libelf [1]. The problem was worked around [3]
> by manually converting BTF SET8 values to native endianness, so that final
> libelf output translation yields data in target byte order.
> 
> Simplify and make the code more robust against future changes like [2] by
> employing libelf-based endian translation on both input and output, which
> is typical of libelf usage.
> 
> [1]: 61e8aeda9398 ("bpf: Fix libelf endian handling in resolv_btfids")
> [2]: ef2c6f370a63 ("tools/resolve_btfids: Add support for 8-byte BTF sets")
> [3]: 903fad439466 ("tools/resolve_btfids: Fix cross-compilation to non-host endianness")
> 
> CC: Viktor Malik <vmalik@redhat.com>
> Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> ---

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

[...]

Re: [PATCH bpf-next v1 1/3] tools/resolve_btfids: Simplify handling cross-endian compilation

[Jiri Olsa]

On Fri, Sep 20, 2024 at 12:49:11AM -0700, Tony Ambardar wrote:

SNIP

> +static int btfids_endian_fix(struct object *obj)
> +{
> +	Elf_Data *btfids = obj->efile.idlist;
> +	Elf *elf = obj->efile.elf;
> +	int file_byteorder;
> +
> +	/* This should always succeed due to prior ELF checks */
> +	file_byteorder = elf_getident(elf, NULL)[EI_DATA];
> +
> +	/* Set type to ensure endian translation occurs, and manually invoke
> +	 * translation on input since .BTF_ids section as created disables it.
> +	 */
> +	btfids->d_type = ELF_T_WORD;
> +	if (gelf_xlatetom(elf, btfids, btfids, file_byteorder) == NULL) {
> +		pr_err("FAILED xlatetom .BTF_ids data: %s\n", elf_errmsg(-1));
> +		return -1;
> +	}
> +	return 0;
> +}
> +
>  static int elf_collect(struct object *obj)
>  {
>  	Elf_Scn *scn = NULL;
>  	size_t shdrstrndx;
> -	GElf_Ehdr ehdr;
>  	int idx = 0;
>  	Elf *elf;
>  	int fd;
> @@ -361,13 +371,6 @@ 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.
> @@ -409,6 +412,8 @@ static int elf_collect(struct object *obj)
>  			obj->efile.idlist       = data;
>  			obj->efile.idlist_shndx = idx;
>  			obj->efile.idlist_addr  = sh.sh_addr;
> +			if (btfids_endian_fix(obj))
> +				return -1;

nit, it'd be bit more clear to me without using the btfids_endian_fix
function and just unwind it in here.. but anyway lgtm

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

>  		} else if (!strcmp(name, BTF_BASE_ELF_SEC)) {
>  			/* If a .BTF.base section is found, do not resolve
>  			 * BTF ids relative to vmlinux; resolve relative
> @@ -706,24 +711,6 @@ static int sets_patch(struct object *obj)
>  			 */
>  			BUILD_BUG_ON((u32 *)set8->pairs != &set8->pairs[0].id);
>  			qsort(set8->pairs, set8->cnt, sizeof(set8->pairs[0]), cmp_id);
> -
> -			/*
> -			 * 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 (obj->efile.encoding != ELFDATANATIVE) {
> -				int i;
> -
> -				set8->flags = bswap_32(set8->flags);
> -				for (i = 0; i < set8->cnt; i++) {
> -					set8->pairs[i].flags =
> -						bswap_32(set8->pairs[i].flags);
> -				}
> -			}
>  		}
>  
>  		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
> @@ -748,9 +735,6 @@ static int symbols_patch(struct object *obj)
>  	if (sets_patch(obj))
>  		return -1;
>  
> -	/* Set type to ensure endian translation occurs. */
> -	obj->efile.idlist->d_type = ELF_T_WORD;
> -
>  	elf_flagdata(obj->efile.idlist, ELF_C_SET, ELF_F_DIRTY);
>  
>  	err = elf_update(obj->efile.elf, ELF_C_WRITE);
> -- 
> 2.34.1
> 

Re: [PATCH bpf-next v1 1/3] tools/resolve_btfids: Simplify handling cross-endian compilation

[Viktor Malik]

On 9/20/24 09:49, Tony Ambardar wrote:
> Initially, the .BTF_ids section was created zero-filled and then patched
> with BTF IDs by resolve_btfids on the build host. Patching was done in
> native endianness and thus failed to work for cross-endian compile targets.
> This was fixed in [1] by using libelf-based translation to output patched
> data in target byte order.
> 
> The addition of 8-byte BTF sets in [2] lead to .BTF_ids creation with both
> target-endian values and zero-filled data to be later patched. This again
> broke cross-endian compilation as the already-correct target-endian values
> were translated on output by libelf [1]. The problem was worked around [3]
> by manually converting BTF SET8 values to native endianness, so that final
> libelf output translation yields data in target byte order.
> 
> Simplify and make the code more robust against future changes like [2] by
> employing libelf-based endian translation on both input and output, which
> is typical of libelf usage.
> 
> [1]: 61e8aeda9398 ("bpf: Fix libelf endian handling in resolv_btfids")
> [2]: ef2c6f370a63 ("tools/resolve_btfids: Add support for 8-byte BTF sets")
> [3]: 903fad439466 ("tools/resolve_btfids: Fix cross-compilation to non-host endianness")
> 
> CC: Viktor Malik <vmalik@redhat.com>
> Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> ---

Acked-by: Viktor Malik <vmalik@redhat.com>

>  tools/bpf/resolve_btfids/main.c | 60 ++++++++++++---------------------
>  1 file changed, 22 insertions(+), 38 deletions(-)
> 
> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> index d54aaa0619df..9f1ab23ed014 100644
> --- a/tools/bpf/resolve_btfids/main.c
> +++ b/tools/bpf/resolve_btfids/main.c
> @@ -90,14 +90,6 @@
>  
>  #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;
> @@ -125,7 +117,6 @@ struct object {
>  		int		 idlist_shndx;
>  		size_t		 strtabidx;
>  		unsigned long	 idlist_addr;
> -		int		 encoding;
>  	} efile;
>  
>  	struct rb_root	sets;
> @@ -325,11 +316,30 @@ static int compressed_section_fix(Elf *elf, Elf_Scn *scn, GElf_Shdr *sh)
>  	return 0;
>  }
>  
> +static int btfids_endian_fix(struct object *obj)
> +{
> +	Elf_Data *btfids = obj->efile.idlist;
> +	Elf *elf = obj->efile.elf;
> +	int file_byteorder;
> +
> +	/* This should always succeed due to prior ELF checks */
> +	file_byteorder = elf_getident(elf, NULL)[EI_DATA];
> +
> +	/* Set type to ensure endian translation occurs, and manually invoke
> +	 * translation on input since .BTF_ids section as created disables it.
> +	 */
> +	btfids->d_type = ELF_T_WORD;
> +	if (gelf_xlatetom(elf, btfids, btfids, file_byteorder) == NULL) {
> +		pr_err("FAILED xlatetom .BTF_ids data: %s\n", elf_errmsg(-1));
> +		return -1;
> +	}
> +	return 0;
> +}
> +
>  static int elf_collect(struct object *obj)
>  {
>  	Elf_Scn *scn = NULL;
>  	size_t shdrstrndx;
> -	GElf_Ehdr ehdr;
>  	int idx = 0;
>  	Elf *elf;
>  	int fd;
> @@ -361,13 +371,6 @@ 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.
> @@ -409,6 +412,8 @@ static int elf_collect(struct object *obj)
>  			obj->efile.idlist       = data;
>  			obj->efile.idlist_shndx = idx;
>  			obj->efile.idlist_addr  = sh.sh_addr;
> +			if (btfids_endian_fix(obj))
> +				return -1;
>  		} else if (!strcmp(name, BTF_BASE_ELF_SEC)) {
>  			/* If a .BTF.base section is found, do not resolve
>  			 * BTF ids relative to vmlinux; resolve relative
> @@ -706,24 +711,6 @@ static int sets_patch(struct object *obj)
>  			 */
>  			BUILD_BUG_ON((u32 *)set8->pairs != &set8->pairs[0].id);
>  			qsort(set8->pairs, set8->cnt, sizeof(set8->pairs[0]), cmp_id);
> -
> -			/*
> -			 * 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 (obj->efile.encoding != ELFDATANATIVE) {
> -				int i;
> -
> -				set8->flags = bswap_32(set8->flags);
> -				for (i = 0; i < set8->cnt; i++) {
> -					set8->pairs[i].flags =
> -						bswap_32(set8->pairs[i].flags);
> -				}
> -			}
>  		}
>  
>  		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
> @@ -748,9 +735,6 @@ static int symbols_patch(struct object *obj)
>  	if (sets_patch(obj))
>  		return -1;
>  
> -	/* Set type to ensure endian translation occurs. */
> -	obj->efile.idlist->d_type = ELF_T_WORD;
> -
>  	elf_flagdata(obj->efile.idlist, ELF_C_SET, ELF_F_DIRTY);
>  
>  	err = elf_update(obj->efile.elf, ELF_C_WRITE);

[PATCH bpf-next v1 2/3] bpf: btf: Ensure natural alignment of .BTF_ids section

[Tony Ambardar]

While building of vmlinux employs a linker script to align the .BTF_ids
section to 4 bytes, other usage leaves .BTF_ids unaligned and may lead to
problems (e.g. [1]). Post-processing and libelf-based endian translation by
resolve_btfids may also potentially suffer from misalignment.

Update encoding macros in btf_ids.h to always align BTF ID data to 4 bytes.

[1]: 3effc06a4dde ("selftests/bpf: Fix alignment of .BTF_ids")

Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 include/linux/btf_ids.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
index c0e3e1426a82..c10b163dc340 100644
--- a/include/linux/btf_ids.h
+++ b/include/linux/btf_ids.h
@@ -89,6 +89,7 @@ word							\
 #define __BTF_ID_LIST(name, scope)			\
 asm(							\
 ".pushsection " BTF_IDS_SECTION ",\"a\";       \n"	\
+".balign 4, 0;                                 \n"	\
 "." #scope " " #name ";                        \n"	\
 #name ":;                                      \n"	\
 ".popsection;                                  \n");
-- 
2.34.1

Re: [PATCH bpf-next v1 2/3] bpf: btf: Ensure natural alignment of .BTF_ids section

[Eduard Zingerman]

On Fri, 2024-09-20 at 00:49 -0700, Tony Ambardar wrote:
> While building of vmlinux employs a linker script to align the .BTF_ids
> section to 4 bytes, other usage leaves .BTF_ids unaligned and may lead to
> problems (e.g. [1]). Post-processing and libelf-based endian translation by
> resolve_btfids may also potentially suffer from misalignment.
> 
> Update encoding macros in btf_ids.h to always align BTF ID data to 4 bytes.
> 
> [1]: 3effc06a4dde ("selftests/bpf: Fix alignment of .BTF_ids")
> 
> Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> ---
>  include/linux/btf_ids.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> index c0e3e1426a82..c10b163dc340 100644
> --- a/include/linux/btf_ids.h
> +++ b/include/linux/btf_ids.h
> @@ -89,6 +89,7 @@ word							\
>  #define __BTF_ID_LIST(name, scope)			\
>  asm(							\
>  ".pushsection " BTF_IDS_SECTION ",\"a\";       \n"	\
> +".balign 4, 0;                                 \n"	\
>  "." #scope " " #name ";                        \n"	\
>  #name ":;                                      \n"	\
>  ".popsection;                                  \n");

This forces all id list symbols to be aligned on 4 bytes.
Should the same be done for __BTF_SET_START?

Also, is it guaranteed that all btf ids are organized in lists and sets?
Grepping through the code it seems they are, but it looks like resolve_btfids
does not really enforce this, simply looking for symbols matching a special name
__BTF_ID__<type>__<symbol>[__<id>] .

Re: [PATCH bpf-next v1 2/3] bpf: btf: Ensure natural alignment of .BTF_ids section

[Jiri Olsa]

On Sat, Sep 21, 2024 at 02:46:17AM -0700, Eduard Zingerman wrote:
> On Fri, 2024-09-20 at 00:49 -0700, Tony Ambardar wrote:
> > While building of vmlinux employs a linker script to align the .BTF_ids
> > section to 4 bytes, other usage leaves .BTF_ids unaligned and may lead to
> > problems (e.g. [1]). Post-processing and libelf-based endian translation by
> > resolve_btfids may also potentially suffer from misalignment.
> > 
> > Update encoding macros in btf_ids.h to always align BTF ID data to 4 bytes.
> > 
> > [1]: 3effc06a4dde ("selftests/bpf: Fix alignment of .BTF_ids")
> > 
> > Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> > ---
> >  include/linux/btf_ids.h | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> > index c0e3e1426a82..c10b163dc340 100644
> > --- a/include/linux/btf_ids.h
> > +++ b/include/linux/btf_ids.h
> > @@ -89,6 +89,7 @@ word							\
> >  #define __BTF_ID_LIST(name, scope)			\
> >  asm(							\
> >  ".pushsection " BTF_IDS_SECTION ",\"a\";       \n"	\
> > +".balign 4, 0;                                 \n"	\
> >  "." #scope " " #name ";                        \n"	\
> >  #name ":;                                      \n"	\
> >  ".popsection;                                  \n");
> 
> This forces all id list symbols to be aligned on 4 bytes.
> Should the same be done for __BTF_SET_START?

it seems all the set macros use __BTF_ID_LIST, so it should be taken
care of by that

> 
> Also, is it guaranteed that all btf ids are organized in lists and sets?
> Grepping through the code it seems they are, but it looks like resolve_btfids
> does not really enforce this, simply looking for symbols matching a special name
> __BTF_ID__<type>__<symbol>[__<id>] .

yes, you need the BTF_ID to be part of list or set to be able to access it

resolve_btfids does not enforce some loose BTF_ID definition without list/set,
but that does not seem to be a problem

thanks,
jirka

Re: [PATCH bpf-next v1 2/3] bpf: btf: Ensure natural alignment of .BTF_ids section

[Eduard Zingerman]

On Mon, 2024-09-23 at 12:54 +0200, Jiri Olsa wrote:

[...]

> > > diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> > > index c0e3e1426a82..c10b163dc340 100644
> > > --- a/include/linux/btf_ids.h
> > > +++ b/include/linux/btf_ids.h
> > > @@ -89,6 +89,7 @@ word							\
> > >  #define __BTF_ID_LIST(name, scope)			\
> > >  asm(							\
> > >  ".pushsection " BTF_IDS_SECTION ",\"a\";       \n"	\
> > > +".balign 4, 0;                                 \n"	\
> > >  "." #scope " " #name ";                        \n"	\
> > >  #name ":;                                      \n"	\
> > >  ".popsection;                                  \n");
> > 
> > This forces all id list symbols to be aligned on 4 bytes.
> > Should the same be done for __BTF_SET_START?
> 
> it seems all the set macros use __BTF_ID_LIST, so it should be taken
> care of by that

Apologies, I don't know how I missed __BTF_ID_LIST invocation in
the __BTF_SET_START definition :(

> > Also, is it guaranteed that all btf ids are organized in lists and sets?
> > Grepping through the code it seems they are, but it looks like resolve_btfids
> > does not really enforce this, simply looking for symbols matching a special name
> > __BTF_ID__<type>__<symbol>[__<id>] .
> 
> yes, you need the BTF_ID to be part of list or set to be able to access it
> 
> resolve_btfids does not enforce some loose BTF_ID definition without list/set,
> but that does not seem to be a problem

Understood, thank you.

[PATCH bpf-next v1 3/3] tools/bpf, selftests/bpf : Sync btf_ids.h to tools

[Tony Ambardar]

Update to include latest changes, including BTF_SET8 support and correct
alignment of .BTF_ids sections, and remove the now-redundant alignment fix
introduced in 3effc06a4dde ("selftests/bpf: Fix alignment of .BTF_ids").

CC: Jean-Philippe Brucker <jean-philippe@linaro.org>
Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
---
 tools/include/linux/btf_ids.h                 | 80 +++++++++++++++++--
 .../selftests/bpf/prog_tests/resolve_btfids.c |  6 --
 2 files changed, 74 insertions(+), 12 deletions(-)

diff --git a/tools/include/linux/btf_ids.h b/tools/include/linux/btf_ids.h
index 72ea363d434d..3ba37d00e3ae 100644
--- a/tools/include/linux/btf_ids.h
+++ b/tools/include/linux/btf_ids.h
@@ -10,6 +10,9 @@ struct btf_id_set {
 	u32 ids[];
 };
 
+/* This flag implies BTF_SET8 holds kfunc(s) */
+#define BTF_SET8_KFUNCS		(1 << 0)
+
 struct btf_id_set8 {
 	u32 cnt;
 	u32 flags;
@@ -21,7 +24,8 @@ struct btf_id_set8 {
 
 #ifdef CONFIG_DEBUG_INFO_BTF
 
-#include <linux/compiler.h> /* for __PASTE */
+#include <linux/compiler.h> /* for __PASTE and __maybe_unused */
+#include <linux/stringify.h>
 
 /*
  * Following macros help to define lists of BTF IDs placed
@@ -35,7 +39,7 @@ struct btf_id_set8 {
 
 #define BTF_IDS_SECTION ".BTF_ids"
 
-#define ____BTF_ID(symbol)				\
+#define ____BTF_ID(symbol, word)			\
 asm(							\
 ".pushsection " BTF_IDS_SECTION ",\"a\";       \n"	\
 ".local " #symbol " ;                          \n"	\
@@ -43,10 +47,11 @@ asm(							\
 ".size  " #symbol ", 4;                        \n"	\
 #symbol ":                                     \n"	\
 ".zero 4                                       \n"	\
+word							\
 ".popsection;                                  \n");
 
-#define __BTF_ID(symbol) \
-	____BTF_ID(symbol)
+#define __BTF_ID(symbol, word) \
+	____BTF_ID(symbol, word)
 
 #define __ID(prefix) \
 	__PASTE(__PASTE(prefix, __COUNTER__), __LINE__)
@@ -56,7 +61,14 @@ asm(							\
  * to 4 zero bytes.
  */
 #define BTF_ID(prefix, name) \
-	__BTF_ID(__ID(__BTF_ID__##prefix##__##name##__))
+	__BTF_ID(__ID(__BTF_ID__##prefix##__##name##__), "")
+
+#define ____BTF_ID_FLAGS(prefix, name, flags) \
+	__BTF_ID(__ID(__BTF_ID__##prefix##__##name##__), ".long " #flags "\n")
+#define __BTF_ID_FLAGS(prefix, name, flags, ...) \
+	____BTF_ID_FLAGS(prefix, name, flags)
+#define BTF_ID_FLAGS(prefix, name, ...) \
+	__BTF_ID_FLAGS(prefix, name, ##__VA_ARGS__, 0)
 
 /*
  * The BTF_ID_LIST macro defines pure (unsorted) list
@@ -76,6 +88,7 @@ asm(							\
 #define __BTF_ID_LIST(name, scope)			\
 asm(							\
 ".pushsection " BTF_IDS_SECTION ",\"a\";       \n"	\
+".balign 4, 0;                                 \n"	\
 "." #scope " " #name ";                        \n"	\
 #name ":;                                      \n"	\
 ".popsection;                                  \n");
@@ -155,10 +168,58 @@ asm(							\
 ".popsection;                                 \n");	\
 extern struct btf_id_set name;
 
+/*
+ * The BTF_SET8_START/END macros pair defines sorted list of
+ * BTF IDs and their flags plus its members count, with the
+ * following layout:
+ *
+ * BTF_SET8_START(list)
+ * BTF_ID_FLAGS(type1, name1, flags)
+ * BTF_ID_FLAGS(type2, name2, flags)
+ * BTF_SET8_END(list)
+ *
+ * __BTF_ID__set8__list:
+ * .zero 8
+ * list:
+ * __BTF_ID__type1__name1__3:
+ * .zero 4
+ * .word (1 << 0) | (1 << 2)
+ * __BTF_ID__type2__name2__5:
+ * .zero 4
+ * .word (1 << 3) | (1 << 1) | (1 << 2)
+ *
+ */
+#define __BTF_SET8_START(name, scope, flags)		\
+__BTF_ID_LIST(name, local)				\
+asm(							\
+".pushsection " BTF_IDS_SECTION ",\"a\";       \n"	\
+"." #scope " __BTF_ID__set8__" #name ";        \n"	\
+"__BTF_ID__set8__" #name ":;                   \n"	\
+".zero 4                                       \n"	\
+".long " __stringify(flags)                   "\n"	\
+".popsection;                                  \n");
+
+#define BTF_SET8_START(name)				\
+__BTF_SET8_START(name, local, 0)
+
+#define BTF_SET8_END(name)				\
+asm(							\
+".pushsection " BTF_IDS_SECTION ",\"a\";      \n"	\
+".size __BTF_ID__set8__" #name ", .-" #name "  \n"	\
+".popsection;                                 \n");	\
+extern struct btf_id_set8 name;
+
+#define BTF_KFUNCS_START(name)				\
+__BTF_SET8_START(name, local, BTF_SET8_KFUNCS)
+
+#define BTF_KFUNCS_END(name)				\
+BTF_SET8_END(name)
+
 #else
 
-#define BTF_ID_LIST(name) static u32 __maybe_unused name[5];
+#define BTF_ID_LIST(name) static u32 __maybe_unused name[64];
 #define BTF_ID(prefix, name)
+#define BTF_ID_FLAGS(prefix, name, ...)
 #define BTF_ID_UNUSED
 #define BTF_ID_LIST_GLOBAL(name, n) u32 __maybe_unused name[n];
 #define BTF_ID_LIST_SINGLE(name, prefix, typename) static u32 __maybe_unused name[1];
@@ -166,6 +227,10 @@ extern struct btf_id_set name;
 #define BTF_SET_START(name) static struct btf_id_set __maybe_unused name = { 0 };
 #define BTF_SET_START_GLOBAL(name) static struct btf_id_set __maybe_unused name = { 0 };
 #define BTF_SET_END(name)
+#define BTF_SET8_START(name) static struct btf_id_set8 __maybe_unused name = { 0 };
+#define BTF_SET8_END(name)
+#define BTF_KFUNCS_START(name) static struct btf_id_set8 __maybe_unused name = { .flags = BTF_SET8_KFUNCS };
+#define BTF_KFUNCS_END(name)
 
 #endif /* CONFIG_DEBUG_INFO_BTF */
 
@@ -215,5 +280,8 @@ MAX_BTF_TRACING_TYPE,
 };
 
 extern u32 btf_tracing_ids[];
+extern u32 bpf_cgroup_btf_id[];
+extern u32 bpf_local_storage_map_btf_id[];
+extern u32 btf_bpf_map_id[];
 
 #endif
diff --git a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
index 51544372f52e..c3c021e3783e 100644
--- a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
+++ b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
@@ -28,12 +28,6 @@ struct symbol test_symbols[] = {
 	{ "func",    BTF_KIND_FUNC,    -1 },
 };
 
-/* Align the .BTF_ids section to 4 bytes */
-asm (
-".pushsection " BTF_IDS_SECTION " ,\"a\"; \n"
-".balign 4, 0;                            \n"
-".popsection;                             \n");
-
 BTF_ID_LIST(test_list_local)
 BTF_ID_UNUSED
 BTF_ID(typedef, S)
-- 
2.34.1

Re: [PATCH bpf-next v1 0/3] Improve .BTF_ids patching and alignment

[Jiri Olsa]

On Fri, Sep 20, 2024 at 12:49:10AM -0700, Tony Ambardar wrote:
> Hello all,
> 
> This patch series offers improvements to the way .BTF_ids section data is
> created and later patched by resolve_btfids.
> 
> Patch #1 simplifies the byte-order translation in resolve_btfids while
> making it more resilient to future .BTF_ids encoding updates.
> 
> Patch #2 makes sure all BTF ID data is 4-byte aligned, and not only the
> .BTF_ids used for vmlinux.
> 
> Patch #3 syncs the above changes in btf_ids.h to tools/include, obviating
> a previous alignment fix in selftests/bpf.
> 
> Feedback and suggestions are welcome!
> 
> Best regards,
> Tony

nice, thanks for the fix

Jean-Philippe, Viktor,
any chance you could check/test this patchset? it changes your recent
cross building fixes

thanks,
jirka

> 
> 
> Tony Ambardar (3):
>   tools/resolve_btfids: Simplify handling cross-endian compilation
>   bpf: btf: Ensure natural alignment of .BTF_ids section
>   tools/bpf, selftests/bpf : Sync btf_ids.h to tools
> 
>  include/linux/btf_ids.h                       |  1 +
>  tools/bpf/resolve_btfids/main.c               | 60 +++++---------
>  tools/include/linux/btf_ids.h                 | 80 +++++++++++++++++--
>  .../selftests/bpf/prog_tests/resolve_btfids.c |  6 --
>  4 files changed, 97 insertions(+), 50 deletions(-)
> 
> -- 
> 2.34.1
>