Skip to content

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

Merged
mrunalp merged 1 commit intoopencontainers:masterfrom
AkihiroSuda:update-ebpf-wrap
Jun 11, 2021
Merged

update cilium/ebpf to fix haveBpfProgReplace() check#3009
mrunalp merged 1 commit intoopencontainers:masterfrom
AkihiroSuda:update-ebpf-wrap

Conversation

@AkihiroSuda
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

@ti-mo
Copy link
Copy Markdown

ti-mo commented Jun 11, 2021

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

@dims
Copy link
Copy Markdown
Contributor

dims commented Jun 11, 2021

thanks @ti-mo !

@cyphar
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Development

Successfully merging this pull request may close these issues.

"failed to call BPF_PROG_ATTACH (BPF_CGROUP_DEVICE, BPF_F_ALLOW_MULTI): can't attach program: invalid argument: unknown" (master, kernel 5.4, cgroup2)

6 participants