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

add macro to control the bpf read method #27

Closed
wants to merge 1 commit into from

Conversation

zhangbo1882
Copy link

@zhangbo1882 zhangbo1882 commented Oct 22, 2021

#21

Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Please see comments.

#define PRINT_SKB_STR_SIZE 2048
#define PRINT_SKB_STR_SIZE 2048

extern u32 LINUX_KERNEL_VERSION __kconfig;
Copy link
Member

Choose a reason for hiding this comment

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

I think LINUX_KERNEL_VERSION is set only when loading with libbpf (we are using github.com/cilium/ebpf to load the prog).

We could ask / contribute to cilium/ebpf to add the support for it (should be relatively trivial, see https://github.com/libbpf/libbpf/blob/v0.5.0/src/libbpf.c#L6804). /cc @ti-mo @lmb

Copy link
Author

Choose a reason for hiding this comment

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

LINUX_KERNEL_VERSION is just to extract a running kernel version or a name that matches one of Kconfig’s keys. I do not think it has something with libbpf.

Copy link
Member

Choose a reason for hiding this comment

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

The version is set during the loadtime. Check https://github.com/libbpf/libbpf/blob/v0.5.0/src/libbpf.c#L6804 to see how it's set in libbpf. Currently, cilium/ebpf doesn't support it.

Also, you can check it by adding into the code:

#if (LINUX_KERNEL_VERSION >= KERNEL_VERSION(5,5,0))
	char fmt[] = "foo\n";
	bpf_trace_printk(foo, sizeof(foo));
#endif

and then

cat /sys/kernel/debug/tracing/trace_pipe

Copy link
Author

Choose a reason for hiding this comment

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

@brb thanks for you explanation. I got the point.

Copy link
Member

Choose a reason for hiding this comment

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

👍 Please let me know if you are going to work on adding LINUX_KERNEL_VERSION support to cilium/ebpf.

Copy link

Choose a reason for hiding this comment

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

Great, apparenly this depends on parsing /proc/config.gz: https://nakryiko.com/posts/bpf-core-reference-guide/#linux-kernel-version. I don't think cilium/ebpf has support for Kconfig externs yet.

We have kernel version detection, though: https://github.com/cilium/ebpf/blob/1eb0e981058a2d5335965c6dc450a6442ad0377e/internal/version.go#L90-L99. I would prefer to use this for LINUX_KERNEL_VERSION over parsing kconfig.gz in the mean time.

@lmb any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

apparenly this depends on parsing /proc/config.gz

It depends on config.gz for CONFIG_ parsing. While LINUX_KERNEL_VERSION - on the uname syscall.

Copy link

Choose a reason for hiding this comment

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

Okay, we're open to implementing the LINUX_KERNEL_VERSION extern using the existing kernel version detection we have in the lib. @zhangbo1882 would you be willing to contribute this as a feature?

Copy link
Author

Choose a reason for hiding this comment

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

@ti-mo , I am very pleased to do that. Let me check cilium/ebpf code first.

bpf/kprobe_pwru.c Outdated Show resolved Hide resolved
bpf/kprobe_pwru.c Show resolved Hide resolved
@brb
Copy link
Member

brb commented Apr 27, 2022

Closing due to inactivity.

@brb brb closed this Apr 27, 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.

3 participants