On Wed, 16 Jun 2021 at 09:38, Jiri Olsa jolsa@redhat.com wrote:
On Wed, Jun 16, 2021 at 08:56:42AM -0700, Yonghong Song wrote:
On 6/16/21 2:25 AM, Tony Ambardar wrote:
While patching the .BTF_ids section in vmlinux, resolve_btfids writes type ids using host-native endianness, and relies on libelf for any required translation when finally updating vmlinux. However, the default type of the .BTF_ids section content is ELF_T_BYTE (i.e. unsigned char), and undergoes no translation. This results in incorrect patched values if cross-compiling to non-native endianness, and can manifest as kernel Oops and test failures which are difficult to debug.
nice catch, great libelf can do that ;-)
Funny, I'd actually assumed that was your intention, but I just couldn't find where the data type was being set, so resorted to this "kludge". While there's a .BTF_ids section definition in include/linux/btf_ids.h, there's no means I can see to specify the data type either (i.e. in the gcc asm .pushsection() options). That approach would be cleaner.
Explicitly set the type of patched data to ELF_T_WORD, allowing libelf to transparently handle the endian conversions.
Fixes: fbbb68de80a4 ("bpf: Add resolve_btfids tool to resolve BTF IDs in ELF object") Cc: stable@vger.kernel.org # v5.10+ Cc: Jiri Olsa jolsa@kernel.org Cc: Yonghong Song yhs@fb.com Link: https://lore.kernel.org/bpf/CAPGftE_eY-Zdi3wBcgDfkz_iOr1KF10n=9mJHm1_a_Pykcs... Signed-off-by: Tony Ambardar Tony.Ambardar@gmail.com
tools/bpf/resolve_btfids/main.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c index d636643ddd35..f32c059fbfb4 100644 --- a/tools/bpf/resolve_btfids/main.c +++ b/tools/bpf/resolve_btfids/main.c @@ -649,6 +649,9 @@ 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;
The change makes sense to me as .BTF_ids contains just a list of u32's.
Jiri, could you double check on this?
the comment in ELF_T_WORD declaration suggests the size depends on elf's class?
ELF_T_WORD, /* Elf32_Word, Elf64_Word, ... */
data in .BTF_ids section are allways u32
I believe the Elf32/Elf64 refer to the arch since some data structures vary between the two, but ELF_T_WORD is common to both, and valid as the data type of Elf_Data struct holding the .BTF_ids contents. See elf(5):
Basic types The following types are used for N-bit architectures (N=32,64, ElfN stands for Elf32 or Elf64, uintN_t stands for uint32_t or uint64_t): ... ElfN_Word uint32_t
Also see the code and comments in "elf.h": /* Types for signed and unsigned 32-bit quantities. */ typedef uint32_t Elf32_Word; typedef uint32_t Elf64_Word;
I have no idea how is this handled in libelf (perhaps it's ok), but just that comment above suggests it could be also 64 bits, cc-ing Frank and Mark for more insight
One other area I'd like to confirm is with section compression. Is it safe to ignore this for .BTF_ids? I've done so because include/linux/btf_ids.h appears to define the section with SHF_ALLOC flag set, which is incompatible with compression based on "libelf.h" comments.
Thanks for reviewing, Tony
thanks, jirka
- elf_flagdata(obj->efile.idlist, ELF_C_SET, ELF_F_DIRTY); err = elf_update(obj->efile.elf, ELF_C_WRITE);