Repository: cilium/ebpf
Pull request #385 (2021-08-23 17:53:13)

link/uprobe: fix offsets for statically linked binaries
Context: https://cilium.slack.com/archives/CKC55JL8L/p1629493947056900

I could reproduce the problem, after applying these changes (https://github.com/iovisor/bcc/blob/master/libbpf-tools/uprobe_helpers.c#L268, https://github.com/iovisor/bcc/pull/3519/commits/af3da2370e656e22383dc2f10aa6b1da7637d0ea), it works for me locally.


lmb: > the test is failing on 4.9 (tracefs) but I can't reproduce locally even if I force that path

What do you mean by that? You can't repro locally on 4.9, or you can't reproduce if you force tracefs usage on a newer kernel?
mmat11: > > the test is failing on 4.9 (tracefs) but I can't reproduce locally even if I force that path
> 
> What do you mean by that? You can't repro locally on 4.9, or you can't reproduce if you force tracefs usage on a newer kernel?

The latter. If I modify the code and force tracefs, the test passes on all kernels except 4.9.

 I wrote "can't reproduce locally" because I assumed the kernel version was not a problem, but it is.
lmb: Maybe @brycekahle or @Gui774ume have an idea what this could be? `ebpf-manager` seems to have similar code to this.
lmb: I think it's worth getting this merged if it fixes stuff on newer kernels. How about we skip the failing test on 4.9 and create an issue that contains a reproducer?

An unrelated idea: could we instrument the `go` binary instead of shipping a pre-compiled elf?

```
$ nm /usr/local/bin/go | grep mainUsage
00000000008c17a0 T main.mainUsage
```
mmat11: > I think it's worth getting this merged if it fixes stuff on newer kernels. How about we skip the failing test on 4.9 and create an issue that contains a reproducer?

Makes sense, my plan was to manually test the same binary on a 4.9 kernel VM. I can do this later and eventually add some comments (or a fix, depending on what's causing it to fail).

> An unrelated idea: could we instrument the `go` binary instead of shipping a pre-compiled elf?
> 
> ```
> $ nm /usr/local/bin/go | grep mainUsage
> 00000000008c17a0 T main.mainUsage
> ```

Good idea, I'll explore this option later today or tomorrow
mmat11: @lmb I skipped the test on 4.9 and opened https://github.com/cilium/ebpf/issues/406 to keep track of it.

I think https://github.com/cilium/ebpf/issues/399 and https://github.com/cilium/ebpf/issues/358 can be closed after this is merged

Patch:
diff --git a/internal/unix/types_linux.go b/internal/unix/types_linux.go
index edd9242..9d204ee 100644
--- a/internal/unix/types_linux.go
+++ b/internal/unix/types_linux.go
@@ -5,6 +5,7 @@ package unix
 
 import (
 	"bytes"
+	"fmt"
 	"syscall"
 
 	linux "golang.org/x/sys/unix"
@@ -71,11 +72,6 @@ type Statfs_t = linux.Statfs_t
 // Rlimit is a wrapper
 type Rlimit = linux.Rlimit
 
-// Setrlimit is a wrapper
-func Setrlimit(resource int, rlim *Rlimit) (err error) {
-	return linux.Setrlimit(resource, rlim)
-}
-
 // Syscall is a wrapper
 func Syscall(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err syscall.Errno) {
 	return linux.Syscall(trap, a1, a2, a3)
@@ -204,3 +200,24 @@ func KernelRelease() (string, error) {
 	release := string(uname.Release[:end])
 	return release, nil
 }
+
+func RemoveMemlockRlimit() (func() error, error) {
+	oldLimit := new(Rlimit)
+	if err := linux.Getrlimit(RLIMIT_MEMLOCK, oldLimit); err != nil {
+		return nil, fmt.Errorf("failed to get memlock rlimit: %w", err)
+	}
+
+	if err := linux.Setrlimit(RLIMIT_MEMLOCK, &Rlimit{
+		Cur: RLIM_INFINITY,
+		Max: RLIM_INFINITY,
+	}); err != nil {
+		return nil, fmt.Errorf("failed to set memlock rlimit: %w", err)
+	}
+
+	return func() error {
+		if err := linux.Setrlimit(RLIMIT_MEMLOCK, oldLimit); err != nil {
+			return fmt.Errorf("failed to reset memlock rlimit: %w", err)
+		}
+		return nil
+	}, nil
+}
diff --git a/internal/unix/types_other.go b/internal/unix/types_other.go
index ef414e1..4245eb9 100644
--- a/internal/unix/types_other.go
+++ b/internal/unix/types_other.go
@@ -88,11 +88,6 @@ type Rlimit struct {
 	Max uint64
 }
 
-// Setrlimit is a wrapper
-func Setrlimit(resource int, rlim *Rlimit) (err error) {
-	return errNonLinux
-}
-
 // Syscall is a wrapper
 func Syscall(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err syscall.Errno) {
 	return 0, 0, syscall.Errno(1)
@@ -263,3 +258,7 @@ func Renameat2(olddirfd int, oldpath string, newdirfd int, newpath string, flags
 func KernelRelease() (string, error) {
 	return "", errNonLinux
 }
+
+func RemoveMemlockRlimit() (func() error, error) {
+	return nil, errNonLinux
+}
diff --git a/link/uprobe.go b/link/uprobe.go
index e7fc5ac..50299c8 100644
--- a/link/uprobe.go
+++ b/link/uprobe.go
@@ -31,8 +31,8 @@ var (
 type Executable struct {
 	// Path of the executable on the filesystem.
 	path string
-	// Parsed ELF symbols and dynamic symbols.
-	symbols map[string]elf.Symbol
+	// Parsed ELF symbols and dynamic symbols offsets.
+	offsets map[string]uint64
 }
 
 // UprobeOptions defines additional parameters that will be used
@@ -67,42 +67,84 @@ func OpenExecutable(path string) (*Executable, error) {
 		return nil, fmt.Errorf("parse ELF file: %w", err)
 	}
 
-	var ex = Executable{
-		path:    path,
-		symbols: make(map[string]elf.Symbol),
+	if se.Type != elf.ET_EXEC && se.Type != elf.ET_DYN {
+		// ELF is not an executable or a shared object.
+		return nil, errors.New("the given file is not an executable or a shared object")
 	}
-	if err := ex.addSymbols(se.Symbols); err != nil {
-		return nil, err
+
+	ex := Executable{
+		path:    path,
+		offsets: make(map[string]uint64),
 	}
 
-	if err := ex.addSymbols(se.DynamicSymbols); err != nil {
+	if err := ex.load(se); err != nil {
 		return nil, err
 	}
 
 	return &ex, nil
 }
 
-func (ex *Executable) addSymbols(f func() ([]elf.Symbol, error)) error {
-	// elf.Symbols and elf.DynamicSymbols return ErrNoSymbols if the section is not found.
-	syms, err := f()
+func (ex *Executable) load(f *internal.SafeELFFile) error {
+	syms, err := f.Symbols()
 	if err != nil && !errors.Is(err, elf.ErrNoSymbols) {
 		return err
 	}
+
+	dynsyms, err := f.DynamicSymbols()
+	if err != nil && !errors.Is(err, elf.ErrNoSymbols) {
+		return err
+	}
+
+	syms = append(syms, dynsyms...)
+
 	for _, s := range syms {
 		if elf.ST_TYPE(s.Info) != elf.STT_FUNC {
 			// Symbol not associated with a function or other executable code.
 			continue
 		}
-		ex.symbols[s.Name] = s
+
+		off := s.Value
+
+		// Loop over ELF segments.
+		for _, prog := range f.Progs {
+			// Skip uninteresting segments.
+			if prog.Type != elf.PT_LOAD || (prog.Flags&elf.PF_X) == 0 {
+				continue
+			}
+
+			if prog.Vaddr <= s.Value && s.Value < (prog.Vaddr+prog.Memsz) {
+				// If the symbol value is contained in the segment, calculate
+				// the symbol offset.
+				//
+				// fn symbol offset = fn symbol VA - .text VA + .text offset
+				//
+				// stackoverflow.com/a/40249502
+				off = s.Value - prog.Vaddr + prog.Off
+				break
+			}
+		}
+
+		ex.offsets[s.Name] = off
 	}
+
 	return nil
 }
 
-func (ex *Executable) symbol(symbol string) (*elf.Symbol, error) {
-	if s, ok := ex.symbols[symbol]; ok {
-		return &s, nil
+func (ex *Executable) offset(symbol string) (uint64, error) {
+	if off, ok := ex.offsets[symbol]; ok {
+		// Symbols with location 0 from section undef are shared library calls and
+		// are relocated before the binary is executed. Dynamic linking is not
+		// implemented by the library, so mark this as unsupported for now.
+		//
+		// Since only offset values are stored and not elf.Symbol, if the value is 0,
+		// assume it's an external symbol.
+		if off == 0 {
+			return 0, fmt.Errorf("cannot resolve %s library call '%s', "+
+				"consider providing the offset via options: %w", ex.path, symbol, ErrNotSupported)
+		}
+		return off, nil
 	}
-	return nil, fmt.Errorf("symbol %s not found", symbol)
+	return 0, fmt.Errorf("symbol %s not found", symbol)
 }
 
 // Uprobe attaches the given eBPF program to a perf event that fires when the
@@ -184,20 +226,11 @@ func (ex *Executable) uprobe(symbol string, prog *ebpf.Program, opts *UprobeOpti
 	if opts != nil && opts.Offset != 0 {
 		offset = opts.Offset
 	} else {
-		sym, err := ex.symbol(symbol)
+		off, err := ex.offset(symbol)
 		if err != nil {
-			return nil, fmt.Errorf("symbol '%s' not found: %w", symbol, err)
+			return nil, err
 		}
-
-		// Symbols with location 0 from section undef are shared library calls and
-		// are relocated before the binary is executed. Dynamic linking is not
-		// implemented by the library, so mark this as unsupported for now.
-		if sym.Section == elf.SHN_UNDEF && sym.Value == 0 {
-			return nil, fmt.Errorf("cannot resolve %s library call '%s', "+
-				"consider providing the offset via options: %w", ex.path, symbol, ErrNotSupported)
-		}
-
-		offset = sym.Value
+		offset = off
 	}
 
 	pid := perfAllThreads
diff --git a/map.go b/map.go
index bf854e3..546ef1b 100644
--- a/map.go
+++ b/map.go
@@ -197,7 +197,7 @@ func NewMap(spec *MapSpec) (*Map, error) {
 //
 // The caller is responsible for ensuring the process' rlimit is set
 // sufficiently high for locking memory during map creation. This can be done
-// by calling unix.Setrlimit with unix.RLIMIT_MEMLOCK prior to calling NewMapWithOptions.
+// by calling ebpf.RemoveMemlockRlimit() prior to calling NewMapWithOptions.
 //
 // May return an error wrapping ErrMapIncompatible.
 func NewMapWithOptions(spec *MapSpec, opts MapOptions) (*Map, error) {
diff --git a/syscalls.go b/syscalls.go
index f8cb5f0..cb38e88 100644
--- a/syscalls.go
+++ b/syscalls.go
@@ -17,6 +17,16 @@ import (
 // Deprecated: use os.ErrNotExist instead.
 var ErrNotExist = os.ErrNotExist
 
+// RemoveMemlockRlimit removes the limit on the amount of memory the current
+// process can lock into RAM. Returns a function that restores the limit to
+// its previous value.
+//
+// This is not required to load eBPF resources on kernel versions 5.11+
+// due to the introduction of cgroup-based memory accounting.
+func RemoveMemlockRlimit() (func() error, error) {
+	return unix.RemoveMemlockRlimit()
+}
+
 // invalidBPFObjNameChar returns true if char may not appear in
 // a BPF object name.
 func invalidBPFObjNameChar(char rune) bool {


Test patch:
diff --git a/internal/testutils/rlimit.go b/internal/testutils/rlimit.go
index 3c4e754..58ae784 100644
--- a/internal/testutils/rlimit.go
+++ b/internal/testutils/rlimit.go
@@ -10,11 +10,7 @@ import (
 func init() {
 	// Increase the memlock for all tests unconditionally. It's a great source of
 	// weird bugs, since different distros have different default limits.
-	err := unix.Setrlimit(unix.RLIMIT_MEMLOCK, &unix.Rlimit{
-		Cur: unix.RLIM_INFINITY,
-		Max: unix.RLIM_INFINITY,
-	})
-	if err != nil {
+	if _, err := unix.RemoveMemlockRlimit(); err != nil {
 		fmt.Fprintln(os.Stderr, "WARNING: Failed to adjust rlimit, tests may fail")
 	}
 }
diff --git a/link/uprobe_test.go b/link/uprobe_test.go
index 51db785..8159bc3 100644
--- a/link/uprobe_test.go
+++ b/link/uprobe_test.go
@@ -3,8 +3,10 @@ package link
 import (
 	"errors"
 	"fmt"
+	"go/build"
 	"os"
 	"os/exec"
+	"path"
 	"testing"
 
 	qt "github.com/frankban/quicktest"
@@ -14,8 +16,10 @@ import (
 	"github.com/cilium/ebpf/internal/unix"
 )
 
-var bashEx, _ = OpenExecutable("/bin/bash")
-var bashSym = "main"
+var (
+	bashEx, _ = OpenExecutable("/bin/bash")
+	bashSym   = "main"
+)
 
 func TestExecutable(t *testing.T) {
 	_, err := OpenExecutable("")
@@ -27,15 +31,12 @@ func TestExecutable(t *testing.T) {
 		t.Fatalf("create executable: unexpected path '%s'", bashEx.path)
 	}
 
-	sym, err := bashEx.symbol(bashSym)
+	_, err = bashEx.offset(bashSym)
 	if err != nil {
-		t.Fatalf("find symbol: %v", err)
-	}
-	if sym.Name != bashSym {
-		t.Fatalf("find symbol: unexpected symbol '%s'", sym.Name)
+		t.Fatalf("find offset: %v", err)
 	}
 
-	_, err = bashEx.symbol("bogus")
+	_, err = bashEx.offset("bogus")
 	if err == nil {
 		t.Fatal("find symbol: expected error")
 	}
@@ -142,19 +143,19 @@ func TestUprobeCreatePMU(t *testing.T) {
 
 	c := qt.New(t)
 
-	// Fetch the elf.Symbol from the /bin/bash Executable already defined.
-	sym, err := bashEx.symbol(bashSym)
+	// Fetch the offset from the /bin/bash Executable already defined.
+	off, err := bashEx.offset(bashSym)
 	c.Assert(err, qt.IsNil)
 
 	// uprobe PMU
-	pu, err := pmuUprobe(sym.Name, bashEx.path, sym.Value, perfAllThreads, false)
+	pu, err := pmuUprobe(bashSym, bashEx.path, off, perfAllThreads, false)
 	c.Assert(err, qt.IsNil)
 	defer pu.Close()
 
 	c.Assert(pu.typ, qt.Equals, uprobeEvent)
 
 	// uretprobe PMU
-	pr, err := pmuUprobe(sym.Name, bashEx.path, sym.Value, perfAllThreads, true)
+	pr, err := pmuUprobe(bashSym, bashEx.path, off, perfAllThreads, true)
 	c.Assert(err, qt.IsNil)
 	defer pr.Close()
 
@@ -165,11 +166,11 @@ func TestUprobeCreatePMU(t *testing.T) {
 func TestUprobePMUUnavailable(t *testing.T) {
 	c := qt.New(t)
 
-	// Fetch the elf.Symbol from the /bin/bash Executable already defined.
-	sym, err := bashEx.symbol(bashSym)
+	// Fetch the offset from the /bin/bash Executable already defined.
+	off, err := bashEx.offset(bashSym)
 	c.Assert(err, qt.IsNil)
 
-	pk, err := pmuUprobe(sym.Name, bashEx.path, sym.Value, perfAllThreads, false)
+	pk, err := pmuUprobe(bashSym, bashEx.path, off, perfAllThreads, false)
 	if err == nil {
 		pk.Close()
 		t.Skipf("Kernel supports perf_uprobe PMU, not asserting error.")
@@ -183,31 +184,31 @@ func TestUprobePMUUnavailable(t *testing.T) {
 func TestUprobeTraceFS(t *testing.T) {
 	c := qt.New(t)
 
-	// Fetch the elf.Symbol from the /bin/bash Executable already defined.
-	sym, err := bashEx.symbol(bashSym)
+	// Fetch the offset from the /bin/bash Executable already defined.
+	off, err := bashEx.offset(bashSym)
 	c.Assert(err, qt.IsNil)
 
 	// Sanitize the symbol in order to be used in tracefs API.
-	ssym := uprobeSanitizedSymbol(sym.Name)
+	ssym := uprobeSanitizedSymbol(bashSym)
 
 	// Open and close tracefs u(ret)probes, checking all errors.
-	up, err := tracefsUprobe(ssym, bashEx.path, sym.Value, perfAllThreads, false)
+	up, err := tracefsUprobe(ssym, bashEx.path, off, perfAllThreads, false)
 	c.Assert(err, qt.IsNil)
 	c.Assert(up.Close(), qt.IsNil)
 	c.Assert(up.typ, qt.Equals, uprobeEvent)
 
-	up, err = tracefsUprobe(ssym, bashEx.path, sym.Value, perfAllThreads, true)
+	up, err = tracefsUprobe(ssym, bashEx.path, off, perfAllThreads, true)
 	c.Assert(err, qt.IsNil)
 	c.Assert(up.Close(), qt.IsNil)
 	c.Assert(up.typ, qt.Equals, uretprobeEvent)
 
 	// Create two identical trace events, ensure their IDs differ.
-	u1, err := tracefsUprobe(ssym, bashEx.path, sym.Value, perfAllThreads, false)
+	u1, err := tracefsUprobe(ssym, bashEx.path, off, perfAllThreads, false)
 	c.Assert(err, qt.IsNil)
 	defer u1.Close()
 	c.Assert(u1.tracefsID, qt.Not(qt.Equals), 0)
 
-	u2, err := tracefsUprobe(ssym, bashEx.path, sym.Value, perfAllThreads, false)
+	u2, err := tracefsUprobe(ssym, bashEx.path, off, perfAllThreads, false)
 	c.Assert(err, qt.IsNil)
 	defer u2.Close()
 	c.Assert(u2.tracefsID, qt.Not(qt.Equals), 0)
@@ -225,12 +226,12 @@ func TestUprobeCreateTraceFS(t *testing.T) {
 
 	c := qt.New(t)
 
-	// Fetch the elf.Symbol from the /bin/bash Executable already defined.
-	sym, err := bashEx.symbol(bashSym)
+	// Fetch the offset from the /bin/bash Executable already defined.
+	off, err := bashEx.offset(bashSym)
 	c.Assert(err, qt.IsNil)
 
 	// Sanitize the symbol in order to be used in tracefs API.
-	ssym := uprobeSanitizedSymbol(sym.Name)
+	ssym := uprobeSanitizedSymbol(bashSym)
 
 	pg, _ := randomGroup("ebpftest")
 	rg, _ := randomGroup("ebpftest")
@@ -242,12 +243,12 @@ func TestUprobeCreateTraceFS(t *testing.T) {
 	}()
 
 	// Create a uprobe.
-	err = createTraceFSProbeEvent(uprobeType, pg, ssym, bashEx.path, sym.Value, false)
+	err = createTraceFSProbeEvent(uprobeType, pg, ssym, bashEx.path, off, false)
 	c.Assert(err, qt.IsNil)
 
 	// Attempt to create an identical uprobe using tracefs,
 	// expect it to fail with os.ErrExist.
-	err = createTraceFSProbeEvent(uprobeType, pg, ssym, bashEx.path, sym.Value, false)
+	err = createTraceFSProbeEvent(uprobeType, pg, ssym, bashEx.path, off, false)
 	c.Assert(errors.Is(err, os.ErrExist), qt.IsTrue,
 		qt.Commentf("expected consecutive uprobe creation to contain os.ErrExist, got: %v", err))
 
@@ -255,10 +256,10 @@ func TestUprobeCreateTraceFS(t *testing.T) {
 	c.Assert(closeTraceFSProbeEvent(uprobeType, pg, ssym), qt.IsNil)
 
 	// Same test for a kretprobe.
-	err = createTraceFSProbeEvent(uprobeType, rg, ssym, bashEx.path, sym.Value, true)
+	err = createTraceFSProbeEvent(uprobeType, rg, ssym, bashEx.path, off, true)
 	c.Assert(err, qt.IsNil)
 
-	err = createTraceFSProbeEvent(uprobeType, rg, ssym, bashEx.path, sym.Value, true)
+	err = createTraceFSProbeEvent(uprobeType, rg, ssym, bashEx.path, off, true)
 	c.Assert(os.IsExist(err), qt.IsFalse,
 		qt.Commentf("expected consecutive uretprobe creation to contain os.ErrExist, got: %v", err))
 
@@ -267,7 +268,7 @@ func TestUprobeCreateTraceFS(t *testing.T) {
 }
 
 func TestUprobeSanitizedSymbol(t *testing.T) {
-	var tests = []struct {
+	tests := []struct {
 		symbol   string
 		expected string
 	}{
@@ -288,7 +289,7 @@ func TestUprobeSanitizedSymbol(t *testing.T) {
 }
 
 func TestUprobePathOffset(t *testing.T) {
-	var tests = []struct {
+	tests := []struct {
 		path     string
 		offset   uint64
 		expected string
@@ -310,47 +311,76 @@ func TestUprobePathOffset(t *testing.T) {
 }
 
 func TestUprobeProgramCall(t *testing.T) {
-	m, p := newUpdaterMapProg(t, ebpf.Kprobe)
-
-	// Load the '/bin/bash' executable.
-	ex, err := OpenExecutable("/bin/bash")
-	if err != nil {
-		t.Fatal(err)
+	tests := []struct {
+		name string
+		elf  string
+		args []string
+		sym  string
+	}{
+		{
+			"bash",
+			"/bin/bash",
+			[]string{"--help"},
+			"main",
+		},
+		{
+			"go-binary",
+			path.Join(build.Default.GOROOT, "bin/go"),
+			[]string{"version"},
+			"main.main",
+		},
 	}
 
-	// Open Uprobe on '/bin/bash' for the symbol 'main'
-	// and attach it to the ebpf program created above.
-	u, err := ex.Uprobe("main", p, nil)
-	if err != nil {
-		t.Fatal(err)
-	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.name == "go-binary" {
+				// https://github.com/cilium/ebpf/issues/406
+				testutils.SkipOnOldKernel(t, "4.14", "uprobes on Go binaries silently fail on kernel < 4.14")
+			}
 
-	// Trigger ebpf program call.
-	trigger := func(t *testing.T) {
-		if err := exec.Command("/bin/bash", "--help").Run(); err != nil {
-			t.Fatal(err)
-		}
-	}
-	trigger(t)
+			m, p := newUpdaterMapProg(t, ebpf.Kprobe)
 
-	// Assert that the value at index 0 has been updated to 1.
-	assertMapValue(t, m, 0, 1)
+			// Load the executable.
+			ex, err := OpenExecutable(tt.elf)
+			if err != nil {
+				t.Fatal(err)
+			}
 
-	// Detach the Uprobe.
-	if err := u.Close(); err != nil {
-		t.Fatal(err)
-	}
+			// Open Uprobe on the executable for the given symbol
+			// and attach it to the ebpf program created above.
+			u, err := ex.Uprobe(tt.sym, p, nil)
+			if err != nil {
+				t.Fatal(err)
+			}
 
-	// Reset map value to 0 at index 0.
-	if err := m.Update(uint32(0), uint32(0), ebpf.UpdateExist); err != nil {
-		t.Fatal(err)
-	}
+			// Trigger ebpf program call.
+			trigger := func(t *testing.T) {
+				if err := exec.Command(tt.elf, tt.args...).Run(); err != nil {
+					t.Fatal(err)
+				}
+			}
+			trigger(t)
 
-	// Retrigger the ebpf program call.
-	trigger(t)
+			// Assert that the value at index 0 has been updated to 1.
+			assertMapValue(t, m, 0, 1)
 
-	// Assert that this time the value has not been updated.
-	assertMapValue(t, m, 0, 0)
+			// Detach the Uprobe.
+			if err := u.Close(); err != nil {
+				t.Fatal(err)
+			}
+
+			// Reset map value to 0 at index 0.
+			if err := m.Update(uint32(0), uint32(0), ebpf.UpdateExist); err != nil {
+				t.Fatal(err)
+			}
+
+			// Retrigger the ebpf program call.
+			trigger(t)
+
+			// Assert that this time the value has not been updated.
+			assertMapValue(t, m, 0, 0)
+		})
+	}
 }
 
 func TestUprobeProgramWrongPID(t *testing.T) {