Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Symbolication is broken when running a 32-bit userspace (and bpftrace binary) on a 64-bit kernel #2854

Closed
jeez opened this issue Nov 28, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@jeez
Copy link
Contributor

jeez commented Nov 28, 2023

Symbolication is broken on platforms running a 64-bit kernel but 32-bit userspace. For example, this is a simple test case that can reproduce the issue: $ bpftrace -e 'profile:hz:99 { @[kstack] = count(); }'

I believe this is happening because bpftrace requests uint64_t addresses to the kernel here https://github.com/iovisor/bpftrace/blob/fb77dae0b0a3d16b9453264401ff778556028576/src/bpftrace.cpp#L1941 but then performs an implicitly conversion to uintptr_t here https://github.com/iovisor/bpftrace/blob/fb77dae0b0a3d16b9453264401ff778556028576/src/bpftrace.cpp#L2076 .

Since bpftrace is a 32-bit binary in this case, then uintptr_t is only 32-bit wide and the higher part of the 64-bit address provided by the kernel is dropped / ignored. This leads to a failure when bcc_symcache_resolve() is called. The implicit conversion should have been flagged by the compiler, but -Wconversion is currently not used by bpftrace: https://github.com/iovisor/bpftrace/blob/79c366d3d3eb93da18b1e75fbe150b0537626ca8/CMakeLists.txt#L70 .

The issue can be fixed if we change resolve_ksym() so it receives an uint64_t addr instead, but I wanted to check here first if there is an alternate preferred solution in case I'm missing something. I've only tested and observed this with kernel symbols so far, but I see uintptr_t being used in other locations throughout the project so this might be an issue for a few other use cases too.

Let me know what you think, please.

bpftrace --info output

$ ./bpftrace --info
System
  OS: Linux 5.15.119-android13
  Arch: armv8l

Build
  version: v0.18.1
  LLVM: 14.0.6
  unsafe probe: yes
  bfd: no
  libdw (DWARF support): no

Kernel helpers
  probe_read: yes
  probe_read_str: yes
  probe_read_user: yes
  probe_read_user_str: yes
  probe_read_kernel: yes
  probe_read_kernel_str: yes
  get_current_cgroup_id: yes
  send_signal: yes
  override_return: no
  get_boot_ns: yes
  dpath: no
  skboutput: no

Kernel features
  Instruction limit: 1000000
  Loop support: yes
  btf: yes
  module btf: yes
  map batch: yes
  uprobe refcount (depends on Build:bcc bpf_attach_uprobe refcount): yes

Map types
  hash: yes
  percpu hash: yes
  array: yes
  percpu array: yes
  stack_trace: yes
  perf_event_array: yes

Probe types
  kprobe: yes
  tracepoint: yes
  perf_event: yes
  kfunc: no
  iter:task: no
  iter:task_file: no
  iter:task_vma: no
  kprobe_multi: no
  raw_tp_special: yes
@jeez jeez added the bug Something isn't working label Nov 28, 2023
@danobi
Copy link
Member

danobi commented Dec 2, 2023

A u64 for the addr seems reasonable. Did making that change fix the issue?

@jeez
Copy link
Contributor Author

jeez commented Dec 5, 2023

A u64 for the addr seems reasonable. Did making that change fix the issue?

Yes it does, as I mentioned above. I will send out a PR then. thanks

jeez added a commit to jeez/bpftrace that referenced this issue Dec 5, 2023
Symbolication is broken on platforms running a 64-bit kernel and a
32-bit userspace. For example, this simple test case can reproduce the
issue: $ bpftrace -e 'profile:hz:99 { @[kstack] = count(); }' .

This is happening because bpftrace requests uint64_t addresses to the
kernel here but then performs an implicit conversion to uintptr_t here
when resolving ksyms, usyms and uids. Fix the issue by changing the
signature of these functions so they receive a uint64_t address instead.

Ideally we should uncomment add_compile_options("-Wconversion") in
bpftrace/CMakeLists.txt to avoid running into simple issues like this in
the future. This patch is one small step towards fixing the current
conversion warnings we have.

[Issue 2854](bpftrace#2854)

Signed-off-by: Jesus Sanchez-Palencia <jesussanp@google.com>
danobi pushed a commit that referenced this issue Dec 6, 2023
Symbolication is broken on platforms running a 64-bit kernel and a
32-bit userspace. For example, this simple test case can reproduce the
issue: $ bpftrace -e 'profile:hz:99 { @[kstack] = count(); }' .

This is happening because bpftrace requests uint64_t addresses to the
kernel here but then performs an implicit conversion to uintptr_t here
when resolving ksyms, usyms and uids. Fix the issue by changing the
signature of these functions so they receive a uint64_t address instead.

Ideally we should uncomment add_compile_options("-Wconversion") in
bpftrace/CMakeLists.txt to avoid running into simple issues like this in
the future. This patch is one small step towards fixing the current
conversion warnings we have.

[Issue 2854](#2854)

Signed-off-by: Jesus Sanchez-Palencia <jesussanp@google.com>
@jeez jeez closed this as completed Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants