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: specify symbol offset #618

Merged
merged 2 commits into from Apr 12, 2022
Merged

link/kprobe: specify symbol offset #618

merged 2 commits into from Apr 12, 2022

Conversation

xpu22
Copy link
Contributor

@xpu22 xpu22 commented Apr 2, 2022

This patch adds kprobe symbol offset and its unit test.
With symbol offset, we can insert kprobe for functions
inlined.

[PR closed https://github.com//pull/613]

Signed-off-by: Tonghao Zhang zhangtonghao@didiglobal.com

This patch adds kprobe symbol offset and its unit test.
With symbol offset, we can insert kprobes for functions
inlined.

Signed-off-by: Tonghao Zhang <zhangtonghao@didiglobal.com>
Copy link
Collaborator

@lmb lmb left a comment

Choose a reason for hiding this comment

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

I'm assuming that the kprobe only fires if the PC is exactly that of sym + offset. Is there a way to add a test where Offset != 0 to make sure this actually works all the way?

@@ -235,8 +240,12 @@ func pmuProbe(typ probeType, args probeArgs) (*perfEvent, error) {
}

attr = unix.PerfEventAttr{
// The minimum size required for PMU kprobes is PERF_ATTR_SIZE_VER1,
// since it added the config2 (Ext2) field. Use Ext2 as probe_offset.
Size: unix.PERF_ATTR_SIZE_VER1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to https://classes.engineering.wustl.edu/cse522/man-pages/perf_event_open.2.pdf VER1 is available from 2.6 so this doesn't change the minimum required version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we don't set .size , the kernel will use the PERF_ATTR_SIZE_VER0 64B as default. The config2 will be not included in perf_event_attr.

the kernel code:
static int perf_copy_attr(struct perf_event_attr __user *uattr, struct perf_event_attr attr)
.....
/
ABI compatibility quirk: */
if (!size)
size = PERF_ATTR_SIZE_VER0; // config2 is not in VER0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Offset == 0 is ok, it is an option for linux kernel.
https://www.kernel.org/doc/html/latest/trace/kprobetrace.html

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I should've been clearer, using ver1 LGTM.

@lmb
Copy link
Collaborator

lmb commented Apr 5, 2022

I'm happy with this change, but would like a test. Maybe we can have a negative test where we get an error if we pass math.MaxUint32 or similar if using a valid offset is too complicated.

@xpu22
Copy link
Contributor Author

xpu22 commented Apr 6, 2022

@lmb Hi
This patch add the kprobeToken unit test only. Offset is different for different kernel function. Offset is set by user who should know what they are doing. For math.MaxUint32, the kprobe(symbol tcp_recvmsg) does't work, because the vmlinux .text section + offset < .text section + math.MaxUint32, then the kernel return err. I think we should allow the user to set Offset they want. If Offset is not suitable, the kernel will return err value.
https://elixir.bootlin.com/linux/v5.13.3/source/kernel/kprobes.c#L1557

@mmat11 @lmb any thoughts ? this pr should be merged to master ?

@mmat11
Copy link
Collaborator

mmat11 commented Apr 9, 2022

@nickcooper-zhangtonghao as @lmb suggested you could test this either with a negative test that will fail when using a bad offset or with a normal test (if you already know the offset for tcp_recvmsg and it's more or less stable from kernel to kernel, you could use that)

Signed-off-by: Tonghao Zhang <zhangtonghao@didiglobal.com>
@xpu22
Copy link
Contributor Author

xpu22 commented Apr 12, 2022

@nickcooper-zhangtonghao as @lmb suggested you could test this either with a negative test that will fail when using a bad offset or with a normal test (if you already know the offset for tcp_recvmsg and it's more or less stable from kernel to kernel, you could use that)

HI @mmat11
I add kprobe offset test. I use the ksym(vprintk) as symbol, because it present on all tested kernels.
the offset is 0/math.MaxUint64, because use them as offset to insert kprobe, we can get return value we expected

@lmb lmb merged commit cf27494 into cilium:master Apr 12, 2022
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.

None yet

3 participants