Re: [PATCH dwarves v3 1/1] btf_encoder: handle .BTF_ids section endianness

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 27/11/2024 11:00, Jiri Olsa wrote:
> On Tue, Nov 26, 2024 at 05:50:06PM -0800, Eduard Zingerman wrote:
>> btf_encoder__tag_kfuncs() reads .BTF_ids section to identify a set of
>> kfuncs present in the ELF file being processed.
>> This section consists of:
>> - arrays of uint32_t elements;
>> - arrays of records with the following structure:
>>    struct btf_id_and_flag {
>>        uint32_t id;
>>        uint32_t flags;
>>    };
>>
>> When endianness of a binary operated by pahole differs from the host
>> system's endianness, these fields require byte-swapping before use.
>> Currently, this byte-swapping does not occur, resulting in kfuncs not
>> being marked with declaration tags.
>>
>> This commit resolves the issue by using elf_getdata_rawchunk()
>> function to read .BTF_ids section data. When called with ELF_T_WORD as
>> 'type' parameter it does necessary byte order conversion
>> (only if host and elf endianness do not match).
>>
>> Cc: Alan Maguire <alan.maguire@oracle.com>
>> Cc: Andrii Nakryiko <andrii@kernel.org>
>> Cc: Daniel Xu <dxu@dxuuu.xyz>
>> Cc: Jiri Olsa <olsajiri@gmail.com>
>> Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>> Cc: Vadim Fedorenko <vadfed@meta.com>
>> Fixes: 72e88f29c6f7 ("pahole: Inject kfunc decl tags into BTF")
>> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
>> ---
>>   btf_encoder.c | 26 ++++++++++++++++++++------
>>   1 file changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/btf_encoder.c b/btf_encoder.c
>> index e1adddf..3754884 100644
>> --- a/btf_encoder.c
>> +++ b/btf_encoder.c
>> @@ -1904,18 +1904,32 @@ static int btf_encoder__tag_kfuncs(struct btf_encoder *encoder)
>>   			goto out;
>>   		}
>>   
>> -		data = elf_getdata(scn, 0);
>> -		if (!data) {
>> -			elf_error("Failed to get ELF section(%d) data", i);
>> -			goto out;
>> -		}
>> -
>>   		if (shdr.sh_type == SHT_SYMTAB) {
>> +			data = elf_getdata(scn, 0);
>> +			if (!data) {
>> +				elf_error("Failed to get ELF section(%d) data", i);
>> +				goto out;
>> +			}
>> +
>>   			symbols_shndx = i;
>>   			symscn = scn;
>>   			symbols = data;
>>   			strtabidx = shdr.sh_link;
>>   		} else if (!strcmp(secname, BTF_IDS_SECTION)) {
>> +			/* .BTF_ids section consists of uint32_t elements,
>> +			 * and thus might need byte order conversion.
>> +			 * However, it has type PROGBITS, hence elf_getdata()
>> +			 * won't automatically do the conversion.
>> +			 * Use elf_getdata_rawchunk() instead,
>> +			 * ELF_T_WORD tells it to do the necessary conversion.
>> +			 */
>> +			data = elf_getdata_rawchunk(elf, shdr.sh_offset, shdr.sh_size, ELF_T_WORD);
> 
> looks good, I'm just curious about one thing..
> 
> so ELF_T_WORD enum has this comment: /* Elf32_Word, Elf64_Word, ... */
> 
> I did just quick check, ***so I might be easily wrong***, but I wonder the
> code in __elf_xfctstom (which I assume is the one called for conversion)
> chooses to swap 32/64 bits values based on elf->class .. so for 64bit ELF
> class we swap 64bit values? ... while .BTF_ids has always 32 bit values

Well according to the doc:

        ELF_T_WORD     Unsigned 32-bit words.
        ELF_T_XWORD    Unsigned 64-bit words.

It shouldn't use 64 bits swap:

const xfct_t __elf_xfctstom[EV_NUM - 1][EV_NUM - 1][ELFCLASSNUM - 
1][ELF_T_NUM] =
....
	[ELF_T_WORD]	= ElfW2(Bits, cvt_Word),			
	[ELF_T_XWORD]	= ElfW2(Bits, cvt_Xword),			
...

Are you looking somewhere else?

> 
> thanks,
> jirka
> 
>> +			if (!data) {
>> +				elf_error("Failed to get %s ELF section(%d) data",
>> +					  BTF_IDS_SECTION, i);
>> +				goto out;
>> +			}
>> +
>>   			idlist_shndx = i;
>>   			idlist_addr = shdr.sh_addr;
>>   			idlist = data;
>> -- 
>> 2.47.0
>>