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

bpf: inline test functions with ctx as input #24662

Merged
merged 1 commit into from Apr 3, 2023
Merged

Conversation

anfernee
Copy link
Contributor

@anfernee anfernee commented Mar 31, 2023

Run ebpf test on kernel v5.11 will raise the following error:

  Validating build_packet() func#1...
  btf_vmlinux is malformed
  Arg#0 type PTR in build_packet() is not supported yet.

It's due to missing kernel patch e5069b9c "bpf: Support pointers in global func args". Inline those functions so the ebpf test can also run on v5.11 and older kernels.

Please ensure your pull request adheres to the following guidelines:

  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes. n/a

@anfernee anfernee requested a review from a team as a code owner March 31, 2023 05:03
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 31, 2023
@anfernee anfernee added the release-note/misc This PR makes changes that have no direct user impact. label Mar 31, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 31, 2023
Copy link
Contributor

@gentoo-root gentoo-root left a comment

Choose a reason for hiding this comment

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

It's due to missing kernel patch e5069b9c "bpf: Support pointers in global func args".

This patch shouldn't be needed. As its commit message says, this patch adds support for pointer types different from PTR_TO_CTX, and PTR_TO_CTX used to be supported before. The pointer in scope of your commit is PTR_TO_CTX.

I believe the real root cause of the error you see is btf_vmlinux is malformed. When it happens, the verifier can't determine the type of the context pointer and treats struct __ctx_buff *ctx as PTR instead of PTR_TO_CTX.

Regarding btf_vmlinux is malformed, I'm not entirely sure why it happens, but a brief search hints that it might be because something was wrong with pahole or gcc during kernel compilation.

@anfernee
Copy link
Contributor Author

Thanks for the quick reply!

It's due to missing kernel patch e5069b9c "bpf: Support pointers in global func args".

This patch shouldn't be needed. As its commit message says, this patch adds support for pointer types different from PTR_TO_CTX, and PTR_TO_CTX used to be supported before. The pointer in scope of your commit is PTR_TO_CTX.

I believe the real root cause of the error you see is btf_vmlinux is malformed. When it happens, the verifier can't determine the type of the context pointer and treats struct __ctx_buff *ctx as PTR instead of PTR_TO_CTX.

I see! This makes sense.

Regarding btf_vmlinux is malformed, I'm not entirely sure why it happens, but a brief search hints that it might be because something was wrong with pahole or gcc during kernel compilation.

I am also not sure why it happens, but inline the functions do fix the problem and make the test runnable on those kernels. Fixing Ubuntu kernel seem to the right approach, but considering it's not going to happen anytime soon. Do you think this patch can be accepted?

@pchaigno pchaigno added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/ci This PR makes changes to the CI. and removed release-note/misc This PR makes changes that have no direct user impact. labels Mar 31, 2023
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

I think it's an acceptable change, but the rationale needs to be updated in the commit description as per Maxim's comment. Other than that, LGTM.

cc @dylandreimerink

Run ebpf test on kernel v5.11 in Ubuntu 20.04 will raise the following
error:

```
  Validating build_packet() func#1...
  btf_vmlinux is malformed
  Arg#0 type PTR in build_packet() is not supported yet.
```

Due to some unknown reason, when running on these kernels, verifier
cannot correctly recognize the function argument is PTR_TO_CTX, instead
of PTR. PTR as global function's argument is only supported after
e5069b9c.

This patch inlines those test functions in question, which makes it
possible to run ebpf unit test on kernel prior to v5.11.

Signed-off-by: Yongkun Gui <ygui@google.com>
@anfernee
Copy link
Contributor Author

anfernee commented Apr 1, 2023

Thanks @pchaigno Re-worded the commit message.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 3, 2023
@dylandreimerink
Copy link
Member

I think it's an acceptable change

Yes, agreed. Making these function always inline should also improve support for older kernels without BPF-to-BPF support (not that we are currently running the suite on those versions).

@squeed squeed merged commit 2f0275e into cilium:master Apr 3, 2023
42 checks passed
@anfernee
Copy link
Contributor Author

anfernee commented Apr 3, 2023

Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants