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

Routines with constprop are not accounted for when attaching #3258

Open
mjguzik opened this issue Jun 20, 2024 · 5 comments
Open

Routines with constprop are not accounted for when attaching #3258

mjguzik opened this issue Jun 20, 2024 · 5 comments
Labels
bug Something isn't working

Comments

@mjguzik
Copy link

mjguzik commented Jun 20, 2024

What reproduces the bug? Provide code if possible.

This arguably could be considered an ebpf bug.

As an example the linux kernel ships with "should_failslab" routine. The kernel image has 2 variants compiled in, one with stock name and another one with the constprop treatment.

Mere bpftrace -e 'kprobe:should_failslab { @[kstack()] = count(); } only attaches to the base variant, missing all calls on the .constprop.0 routine.

The expected behavior is that all emitted variants of the routine get attached to, extra points if only the base name is reported. Perhaps an additional switch could be used to not do that, then interested parties could attach to foo and foo.constprop.* and explicitly see the difference if they really wanted to.

bpftrace --info output

System
OS: Linux 6.10.0-rc4-next-20240619 #90 SMP PREEMPT_DYNAMIC Wed Jun 19 23:02:55 UTC 2024
Arch: x86_64

Build
version: v0.20.0-205-g81b6-dirty
LLVM: 14.0.6
unsafe probe: no
bfd: no
liblldb (DWARF support): no
libsystemd (systemd notify support): no

libbpf: failed to find valid kernel BTF
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
get_tai_ns: yes
get_func_ip: yes
jiffies64: yes
for_each_map_elem: yes

Kernel features
Instruction limit: 1000000
Loop support: yes
btf: no
module btf: no
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
ringbuf: yes

Probe types
kprobe: yes
tracepoint: yes
perf_event: yes
kfunc: no
kprobe_multi: no
uprobe_multi: no
raw_tp_special: yes
iter: no

@mjguzik mjguzik added the bug Something isn't working label Jun 20, 2024
@hramrach
Copy link

The clones exist because they do not implement the whole functionality and in particular the whole API (eg. may not take some arguments). If you want to attach a function make sure it is not cloned.

@mjguzik
Copy link
Author

mjguzik commented Jun 20, 2024

Note the code at hand does not inspect any arguments, merely wants to know who ends up calling the func, so it is equally applicable to all variants of generated routine.

Let's say it makes sense to not deal with the corner cases of regular vs constpropped' func and for consistency you don't want the above to automatically attach to all of them.

Even then bare minimum bpftrace should emit a warning that additional variants exist and are not covered. Otherwise one is left wondering why the probe does not fire.

@hramrach
Copy link

That, however, requires knowing that those variants exist, what their APIs are, and if the code you want to attach can be munged to work with those APIs.

So a big feature to implement. While somebody might be interested in working on this problem the practical solution is to mark the function with some compiler attributes that prevent inlining, cloning, and the like.

You could also request attaching inlined calls, or a warning that inlined sites exist.

@mjguzik
Copy link
Author

mjguzik commented Jun 20, 2024

This is only practical if one only uses bpftrace for development and not checking production systems.

I agree checking for all inlined and otherwise mucked cases would be quite a challenge.

I do claim finding out if .constprop.x variants exist is trivial and as bare minimum bpftrace could warn about their existence for the given routine (if one only attaches to the "base" variant).

@hramrach
Copy link

However, that will only increase confusion. If in some trivial cases bpftrace warns that there are places where the function was not attached because the code was transformed by a compiler optimization and in more complex cases it does not people will be even more confused when encountering the more complex cases.

I am not even sure there is any trace of the function left when it is inlined but no code is generated for it or all of it is eliminated. Which means that it's not clear such warning can be fully implemented.

As such people should only attach to designated attach points, or live with the fact that attaching a BPF function is best-effort and will sometimes work somewhere, sometimes fail, and sometimes appear to work but not change all uses.

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