Repository: cilium/ebpf
Pull request #247 (2021-03-09 06:50:08)

Add Unpin for Program
This is a follow-up PR to #204 to add similar support for `Program`.

Signed-off-by: Aditi Ghag aditi@cilium.io

aditighag: Thanks for the review. I've addressed most of the review comments.
aditighag: Sorry, I missed the most straight forward case while addressing the review comments. :woman-facepalming: Pushing a new commit shortly. 
aditighag: Some of the suggested approaches didn't really cover all the possible cases. Based on the review feedback, the latest commit tries to avoid nested paths, and keeps the happy path aligned to the left. I've also added a couple of comments so that when the next time we see the code, it doesn't trip us.

Patch:
diff --git a/map.go b/map.go
index 6975cfb..316fc37 100644
--- a/map.go
+++ b/map.go
@@ -4,7 +4,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"os"
 	"path/filepath"
 	"reflect"
 	"strings"
@@ -803,53 +802,35 @@ func (m *Map) Clone() (*Map, error) {
 	}, nil
 }
 
-// Pin persists the map past the lifetime of the process that created it.
+// Pin persists the map on the BPF virtual file system past the lifetime of
+// the process that created it .
 //
 // Calling Pin on a previously pinned map will override the path.
 // You can Clone a map to pin it to a different path.
 //
 // This requires bpffs to be mounted above fileName. See https://docs.cilium.io/en/k8s-doc/admin/#admin-mount-bpffs
 func (m *Map) Pin(fileName string) error {
-	if fileName == "" {
-		return fmt.Errorf("pinned path cannot be empty")
-	}
-	if m.IsPinned() {
-		path := m.pinnedPath
-		if path == fileName {
-			return nil
-		}
-		if err := os.Rename(m.pinnedPath, fileName); err != nil {
-			if !os.IsNotExist(err) {
-				return fmt.Errorf("unable to pin the map at new path %v: %w", fileName, err)
-			}
-		} else {
-			m.pinnedPath = fileName
-			return nil
-		}
-	}
-	err := internal.BPFObjPin(fileName, m.fd)
-	if err == nil {
-		m.pinnedPath = fileName
+	if err := pin(m.pinnedPath, fileName, m.fd); err != nil {
+		return err
 	}
-	return err
+	m.pinnedPath = fileName
+	return nil
 }
 
-// Unpin removes the persisted state for the map.
+// Unpin removes the persisted state for the map from the BPF virtual filesystem.
+//
+// Failed calls to Unpin will not alter the state returned by IsPinned.
 //
-// Unpinning an un-pinned Map returns nil.
+// Unpinning an unpinned Map returns nil.
 func (m *Map) Unpin() error {
-	if m.pinnedPath == "" {
-		return nil
-	}
-	err := os.Remove(m.pinnedPath)
-	if err == nil || os.IsNotExist(err) {
-		m.pinnedPath = ""
-		return nil
+	if err := unpin(m.pinnedPath); err != nil {
+		return err
 	}
-	return err
+	m.pinnedPath = ""
+	return nil
 }
 
-// IsPinned returns true if the map has non-empty pinned path.
+// IsPinned returns true if the map has a non-empty pinned path.
 func (m *Map) IsPinned() bool {
 	if m.pinnedPath == "" {
 		return false
diff --git a/pinning.go b/pinning.go
new file mode 100644
index 0000000..7881236
--- /dev/null
+++ b/pinning.go
@@ -0,0 +1,42 @@
+package ebpf
+
+import (
+	"errors"
+	"fmt"
+	"os"
+
+	"github.com/cilium/ebpf/internal"
+)
+
+func pin(currentPath, newPath string, fd *internal.FD) error {
+	if newPath == "" {
+		return errors.New("given pinning path cannot be empty")
+	}
+	if currentPath == "" {
+		return internal.BPFObjPin(newPath, fd)
+	}
+	if currentPath == newPath {
+		return nil
+	}
+	var err error
+	// Object is now moved to the new pinning path.
+	if err = os.Rename(currentPath, newPath); err == nil {
+		return nil
+	}
+	if !os.IsNotExist(err) {
+		return fmt.Errorf("unable to move pinned object to new path %v: %w", newPath, err)
+	}
+	// Internal state not in sync with the file system so let's fix it.
+	return internal.BPFObjPin(newPath, fd)
+}
+
+func unpin(pinnedPath string) error {
+	if pinnedPath == "" {
+		return nil
+	}
+	err := os.Remove(pinnedPath)
+	if err == nil || os.IsNotExist(err) {
+		return nil
+	}
+	return err
+}
diff --git a/prog.go b/prog.go
index a91fd03..4b65f23 100644
--- a/prog.go
+++ b/prog.go
@@ -103,9 +103,10 @@ type Program struct {
 	// otherwise it is empty.
 	VerifierLog string
 
-	fd   *internal.FD
-	name string
-	typ  ProgramType
+	fd         *internal.FD
+	name       string
+	pinnedPath string
+	typ        ProgramType
 }
 
 // NewProgram creates a new Program.
@@ -228,7 +229,7 @@ func newProgramWithOptions(spec *ProgramSpec, opts ProgramOptions, btfs btfHandl
 
 	fd, err := bpfProgLoad(attr)
 	if err == nil {
-		return &Program{internal.CString(logBuf), fd, spec.Name, spec.Type}, nil
+		return &Program{internal.CString(logBuf), fd, spec.Name, "", spec.Type}, nil
 	}
 
 	logErr := err
@@ -287,7 +288,7 @@ func newProgramFromFD(fd *internal.FD) (*Program, error) {
 		return nil, fmt.Errorf("discover program type: %w", err)
 	}
 
-	return &Program{"", fd, "", info.Type}, nil
+	return &Program{"", fd, "", "", info.Type}, nil
 }
 
 func (p *Program) String() string {
@@ -338,19 +339,42 @@ func (p *Program) Clone() (*Program, error) {
 		return nil, fmt.Errorf("can't clone program: %w", err)
 	}
 
-	return &Program{p.VerifierLog, dup, p.name, p.typ}, nil
+	return &Program{p.VerifierLog, dup, p.name, "", p.typ}, nil
 }
 
-// Pin persists the Program past the lifetime of the process that created it
+// Pin persists the Program on the BPF virtual file system past the lifetime of
+// the process that created it
 //
 // This requires bpffs to be mounted above fileName. See https://docs.cilium.io/en/k8s-doc/admin/#admin-mount-bpffs
 func (p *Program) Pin(fileName string) error {
-	if err := internal.BPFObjPin(fileName, p.fd); err != nil {
-		return fmt.Errorf("can't pin program: %w", err)
+	if err := pin(p.pinnedPath, fileName, p.fd); err != nil {
+		return err
 	}
+	p.pinnedPath = fileName
 	return nil
 }
 
+// Unpin removes the persisted state for the Program from the BPF virtual filesystem.
+//
+// Failed calls to Unpin will not alter the state returned by IsPinned.
+//
+// Unpinning an unpinned Program returns nil.
+func (p *Program) Unpin() error {
+	if err := unpin(p.pinnedPath); err != nil {
+		return err
+	}
+	p.pinnedPath = ""
+	return nil
+}
+
+// IsPinned returns true if the Program has a non-empty pinned path.
+func (p *Program) IsPinned() bool {
+	if p.pinnedPath == "" {
+		return false
+	}
+	return true
+}
+
 // Close unloads the program from the kernel.
 func (p *Program) Close() error {
 	if p == nil {
@@ -585,7 +609,7 @@ func LoadPinnedProgram(fileName string) (*Program, error) {
 		return nil, fmt.Errorf("info for %s: %w", fileName, err)
 	}
 
-	return &Program{"", fd, filepath.Base(fileName), info.Type}, nil
+	return &Program{"", fd, filepath.Base(fileName), "", info.Type}, nil
 }
 
 // SanitizeName replaces all invalid characters in name with replacement.


Test patch:
diff --git a/prog_test.go b/prog_test.go
index 5b2e177..0622a9a 100644
--- a/prog_test.go
+++ b/prog_test.go
@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"math"
+	"os"
 	"path/filepath"
 	"runtime"
 	"strings"
@@ -13,6 +14,8 @@ import (
 	"testing"
 	"time"
 
+	qt "github.com/frankban/quicktest"
+
 	"github.com/cilium/ebpf/asm"
 	"github.com/cilium/ebpf/internal"
 	"github.com/cilium/ebpf/internal/testutils"
@@ -180,6 +183,7 @@ func TestProgramClose(t *testing.T) {
 
 func TestProgramPin(t *testing.T) {
 	prog := createSocketFilter(t)
+	c := qt.New(t)
 	defer prog.Close()
 
 	tmp := tempBPFFS(t)
@@ -188,6 +192,10 @@ func TestProgramPin(t *testing.T) {
 	if err := prog.Pin(path); err != nil {
 		t.Fatal(err)
 	}
+
+	pinned := prog.IsPinned()
+	c.Assert(pinned, qt.Equals, true)
+
 	prog.Close()
 
 	prog, err := LoadPinnedProgram(path)
@@ -202,6 +210,29 @@ func TestProgramPin(t *testing.T) {
 	}
 }
 
+func TestProgramUnpin(t *testing.T) {
+	prog := createSocketFilter(t)
+	c := qt.New(t)
+	defer prog.Close()
+
+	tmp := tempBPFFS(t)
+
+	path := filepath.Join(tmp, "program")
+	if err := prog.Pin(path); err != nil {
+		t.Fatal(err)
+	}
+
+	pinned := prog.IsPinned()
+	c.Assert(pinned, qt.Equals, true)
+
+	if err := prog.Unpin(); err != nil {
+		t.Fatal("Failed to unpin program:", err)
+	}
+	if _, err := os.Stat(path); err == nil {
+		t.Fatal("Pinned program path still exists after unpinning:", err)
+	}
+}
+
 func TestProgramVerifierOutputOnError(t *testing.T) {
 	_, err := NewProgram(&ProgramSpec{
 		Type: SocketFilter,