Skip to content

Commit

Permalink
elf: special case current version for Ubuntu/Debian
Browse files Browse the repository at this point in the history
Signed-off-by: Michael Schubert <[email protected]>
  • Loading branch information
schu committed Jan 10, 2017
1 parent 554883c commit 4efd5ba
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 13 deletions.
2 changes: 0 additions & 2 deletions bpf_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,6 @@ func TestModuleLoadBCC(t *testing.T) {
}

func TestModuleLoadELF(t *testing.T) {
// Does not work yet reliable across distributions
return
b := elf.NewModule("./tests/dummy.o")
if b == nil {
t.Fatal("prog is nil")
Expand Down
72 changes: 61 additions & 11 deletions elf/elf.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ import (
"errors"
"fmt"
"io"
"io/ioutil"
"os"
"regexp"
"strconv"
"strings"
"syscall"
Expand Down Expand Up @@ -257,17 +259,8 @@ func utsnameStr(in []int8) string {
return string(out)
}

func currentVersion() (int, error) {
var buf syscall.Utsname
if err := syscall.Uname(&buf); err != nil {
return -1, err
}

releaseStr := strings.Trim(utsnameStr(buf.Release[:]), "\x00")

kernelVersionStr := strings.Split(releaseStr, "-")[0]

kernelVersionParts := strings.Split(kernelVersionStr, ".")
func kernelVersionFromString(versionString string) (int, error) {
kernelVersionParts := strings.Split(versionString, ".")
if len(kernelVersionParts) != 3 {
return -1, errors.New("not enough version information")
}
Expand All @@ -292,6 +285,63 @@ func currentVersion() (int, error) {
return out, nil
}

func currentVersionUname() (int, error) {
var buf syscall.Utsname
if err := syscall.Uname(&buf); err != nil {
return -1, err
}
releaseStr := strings.Trim(utsnameStr(buf.Release[:]), "\x00")
kernelVersionStr := strings.Split(releaseStr, "-")[0]
return kernelVersionFromString(kernelVersionStr)

This comment has been minimized.

Copy link
@alban

alban Jan 10, 2017

Member

kernelVersionFromString is always preceded by the Split on "-". Could this be factored inside kernelVersionFromString?

Also does the Split() work when there is no dash or when the dash is added in an unconventional location?

An example of missing dash / unconventional location is 4.8.6rkt-v1. See rkt/rkt#3489 for details.

Those cases are worth being tested in an unit test in elf_test.go. Or better, in a separate file kernversion.go and kernversion_test.go.

}

func currentVersionUbuntu() (int, error) {
procVersion, err := ioutil.ReadFile("/proc/version_signature")
if err != nil {
return -1, err
}
var u1, u2, releaseStr string
_, err = fmt.Sscanf(string(procVersion), "%s %s %s", &u1, &u2, &releaseStr)

This comment has been minimized.

Copy link
@alban

alban Jan 10, 2017

Member

Can we use Sscanf(string(procVersion, "%*s %*s %s", &releaseStr) with the stars like in C to avoid the dummy variables? http://stackoverflow.com/questions/7607550/scanf-skip-variable
I don't know if Golang's scanf supports that.

This comment has been minimized.

Copy link
@schu

schu Jan 10, 2017

Author

AFAIK not supported with Golang.

This comment has been minimized.

Copy link
@alban

alban Jan 10, 2017

Member

ack.

if err != nil {
return -1, err
}
kernelVersionStr := strings.Split(releaseStr, "-")[0]
return kernelVersionFromString(kernelVersionStr)
}

var debianVersionRegex = regexp.MustCompile(`.* SMP Debian (\d+\.\d+.\d+-\d+) .*`)

func currentVersionDebian() (int, error) {
procVersion, err := ioutil.ReadFile("/proc/version")
if err != nil {
return -1, err
}
match := debianVersionRegex.FindStringSubmatch(string(procVersion))
if len(match) != 2 {
return -1, fmt.Errorf("failed to get kernel version from /proc/version: %s", procVersion)
}
kernelVersionStr := strings.Split(match[1], "-")[0]
return kernelVersionFromString(kernelVersionStr)
}

func currentVersion() (int, error) {

This comment has been minimized.

Copy link
@alban

alban Jan 10, 2017

Member

Please add comments explaining why we do several version checks with links to

https://wiki.ubuntu.com/Kernel/FAQ
https://kernel-handbook.alioth.debian.org/ch-versions.html

And say the perf tool uses a similar strategy in torvalds/linux@d18acd15c

onUbuntu, err := pathExists("/proc/version_signature")
if err != nil {
return -1, err
}
if onUbuntu {
return currentVersionUbuntu()

This comment has been minimized.

Copy link
@alban

alban Jan 10, 2017

Member

Instead of checking if /proc/version_signature exists before, I would directly call currentVersionUbuntu inconditionally and if it returns err != nil, then continue with currentVersionDebian & currentVersionUname.

Although we don't have TOCTOU issues in this case with /proc files, I think it is a better pattern.

}
onDebian, err := pathExists("/etc/debian_version")

This comment has been minimized.

Copy link
@alban

alban Jan 10, 2017

Member

we should not rely on that because the code might run in a container without a bind mount of /etc/debian_version

Instead, we should try to call currentVersionDebian, and that would return an error if the uname contains the "Debian" regexp. Then, fallback on currentVersionUname in case of error.

So, no dependencies on files. Only on the kernel with the uname syscall and the procfs file.

if err != nil {
return -1, err
}
if onDebian {
return currentVersionDebian()
}
return currentVersionUname()
}

func elfReadMaps(file *elf.File) (map[string]*Map, error) {
maps := make(map[string]*Map)
for sectionIdx, section := range file.Sections {
Expand Down
14 changes: 14 additions & 0 deletions elf/utils.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package elf

import "os"

func pathExists(path string) (bool, error) {

This comment has been minimized.

Copy link
@alban

alban Jan 10, 2017

Member

that helper function would be unnecessary with the changes suggested above.

_, err := os.Stat(path)
if err == nil {
return true, nil
}
if os.IsNotExist(err) {
return false, nil
}
return true, err
}

0 comments on commit 4efd5ba

Please sign in to comment.