On Thu, Nov 21, 2024 at 11:02:18PM -0800, Eduard Zingerman wrote:
> btf_encoder__tag_kfuncs() reads .BTF_ids section to identify a set of
> kfuncs present in the ELF being processed. This section consists of
> records of the following shape:
>
> struct btf_id_and_flag {
> uint32_t id;
> uint32_t flags;
> };
it contains pairs like above and also just id arrays with no flags, but
that does not matter for the patch functionality, because you swap by
u32 values anyway
>
> When endianness of binary operated by pahole differs from the
> host endianness these fields require byte swap before using.
>
> At the moment such byte swap does not happen and kfuncs are not marked
> with decl tags when e.g. s390 kernel is compiled on x86.
> To reproduces the bug:
> - follow instructions from [0] to build an s390 vmlinux;
> - execute:
> pahole --btf_features_strict=decl_tag_kfuncs,decl_tag \
> --btf_encode_detached=test.btf vmlinux
> - observe no kfuncs generated:
> bpftool btf dump test.btf format c | grep __ksym
>
> This commit fixes the issue by adding an endianness conversion step
> for .BTF_ids section data before main processing step, modifying the
> Elf_Data object in-place.
> The choice is such in order to:
> - minimize changes;
> - keep using Elf_Data, as it provides fields {d_size,d_off} used
> by kfunc processing routines;
> - avoid sprinkling bswap_32 at each 'struct btf_id_and_flag' field
> access in fear of forgetting to add new ones when code is modified.
lgtm, some questions below
>
> [0] https://docs.kernel.org/bpf/s390.html
>
> Cc: Alan Maguire <alan.maguire@xxxxxxxxxx>
> Cc: Daniel Xu <dxu@xxxxxxxxx>
> Cc: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> Cc: Vadim Fedorenko <vadfed@xxxxxxxx>
> Fixes: 72e88f29c6f7 ("pahole: Inject kfunc decl tags into BTF")
> Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> ---
> btf_encoder.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> lib/bpf | 2 +-
> 2 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index e1adddf..3bdb73b 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -33,6 +33,7 @@
> #include <stdint.h>
> #include <search.h> /* for tsearch(), tfind() and tdestroy() */
> #include <pthread.h>
> +#include <byteswap.h>
>
> #define BTF_IDS_SECTION ".BTF_ids"
> #define BTF_ID_FUNC_PFX "__BTF_ID__func__"
> @@ -1847,11 +1848,47 @@ static int btf_encoder__tag_kfunc(struct btf_encoder *encoder, struct gobuffer *
> return 0;
> }
>
> +/* If byte order of 'elf' differs from current byte order, convert the data->d_buf.
> + * ELF file is opened in a readonly mode, so data->d_buf cannot be modified in place.
> + * Instead, allocate a new buffer if modification is necessary.
> + */
> +static int convert_idlist_endianness(Elf *elf, Elf_Data *data, bool *copied)
> +{
> + int byteorder, i;
> + char *elf_ident;
> + uint32_t *tmp;
> +
> + *copied = false;
> + elf_ident = elf_getident(elf, NULL);
> + if (elf_ident == NULL) {
> + fprintf(stderr, "Cannot get ELF identification from header\n");
> + return -EINVAL;
> + }
> + byteorder = elf_ident[EI_DATA];
> + if ((BYTE_ORDER == LITTLE_ENDIAN && byteorder == ELFDATA2LSB)
> + || (BYTE_ORDER == BIG_ENDIAN && byteorder == ELFDATA2MSB))
> + return 0;
> + tmp = malloc(data->d_size);
> + if (tmp == NULL) {
> + fprintf(stderr, "Cannot allocate %lu bytes of memory\n", data->d_size);
> + return -ENOMEM;
> + }
> + memcpy(tmp, data->d_buf, data->d_size);
> + data->d_buf = tmp;
will the original data->d_buf be leaked? are we allowed to assign d_buf like that? ;-)
> + *copied = true;
> +
> + /* .BTF_ids sections consist of u32 objects */
> + for (i = 0; i < data->d_size / sizeof(uint32_t); i++)
> + tmp[i] = bswap_32(tmp[i]);
> + return 0;
> +}
> +
> static int btf_encoder__tag_kfuncs(struct btf_encoder *encoder)
> {
> const char *filename = encoder->source_filename;
> struct gobuffer btf_kfunc_ranges = {};
> struct gobuffer btf_funcs = {};
> + bool free_idlist = false;
> Elf_Data *symbols = NULL;
> Elf_Data *idlist = NULL;
> Elf_Scn *symscn = NULL;
> @@ -1919,6 +1956,9 @@ static int btf_encoder__tag_kfuncs(struct btf_encoder *encoder)
> idlist_shndx = i;
> idlist_addr = shdr.sh_addr;
> idlist = data;
> + err = convert_idlist_endianness(elf, idlist, &free_idlist);
> + if (err < 0)
> + goto out;
> }
> }
>
> @@ -2031,6 +2071,8 @@ static int btf_encoder__tag_kfuncs(struct btf_encoder *encoder)
> out:
> __gobuffer__delete(&btf_funcs);
> __gobuffer__delete(&btf_kfunc_ranges);
> + if (free_idlist)
> + free(idlist->d_buf);
> if (elf)
> elf_end(elf);
curious, would elf_end try to free the d_buf at some point?
> if (fd != -1)
> diff --git a/lib/bpf b/lib/bpf
> index 09b9e83..caa17bd 160000
> --- a/lib/bpf
> +++ b/lib/bpf
> @@ -1 +1 @@
> -Subproject commit 09b9e83102eb8ab9e540d36b4559c55f3bcdb95d
> +Subproject commit caa17bdcbfc58e68eaf4d017c058e6577606bf56
I think this should not be part of the patch
thanks,
jirka
