Repository: cilium/ebpf
Pull request #336 (2021-07-01 16:38:49)

improve error wrapping
The library is inconsistent in how it wraps errors from syscalls. Sometimes it obscures the underlying error, sometimes it wraps it, sometimes it replaces it with a more specific error. This PR introduces a new approach:

- We always wrap errors returned by SYS_BPF. This means that err == unix.EFOO will never work.
- We always include the original errno when introducing a more specific error. This means that errors.Is(err, unix.EFOO) will always work.

This approach gives users access to syscall.Errno without painting us into a corner.

lmb: @cyphar please take a look, I think this should solve your problem.

Patch:
diff --git a/internal/syscall.go b/internal/syscall.go
index b9821f7..b766e64 100644
--- a/internal/syscall.go
+++ b/internal/syscall.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"path/filepath"
 	"runtime"
+	"syscall"
 	"unsafe"
 
 	"github.com/cilium/ebpf/internal/unix"
@@ -61,7 +62,7 @@ func BPF(cmd BPFCmd, attr unsafe.Pointer, size uintptr) (uintptr, error) {
 
 	var err error
 	if errNo != 0 {
-		err = errNo
+		err = wrappedErrno{errNo}
 	}
 
 	return r1, err
@@ -213,3 +214,32 @@ func BPFMapCreate(attr *BPFMapCreateAttr) (*FD, error) {
 
 	return NewFD(uint32(fd)), nil
 }
+
+// wrappedErrno wraps syscall.Errno to prevent direct comparisons with
+// syscall.E* or unix.E* constants.
+//
+// You should never export an error of this type.
+type wrappedErrno struct {
+	syscall.Errno
+}
+
+func (we wrappedErrno) Unwrap() error {
+	return we.Errno
+}
+
+type syscallError struct {
+	error
+	errno syscall.Errno
+}
+
+func SyscallError(err error, errno syscall.Errno) error {
+	return &syscallError{err, errno}
+}
+
+func (se *syscallError) Is(target error) bool {
+	return target == se.error
+}
+
+func (se *syscallError) Unwrap() error {
+	return se.errno
+}
diff --git a/syscalls.go b/syscalls.go
index c3cc523..f5a3854 100644
--- a/syscalls.go
+++ b/syscalls.go
@@ -3,6 +3,7 @@ package ebpf
 import (
 	"errors"
 	"fmt"
+	"os"
 	"unsafe"
 
 	"github.com/cilium/ebpf/internal"
@@ -10,8 +11,10 @@ import (
 	"github.com/cilium/ebpf/internal/unix"
 )
 
-// Generic errors returned by BPF syscalls.
-var ErrNotExist = errors.New("requested object does not exist")
+// ErrNotExist is returned when loading a non-existing map or program.
+//
+// Deprecated: use os.ErrNotExist instead.
+var ErrNotExist = os.ErrNotExist
 
 // invalidBPFObjNameChar returns true if char may not appear in
 // a BPF object name.
@@ -162,7 +165,7 @@ func bpfProgLoad(attr *bpfProgLoadAttr) (*internal.FD, error) {
 		fd, err := internal.BPF(internal.BPF_PROG_LOAD, unsafe.Pointer(attr), unsafe.Sizeof(*attr))
 		// As of ~4.20 the verifier can be interrupted by a signal,
 		// and returns EAGAIN in that case.
-		if err == unix.EAGAIN {
+		if errors.Is(err, unix.EAGAIN) {
 			continue
 		}
 
@@ -326,7 +329,7 @@ func objGetNextID(cmd internal.BPFCmd, start uint32) (uint32, error) {
 		startID: start,
 	}
 	_, err := internal.BPF(cmd, unsafe.Pointer(&attr), unsafe.Sizeof(attr))
-	return attr.nextID, wrapObjError(err)
+	return attr.nextID, err
 }
 
 func bpfMapBatch(cmd internal.BPFCmd, m *internal.FD, inBatch, outBatch, keys, values internal.Pointer, count uint32, opts *BatchOptions) (uint32, error) {
@@ -352,32 +355,21 @@ func bpfMapBatch(cmd internal.BPFCmd, m *internal.FD, inBatch, outBatch, keys, v
 	return attr.count, wrapMapError(err)
 }
 
-func wrapObjError(err error) error {
-	if err == nil {
-		return nil
-	}
-	if errors.Is(err, unix.ENOENT) {
-		return fmt.Errorf("%w", ErrNotExist)
-	}
-
-	return errors.New(err.Error())
-}
-
 func wrapMapError(err error) error {
 	if err == nil {
 		return nil
 	}
 
 	if errors.Is(err, unix.ENOENT) {
-		return ErrKeyNotExist
+		return internal.SyscallError(ErrKeyNotExist, unix.ENOENT)
 	}
 
 	if errors.Is(err, unix.EEXIST) {
-		return ErrKeyExist
+		return internal.SyscallError(ErrKeyExist, unix.EEXIST)
 	}
 
 	if errors.Is(err, unix.ENOTSUPP) {
-		return ErrNotSupported
+		return internal.SyscallError(ErrNotSupported, unix.ENOTSUPP)
 	}
 
 	return err
@@ -484,5 +476,5 @@ func bpfObjGetFDByID(cmd internal.BPFCmd, id uint32) (*internal.FD, error) {
 		id: id,
 	}
 	ptr, err := internal.BPF(cmd, unsafe.Pointer(&attr), unsafe.Sizeof(attr))
-	return internal.NewFD(uint32(ptr)), wrapObjError(err)
+	return internal.NewFD(uint32(ptr)), err
 }


Test patch:
diff --git a/internal/syscall_test.go b/internal/syscall_test.go
index f1f9007..9ff949d 100644
--- a/internal/syscall_test.go
+++ b/internal/syscall_test.go
@@ -1,6 +1,7 @@
 package internal
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/cilium/ebpf/internal/unix"
@@ -15,3 +16,41 @@ func TestObjName(t *testing.T) {
 		t.Errorf("Name is %d instead of %d bytes long", len(name), unix.BPF_OBJ_NAME_LEN)
 	}
 }
+
+func TestWrappedErrno(t *testing.T) {
+	a := error(wrappedErrno{unix.EINVAL})
+	b := error(unix.EINVAL)
+
+	if a == b {
+		t.Error("wrappedErrno is comparable to plain errno")
+	}
+
+	if !errors.Is(a, b) {
+		t.Error("errors.Is(wrappedErrno, errno) returns false")
+	}
+
+	if errors.Is(a, unix.EAGAIN) {
+		t.Error("errors.Is(wrappedErrno, EAGAIN) returns true")
+	}
+}
+
+func TestSyscallError(t *testing.T) {
+	err := errors.New("foo")
+	foo := SyscallError(err, unix.EINVAL)
+
+	if !errors.Is(foo, unix.EINVAL) {
+		t.Error("SyscallError is not the wrapped errno")
+	}
+
+	if !errors.Is(foo, err) {
+		t.Error("SyscallError is not the wrapped error")
+	}
+
+	if errors.Is(unix.EINVAL, foo) {
+		t.Error("Errno is the SyscallError")
+	}
+
+	if errors.Is(err, foo) {
+		t.Error("Error is the SyscallError")
+	}
+}