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

link/kprobe: set the correct size to perf_event_attr #293

Merged
merged 1 commit into from
Apr 27, 2021

Conversation

mmat11
Copy link
Contributor

@mmat11 mmat11 commented Apr 22, 2021

Related to #292.

This commit sets the size attribute of perf_event_attr. If the size is left blank, a wrong version is created and certain fields will be ignored (example: config2). https://elixir.bootlin.com/linux/v5.11.15/source/include/uapi/linux/perf_event.h#L304

I've tested this by running the uretprobe example which now works properly since config2 is being set:

[pid 34603] perf_event_open({type=0x7 /* PERF_TYPE_??? */, size=PERF_ATTR_SIZE_VER5, config=0x1, sample_period=0, sample_type=0, read_format=0, disabled=0, inherit=0, pinned=0, exclusive=0, exclusive_user=0, exclude_kernel=0, exclude_hv=0, exclude_idle=0, mmap=0, comm=0, freq=0, inherit_stat=0, enable_on_exec=0, task=0, watermark=0, precise_ip=0 /* arbitrary skid */, mmap_data=0, sample_id_all=0, exclude_host=0, exclude_guest=0, exclude_callchain_kernel=0, exclude_callchain_user=0, mmap2=0, comm_exec=0, use_clockid=0, context_switch=0, write_backward=0, namespaces=0, wakeup_events=0, config1=0xc000241d60, config2=0xca780, sample_regs_user=0, sample_regs_intr=0, aux_watermark=0, sample_max_stack=0}, -1, 0, -1, PERF_FLAG_FD_CLOEXEC) = 3

I've also fixed the ret flag, which wasn't being set correctly.

Unfortunately I don't know a way to test this properly (I could make unit tests but it wouldn't work in this specific case) without running an ebpf program and checking that the function is being called

link/kprobe.go Outdated Show resolved Hide resolved
link/kprobe.go Outdated Show resolved Hide resolved
@mmat11 mmat11 force-pushed the fix-perfeventattr branch 2 times, most recently from 78ac2d8 to 1dcfe42 Compare April 22, 2021 15:44
@lmb
Copy link
Collaborator

lmb commented Apr 22, 2021

Unfortunately I don't know a way to test this properly (I could make unit tests but it wouldn't work in this specific case)

Can you explain why it wouldn't work in this case?

without running an ebpf program and checking that the function is being called

I think that is fine.

@mmat11
Copy link
Contributor Author

mmat11 commented Apr 22, 2021

Unfortunately I don't know a way to test this properly (I could make unit tests but it wouldn't work in this specific case)

Can you explain why it wouldn't work in this case?

I don't get any error from perf_event_open, regardless of the version of perf_event_attr and the parameters I am setting. Checking that the struct is correct (with the parameters I want), is not enough because the syscall will silently translate my request in a wrong way

Copy link
Collaborator

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mmat11, thanks for taking a look at this.

  • Please include a useful commit message
  • Refactoring this into a separate function obfuscates what the actual bug was. Could the code structure be kept as it was before?
  • Try to aim for minimal change sets, it makes reviews easier and regressions less likely

link/kprobe.go Outdated Show resolved Hide resolved
This commit sets the size attribute of perf_event_attr.
If the size is left blank, a struct version PERF_ATTR_SIZE_VER0 is created and
while this is ok for perf_kprobe, it is not ok for perf_uprobe since they require
an additional field config2 which got added in version PERF_ATTR_SIZE_VER1.

Additionally, this commit fixes the config attribute for retprobes.

Signed-off-by: Mattia Meleleo <melmat@tuta.io>
Signed-off-by: Timo Beckers <timo@isovalent.com>
@ti-mo
Copy link
Collaborator

ti-mo commented Apr 27, 2021

I've also looked into this today, nice catch on having to set size, that's a stinker. 🙁 I've pushed a couple of changes:

  • only set Size for uprobe PMU (kprobe doesn't use config2/Ext2)
  • Ext2 doesn't need to be a pointer, so I pass a bare offset. I see the same value going across the syscall with or without this being a pointer, not sure why that is.
  • VER1 is all that's required for uprobes, no need for VER5

Unfortunately, this doesn't fix the problem of attempting to trace /bin/bash:readline as reported in #292 .Will elaborate in that issue.

@ti-mo ti-mo merged commit 11e9d77 into cilium:master Apr 27, 2021
@mmat11
Copy link
Contributor Author

mmat11 commented Apr 27, 2021

thanks @ti-mo ; I noticed the pointer thing aswell and I thought I had removed it already.

@mmat11 mmat11 deleted the fix-perfeventattr branch November 1, 2022 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants