On Wed, Jun 16, 2021 at 03:09:13PM -0700, Tony Ambardar wrote:

On Wed, 16 Jun 2021 at 09:38, Jiri Olsa [off-list ref] wrote:

quoted

On Wed, Jun 16, 2021 at 08:56:42AM -0700, Yonghong Song wrote:

quoted

On 6/16/21 2:25 AM, Tony Ambardar wrote:

quoted

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.

quoted

quoted

quoted

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 <redacted>
Link: https://lore.kernel.org/bpf/CAPGftE_eY-Zdi3wBcgDfkz_iOr1KF10n=9mJHm1_a_PykcsoeA@mail.gmail.com/ (local)
Signed-off-by: Tony Ambardar <redacted>
---
  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;

ok

quoted

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.

not sure what you mean.. where it wouldn't be safe?
what workflow/processing

thanks,
jirka