From a890a256ca0f7fecc1712da061db768431f9cd55 Mon Sep 17 00:00:00 2001 From: Lorenz Bauer Date: Thu, 1 Jul 2021 16:35:11 +0100 Subject: [PATCH 1/3] internal: always wrap SYS_BPF errors Make sure we never return a plain syscall.Errno so that comparisons using plain == never succeed. We do this to ensure that we can influence error comparisons via errors.Is later on. --- internal/syscall.go | 15 ++++++++++++++- internal/syscall_test.go | 18 ++++++++++++++++++ syscalls.go | 2 +- 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/internal/syscall.go b/internal/syscall.go index b9821f706..16257f9ca 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,15 @@ 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 +} diff --git a/internal/syscall_test.go b/internal/syscall_test.go index f1f900752..05f7be0f0 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,20 @@ 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") + } +} diff --git a/syscalls.go b/syscalls.go index c3cc523f2..ce5a3764c 100644 --- a/syscalls.go +++ b/syscalls.go @@ -162,7 +162,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 } From d9f5eb50132200b2563eea7d1cd24ce4078cbd5c Mon Sep 17 00:00:00 2001 From: Lorenz Bauer Date: Thu, 1 Jul 2021 16:52:33 +0100 Subject: [PATCH 2/3] map, program: remove wrapObjErr Get rid of wrapObjErr. This works by aliasing ErrNotExist to os.ErrNotExist, since errors.Is(unix.ENOENT, os.ErrNotExist) is true. --- syscalls.go | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/syscalls.go b/syscalls.go index ce5a3764c..e98259135 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. @@ -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,17 +355,6 @@ 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 @@ -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 } From 23261d1598c3950278e57e9ec37ea0d6c047ac45 Mon Sep 17 00:00:00 2001 From: Lorenz Bauer Date: Thu, 1 Jul 2021 17:34:13 +0100 Subject: [PATCH 3/3] map: include errnos with errors We replace some errnos from map syscalls with more descriptive sentinel errors. Include the original errno with the error so that both errors.Is(err, ErrKeyNotExist) errors.Is(err, unix.ENOENT) work. We'll use this for other errors in the future where we want to introduce our own errors without breaking callers that rely on the (coarser) errno semantics. --- internal/syscall.go | 17 +++++++++++++++++ internal/syscall_test.go | 21 +++++++++++++++++++++ syscalls.go | 6 +++--- 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/internal/syscall.go b/internal/syscall.go index 16257f9ca..b766e643e 100644 --- a/internal/syscall.go +++ b/internal/syscall.go @@ -226,3 +226,20 @@ type wrappedErrno struct { 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/internal/syscall_test.go b/internal/syscall_test.go index 05f7be0f0..9ff949df7 100644 --- a/internal/syscall_test.go +++ b/internal/syscall_test.go @@ -33,3 +33,24 @@ func TestWrappedErrno(t *testing.T) { 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") + } +} diff --git a/syscalls.go b/syscalls.go index e98259135..f5a38549b 100644 --- a/syscalls.go +++ b/syscalls.go @@ -361,15 +361,15 @@ func wrapMapError(err error) error { } 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