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

update cilium/ebpf to fix haveBpfProgReplace() check #3009

Merged
merged 1 commit into from
Jun 11, 2021

Conversation

AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Jun 8, 2021

The errors.Is(err, unix.EINVAL) check in haveBpfProgReplace() was broken because the cilium/ebpf library did not "wrap" errors: /~https://github.com/cilium/ebpf/blob/v0.6.0/link/program.go#L72

So the eBPF support of runc was broken for kernel prior to 5.6.

The fix for the above is cilium/ebpf#320. This PR bumps cilium/ebpf to include the fix.

Fixes: #3008


Tested in Moby CI moby/moby#42450

@AkihiroSuda AkihiroSuda added this to the 1.0.0 milestone Jun 8, 2021
@AkihiroSuda AkihiroSuda marked this pull request as ready for review June 9, 2021 09:06
@AkihiroSuda AkihiroSuda requested review from cyphar and kolyshkin June 9, 2021 09:07
cyphar
cyphar previously approved these changes Jun 9, 2021
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

LGTM, hopefully this works into the future. I can implement the fallback in a more ugly way (assuming the failure is EINVAL and retry without BPF_F_REPLACE to see if it works). But let's keep that ace in our back pocket for now...

@kolyshkin
Copy link
Contributor

(updated description to link to cilium/ebpf#320)

The only two minor issues I see are:

  1. Dependency on an untagged version of cilium/ebpf.
  2. Lack of TODO comment to use cilium/ebpf feature probe mechanism (once it is available).

I guess this is minor and we really should release 1.0.0 GA now, so LGTM.

kolyshkin
kolyshkin previously approved these changes Jun 9, 2021
@kolyshkin
Copy link
Contributor

Filed an issue to cilium asking them to tag a release cilium/ebpf#323

@ti-mo
Copy link

ti-mo commented Jun 11, 2021

We've tagged /~https://github.com/cilium/ebpf/releases/tag/v0.6.1.

@dims
Copy link
Contributor

dims commented Jun 11, 2021

thanks @ti-mo !

@cyphar
Copy link
Member

cyphar commented Jun 11, 2021

@AkihiroSuda Can you update this to use the new release?

@AkihiroSuda AkihiroSuda dismissed stale reviews from kolyshkin and cyphar via c28d402 June 11, 2021 17:07
@AkihiroSuda
Copy link
Member Author

Thanks, updated

The `errors.Is(err, unix.EINVAL)` check in `haveBpfProgReplace()` was
broken because the `cilium/ebpf` library did not "wrap" errors.
/~https://github.com/cilium/ebpf/blob/v0.6.0/link/program.go#L72

So the eBPF support of runc was broken for kernel prior to 5.6.

This commit bumps up cilium/ebpf to contain cilium/ebpf PR 320.

Fix opencontainers/runc issue 3008

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@mrunalp mrunalp merged commit 93a01cd into opencontainers:master Jun 11, 2021
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants