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

syscalls: omit AttachType field in haveProgAttach #652

Merged
merged 1 commit into from
May 3, 2022

Conversation

ti-mo
Copy link
Collaborator

@ti-mo ti-mo commented May 2, 2022

Discovered while running the test suite on kernel 4.14.

With the addition BPF_PROG_ATTACH tests in package link, the haveProgAttach feature test was added, gated on 4.10. This helper uses the AttachType field, which sets the bpf_attr expected_attach_type field, available as of 4.17.

There are more issues to address for 4.14, but this patch fixes these tests on kernels between 4.10 and 4.17.

cc @rgo3

@ti-mo ti-mo requested a review from lmb May 2, 2022 15:36
@rgo3
Copy link
Contributor

rgo3 commented May 2, 2022

Is there a way to trigger CI if it hasn't started automatically? Looking on semaphore and it doesn't seem like this got picked up.

@lmb
Copy link
Collaborator

lmb commented May 3, 2022

CI seems currently buggy, see the thread on slack.

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.

Do we really need the fallback behaviour? What if we change the feature test to omit AttachType instead? I'm a bit fuzzy what the difference is when AttachType is present vs. not present to be honest, so I might be missing the point.

prog.go Outdated Show resolved Hide resolved
syscalls.go Outdated Show resolved Hide resolved
Discovered while running the test suite on kernel 4.14.

With the addition BPF_PROG_ATTACH tests in package link, the haveProgAttach
feature test was added, gated on 4.10. This helper uses the AttachType field,
which sets the bpf_attr expected_attach_type field, which is only available
as of 4.17. As probing CGroupSKB is sufficient, omit setting AttachType.

This patch fixes the tests on kernels between 4.10 and 4.17.

Signed-off-by: Timo Beckers <timo@isovalent.com>
@ti-mo
Copy link
Collaborator Author

ti-mo commented May 3, 2022

What if we change the feature test to omit AttachType instead?

Turns out I had a .gitignored UAPI header in my linux repo that contained expected_attach_type, so it popped up in grep even after checking out v4.10.. 😞 This field wasn't added until 4.17, so this is just a bug in the test suite that we didn't notice because of the 4.9-4.19 gap.

And indeed, probing CGroupSKB is a representative proxy for BPF_PROG_ATTACH, since it was introduced in the same patch series as you commented in the code. No need for the AttachType field.

@ti-mo ti-mo changed the title prog,syscalls: probe expected_attach_type field in BPF_PROG_LOAD syscalls: omit AttachType field in haveProgAttach May 3, 2022
@ti-mo ti-mo merged commit 18d817b into cilium:master May 3, 2022
@ti-mo ti-mo deleted the tb/probe-attachtype branch May 3, 2022 09:48
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