-
Notifications
You must be signed in to change notification settings - Fork 185
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
pkg/helpers: Fix logic to detect if kernel supports bpf_ktime_get_boo… #2083
Conversation
8e8d047
to
fa1bb9a
Compare
Why not to loop on the enum values looking for the one corresponding to this function:
It's slower but I think the code is easier to follow and we don't have to rely on the last one being |
OK, but I used the above solution to profit from the array and |
fa1bb9a
to
f26afae
Compare
pkg/gadgets/helpers.go
Outdated
return | ||
} | ||
|
||
bpfKtimeGetBootNsExists = len(enum.Values) >= BpfKtimeGetBootNsFuncID | ||
for _, value := range enum.Values { | ||
if value.Value == BpfKtimeGetBootNsFuncID { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to check the string value, otherwise we'll have an issue similar to the one before. For instance, this check will fail on this specific commit https://github.com/torvalds/linux/blob/0a05861f80fe7d4dcfdabcc98d9854947573e072/include/uapi/linux/bpf.h#L3154, sk_assign
is 124 and __BPF_FUNC_MAX_ID
is 125, so this logic will indicate the helper is available when it's not.
If we want to avoid looping through the array, we can take into consideration that the array is built as:
[0] -> unspec
[1...N-2]: helpers (N-2 being the last helper)
[N-1]: FUNC_MAX_ID
To check if the helper exists we can do bpfKtimeGetBootNsExists = len(enum.Values) >= BpfKtimeGetBootNsFuncID + 2
But TBH I think a simple linear search is a lot easier to understand (and to get it right).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to check the string value, otherwise we'll have an issue similar to the one before. For instance, this check will fail on this specific commit https://github.com/torvalds/linux/blob/0a05861f80fe7d4dcfdabcc98d9854947573e072/include/uapi/linux/bpf.h#L3154, sk_assign is 124 and __BPF_FUNC_MAX_ID is 125, so this logic will indicate the helper is available when it's not.
Indeed, but I think we can only check the name and ID is not really useful here.
If we want to avoid looping through the array, we can take into consideration that the array is built as:
[0] -> unspec
[1...N-2]: helpers (N-2 being the last helper)
[N-1]: FUNC_MAX_ID
Or we can do what I did at first in this PR.
To check if the helper exists we can do bpfKtimeGetBootNsExists = len(enum.Values) >= BpfKtimeGetBootNsFuncID + 2
I really would like avoid this magic + 2
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can only check the name and ID is not really useful here.
Right, I added that for two (not really important) reasons:
- To speed up a bit the check
- I remembered somebody mentioned that a kernel got a backporting wrong and the ID of a helper was messed up.
Just to be sure we're on the same page, the function is called once in the process life as it uses bpfKtimeGetBootNsOnce. |
…t_ns(). The kernel defines many BPF helpers functions and all of them have an ID [1]. Sadly, we cannot compare the number of helper functions with the ID to know if the kernel supports the given function. Indeed, the kernel defines two abstract helper functions: * unspec which has ID 0 [2] * and __BPF_FUNC_MAX_ID which indicates the last ID [3]. The solution is to search bpf_ktime_get_boot_ns() within all the provided helpers. Fixes: f874869 ("Rewrite detectBpfKtimeGetBootNs using sync.Once") Fixes: ffb7d99 ("gadgets: get timestamp from userspace on Linux <5.7") Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com> [1]: https://github.com/torvalds/linux/blob/0e945134b680040b8613e962f586d91b6d40292d/include/uapi/linux/bpf.h#L5646 [2]: https://github.com/torvalds/linux/blob/0e945134b680040b8613e962f586d91b6d40292d/include/uapi/linux/bpf.h#L5647 [3]: https://github.com/torvalds/linux/blob/0e945134b680040b8613e962f586d91b6d40292d/include/uapi/linux/bpf.h#L5873
f26afae
to
b3fad6c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the fix!
Thank you for the review! |
…t_ns().
The kernel defines many BPF helpers functions and all of them have an ID [1]. Sadly, we cannot compare the number of helper functions with the ID to know if the kernel supports the given function.
Indeed, the kernel defines two abstract helper functions:
The solution is then to test that latest helper is __BPF_FUNC_MAX_ID and that bpf_ktime_get_boot_ns() ID is lower.
Fixes: f874869 ("Rewrite detectBpfKtimeGetBootNs using sync.Once")
Fixes: ffb7d99 ("gadgets: get timestamp from userspace on Linux <5.7")
[1]: https://github.com/torvalds/linux/blob/0e945134b680040b8613e962f586d91b6d40292d/include/uapi/linux/bpf.h#L5646
[2]: https://github.com/torvalds/linux/blob/0e945134b680040b8613e962f586d91b6d40292d/include/uapi/linux/bpf.h#L5647
[3]: https://github.com/torvalds/linux/blob/0e945134b680040b8613e962f586d91b6d40292d/include/uapi/linux/bpf.h#L5873