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

ebpf: replace deprecated prog.Attach/prog.Detach #2903

Merged
merged 1 commit into from
Apr 14, 2021

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Apr 13, 2021

fixes #2699

Caught by golangci-lint when enabling golint:

libcontainer/cgroups/ebpf/ebpf.go:35:12: SA1019: prog.Attach is deprecated: use link.RawAttachProgram instead. (staticcheck)
    if err := prog.Attach(dirFD, ebpf.AttachCGroupDevice, unix.BPF_F_ALLOW_MULTI); err != nil {
              ^
libcontainer/cgroups/ebpf/ebpf.go:39:13: SA1019: prog.Detach is deprecated: use link.RawDetachProgram instead. (staticcheck)
        if err := prog.Detach(dirFD, ebpf.AttachCGroupDevice, unix.BPF_F_ALLOW_MULTI); err != nil {
                  ^

Worth noting that we currently call prog.Detach() with unix.BPF_F_ALLOW_MULTI;
https://github.com/golang/sys/blob/22da62e12c0cd9c1da93581e1113ca4d82a5be14/unix/zerrors_linux.go#L178

BPF_F_ALLOW_MULTI = 0x2

⚠️ Looking at the source code for prog.Detach(); https://github.com/cilium/ebpf/blob/v0.4.0/prog.go#L579-L581,
this would always produce an error:

if flags != 0 {
    return errors.New("flags must be zero")
}

Note that the flags parameter is not used (except for that validation)

Caught by golangci-lint when enabling golint:

    libcontainer/cgroups/ebpf/ebpf.go:35:12: SA1019: prog.Attach is deprecated: use link.RawAttachProgram instead. (staticcheck)
        if err := prog.Attach(dirFD, ebpf.AttachCGroupDevice, unix.BPF_F_ALLOW_MULTI); err != nil {
                  ^
    libcontainer/cgroups/ebpf/ebpf.go:39:13: SA1019: prog.Detach is deprecated: use link.RawDetachProgram instead. (staticcheck)
            if err := prog.Detach(dirFD, ebpf.AttachCGroupDevice, unix.BPF_F_ALLOW_MULTI); err != nil {
                      ^

Worth noting that we currently call prog.Detach() with unix.BPF_F_ALLOW_MULTI;
https://github.com/golang/sys/blob/22da62e12c0cd9c1da93581e1113ca4d82a5be14/unix/zerrors_linux.go#L178

    BPF_F_ALLOW_MULTI = 0x2

Looking at the source code for prog.Detach(); https://github.com/cilium/ebpf/blob/v0.4.0/prog.go#L579-L581,
this would _always_ produce an error:

    if flags != 0 {
        return errors.New("flags must be zero")
    }

Note that the flags parameter is not used (except for that validation)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Member Author

@AkihiroSuda @giuseppe @kolyshkin PTAL; please double check if I did the conversion correctly, and if my reading of the prog.Detach() issue is correct, as I'm really not familiar with this.

I see the prog.Detach() was added in faf673e (#2145), based on the c implementation in crun

@thaJeztah
Copy link
Member Author

also /cc @cyphar (I see there was discussion about this code in #2366)

@kolyshkin kolyshkin requested a review from cyphar April 13, 2021 18:47
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, but please DNM until we get LGTM from @cyphar

@kolyshkin
Copy link
Contributor

Also, I agree with your analysis (Detach should not be called with BPF_F_ALLOW_MULTI).

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.

Oh, good -- looking at RawDetachProgram it looks like this will mean we only detach the program we just inserted (though more broadly we still do the wrong thing on cgroup updates). This was not the case with our incorrect proper usage of Detach AFAICS.

LGTM.

@cyphar cyphar closed this in b474993 Apr 14, 2021
@cyphar cyphar merged commit b474993 into opencontainers:master Apr 14, 2021
@thaJeztah thaJeztah deleted the fix_deprecated_ebpf branch April 14, 2021 09:58
saschagrunert pushed a commit to saschagrunert/runc that referenced this pull request Apr 14, 2021
Sebastiaan van Stijn (1):
  ebpf: replace deprecated prog.Attach/prog.Detach

LGTMs: AkihiroSuda kolyshkin cyphar
Closes opencontainers#2903

Signed-off-by: Sascha Grunert <[email protected]>
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.

cgroups/ebpf: switch to new cilium/ebpf API
4 participants